Re: [PATCH v2 6/6] media: i2c: og01a1b: Add management of optional sensor supply lines

[Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>]

Hi Kieran,

On 8/13/24 15:53, Kieran Bingham wrote:
> Quoting Vladimir Zapolskiy (2024-08-13 11:20:35)
>> Omnivision OG01A1B camera sensor is supplied by tree power rails,
> 
> three?

that's it, thanks for catching the typo.

>> if supplies are present as device properties, include them into
>> sensor power up sequence.
>>
>> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
>> ---
>>   drivers/media/i2c/og01a1b.c | 86 ++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 85 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/i2c/og01a1b.c b/drivers/media/i2c/og01a1b.c
>> index 90a68201f43f..0150fdd2f424 100644
>> --- a/drivers/media/i2c/og01a1b.c
>> +++ b/drivers/media/i2c/og01a1b.c
>> @@ -9,6 +9,7 @@
>>   #include <linux/i2c.h>
>>   #include <linux/module.h>
>>   #include <linux/pm_runtime.h>
>> +#include <linux/regulator/consumer.h>
>>   #include <media/v4l2-ctrls.h>
>>   #include <media/v4l2-device.h>
>>   #include <media/v4l2-fwnode.h>
>> @@ -422,6 +423,9 @@ static const struct og01a1b_mode supported_modes[] = {
>>   struct og01a1b {
>>          struct clk *xvclk;
>>          struct gpio_desc *reset_gpio;
>> +       struct regulator *avdd;
>> +       struct regulator *dovdd;
>> +       struct regulator *dvdd;
>>   
>>          struct v4l2_subdev sd;
>>          struct media_pad pad;
>> @@ -982,11 +986,46 @@ static int og01a1b_power_on(struct device *dev)
>>   {
>>          struct v4l2_subdev *sd = dev_get_drvdata(dev);
>>          struct og01a1b *og01a1b = to_og01a1b(sd);
>> +       int ret;
>> +
>> +       if (og01a1b->avdd) {
>> +               ret = regulator_enable(og01a1b->avdd);
>> +               if (ret)
>> +                       return ret;
>> +       }
>> +
>> +       if (og01a1b->dovdd) {
>> +               ret = regulator_enable(og01a1b->dovdd);
>> +               if (ret)
>> +                       goto avdd_disable;
>> +       }
>> +
>> +       if (og01a1b->dvdd) {
>> +               ret = regulator_enable(og01a1b->dvdd);
>> +               if (ret)
>> +                       goto dovdd_disable;
>> +       }
> 
> Perhaps regulator_bulk_enable()/regulator_bulk_disable() would be
> suitable here to reduce lots of repetitive code and error handling?

It won't be possible to support optional regulators with bulk operations,
thus likely it's not an option.

Note, that in ACPI case there are no regulators at all, at least this
should be functionally preserved in the driver.

It might be an option to define all supply regulators as strictly required
for the OF case, but since there is actually no need for it, I'm reluctant
to push the limits into the device tree node schema.

So, from my opinion the left option is the published one.

--
Best wishes,
Vladimir