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

["chr[]" <chris@rudorff.com>]

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

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