Re: [PATCH 2/2] media: i2c: ov9282: Add support for regulators.

[Alexander Stein <alexander.stein@ew.tq-group.com>]

Hi Dave,

Am Donnerstag, 24. November 2022, 12:58:08 CET schrieb Dave Stevenson:
> Hi Alexander
> 
> Thanks for the review.
> 
> Sakari has already picked this up and included it in a pull to Mauro for
> 6.2. https://www.spinics.net/lists/linux-media/msg222346.html

A quite recent, I wasn't aware of that. Thanks for the hint.

> On Thu, 24 Nov 2022 at 09:31, Alexander Stein
> 
> <alexander.stein@ew.tq-group.com> wrote:
> > Hello Dave,
> > 
> > Am Mittwoch, 5. Oktober 2022, 17:20:18 CET schrieb Dave Stevenson:
> > > The sensor takes 3 supply rails - AVDD, DVDD, and DOVDD.
> > > 
> > > Add hooks into the regulator framework for each of these
> > > regulators.
> > > 
> > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > ---
> > > 
> > >  drivers/media/i2c/ov9282.c | 38 ++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 38 insertions(+)
> > > 
> > > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > > index 2e0b315801e5..699fc5b753b4 100644
> > > --- a/drivers/media/i2c/ov9282.c
> > > +++ b/drivers/media/i2c/ov9282.c
> > > @@ -11,6 +11,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-fwnode.h>
> > > 
> > > @@ -55,6 +56,14 @@
> > > 
> > >  #define OV9282_REG_MIN               0x00
> > >  #define OV9282_REG_MAX               0xfffff
> > > 
> > > +static const char * const ov9282_supply_names[] = {
> > > +     "avdd",         /* Analog power */
> > > +     "dovdd",        /* Digital I/O power */
> > > +     "dvdd",         /* Digital core power */
> > > +};
> > > +
> > > +#define OV9282_NUM_SUPPLIES ARRAY_SIZE(ov9282_supply_names)
> > > +
> > > 
> > >  /**
> > >  
> > >   * struct ov9282_reg - ov9282 sensor register
> > >   * @address: Register address
> > > 
> > > @@ -128,6 +137,7 @@ struct ov9282 {
> > > 
> > >       struct media_pad pad;
> > >       struct gpio_desc *reset_gpio;
> > >       struct clk *inclk;
> > > 
> > > +     struct regulator_bulk_data supplies[OV9282_NUM_SUPPLIES];
> > 
> > Please add documentation for supplies.
> 
> Is it the place for the driver to document the supplies beyond the
> comments in ov9282_supply_names with regard to which sensor rail they
> relate to?
> Some drivers include the typical values for each supply, but those are
> technically inaccurate as each will have a min and max value.
> 
> Anyone interfacing with a sensor is going to have the datasheet for it
> and should be referring to that for the characteristics of supply
> rails. Duplicating some of that in the driver seems redundant, and has
> the potential to be incorrect.

What I meant was adding " @supplies: power supply regulators" to the doxygen 
(?) documentation directly above.
I agree that no details about those supplies should be added to driver code 
though.

Alexander

> 
> > >       struct v4l2_ctrl_handler ctrl_handler;
> > >       struct v4l2_ctrl *link_freq_ctrl;
> > >       struct v4l2_ctrl *pclk_ctrl;
> > > 
> > > @@ -767,6 +777,18 @@ static int ov9282_detect(struct ov9282 *ov9282)
> > > 
> > >       return 0;
> > >  
> > >  }
> > > 
> > > +static int ov9282_configure_regulators(struct ov9282 *ov9282)
> > > +{
> > > +     unsigned int i;
> > > +
> > > +     for (i = 0; i < OV9282_NUM_SUPPLIES; i++)
> > > +             ov9282->supplies[i].supply = ov9282_supply_names[i];
> > > +
> > > +     return devm_regulator_bulk_get(ov9282->dev,
> > > +                                    OV9282_NUM_SUPPLIES,
> > > +                                    ov9282->supplies);
> > > +}
> > > +
> > > 
> > >  /**
> > >  
> > >   * ov9282_parse_hw_config() - Parse HW configuration and check if
> > >   supported
> > > 
> > > * @ov9282: pointer to ov9282 device
> > > @@ -803,6 +825,12 @@ static int ov9282_parse_hw_config(struct ov9282
> > > *ov9282) return PTR_ERR(ov9282->inclk);
> > > 
> > >       }
> > > 
> > > +     ret = ov9282_configure_regulators(ov9282);
> > > +     if (ret) {
> > > +             dev_err(ov9282->dev, "Failed to get power regulators\n");
> > 
> > dev_err_probe seems sensible here.
> 
> That would have been good - sorry. I must get into the habit of
> remembering to use dev_err_probe.
> 
>   Dave
> 
> > > +             return ret;
> > > +     }
> > > +
> > > 
> > >       rate = clk_get_rate(ov9282->inclk);
> > >       if (rate != OV9282_INCLK_RATE) {
> > >       
> > >               dev_err(ov9282->dev, "inclk frequency mismatch");
> > > 
> > > @@ -874,6 +902,12 @@ static int ov9282_power_on(struct device *dev)
> > > 
> > >       struct ov9282 *ov9282 = to_ov9282(sd);
> > >       int ret;
> > > 
> > > +     ret = regulator_bulk_enable(OV9282_NUM_SUPPLIES,
> > > ov9282->supplies);
> > > +     if (ret < 0) {
> > > +             dev_err(dev, "Failed to enable regulators\n");
> > > +             return ret;
> > > +     }
> > > +
> > > 
> > >       usleep_range(400, 600);
> > >       
> > >       gpiod_set_value_cansleep(ov9282->reset_gpio, 1);
> > > 
> > > @@ -891,6 +925,8 @@ static int ov9282_power_on(struct device *dev)
> > > 
> > >  error_reset:
> > >       gpiod_set_value_cansleep(ov9282->reset_gpio, 0);
> > > 
> > > +     regulator_bulk_disable(OV9282_NUM_SUPPLIES, ov9282->supplies);
> > > +
> > > 
> > >       return ret;
> > >  
> > >  }
> > > 
> > > @@ -909,6 +945,8 @@ static int ov9282_power_off(struct device *dev)
> > > 
> > >       clk_disable_unprepare(ov9282->inclk);
> > > 
> > > +     regulator_bulk_disable(OV9282_NUM_SUPPLIES, ov9282->supplies);
> > > +
> > > 
> > >       return 0;
> > >  
> > >  }
> > 
> > Despite the nits above
> > Acked-by: Alexander Stein <alexander.stein@ew.tq-group.com>