diff mbox series

[05/21] media: i2c: imx258: Add regulator control

Message ID 20230530173000.3060865-6-dave.stevenson@raspberrypi.com (mailing list archive)
State New, archived
Headers show
Series imx258 improvements series | expand

Commit Message

Dave Stevenson May 30, 2023, 5:29 p.m. UTC 
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@raspberrypi.com>
---
 drivers/media/i2c/imx258.c | 42 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

Comments

Jacopo Mondi May 31, 2023, 3:11 p.m. UTC | #1 
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@raspberrypi.com>
> ---
>  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

> +}
> +
>  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 ?

> +
>  	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
>
Dave Stevenson May 31, 2023, 3:26 p.m. UTC | #2 
Hi Jacopo

On Wed, 31 May 2023 at 16:11, Jacopo Mondi
<jacopo.mondi@ideasonboard.com> 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@raspberrypi.com>
> > ---
> >  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
> >
diff mbox series

Patch

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);
+}
+
 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;
+
 	imx258->clk = devm_clk_get_optional(&client->dev, NULL);
 	if (IS_ERR(imx258->clk))
 		return dev_err_probe(&client->dev, PTR_ERR(imx258->clk),