Re: [PATCH v3 3/4] mfd: aat2870: Convert to use OF bindings

[Lee Jones]

On Tue, 13 Oct 2026, Svyatoslav Ryhel wrote:

> Conversion of the AAT2870 driver to use OF bindings requires a few complex
> changes that should be done simultaneously.
>
> The AAT2870 essentially provides two functions via child devices:
> backlight and regulators. Both functions are fairly self-sufficient and
> may not be populated on the final board. Consequently, the MFD
> registration API was replaced with of_platform_populate(), and each
> sub-device was given its own compatible string.
>
> Additionally, aat2870-core utilizes an enable GPIO. Obtaining this GPIO
> was converted to use modern gpiod/OF helpers, allowing the redundant
> aat2870_enable() and aat2870_disable() helpers to be removed.
>
> The aat2870-regulator driver was updated to register each of the four LDOs
> from a dedicated OF node using modern regulator framework.
>
> The aat2870-backlight driver was changed to populate all required
> properties from a dedicated Device Tree node. Its channel map was updated
> to use u8 instead of int. Furthermore, because the maximum current is now
> parsed as an absolute value rather than an enum entry, the calculation and
> application of the maximum current were updated accordingly.
>
> All of the changes above allow platform data to be removed entirely.
>
> Signed-off-by: Svyatoslav Ryhel <clamor95@xxxxxxxxx>
> ---
>  drivers/mfd/aat2870-core.c            | 115 +++++--------------
>  drivers/regulator/aat2870-regulator.c | 159 +++++++++++---------------
>  drivers/video/backlight/aat2870_bl.c  |  51 +++++----
>  include/linux/mfd/aat2870.h           |  95 +--------------
>  4 files changed, 121 insertions(+), 299 deletions(-)
>
> 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
> @@ -16,8 +16,15 @@
>  #include <linux/gpio/legacy.h>
>  #include <linux/mfd/core.h>
>  #include <linux/mfd/aat2870.h>
> +#include <linux/of_platform.h>
>  #include <linux/regulator/machine.h>

Since we are moving away from legacy GPIO and MFD APIs here, does it
make sense to replace '<linux/gpio/legacy.h>' and '<linux/mfd/core.h>'
with '<linux/gpio/consumer.h>'?

> +struct aat2870_register {
> +	bool readable;
> +	bool writeable;
> +	u8 value;
> +};
> +
>  static struct aat2870_register aat2870_regs[AAT2870_REG_NUM] = {
>  	/* readable, writeable, value */
>  	{ 0, 1, 0x00 },	/* 0x00 AAT2870_BL_CH_EN */
> @@ -61,34 +68,6 @@ static struct aat2870_register aat2870_regs[AAT2870_REG_NUM] = {
>  	{ 0, 1, 0x00 },	/* 0x26 AAT2870_LDO_EN */
>  };
>
> -static struct mfd_cell aat2870_devs[] = {
> -	{
> -		.name = "aat2870-backlight",
> -		.id = AAT2870_ID_BL,
> -		.pdata_size = sizeof(struct aat2870_bl_platform_data),
> -	},
> -	{
> -		.name = "aat2870-regulator",
> -		.id = AAT2870_ID_LDOA,
> -		.pdata_size = sizeof(struct regulator_init_data),
> -	},
> -	{
> -		.name = "aat2870-regulator",
> -		.id = AAT2870_ID_LDOB,
> -		.pdata_size = sizeof(struct regulator_init_data),
> -	},
> -	{
> -		.name = "aat2870-regulator",
> -		.id = AAT2870_ID_LDOC,
> -		.pdata_size = sizeof(struct regulator_init_data),
> -	},
> -	{
> -		.name = "aat2870-regulator",
> -		.id = AAT2870_ID_LDOD,
> -		.pdata_size = sizeof(struct regulator_init_data),
> -	},
> -};
> -
>  static int __aat2870_read(struct aat2870_data *aat2870, u8 addr, u8 *val)
>  {
>  	int ret;
> @@ -196,22 +175,6 @@ static int aat2870_update(struct aat2870_data *aat2870, u8 addr, u8 mask,
>  	return ret;
>  }
>
> -static inline void aat2870_enable(struct aat2870_data *aat2870)
> -{
> -	if (aat2870->en_pin >= 0)
> -		gpio_set_value(aat2870->en_pin, 1);
> -
> -	aat2870->is_enable = 1;
> -}
> -
> -static inline void aat2870_disable(struct aat2870_data *aat2870)
> -{
> -	if (aat2870->en_pin >= 0)
> -		gpio_set_value(aat2870->en_pin, 0);
> -
> -	aat2870->is_enable = 0;
> -}
> -
>  #ifdef CONFIG_DEBUG_FS
>  static ssize_t aat2870_dump_reg(struct aat2870_data *aat2870, char *buf)
>  {
> @@ -332,9 +295,7 @@ static inline void aat2870_init_debugfs(struct aat2870_data *aat2870)
>
>  static int aat2870_i2c_probe(struct i2c_client *client)
>  {
> -	struct aat2870_platform_data *pdata = dev_get_platdata(&client->dev);
>  	struct aat2870_data *aat2870;
> -	int i, j;
>  	int ret = 0;
>
>  	aat2870 = devm_kzalloc(&client->dev, sizeof(struct aat2870_data),
> @@ -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);

Is this needed when 'devm_gpiod_get_optional()' already requested the pin with
'GPIOD_OUT_HIGH'?

>
> -	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);
> +		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);
>
>  	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);
>
>  	/* restore registers */
>  	for (i = 0; i < AAT2870_REG_NUM; i++) {
> @@ -436,6 +366,12 @@ static int aat2870_i2c_resume(struct device *dev)
>  static DEFINE_SIMPLE_DEV_PM_OPS(aat2870_pm_ops, aat2870_i2c_suspend,
>  				aat2870_i2c_resume);
>
> +static const struct of_device_id aat2870_match_table[] = {
> +	{ .compatible = "skyworks,aat2870" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, aat2870_match_table);
> +
>  static const struct i2c_device_id aat2870_i2c_id_table[] = {
>  	{ "aat2870" },
>  	{ }
> @@ -444,6 +380,7 @@ static const struct i2c_device_id aat2870_i2c_id_table[] = {
>  static struct i2c_driver aat2870_i2c_driver = {
>  	.driver = {
>  		.name			= "aat2870",
> +		.of_match_table		= aat2870_match_table,
>  		.pm			= pm_sleep_ptr(&aat2870_pm_ops),
>  		.suppress_bind_attrs	= true,
>  	},

-- 
Lee Jones