Quoting Sakari Ailus (2023-10-10 07:12:08)
> Hi Kieran,
>
> On Tue, Oct 10, 2023 at 01:51:23AM +0100, Kieran Bingham wrote:
> > Provide support for enabling and disabling regulator supplies to control
> > power to the camera sensor.
> >
> > Signed-off-by: Kieran Bingham <kieran.bingham@xxxxxxxxxxxxxxxx>
> > ---
> > drivers/media/i2c/imx335.c | 41 ++++++++++++++++++++++++++++++++++++--
> > 1 file changed, 39 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
> > index ec729126274b..bf12b9b69fce 100644
> > --- a/drivers/media/i2c/imx335.c
> > +++ b/drivers/media/i2c/imx335.c
> > @@ -75,6 +75,19 @@ struct imx335_reg_list {
> > const struct imx335_reg *regs;
> > };
> >
> > +static const char * const imx335_supply_name[] = {
> > + /*
> > + * Turn on the power supplies so that they rise in order of
> > + * 1.2v,-> 1.8 -> 2.9v
>
> This won't happen with regulator_bulk_enable(). Does the spec require this?
Every camera I've ever seen handles this in hardware. (I know that's not
an excuse as somewhere there could be a device that routes each of these
separately).
Perhaps I shouldn't have added the comment ;-) But I thought it was
useful while I was working through anyway, and could be important for
other devices indeed.
The datasheet states:
```
1. Turn On the power supplies so that the power supplies rise in order
of 1.2 V power supply (DVDD1, DVDD2) → 1.8 V power supply (OVDD) → 2.9 V
power supply (AVDD1, AVDD2). In addition, all power supplies should
finish rising within 200 ms.
2. The register values are undefined immediately after power-on, so the
system must be cleared. Hold XCLR at Low level for 500 ns or more after
all the power supplies have finished rising. (The register values after
a system clear are the default values.)
3. The system clear is applied by setting XCLR to High level. The maser
clock input after setting the XCLR pin to High level.
4. Make the sensor setting by register communication after the system clear.
```
Regarding 1: T0, and T1 minimum values are '0', so they can all be
enabled at the same time - but of course there will be 'some' interval
between each one. I don't know if that still stipulates the exact
ordering is essential. Perhaps it does.
I don't know how far splitting this out becomes overengineering. Are
there other sensor drivers that already split out each regulator line
with a dedicated ordering? If so - this probably calls for some sort of
ordered bulk regulator enable helper.
> > + * All power supplies should finish rising within 200ms.
> > + */
> > + "avdd", /* Analog (2.9V) supply */
> > + "ovdd", /* Digital I/O (1.8V) supply */
> > + "dvdd", /* Digital Core (1.2V) supply */
> > +};
> > +
> > +#define IMX335_NUM_SUPPLIES ARRAY_SIZE(imx335_supply_name)
> > +
> > /**
> > * struct imx335_mode - imx335 sensor mode structure
> > * @width: Frame width
> > @@ -126,6 +139,8 @@ struct imx335 {
> > struct v4l2_subdev sd;
> > struct media_pad pad;
> > struct gpio_desc *reset_gpio;
> > + struct regulator_bulk_data supplies[IMX335_NUM_SUPPLIES];
> > +
> > struct clk *inclk;
> > struct v4l2_ctrl_handler ctrl_handler;
> > struct v4l2_ctrl *link_freq_ctrl;
> > @@ -781,6 +796,17 @@ static int imx335_parse_hw_config(struct imx335 *imx335)
> > return PTR_ERR(imx335->reset_gpio);
> > }
> >
> > + for (i = 0; i < IMX335_NUM_SUPPLIES; i++)
> > + imx335->supplies[i].supply = imx335_supply_name[i];
> > +
> > + ret = devm_regulator_bulk_get(imx335->dev,
> > + IMX335_NUM_SUPPLIES,
> > + imx335->supplies);
> > + if (ret) {
> > + dev_err(imx335->dev, "Failed to get regulators\n");
> > + return ret;
> > + }
> > +
> > /* Get sensor input clock */
> > imx335->inclk = devm_clk_get(imx335->dev, NULL);
> > if (IS_ERR(imx335->inclk)) {
> > @@ -859,6 +885,17 @@ static int imx335_power_on(struct device *dev)
> > struct imx335 *imx335 = to_imx335(sd);
> > int ret;
> >
> > + ret = regulator_bulk_enable(IMX335_NUM_SUPPLIES,
> > + imx335->supplies);
> > + if (ret) {
> > + dev_err(dev, "%s: failed to enable regulators\n",
> > + __func__);
> > + return ret;
> > + }
> > +
> > + usleep_range(500, 550); /* Tlow */
>
> You're not handling the error case later on in the function.
Ah yes - thanks. I'll fix this.
--
Kieran
>
> > +
> > + /* Set XCLR */
> > gpiod_set_value_cansleep(imx335->reset_gpio, 1);
> >
> > ret = clk_prepare_enable(imx335->inclk);
> > @@ -867,7 +904,7 @@ static int imx335_power_on(struct device *dev)
> > goto error_reset;
> > }
> >
> > - usleep_range(20, 22);
> > + usleep_range(20, 22); /* T4 */
> >
> > return 0;
> >
> > @@ -889,8 +926,8 @@ static int imx335_power_off(struct device *dev)
> > struct imx335 *imx335 = to_imx335(sd);
> >
> > gpiod_set_value_cansleep(imx335->reset_gpio, 0);
> > -
> > clk_disable_unprepare(imx335->inclk);
> > + regulator_bulk_disable(IMX335_NUM_SUPPLIES, imx335->supplies);
> >
> > return 0;
> > }
>
> --
> Regards,
>
> Sakari Ailus
