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

[Jonathan Cameron <jic23@kernel.org>]

On Sat, 17 Oct 2026 19:27:42 +0200
Christoph Rudorff <chris@rudorff.com> wrote:

> The led power might be attached to an extra supply.

I'd add a mention of LEDA.   + the fact it is always attached
to a supply, what is 'maybe' is whether we need to control it.

> 
> Signed-off-by: Christoph Rudorff <chris@rudorff.com>
> ---
>  drivers/iio/light/gp2ap002.c | 101 +++++++++++++++++++++++--------------------
>  1 file changed, 55 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/iio/light/gp2ap002.c b/drivers/iio/light/gp2ap002.c
> index de29081fd3b2..5e4599e40376 100644
> --- a/drivers/iio/light/gp2ap002.c
> +++ b/drivers/iio/light/gp2ap002.c
> @@ -123,12 +123,26 @@
>  #define GP2AP002_CON_OCON_HIGH		(0x3 << GP2AP002_CON_OCON_SHIFT)
>  #define GP2AP002_CON_OCON_MASK		(0x3 << GP2AP002_CON_OCON_SHIFT)
>  
> +/*
> + * @vio: regulator name controlling VIO
> + * @vdd: regulator name controlling VDD
> + * @vled: regulator name controlling VLED (optional)
> + */
> +static const char *const gp2ap002_supply_names[] = {
> +	"vio", "vdd", "vled"
> +};
blank line here.
> +enum {
> +	SUPPLY_VIO,
> +	SUPPLY_VDD,
> +	SUPPLY_VLED,
> +	NUM_SUPPLIES
> +};
Force these to match up via

static const char *const gp2ap002_supply_names[NUM_SUPPLIES] = {
	[SUPPLY_VIO] = "vio",
	[SUPPLY_VDD] = "vdd",
	[SUPPLY_VLED] = "vled",
};

You might as well also move this naming array local to where
it is used.

> +
>  /**
>   * struct gp2ap002 - GP2AP002 state
>   * @map: regmap pointer for the i2c regmap
>   * @dev: pointer to parent device
> - * @vdd: regulator controlling VDD
> - * @vio: regulator controlling VIO
> + * @supplies bulk regulator data
>   * @alsout: IIO ADC channel to convert the ALSOUT signal
>   * @hys_far: hysteresis control from device tree
>   * @hys_close: hysteresis control from device tree
> @@ -140,8 +154,7 @@
>  struct gp2ap002 {
>  	struct regmap *map;
>  	struct device *dev;
> -	struct regulator *vdd;
> -	struct regulator *vio;
> +	struct regulator_bulk_data supplies[NUM_SUPPLIES];
>  	struct iio_channel *alsout;
>  	u8 hys_far;
>  	u8 hys_close;
> @@ -521,45 +534,52 @@ static int gp2ap002_probe(struct i2c_client *client)
>  		}
>  	}
>  
> -	gp2ap002->vdd = devm_regulator_get(dev, "vdd");
> -	if (IS_ERR(gp2ap002->vdd))
> -		return dev_err_probe(dev, PTR_ERR(gp2ap002->vdd),
> -				     "failed to get VDD regulator\n");
> +	regulator_bulk_set_supply_names(gp2ap002->supplies,
> +		gp2ap002_supply_names,
> +		NUM_SUPPLIES);
I was kind of expecting this to be local style you were copying, but nope the
driver is at least fairly consistent in indenting to align parameters.
e.g.

	regulator_bulk_set_supply_names(gp2ap002->supplies,
					gp2ap002_supply_names,
					NUM_SUPPLIES);

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

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


>  
> -	gp2ap002->vio = devm_regulator_get(dev, "vio");
> -	if (IS_ERR(gp2ap002->vio))
> -		return dev_err_probe(dev, PTR_ERR(gp2ap002->vio),
> -				     "failed to get VIO regulator\n");
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to get regulators\n");
>  
> -	/* Operating voltage 2.4V .. 3.6V according to datasheet */
> -	ret = regulator_set_voltage(gp2ap002->vdd, 2400000, 3600000);
> +	/* vdd operating voltage 2.4V .. 3.6V according to datasheet */
> +	ret = regulator_set_voltage(gp2ap002->supplies[SUPPLY_VDD].consumer, 2400000, 3600000);
>  	if (ret) {
> -		dev_err(dev, "failed to sett VDD voltage\n");
> +		dev_err(dev, "failed to set VDD voltage\n");
>  		return ret;
>  	}
> -
>  	/* VIO should be between 1.65V and VDD */
> -	ret = regulator_get_voltage(gp2ap002->vdd);
> +	ret = regulator_get_voltage(gp2ap002->supplies[SUPPLY_VDD].consumer);

