Hi Jacopo
On Wed, 31 May 2023 at 16:11, Jacopo Mondi
<jacopo.mondi@xxxxxxxxxxxxxxxx> wrote:
>
> Hi Dave
>
> On Tue, May 30, 2023 at 06:29:44PM +0100, Dave Stevenson wrote:
> > The device tree bindings define the relevant regulators for the
> > sensor, so update the driver to request the regulators and control
> > them at the appropriate times.
> >
> > Signed-off-by: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx>
> > ---
> > drivers/media/i2c/imx258.c | 42 +++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 41 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> > index b695fd987b71..30bae7388c3a 100644
> > --- a/drivers/media/i2c/imx258.c
> > +++ b/drivers/media/i2c/imx258.c
> > @@ -7,6 +7,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-device.h>
> > #include <media/v4l2-fwnode.h>
> > @@ -507,6 +508,16 @@ static const char * const imx258_test_pattern_menu[] = {
> > "Pseudorandom Sequence (PN9)",
> > };
> >
> > +/* regulator supplies */
> > +static const char * const imx258_supply_name[] = {
> > + /* Supplies can be enabled in any order */
> > + "vana", /* Analog (2.8V) supply */
> > + "vdig", /* Digital Core (1.2V) supply */
> > + "vif", /* IF (1.8V) supply */
> > +};
> > +
> > +#define IMX258_NUM_SUPPLIES ARRAY_SIZE(imx258_supply_name)
> > +
> > /* Configurations for supported link frequencies */
> > #define IMX258_LINK_FREQ_634MHZ 633600000ULL
> > #define IMX258_LINK_FREQ_320MHZ 320000000ULL
> > @@ -614,6 +625,7 @@ struct imx258 {
> > bool streaming;
> >
> > struct clk *clk;
> > + struct regulator_bulk_data supplies[IMX258_NUM_SUPPLIES];
> > };
> >
> > static inline struct imx258 *to_imx258(struct v4l2_subdev *_sd)
> > @@ -999,9 +1011,19 @@ static int imx258_power_on(struct device *dev)
> > struct imx258 *imx258 = to_imx258(sd);
> > int ret;
> >
> > + ret = regulator_bulk_enable(IMX258_NUM_SUPPLIES,
> > + imx258->supplies);
> > + if (ret) {
> > + dev_err(dev, "%s: failed to enable regulators\n",
> > + __func__);
> > + return ret;
> > + }
> > +
> > ret = clk_prepare_enable(imx258->clk);
> > - if (ret)
> > + if (ret) {
> > dev_err(dev, "failed to enable clock\n");
> > + regulator_bulk_disable(IMX258_NUM_SUPPLIES, imx258->supplies);
> > + }
> >
> > return ret;
> > }
> > @@ -1012,6 +1034,7 @@ static int imx258_power_off(struct device *dev)
> > struct imx258 *imx258 = to_imx258(sd);
> >
> > clk_disable_unprepare(imx258->clk);
> > + regulator_bulk_disable(IMX258_NUM_SUPPLIES, imx258->supplies);
> >
> > return 0;
> > }
> > @@ -1260,6 +1283,19 @@ static void imx258_free_controls(struct imx258 *imx258)
> > mutex_destroy(&imx258->mutex);
> > }
> >
> > +static int imx258_get_regulators(struct imx258 *imx258,
> > + struct i2c_client *client)
> > +{
> > + unsigned int i;
> > +
> > + for (i = 0; i < IMX258_NUM_SUPPLIES; i++)
> > + imx258->supplies[i].supply = imx258_supply_name[i];
> > +
> > + return devm_regulator_bulk_get(&client->dev,
> > + IMX258_NUM_SUPPLIES,
> > + imx258->supplies);
>
> nit: fits on 2 lines
Done
> > +}
> > +
> > static int imx258_probe(struct i2c_client *client)
> > {
> > struct imx258 *imx258;
> > @@ -1270,6 +1306,10 @@ static int imx258_probe(struct i2c_client *client)
> > if (!imx258)
> > return -ENOMEM;
> >
> > + ret = imx258_get_regulators(imx258, client);
> > + if (ret)
> > + return ret;
>
> Is dev_err_probe() useful here ?
Sure, can do.
Dave
> > +
> > imx258->clk = devm_clk_get_optional(&client->dev, NULL);
> > if (IS_ERR(imx258->clk))
> > return dev_err_probe(&client->dev, PTR_ERR(imx258->clk),
> > --
> > 2.25.1
> >
