| Message ID | 20221005152018.3783890-3-dave.stevenson@raspberrypi.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: Add regulator support to ov9282 | expand |
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. > 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. > + 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>
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 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. > > 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>
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>
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]; 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"); + 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; }
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(+)