Ah. I'd failed to register the voltages were being manually set (generally
we leave that to firmware in more modern drivers).
That rather reduces the advantage in using bulk regulators.  Probably
still just about worth doing.


>  	if (ret < 0) {
>  		dev_err(dev, "failed to get VDD voltage\n");
>  		return ret;
>  	}
> -	ret = regulator_set_voltage(gp2ap002->vio, 1650000, ret);
> +	ret = regulator_set_voltage(gp2ap002->supplies[SUPPLY_VIO].consumer, 1650000, ret);
>  	if (ret) {
>  		dev_err(dev, "failed to set VIO voltage\n");
>  		return ret;
>  	}
> +	/* LED voltage 2.4V .. 3.6V according to datasheet */
> +	/* We need to check this, as dummy or fixed controllers trigger an error. */
I'd make that a multiline comment and say what 'this' is.
	/*
	 * LED voltage 2.4V .. 3.6V according to datasheet
	 * We need to check control is supported, as controlling dummy or fixed
	 * controllers results in an error.
	 */

Do you actually need to control this for your board?  I'm not that
keen to replicate the controlling of voltages if we don't have
to.  We can't drop existing code as maybe a board is relying on it
but we can avoid adding new examples!

> +	ret = regulator_is_supported_voltage(gp2ap002->supplies[SUPPLY_VLED].consumer,
> +		2400000, 3600000);
> +	if (ret == 1) {
> +		ret = regulator_set_voltage(gp2ap002->supplies[SUPPLY_VLED].consumer,
> +		2400000, 3600000);
> +		if (ret) {
> +			dev_err(dev, "failed to set VLED voltage\n");
> +			return ret;
> +		}
> +	}
>  
> -	ret = regulator_enable(gp2ap002->vdd);
> +	ret = regulator_bulk_enable(NUM_SUPPLIES, gp2ap002->supplies);
>  	if (ret) {
> -		dev_err(dev, "failed to enable VDD regulator\n");
> +		dev_err(dev, "failed to enable regulators\n");
>  		return ret;
>  	}
> -	ret = regulator_enable(gp2ap002->vio);
> -	if (ret) {
> -		dev_err(dev, "failed to enable VIO regulator\n");
> -		goto out_disable_vdd;
> -	}
>  
>  	msleep(20);
>  
> @@ -570,7 +590,7 @@ static int gp2ap002_probe(struct i2c_client *client)
>  	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 +636,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;
>  }
>  
> @@ -632,9 +650,8 @@ static void gp2ap002_remove(struct i2c_client *client)
>  	pm_runtime_get_sync(dev);
>  	pm_runtime_put_noidle(dev);
>  	pm_runtime_disable(dev);
> +	regulator_bulk_disable(NUM_SUPPLIES, gp2ap002->supplies);

Why at that point in sequence?  You are turning the power off
before removing the userspace interfaces which seems unlikely to
be race free. More generally it is no longer reverse of probe
order. If that is necessary for some reason I'd expect a comment
to say why.

>  	iio_device_unregister(indio_dev);
> -	regulator_disable(gp2ap002->vio);
> -	regulator_disable(gp2ap002->vdd);
>  }