Hi Jacopo,
On Wed, May 03, 2023 at 09:25:37AM +0200, Jacopo Mondi wrote:
> Hi Sakari
>
> On Tue, May 02, 2023 at 03:51:50PM +0300, Sakari Ailus wrote:
> > Leftovers from the earlier fix. Fix also the conditions for reading the
> > clock-frequency property as well as accessing the clock.
> >
> > Fixes: 8df08ba4a331 ("media: ov5670: Fix probe on ACPI")
> > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > ---
> > drivers/media/i2c/ov5670.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
> > index c5e783a06f06c..5437cf32a7b3a 100644
> > --- a/drivers/media/i2c/ov5670.c
> > +++ b/drivers/media/i2c/ov5670.c
> > @@ -2661,9 +2661,9 @@ static int ov5670_probe(struct i2c_client *client)
> > }
> >
> > ov5670->xvclk = devm_clk_get_optional(&client->dev, NULL);
> > - if (!IS_ERR_OR_NULL(ov5670->xvclk))
> > + if (!IS_ERR(ov5670->xvclk))
>
> Can't clk_get_optional() return NULL in the case the clock source is not
> available ?
>
> Also, if !CONFIG_HAVE_CLK devm_clk_get_optional() returns 0.
Correct. And in that case the clock-frequency property should be read
instead.
>
> I would then keep the _OR_NULL() part
>
> > input_clk = clk_get_rate(ov5670->xvclk);
> > - else if (PTR_ERR(ov5670->xvclk) == -ENOENT)
> > + else if (!ov5670->xvclk)
>
> While here it's probably correct, we only want to read
> "clock-frequency" if xvclk == NULL (when either the clock providere is
> not there, or !CONFIG_HAVE_CLK)
>
> > device_property_read_u32(&client->dev, "clock-frequency",
> > &input_clk);
> > else
>
> Everything else is an error (in example, -EPROBE_DEFER)
>
--
Regards,
Sakari Ailus
