Quoting Umang Jain (2023-10-10 05:06:45)
> Hi Kieran,
>
> Thank you for the patch.
>
> On 10/10/23 6:21 AM, 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
> > + * 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);
>
> Shouldn't this go directly in probe() function ? (Taking IMX219 driver
> as a reference..)
I don't think it needs to no. This is a convenience function called
"imx335_parse_hw_config()" which is called from probe(). It just wraps
all of the interactions with the device-tree/firmware layer, and I think
identifying regulators counts as within that remit.
> > + 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 */
> > +
> > + /* 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 */
>
> It would be help to document this addition of comment in the commit
> message as well.
Yes, I can add a statement saying that "I also extend the comments of the
existing code regarding the power on sequence". T4 in this instance
relates to the entry in the data sheet which specifies how long this
delay should be, and the 'reset_gpio' is known as XCLR in the datasheet.
> >
> > 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;
> > }
>
