Re: [PATCH] regulator: s5m8767: Convert to GPIO descriptors

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Tue, Mar 18, 2025 at 01:27:09PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> Update the driver to fetch buck_gpio and buck_ds as gpio descriptors.

GPIO

> Then drop the usage of 'of_gpio.h' which should be deprecated.

s/should/is/

> Take commit 84618d5e31cf ("regulator: max8997:

s/Take/Based on/

> Convert to GPIO descriptors") as a reference to make the changes.

> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Bartosz Golaszewski <brgl@bgdev.pl>

Can be done via --to parameter to git-send-email.

...

> +	gpiod_set_value(s5m8767->buck_gpios[0], (temp_index >> 2) & 0x1);

Can be simply done as !!(temp_index & BIT(2)).

> +	gpiod_set_value(s5m8767->buck_gpios[1], (temp_index >> 1) & 0x1);
> +	gpiod_set_value(s5m8767->buck_gpios[2], temp_index & 0x1);

Ditto.

...

> +	gpiod_set_value(s5m8767->buck_gpios[2], temp_index & 0x1);
> +	gpiod_set_value(s5m8767->buck_gpios[1], (temp_index >> 1) & 0x1);
> +	gpiod_set_value(s5m8767->buck_gpios[0], (temp_index >> 2) & 0x1);

As per above.

...

Also the commit message doesn't tell anything about the existing DTS files.
Do we have this device described in any in the kernel? Do we have any googled
examples? Why I'm asking because often the issue is the incorrect setting of
the polarity, which needs to be carefully checked, esp. for the voltage regulators
case.

...

> +	const char *gpiods_names[3] = {"S5M8767 DS2", "S5M8767 DS3", "S5M8767 DS4"};
> +	const char *gpiodvs_names[3] = {"S5M8767 SET1", "S5M8767 SET2", "S5M8767 SET3"};

Add spaces after { and before }.

...

> +		for (i = 0; i < 3; i++) {
> +			enum gpiod_flags flags;
>  
> +			if (s5m8767->buck_gpioindex & BIT(2 - i))
> +				flags = GPIOD_OUT_HIGH;
> +			else
> +				flags = GPIOD_OUT_LOW;
> +
> +			s5m8767->buck_gpios[i] = devm_gpiod_get_index(iodev->dev,
> +								      "s5m8767,pmic-buck-dvs",
> +								      i,
> +								      flags);

i and flags can be located on the same line, or I would rather move i to
the line with the con_id.

			s5m8767->buck_gpios[i] = devm_gpiod_get_index(iodev->dev,
								      "s5m8767,pmic-buck-dvs", i,
								      flags);

> +			if (IS_ERR(s5m8767->buck_gpios[i])) {
> +				ret = PTR_ERR(s5m8767->buck_gpios[i]);
> +				return dev_err_probe(iodev->dev, ret, "invalid gpio[%d]: %d\n",
> +						     i, ret);

ret will be printed twice. This should be as simple as

			if (IS_ERR(s5m8767->buck_gpios[i]))
				return dev_err_probe(iodev->dev, PTR_ERR(s5m8767->buck_gpios[i]),
						     "invalid gpio[%d]\n", i);

> +			}
>  
> +			gpiod_set_consumer_name(s5m8767->buck_gpios[i], gpiodvs_names[i]);
> +		}

...

> +	for (i = 0; i < 3; i++) {

Both comments as per above apply here, in this for-loop.

> +		s5m8767->buck_ds[i] = devm_gpiod_get_index(iodev->dev,
> +							   "s5m8767,pmic-buck-ds",
> +							   i, GPIOD_OUT_LOW);
> +		if (IS_ERR(s5m8767->buck_ds[i])) {
> +			ret = PTR_ERR(s5m8767->buck_ds[i]);
> +			return dev_err_probe(iodev->dev, ret, "can't get GPIO %d (%d)\n",
> +					     i, ret);
> +		}
> +		gpiod_set_consumer_name(s5m8767->buck_ds[i], gpiods_names[i]);
> +	}

-- 
With Best Regards,
Andy Shevchenko