| Message ID | 20220314162714.153970-4-jacopo@jmondi.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: i2c: ov5670: OF support, runtime_pm, regulators | expand |
Hi Jacopo, Thank you for the patch. On Mon, Mar 14, 2022 at 05:27:09PM +0100, Jacopo Mondi wrote: > Add support for probing the main system clock using the common clock > framework and its OF bindings. > > Maintain ACPI compatibility by falling back to parse 'clock-frequency' > if the no clock device reference is available. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > drivers/media/i2c/ov5670.c | 21 +++++++++++++++++---- > 1 file changed, 17 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c > index 721441024598..25d792794fc7 100644 > --- a/drivers/media/i2c/ov5670.c > +++ b/drivers/media/i2c/ov5670.c > @@ -2,6 +2,7 @@ > // Copyright (c) 2017 Intel Corporation. > > #include <linux/acpi.h> > +#include <linux/clk.h> > #include <linux/i2c.h> > #include <linux/mod_devicetable.h> > #include <linux/module.h> > @@ -1819,6 +1820,8 @@ struct ov5670 { > struct v4l2_subdev sd; > struct media_pad pad; > > + struct clk *clk; > + > struct v4l2_ctrl_handler ctrl_handler; > /* V4L2 Controls */ > struct v4l2_ctrl *link_freq; > @@ -2478,10 +2481,6 @@ static int ov5670_probe(struct i2c_client *client) > bool full_power; > int ret; > > - device_property_read_u32(&client->dev, "clock-frequency", &input_clk); > - if (input_clk != 19200000) > - return -EINVAL; > - > ov5670 = devm_kzalloc(&client->dev, sizeof(*ov5670), GFP_KERNEL); > if (!ov5670) { > ret = -ENOMEM; > @@ -2489,6 +2488,20 @@ static int ov5670_probe(struct i2c_client *client) > goto error_print; > } > > + /* OF uses the common clock framework, ACPI uses "clock-frequency". */ > + ov5670->clk = devm_clk_get_optional(&client->dev, NULL); > + if (IS_ERR(ov5670->clk)) > + return dev_err_probe(&client->dev, PTR_ERR(ov5670->clk), > + "error getting clock\n"); > + > + if (ov5670->clk) > + input_clk = clk_get_rate(ov5670->clk); > + else > + device_property_read_u32(&client->dev, "clock-frequency", > + &input_clk); This will try to use the clock-frequency property on OF-based systems if no clock is specified. Could we instead have if (probed through OF) { use clock } else { use clock-frequency } ? > + if (input_clk != 19200000) > + return -EINVAL; > + > /* Initialize subdev */ > v4l2_i2c_subdev_init(&ov5670->sd, client, &ov5670_subdev_ops); >
Hi Laurent, On Tue, Mar 15, 2022 at 10:11:46AM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Mon, Mar 14, 2022 at 05:27:09PM +0100, Jacopo Mondi wrote: > > Add support for probing the main system clock using the common clock > > framework and its OF bindings. > > > > Maintain ACPI compatibility by falling back to parse 'clock-frequency' > > if the no clock device reference is available. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > drivers/media/i2c/ov5670.c | 21 +++++++++++++++++---- > > 1 file changed, 17 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c > > index 721441024598..25d792794fc7 100644 > > --- a/drivers/media/i2c/ov5670.c > > +++ b/drivers/media/i2c/ov5670.c > > @@ -2,6 +2,7 @@ > > // Copyright (c) 2017 Intel Corporation. > > > > #include <linux/acpi.h> > > +#include <linux/clk.h> > > #include <linux/i2c.h> > > #include <linux/mod_devicetable.h> > > #include <linux/module.h> > > @@ -1819,6 +1820,8 @@ struct ov5670 { > > struct v4l2_subdev sd; > > struct media_pad pad; > > > > + struct clk *clk; > > + > > struct v4l2_ctrl_handler ctrl_handler; > > /* V4L2 Controls */ > > struct v4l2_ctrl *link_freq; > > @@ -2478,10 +2481,6 @@ static int ov5670_probe(struct i2c_client *client) > > bool full_power; > > int ret; > > > > - device_property_read_u32(&client->dev, "clock-frequency", &input_clk); > > - if (input_clk != 19200000) > > - return -EINVAL; > > - > > ov5670 = devm_kzalloc(&client->dev, sizeof(*ov5670), GFP_KERNEL); > > if (!ov5670) { > > ret = -ENOMEM; > > @@ -2489,6 +2488,20 @@ static int ov5670_probe(struct i2c_client *client) > > goto error_print; > > } > > > > + /* OF uses the common clock framework, ACPI uses "clock-frequency". */ > > + ov5670->clk = devm_clk_get_optional(&client->dev, NULL); > > + if (IS_ERR(ov5670->clk)) > > + return dev_err_probe(&client->dev, PTR_ERR(ov5670->clk), > > + "error getting clock\n"); > > + > > + if (ov5670->clk) > > + input_clk = clk_get_rate(ov5670->clk); > > + else > > + device_property_read_u32(&client->dev, "clock-frequency", > > + &input_clk); > > This will try to use the clock-frequency property on OF-based systems if > no clock is specified. Could we instead have > 'clocks' is listed as a required property in the OF bindings and my understanding is that driver assume DTS are correct. > if (probed through OF) { Otherwise yes, I can check for dev->of_node But again, I don't think drivers should have to work-around broken DTS > use clock > } else { > use clock-frequency > } > > ? > > > + if (input_clk != 19200000) > > + return -EINVAL; > > + > > /* Initialize subdev */ > > v4l2_i2c_subdev_init(&ov5670->sd, client, &ov5670_subdev_ops); > > > > -- > Regards, > > Laurent Pinchart
On 15/03/2022 09:46, Jacopo Mondi wrote: > Hi Laurent, > > On Tue, Mar 15, 2022 at 10:11:46AM +0200, Laurent Pinchart wrote: >> Hi Jacopo, >> >> Thank you for the patch. >> >> On Mon, Mar 14, 2022 at 05:27:09PM +0100, Jacopo Mondi wrote: >>> Add support for probing the main system clock using the common clock >>> framework and its OF bindings. >>> >>> Maintain ACPI compatibility by falling back to parse 'clock-frequency' >>> if the no clock device reference is available. >>> >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> >>> --- >>> drivers/media/i2c/ov5670.c | 21 +++++++++++++++++---- >>> 1 file changed, 17 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c >>> index 721441024598..25d792794fc7 100644 >>> --- a/drivers/media/i2c/ov5670.c >>> +++ b/drivers/media/i2c/ov5670.c >>> @@ -2,6 +2,7 @@ >>> // Copyright (c) 2017 Intel Corporation. >>> >>> #include <linux/acpi.h> >>> +#include <linux/clk.h> >>> #include <linux/i2c.h> >>> #include <linux/mod_devicetable.h> >>> #include <linux/module.h> >>> @@ -1819,6 +1820,8 @@ struct ov5670 { >>> struct v4l2_subdev sd; >>> struct media_pad pad; >>> >>> + struct clk *clk; >>> + >>> struct v4l2_ctrl_handler ctrl_handler; >>> /* V4L2 Controls */ >>> struct v4l2_ctrl *link_freq; >>> @@ -2478,10 +2481,6 @@ static int ov5670_probe(struct i2c_client *client) >>> bool full_power; >>> int ret; >>> >>> - device_property_read_u32(&client->dev, "clock-frequency", &input_clk); >>> - if (input_clk != 19200000) >>> - return -EINVAL; >>> - >>> ov5670 = devm_kzalloc(&client->dev, sizeof(*ov5670), GFP_KERNEL); >>> if (!ov5670) { >>> ret = -ENOMEM; >>> @@ -2489,6 +2488,20 @@ static int ov5670_probe(struct i2c_client *client) >>> goto error_print; >>> } >>> >>> + /* OF uses the common clock framework, ACPI uses "clock-frequency". */ >>> + ov5670->clk = devm_clk_get_optional(&client->dev, NULL); >>> + if (IS_ERR(ov5670->clk)) >>> + return dev_err_probe(&client->dev, PTR_ERR(ov5670->clk), >>> + "error getting clock\n"); >>> + >>> + if (ov5670->clk) >>> + input_clk = clk_get_rate(ov5670->clk); >>> + else >>> + device_property_read_u32(&client->dev, "clock-frequency", >>> + &input_clk); >> >> This will try to use the clock-frequency property on OF-based systems if >> no clock is specified. Could we instead have >> > > 'clocks' is listed as a required property in the OF bindings and my > understanding is that driver assume DTS are correct. > >> if (probed through OF) { > > Otherwise yes, I can check for dev->of_node > But again, I don't think drivers should have to work-around broken DTS > Driver should expect broken DTS and handle it. You're introducing new feature (OF support) so you can be strict about DTS and reject the ones which do not provide clock. Best regards, Krzysztof
Hi Jacopo, On Tue, Mar 15, 2022 at 09:46:18AM +0100, Jacopo Mondi wrote: > On Tue, Mar 15, 2022 at 10:11:46AM +0200, Laurent Pinchart wrote: > > On Mon, Mar 14, 2022 at 05:27:09PM +0100, Jacopo Mondi wrote: > > > Add support for probing the main system clock using the common clock > > > framework and its OF bindings. > > > > > > Maintain ACPI compatibility by falling back to parse 'clock-frequency' > > > if the no clock device reference is available. > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > drivers/media/i2c/ov5670.c | 21 +++++++++++++++++---- > > > 1 file changed, 17 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c > > > index 721441024598..25d792794fc7 100644 > > > --- a/drivers/media/i2c/ov5670.c > > > +++ b/drivers/media/i2c/ov5670.c > > > @@ -2,6 +2,7 @@ > > > // Copyright (c) 2017 Intel Corporation. > > > > > > #include <linux/acpi.h> > > > +#include <linux/clk.h> > > > #include <linux/i2c.h> > > > #include <linux/mod_devicetable.h> > > > #include <linux/module.h> > > > @@ -1819,6 +1820,8 @@ struct ov5670 { > > > struct v4l2_subdev sd; > > > struct media_pad pad; > > > > > > + struct clk *clk; > > > + > > > struct v4l2_ctrl_handler ctrl_handler; > > > /* V4L2 Controls */ > > > struct v4l2_ctrl *link_freq; > > > @@ -2478,10 +2481,6 @@ static int ov5670_probe(struct i2c_client *client) > > > bool full_power; > > > int ret; > > > > > > - device_property_read_u32(&client->dev, "clock-frequency", &input_clk); > > > - if (input_clk != 19200000) > > > - return -EINVAL; > > > - > > > ov5670 = devm_kzalloc(&client->dev, sizeof(*ov5670), GFP_KERNEL); > > > if (!ov5670) { > > > ret = -ENOMEM; > > > @@ -2489,6 +2488,20 @@ static int ov5670_probe(struct i2c_client *client) > > > goto error_print; > > > } > > > > > > + /* OF uses the common clock framework, ACPI uses "clock-frequency". */ > > > + ov5670->clk = devm_clk_get_optional(&client->dev, NULL); > > > + if (IS_ERR(ov5670->clk)) > > > + return dev_err_probe(&client->dev, PTR_ERR(ov5670->clk), > > > + "error getting clock\n"); > > > + > > > + if (ov5670->clk) > > > + input_clk = clk_get_rate(ov5670->clk); > > > + else > > > + device_property_read_u32(&client->dev, "clock-frequency", > > > + &input_clk); > > > > This will try to use the clock-frequency property on OF-based systems if > > no clock is specified. Could we instead have > > 'clocks' is listed as a required property in the OF bindings and my > understanding is that driver assume DTS are correct. > > > if (probed through OF) { > > Otherwise yes, I can check for dev->of_node > But again, I don't think drivers should have to work-around broken DTS Not working around, but if we fail when DT is broken, it can help avoiding broken DT spreading in the wild, which we would then need to support forever. > > use clock > > } else { > > use clock-frequency > > } > > > > ? > > > > > + if (input_clk != 19200000) > > > + return -EINVAL; > > > + > > > /* Initialize subdev */ > > > v4l2_i2c_subdev_init(&ov5670->sd, client, &ov5670_subdev_ops); > > >
Hi Laurent, Jacopo, On Tue, Mar 15, 2022 at 10:56:41AM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > On Tue, Mar 15, 2022 at 09:46:18AM +0100, Jacopo Mondi wrote: > > On Tue, Mar 15, 2022 at 10:11:46AM +0200, Laurent Pinchart wrote: > > > On Mon, Mar 14, 2022 at 05:27:09PM +0100, Jacopo Mondi wrote: > > > > Add support for probing the main system clock using the common clock > > > > framework and its OF bindings. > > > > > > > > Maintain ACPI compatibility by falling back to parse 'clock-frequency' > > > > if the no clock device reference is available. > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > --- > > > > drivers/media/i2c/ov5670.c | 21 +++++++++++++++++---- > > > > 1 file changed, 17 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c > > > > index 721441024598..25d792794fc7 100644 > > > > --- a/drivers/media/i2c/ov5670.c > > > > +++ b/drivers/media/i2c/ov5670.c > > > > @@ -2,6 +2,7 @@ > > > > // Copyright (c) 2017 Intel Corporation. > > > > > > > > #include <linux/acpi.h> > > > > +#include <linux/clk.h> > > > > #include <linux/i2c.h> > > > > #include <linux/mod_devicetable.h> > > > > #include <linux/module.h> > > > > @@ -1819,6 +1820,8 @@ struct ov5670 { > > > > struct v4l2_subdev sd; > > > > struct media_pad pad; > > > > > > > > + struct clk *clk; > > > > + > > > > struct v4l2_ctrl_handler ctrl_handler; > > > > /* V4L2 Controls */ > > > > struct v4l2_ctrl *link_freq; > > > > @@ -2478,10 +2481,6 @@ static int ov5670_probe(struct i2c_client *client) > > > > bool full_power; > > > > int ret; > > > > > > > > - device_property_read_u32(&client->dev, "clock-frequency", &input_clk); > > > > - if (input_clk != 19200000) > > > > - return -EINVAL; > > > > - > > > > ov5670 = devm_kzalloc(&client->dev, sizeof(*ov5670), GFP_KERNEL); > > > > if (!ov5670) { > > > > ret = -ENOMEM; > > > > @@ -2489,6 +2488,20 @@ static int ov5670_probe(struct i2c_client *client) > > > > goto error_print; > > > > } > > > > > > > > + /* OF uses the common clock framework, ACPI uses "clock-frequency". */ > > > > + ov5670->clk = devm_clk_get_optional(&client->dev, NULL); > > > > + if (IS_ERR(ov5670->clk)) > > > > + return dev_err_probe(&client->dev, PTR_ERR(ov5670->clk), > > > > + "error getting clock\n"); > > > > + > > > > + if (ov5670->clk) > > > > + input_clk = clk_get_rate(ov5670->clk); > > > > + else > > > > + device_property_read_u32(&client->dev, "clock-frequency", > > > > + &input_clk); > > > > > > This will try to use the clock-frequency property on OF-based systems if > > > no clock is specified. Could we instead have > > > > 'clocks' is listed as a required property in the OF bindings and my > > understanding is that driver assume DTS are correct. > > > > > if (probed through OF) { > > > > Otherwise yes, I can check for dev->of_node > > But again, I don't think drivers should have to work-around broken DTS > > Not working around, but if we fail when DT is broken, it can help > avoiding broken DT spreading in the wild, which we would then need to > support forever. If you think this is necessary (I'm not all that sure), then please check this using is_of_node(). There are other drivers that could take clock-frequency from DT (grep for reading clock-frequency with more indentation than one tab stop), too, even if the bindings don't document that. I wouldn't expect that anyway since clock control won't work there anyway.
diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c index 721441024598..25d792794fc7 100644 --- a/drivers/media/i2c/ov5670.c +++ b/drivers/media/i2c/ov5670.c @@ -2,6 +2,7 @@ // Copyright (c) 2017 Intel Corporation. #include <linux/acpi.h> +#include <linux/clk.h> #include <linux/i2c.h> #include <linux/mod_devicetable.h> #include <linux/module.h> @@ -1819,6 +1820,8 @@ struct ov5670 { struct v4l2_subdev sd; struct media_pad pad; + struct clk *clk; + struct v4l2_ctrl_handler ctrl_handler; /* V4L2 Controls */ struct v4l2_ctrl *link_freq; @@ -2478,10 +2481,6 @@ static int ov5670_probe(struct i2c_client *client) bool full_power; int ret; - device_property_read_u32(&client->dev, "clock-frequency", &input_clk); - if (input_clk != 19200000) - return -EINVAL; - ov5670 = devm_kzalloc(&client->dev, sizeof(*ov5670), GFP_KERNEL); if (!ov5670) { ret = -ENOMEM; @@ -2489,6 +2488,20 @@ static int ov5670_probe(struct i2c_client *client) goto error_print; } + /* OF uses the common clock framework, ACPI uses "clock-frequency". */ + ov5670->clk = devm_clk_get_optional(&client->dev, NULL); + if (IS_ERR(ov5670->clk)) + return dev_err_probe(&client->dev, PTR_ERR(ov5670->clk), + "error getting clock\n"); + + if (ov5670->clk) + input_clk = clk_get_rate(ov5670->clk); + else + device_property_read_u32(&client->dev, "clock-frequency", + &input_clk); + if (input_clk != 19200000) + return -EINVAL; + /* Initialize subdev */ v4l2_i2c_subdev_init(&ov5670->sd, client, &ov5670_subdev_ops);
Add support for probing the main system clock using the common clock framework and its OF bindings. Maintain ACPI compatibility by falling back to parse 'clock-frequency' if the no clock device reference is available. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- drivers/media/i2c/ov5670.c | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-)