[05/21] media: i2c: imx258: Add regulator control
Commit Message
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
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
>
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
> >
@@ -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),