[1/1] media: i2c: ov5670: Fix conditions for clock access

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@linux.intel.com>
---
 drivers/media/i2c/ov5670.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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@linux.intel.com>
> ---
>  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.

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)

> --
> 2.30.2
>

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@linux.intel.com>
> > ---
> >  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)
>