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