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

[chr[]]

Hi,

In preparation of v3:

Am 17.10.26 um 20:00 schrieb Jonathan Cameron:
> On Sat, 17 Oct 2026 19:27:42 +0200
> Christoph Rudorff <chris@xxxxxxxxxxx> wrote:
>
> >   	/* 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.

Yes, this is why I didn't made it in v1.

> >   	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!

So we don't want to drop all the set_voltage code in general.
My board does not need that at all, all voltages are already set
to valid values in DT, ... I hope ... I check downstream ...

So I skip the "example" for set_voltage(vled)?!

And thanks for the advice to pretty up the code!

chr[]

> > @@ -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.

Sorry, just a stupid mistake.

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