Re: [PATCH v3 2/3] iio: light: gp2ap002: add led power supply

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

On Tue, Oct 20, 2026 at 01:35:39PM +0200, Christoph Rudorff wrote:

> tested on samsung-j5
> 
> v2: Use regulator bulk api.
> v3: pretty up, rename vled to leda

The comment and changelog area is...

> Signed-off-by: Christoph Rudorff <chris@rudorff.com>
> ---

...here, but the commit message is effectively missing. We may not accept such
a change, so please provide a commit message and move unnecessary bits to the
comment and changelog area.

...

> +enum {
> +	SUPPLY_VIO,
> +	SUPPLY_VDD,
> +	SUPPLY_LEDA,
> +	NUM_SUPPLIES

Not critical, but the naming is too broad, better to have namespaced variants

	GP2AP002_SUPPLY_VIO,

et cetera.

> +};

>  	struct regmap *regmap;
>  	int num_chan;
>  	const char *compat;
> +	static const char *const gp2ap002_supply_names[NUM_SUPPLIES] = {
> +		[SUPPLY_VIO] = "vio",
> +		[SUPPLY_VDD] = "vdd",
> +		[SUPPLY_LEDA] = "leda",
> +	};

Can be moved out of the function to see the static data (that's effectively
global anyway).

>  	u8 val;
>  	int ret;

...

> +	ret = devm_regulator_bulk_get(dev, NUM_SUPPLIES, gp2ap002->supplies);

> +

Redundant blank line.

> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to get regulators\n");

...

> +	ret = regulator_set_voltage(gp2ap002->supplies[SUPPLY_VDD].consumer, 2400000, 3600000);

> -	ret = regulator_get_voltage(gp2ap002->vdd);
> +	ret = regulator_get_voltage(gp2ap002->supplies[SUPPLY_VDD].consumer);

> -	ret = regulator_set_voltage(gp2ap002->vio, 1650000, ret);
> +	ret = regulator_set_voltage(gp2ap002->supplies[SUPPLY_VIO].consumer, 1650000, ret);

To make these lines shorter you can introduce a temporary variable (or reuse
"compat" if it's feasible, might require to rename the variable, though)

	rname = gp2ap002->supplies[SUPPLY_VDD].consumer;
	ret = regulator_set_voltage(rname, 2400000, 3600000);
	...
	rname = gp2ap002->supplies[SUPPLY_VIO].consumer;
	ret = regulator_set_voltage(rname, 1650000, ret);

(Side note: This ret = foo(ret) doesn't sound good, the rule of thumb to have a separate
 variable for the 'ret', so you will have ret = foo($VAR) in the code.)

...

>  	ret = gp2ap002_init(gp2ap002);
>  	if (ret) {
>  		dev_err(dev, "initialization failed\n");
> -		goto out_disable_vio;
> +		goto out_disable_power;
>  	}
>  	pm_runtime_get_noresume(dev);
>  	pm_runtime_set_active(dev);
> @@ -616,10 +619,8 @@ static int gp2ap002_probe(struct i2c_client *client)
>  	pm_runtime_put_noidle(dev);
>  out_disable_pm:
>  	pm_runtime_disable(dev);
> -out_disable_vio:
> -	regulator_disable(gp2ap002->vio);
> -out_disable_vdd:
> -	regulator_disable(gp2ap002->vdd);
> +out_disable_power:
> +	regulator_bulk_disable(NUM_SUPPLIES, gp2ap002->supplies);
>  	return ret;

Wondering if we can use managed resources to disable regulators.

-- 
With Best Regards,
Andy Shevchenko