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

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 18 Oct 2026 16:41:48 +0200
"chr[]" <chris@rudorff.com> wrote:

> 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@rudorff.com> 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 ...
Unfortunately it would be a bit of a gamble to remove the in
driver voltage control on existing supplies. Given it works
for yours though let us not do it for the new one! 
> 
> So I skip the "example" for set_voltage(vled)?!

Exactly.

Jonathan

> 
> 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);
> >>   }  
> > 
> >   
>