Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [Medium] video: missing default value for optional led-max-microamp property - [Medium] mfd: aat2870: WARNING triggered by gpiod_set_value on sleepable GPIOs - [Low] video: incompatible pointer type in device_property_read_u32 - [Low] regulator: missing const on aat2870_regulators array --
commit 44f47c7fe6dfacca37d21c28bab7fce4a68f379a Author: Svyatoslav Ryhel <[email protected]> mfd: aat2870: Convert to use OF bindings This commit converts the AAT2870 driver to use OF bindings by replacing mfd_add_devices() with of_platform_populate() and adding separate compatible strings for its backlight and regulator sub-devices. It also updates the GPIO enable logic to use gpiod helpers and modifies the child drivers to parse properties directly from the Device Tree, allowing the removal of platform data. > diff --git a/drivers/mfd/aat2870-core.c b/drivers/mfd/aat2870-core.c > index 0d56cd6fbc6a5..909c14ee60323 100644 > --- a/drivers/mfd/aat2870-core.c > +++ b/drivers/mfd/aat2870-core.c [ ... ] > @@ -348,60 +309,29 @@ static int aat2870_i2c_probe(struct i2c_client *client) > > aat2870->reg_cache = aat2870_regs; > > - if (pdata->en_pin < 0) > - aat2870->en_pin = -1; > - else > - aat2870->en_pin = pdata->en_pin; > + aat2870->en_pin = devm_gpiod_get_optional(&client->dev, "enable", > + GPIOD_OUT_HIGH); > + if (IS_ERR(aat2870->en_pin)) > + return dev_err_probe(&client->dev, PTR_ERR(aat2870->en_pin), > + "Failed to get EN GPIO\n"); > > - aat2870->init = pdata->init; > - aat2870->uninit = pdata->uninit; > aat2870->read = aat2870_read; > aat2870->write = aat2870_write; > aat2870->update = aat2870_update; > > mutex_init(&aat2870->io_lock); > > - if (aat2870->init) > - aat2870->init(aat2870); > - > - if (aat2870->en_pin >= 0) { > - ret = devm_gpio_request_one(&client->dev, aat2870->en_pin, > - GPIOF_OUT_INIT_HIGH, "aat2870-en"); > - if (ret < 0) { > - dev_err(&client->dev, > - "Failed to request GPIO %d\n", aat2870->en_pin); > - return ret; > - } > - } > - > - aat2870_enable(aat2870); > - > - for (i = 0; i < pdata->num_subdevs; i++) { > - for (j = 0; j < ARRAY_SIZE(aat2870_devs); j++) { > - if ((pdata->subdevs[i].id == aat2870_devs[j].id) && > - !strcmp(pdata->subdevs[i].name, > - aat2870_devs[j].name)) { > - aat2870_devs[j].platform_data = > - pdata->subdevs[i].platform_data; > - break; > - } > - } > - } > + gpiod_set_value(aat2870->en_pin, 1); [Severity: Medium] Should this use gpiod_set_value_cansleep() instead? Since this device is an I2C MFD, the enable GPIO could be attached to an I2C or SPI expander, which means toggling it might sleep. Using the non-cansleep gpiod_set_value() here, as well as in the probe error path and suspend/resume callbacks, could trigger a WARN_ON() in gpiolib if the underlying controller requires sleeping. > > - ret = mfd_add_devices(aat2870->dev, 0, aat2870_devs, > - ARRAY_SIZE(aat2870_devs), NULL, 0, NULL); > - if (ret != 0) { > - dev_err(aat2870->dev, "Failed to add subdev: %d\n", ret); > - goto out_disable; > + ret = devm_of_platform_populate(&client->dev); > + if (ret) { > + gpiod_set_value(aat2870->en_pin, 0); [Severity: Medium] Similar to the above, would gpiod_set_value_cansleep() be safer here on the error path to avoid a potential gpiolib WARN_ON()? > + return dev_err_probe(&client->dev, ret, "Failed to populate > cells\n"); > } > > aat2870_init_debugfs(aat2870); > > return 0; > - > -out_disable: > - aat2870_disable(aat2870); > - return ret; > } > > static int aat2870_i2c_suspend(struct device *dev) > @@ -409,7 +339,7 @@ static int aat2870_i2c_suspend(struct device *dev) > struct i2c_client *client = to_i2c_client(dev); > struct aat2870_data *aat2870 = i2c_get_clientdata(client); > > - aat2870_disable(aat2870); > + gpiod_set_value(aat2870->en_pin, 0); [Severity: Medium] Should we use gpiod_set_value_cansleep() here in the suspend handler? > > return 0; > } > @@ -421,7 +351,7 @@ static int aat2870_i2c_resume(struct device *dev) > struct aat2870_register *reg = NULL; > int i; > > - aat2870_enable(aat2870); > + gpiod_set_value(aat2870->en_pin, 1); [Severity: Medium] Should we use gpiod_set_value_cansleep() here in the resume handler? > > /* restore registers */ > for (i = 0; i < AAT2870_REG_NUM; i++) { > diff --git a/drivers/regulator/aat2870-regulator.c > b/drivers/regulator/aat2870-regulator.c > index 970d86f2bbb81..9b8eb8df220a7 100644 > --- a/drivers/regulator/aat2870-regulator.c > +++ b/drivers/regulator/aat2870-regulator.c [ ... ] > @@ -101,84 +102,60 @@ static const unsigned int aat2870_ldo_voltages[] = { > 3000000, 3100000, 3200000, 3300000, > }; > > -#define AAT2870_LDO(ids) \ > - { \ > - .desc = { \ > - .name = #ids, \ > - .id = AAT2870_ID_##ids, \ > - .n_voltages = ARRAY_SIZE(aat2870_ldo_voltages), \ > - .volt_table = aat2870_ldo_voltages, \ > - .ops = &aat2870_ldo_ops, \ > - .type = REGULATOR_VOLTAGE, \ > - .owner = THIS_MODULE, \ > - }, \ > - } > +#define AAT2870_LDO(ids, match, vsel, vmask) { \ > + .name = #ids, \ > + .of_match = (match), \ > + .id = AAT2870_ID_##ids, \ > + .n_voltages = ARRAY_SIZE(aat2870_ldo_voltages), \ > + .volt_table = aat2870_ldo_voltages, \ > + .ops = &aat2870_ldo_ops, \ > + .type = REGULATOR_VOLTAGE, \ > + .owner = THIS_MODULE, \ > + .vsel_reg = (vsel), \ > + .vsel_mask = (vmask), \ > + .enable_reg = AAT2870_LDO_EN, \ > + .enable_mask = BIT(AAT2870_ID_##ids), \ > +} > > -static struct aat2870_regulator aat2870_regulators[] = { > - AAT2870_LDO(LDOA), > - AAT2870_LDO(LDOB), > - AAT2870_LDO(LDOC), > - AAT2870_LDO(LDOD), > +static struct regulator_desc aat2870_regulators[] = { [Severity: Low] Could we add the const qualifier to this array? Since the descriptions are static and never modified at runtime, and devm_regulator_register() expects a const struct regulator_desc *, adding const would allow this array to be placed in read-only .rodata memory instead of mutable .data. > + AAT2870_LDO(LDOA, "ldo-a", AAT2870_LDO_AB, AAT2870_LDO_AB_A_MASK), > + AAT2870_LDO(LDOB, "ldo-b", AAT2870_LDO_AB, AAT2870_LDO_AB_B_MASK), > + AAT2870_LDO(LDOC, "ldo-c", AAT2870_LDO_CD, AAT2870_LDO_CD_C_MASK), > + AAT2870_LDO(LDOD, "ldo-d", AAT2870_LDO_CD, AAT2870_LDO_CD_D_MASK), > }; > > diff --git a/drivers/video/backlight/aat2870_bl.c > b/drivers/video/backlight/aat2870_bl.c > index eb686e315921d..705a8d32d224f 100644 > --- a/drivers/video/backlight/aat2870_bl.c > +++ b/drivers/video/backlight/aat2870_bl.c [ ... ] > @@ -129,21 +124,20 @@ static int aat2870_bl_probe(struct platform_device > *pdev) > > aat2870_bl->bd = bd; > > - if (pdata->channels > 0) > - aat2870_bl->channels = pdata->channels; > - else > - aat2870_bl->channels = AAT2870_BL_CH_ALL; > + aat2870_bl->channels = AAT2870_BL_CH_ALL; > + device_property_read_u8(&pdev->dev, "skyworks,channels", > &aat2870_bl->channels); > > - if (pdata->max_current > 0) > - aat2870_bl->max_current = pdata->max_current; > - else > - aat2870_bl->max_current = AAT2870_CURRENT_27_9; > + device_property_read_u32(&pdev->dev, "led-max-microamp", > &aat2870_bl->max_current); [Severity: Low] Does this cause an incompatible pointer type warning? aat2870_bl->max_current is defined as a signed int, but device_property_read_u32() expects a u32 pointer. This could break builds that treat -Wincompatible-pointer-types as an error. [Severity: Medium] What happens if the optional "led-max-microamp" property is missing from the device tree? If the property is missing, device_property_read_u32() leaves max_current at 0 (its initial value from devm_kzalloc). When passed into clamp(0, 450, 27900), it gets forced to 450, and dividing by 900 evaluates to 0. This sets bd->props.max_brightness to 1 (0 + 1), silently degrading the backlight limits to an almost invisible state, whereas the previous behavior correctly defaulted to 27.9mA (32 steps). Should max_current be initialized to a default value before reading the property? > + aat2870_bl->max_current = clamp(aat2870_bl->max_current, > AAT2870_CURRENT_MIN, > + AAT2870_CURRENT_MAX); > + aat2870_bl->max_current /= AAT2870_CURRENT_STEP; > > /* > * The maximum number of brightness steps is determined by the max > current. > - * The aat2870_current enum maps directly to the allowed brightness > steps. > + * The aat2870_bl->max_current value maps to the allowed brightness > steps > + * with +1 offset since max_current 0 is still 450uA. > */ > - bd->props.max_brightness = pdata->max_current; > + bd->props.max_brightness = aat2870_bl->max_current + 1; > > aat2870_bl->brightness = 0; > bd->props.power = BACKLIGHT_POWER_ON; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
