Re: [PATCH v2 3/3] media: imx258: Get clock from device properties and enable it via runtime PM
On Wed, Sep 02, 2020 at 10:39:35AM +0300, Sakari Ailus wrote:
> Hi Krzysztof,
>
> Thanks for the update.
>
> On Wed, Sep 02, 2020 at 09:18:10AM +0200, Krzysztof Kozlowski wrote:
> > The IMX258 sensor driver checked in device properties for a
> > clock-frequency property which actually does not mean that the clock is
> > really running such frequency or is it even enabled.
> >
> > Get the provided clock and check it frequency. If none is provided,
> > fall back to old property.
> >
> > Enable the clock when accessing the IMX258 registers and when streaming
> > starts with runtime PM.
> >
> > Signed-off-by: Krzysztof Kozlowski
> >
> > ---
> >
> > Changes since v1:
> > 1. Use runtime PM for clock toggling
> > ---
> > drivers/media/i2c/imx258.c | 68
> > --
> > 1 file changed, 59 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> > index c20bac9b00ec..ee38dafb8450 100644
> > --- a/drivers/media/i2c/imx258.c
> > +++ b/drivers/media/i2c/imx258.c
> > @@ -2,6 +2,7 @@
> > // Copyright (C) 2018 Intel Corporation
> >
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -68,6 +69,9 @@
> > #define REG_CONFIG_MIRROR_FLIP 0x03
> > #define REG_CONFIG_FLIP_TEST_PATTERN 0x02
> >
> > +/* Input clock frequency in Hz */
> > +#define IMX258_INPUT_CLOCK_FREQ1920
> > +
> > struct imx258_reg {
> > u16 address;
> > u8 val;
> > @@ -610,6 +614,8 @@ struct imx258 {
> >
> > /* Streaming on/off */
> > bool streaming;
> > +
> > + struct clk *clk;
> > };
> >
> > static inline struct imx258 *to_imx258(struct v4l2_subdev *_sd)
> > @@ -972,6 +978,27 @@ static int imx258_stop_streaming(struct imx258 *imx258)
> > return 0;
> > }
> >
> > +static int imx258_power_on(struct device *dev)
> > +{
> > + struct imx258 *imx258 = dev_get_drvdata(dev);
> > + int ret;
> > +
> > + ret = clk_prepare_enable(imx258->clk);
> > + if (ret)
> > + dev_err(dev, "failed to enable clock\n");
> > +
> > + return ret;
> > +}
> > +
> > +static int imx258_power_off(struct device *dev)
> > +{
> > + struct imx258 *imx258 = dev_get_drvdata(dev);
> > +
> > + clk_disable_unprepare(imx258->clk);
> > +
> > + return 0;
> > +}
> > +
> > static int imx258_set_stream(struct v4l2_subdev *sd, int enable)
> > {
> > struct imx258 *imx258 = to_imx258(sd);
> > @@ -1201,9 +1228,27 @@ static int imx258_probe(struct i2c_client *client)
> > int ret;
> > u32 val = 0;
> >
> > - device_property_read_u32(>dev, "clock-frequency", );
> > - if (val != 1920)
> > - return -EINVAL;
> > + imx258 = devm_kzalloc(>dev, sizeof(*imx258), GFP_KERNEL);
> > + if (!imx258)
> > + return -ENOMEM;
> > +
> > + dev_set_drvdata(>dev, imx258);
>
> This you cannot do --- it'll be overwritten by v4l2_i2c_subdev_init().
Right, thanks.
>
> > +
> > + imx258->clk = devm_clk_get_optional(>dev, NULL);
> > + if (!imx258->clk) {
>
> You can move declaration of val here (I think).
No, the val is used later in further device_property_read* calls.
>
> > + dev_info(>dev, "no clock provided, using
> > clock-frequency property\n");
>
> As this is showing up on all ACPI based systems, I guess dev_dbg() would be
> more appropriate.
Sure, I'll make it debug.
>
> Please also wrap lines over 80 if they reasonably can be.
OK
Thanks for the review.
Best regards,
Krzysztof
Re: [PATCH v2 3/3] media: imx258: Get clock from device properties and enable it via runtime PM
Hi Krzysztof,
Thanks for the update.
On Wed, Sep 02, 2020 at 09:18:10AM +0200, Krzysztof Kozlowski wrote:
> The IMX258 sensor driver checked in device properties for a
> clock-frequency property which actually does not mean that the clock is
> really running such frequency or is it even enabled.
>
> Get the provided clock and check it frequency. If none is provided,
> fall back to old property.
>
> Enable the clock when accessing the IMX258 registers and when streaming
> starts with runtime PM.
>
> Signed-off-by: Krzysztof Kozlowski
>
> ---
>
> Changes since v1:
> 1. Use runtime PM for clock toggling
> ---
> drivers/media/i2c/imx258.c | 68
> --
> 1 file changed, 59 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> index c20bac9b00ec..ee38dafb8450 100644
> --- a/drivers/media/i2c/imx258.c
> +++ b/drivers/media/i2c/imx258.c
> @@ -2,6 +2,7 @@
> // Copyright (C) 2018 Intel Corporation
>
> #include
> +#include
> #include
> #include
> #include
> @@ -68,6 +69,9 @@
> #define REG_CONFIG_MIRROR_FLIP 0x03
> #define REG_CONFIG_FLIP_TEST_PATTERN 0x02
>
> +/* Input clock frequency in Hz */
> +#define IMX258_INPUT_CLOCK_FREQ 1920
> +
> struct imx258_reg {
> u16 address;
> u8 val;
> @@ -610,6 +614,8 @@ struct imx258 {
>
> /* Streaming on/off */
> bool streaming;
> +
> + struct clk *clk;
> };
>
> static inline struct imx258 *to_imx258(struct v4l2_subdev *_sd)
> @@ -972,6 +978,27 @@ static int imx258_stop_streaming(struct imx258 *imx258)
> return 0;
> }
>
> +static int imx258_power_on(struct device *dev)
> +{
> + struct imx258 *imx258 = dev_get_drvdata(dev);
> + int ret;
> +
> + ret = clk_prepare_enable(imx258->clk);
> + if (ret)
> + dev_err(dev, "failed to enable clock\n");
> +
> + return ret;
> +}
> +
> +static int imx258_power_off(struct device *dev)
> +{
> + struct imx258 *imx258 = dev_get_drvdata(dev);
> +
> + clk_disable_unprepare(imx258->clk);
> +
> + return 0;
> +}
> +
> static int imx258_set_stream(struct v4l2_subdev *sd, int enable)
> {
> struct imx258 *imx258 = to_imx258(sd);
> @@ -1201,9 +1228,27 @@ static int imx258_probe(struct i2c_client *client)
> int ret;
> u32 val = 0;
>
> - device_property_read_u32(>dev, "clock-frequency", );
> - if (val != 1920)
> - return -EINVAL;
> + imx258 = devm_kzalloc(>dev, sizeof(*imx258), GFP_KERNEL);
> + if (!imx258)
> + return -ENOMEM;
> +
> + dev_set_drvdata(>dev, imx258);
This you cannot do --- it'll be overwritten by v4l2_i2c_subdev_init().
> +
> + imx258->clk = devm_clk_get_optional(>dev, NULL);
> + if (!imx258->clk) {
You can move declaration of val here (I think).
> + dev_info(>dev, "no clock provided, using
> clock-frequency property\n");
As this is showing up on all ACPI based systems, I guess dev_dbg() would be
more appropriate.
Please also wrap lines over 80 if they reasonably can be.
> +
> + device_property_read_u32(>dev, "clock-frequency", );
> + if (val != IMX258_INPUT_CLOCK_FREQ)
> + return -EINVAL;
> + } else if (IS_ERR(imx258->clk)) {
> + return dev_err_probe(>dev, PTR_ERR(imx258->clk), "error
> getting clock\n");
> + } else {
> + if (clk_get_rate(imx258->clk) != IMX258_INPUT_CLOCK_FREQ) {
> + dev_err(>dev, "input clock frequency not
> supported\n");
> + return -EINVAL;
> + }
> + }
>
> /*
>* Check that the device is mounted upside down. The driver only
> @@ -1213,24 +1258,25 @@ static int imx258_probe(struct i2c_client *client)
> if (ret || val != 180)
> return -EINVAL;
>
> - imx258 = devm_kzalloc(>dev, sizeof(*imx258), GFP_KERNEL);
> - if (!imx258)
> - return -ENOMEM;
> -
> /* Initialize subdev */
> v4l2_i2c_subdev_init(>sd, client, _subdev_ops);
>
> + /* Will be powered off via pm_runtime_idle */
> + ret = imx258_power_on(>dev);
> + if (ret)
> + return ret;
> +
> /* Check module identity */
> ret = imx258_identify_module(imx258);
> if (ret)
> - return ret;
> + goto error_identify;
>
> /* Set default mode to max resolution */
> imx258->cur_mode = _modes[0];
>
> ret = imx258_init_controls(imx258);
> if (ret)
> - return ret;
> + goto error_identify;
>
> /* Initialize subdev */
> imx258->sd.internal_ops = _internal_ops;
> @@ -1260,6 +1306,9 @@ static int imx258_probe(struct i2c_client *client)
> error_handler_free:
> imx258_free_controls(imx258);
>
> +error_identify:
> + imx258_power_off(>dev);
> +
> return ret;
> }
>
> @@ -1280,6 +1329,7 @@ static int
[PATCH v2 3/3] media: imx258: Get clock from device properties and enable it via runtime PM
The IMX258 sensor driver checked in device properties for a
clock-frequency property which actually does not mean that the clock is
really running such frequency or is it even enabled.
Get the provided clock and check it frequency. If none is provided,
fall back to old property.
Enable the clock when accessing the IMX258 registers and when streaming
starts with runtime PM.
Signed-off-by: Krzysztof Kozlowski
---
Changes since v1:
1. Use runtime PM for clock toggling
---
drivers/media/i2c/imx258.c | 68 --
1 file changed, 59 insertions(+), 9 deletions(-)
diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
index c20bac9b00ec..ee38dafb8450 100644
--- a/drivers/media/i2c/imx258.c
+++ b/drivers/media/i2c/imx258.c
@@ -2,6 +2,7 @@
// Copyright (C) 2018 Intel Corporation
#include
+#include
#include
#include
#include
@@ -68,6 +69,9 @@
#define REG_CONFIG_MIRROR_FLIP 0x03
#define REG_CONFIG_FLIP_TEST_PATTERN 0x02
+/* Input clock frequency in Hz */
+#define IMX258_INPUT_CLOCK_FREQ1920
+
struct imx258_reg {
u16 address;
u8 val;
@@ -610,6 +614,8 @@ struct imx258 {
/* Streaming on/off */
bool streaming;
+
+ struct clk *clk;
};
static inline struct imx258 *to_imx258(struct v4l2_subdev *_sd)
@@ -972,6 +978,27 @@ static int imx258_stop_streaming(struct imx258 *imx258)
return 0;
}
+static int imx258_power_on(struct device *dev)
+{
+ struct imx258 *imx258 = dev_get_drvdata(dev);
+ int ret;
+
+ ret = clk_prepare_enable(imx258->clk);
+ if (ret)
+ dev_err(dev, "failed to enable clock\n");
+
+ return ret;
+}
+
+static int imx258_power_off(struct device *dev)
+{
+ struct imx258 *imx258 = dev_get_drvdata(dev);
+
+ clk_disable_unprepare(imx258->clk);
+
+ return 0;
+}
+
static int imx258_set_stream(struct v4l2_subdev *sd, int enable)
{
struct imx258 *imx258 = to_imx258(sd);
@@ -1201,9 +1228,27 @@ static int imx258_probe(struct i2c_client *client)
int ret;
u32 val = 0;
- device_property_read_u32(>dev, "clock-frequency", );
- if (val != 1920)
- return -EINVAL;
+ imx258 = devm_kzalloc(>dev, sizeof(*imx258), GFP_KERNEL);
+ if (!imx258)
+ return -ENOMEM;
+
+ dev_set_drvdata(>dev, imx258);
+
+ imx258->clk = devm_clk_get_optional(>dev, NULL);
+ if (!imx258->clk) {
+ dev_info(>dev, "no clock provided, using
clock-frequency property\n");
+
+ device_property_read_u32(>dev, "clock-frequency", );
+ if (val != IMX258_INPUT_CLOCK_FREQ)
+ return -EINVAL;
+ } else if (IS_ERR(imx258->clk)) {
+ return dev_err_probe(>dev, PTR_ERR(imx258->clk), "error
getting clock\n");
+ } else {
+ if (clk_get_rate(imx258->clk) != IMX258_INPUT_CLOCK_FREQ) {
+ dev_err(>dev, "input clock frequency not
supported\n");
+ return -EINVAL;
+ }
+ }
/*
* Check that the device is mounted upside down. The driver only
@@ -1213,24 +1258,25 @@ static int imx258_probe(struct i2c_client *client)
if (ret || val != 180)
return -EINVAL;
- imx258 = devm_kzalloc(>dev, sizeof(*imx258), GFP_KERNEL);
- if (!imx258)
- return -ENOMEM;
-
/* Initialize subdev */
v4l2_i2c_subdev_init(>sd, client, _subdev_ops);
+ /* Will be powered off via pm_runtime_idle */
+ ret = imx258_power_on(>dev);
+ if (ret)
+ return ret;
+
/* Check module identity */
ret = imx258_identify_module(imx258);
if (ret)
- return ret;
+ goto error_identify;
/* Set default mode to max resolution */
imx258->cur_mode = _modes[0];
ret = imx258_init_controls(imx258);
if (ret)
- return ret;
+ goto error_identify;
/* Initialize subdev */
imx258->sd.internal_ops = _internal_ops;
@@ -1260,6 +1306,9 @@ static int imx258_probe(struct i2c_client *client)
error_handler_free:
imx258_free_controls(imx258);
+error_identify:
+ imx258_power_off(>dev);
+
return ret;
}
@@ -1280,6 +1329,7 @@ static int imx258_remove(struct i2c_client *client)
static const struct dev_pm_ops imx258_pm_ops = {
SET_SYSTEM_SLEEP_PM_OPS(imx258_suspend, imx258_resume)
+ SET_RUNTIME_PM_OPS(imx258_power_off, imx258_power_on, NULL)
};
#ifdef CONFIG_ACPI
--
2.7.4
