Re: [PATCH v2 3/8] media: i2c: ov5670: Probe clocks with OF

[Jacopo Mondi <jacopo@jmondi.org>]

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