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@xxxxxxxxxx>
> > > > ---
> > > > 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.
--
Regards,
Sakari Ailus
