[PATCH] media: i2c: imx219: Add support for 'clock-frequency' parsing

[PATCH] media: i2c: imx219: Add support for 'clock-frequency' parsing

[Mehdi Djait]

Make the clock producer reference lookup optional

Add support for ACPI-based platforms by parsing the 'clock-frequency'
property when no clock producer is available

Signed-off-by: Mehdi Djait <mehdi.djait@linux.intel.com>
---
 drivers/media/i2c/imx219.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index 2d54cea113e1..a876a6d80a47 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -1103,12 +1103,22 @@ static int imx219_probe(struct i2c_client *client)
 				     "failed to initialize CCI\n");
 
 	/* Get system clock (xclk) */
-	imx219->xclk = devm_clk_get(dev, NULL);
+	imx219->xclk = devm_clk_get_optional(dev, NULL);
 	if (IS_ERR(imx219->xclk))
 		return dev_err_probe(dev, PTR_ERR(imx219->xclk),
 				     "failed to get xclk\n");
 
-	imx219->xclk_freq = clk_get_rate(imx219->xclk);
+	if (imx219->xclk) {
+		imx219->xclk_freq = clk_get_rate(imx219->xclk);
+	} else {
+		ret = fwnode_property_read_u32(dev_fwnode(dev),
+					       "clock-frequency",
+					       &imx219->xclk_freq);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "failed to get clock frequency");
+	}
+
 	if (imx219->xclk_freq != IMX219_XCLK_FREQ)
 		return dev_err_probe(dev, -EINVAL,
 				     "xclk frequency not supported: %d Hz\n",
-- 
2.48.1

Re: [PATCH] media: i2c: imx219: Add support for 'clock-frequency' parsing

[Laurent Pinchart]

Hi Mehdi,

Thank you for the patch.

On Thu, Feb 20, 2025 at 04:49:09PM +0100, Mehdi Djait wrote:
> Make the clock producer reference lookup optional
> 
> Add support for ACPI-based platforms by parsing the 'clock-frequency'
> property when no clock producer is available
> 
> Signed-off-by: Mehdi Djait <mehdi.djait@linux.intel.com>
> ---
>  drivers/media/i2c/imx219.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index 2d54cea113e1..a876a6d80a47 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -1103,12 +1103,22 @@ static int imx219_probe(struct i2c_client *client)
>  				     "failed to initialize CCI\n");
>  
>  	/* Get system clock (xclk) */
> -	imx219->xclk = devm_clk_get(dev, NULL);
> +	imx219->xclk = devm_clk_get_optional(dev, NULL);
>  	if (IS_ERR(imx219->xclk))
>  		return dev_err_probe(dev, PTR_ERR(imx219->xclk),
>  				     "failed to get xclk\n");
>  
> -	imx219->xclk_freq = clk_get_rate(imx219->xclk);
> +	if (imx219->xclk) {
> +		imx219->xclk_freq = clk_get_rate(imx219->xclk);
> +	} else {
> +		ret = fwnode_property_read_u32(dev_fwnode(dev),
> +					       "clock-frequency",
> +					       &imx219->xclk_freq);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "failed to get clock frequency");
> +	}
> +

This doesn't seem specific to the imx219 driver. Could you turn this
into a generic V4L2 sensor helper that would take a struct device and a
clock name, and return the frequency, either retrieved from the clock,
or from the clock-frequency property as a fallback ?

Some drivers will also need to control the clock, so the clock should
probably be returned too.

>  	if (imx219->xclk_freq != IMX219_XCLK_FREQ)
>  		return dev_err_probe(dev, -EINVAL,
>  				     "xclk frequency not supported: %d Hz\n",

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: i2c: imx219: Add support for 'clock-frequency' parsing

[Mehdi Djait]

Hi Laurent,

On Mon, Feb 24, 2025 at 01:06:49AM +0200, Laurent Pinchart wrote:
> Hi Mehdi,
> 
> Thank you for the patch.
> 
> On Thu, Feb 20, 2025 at 04:49:09PM +0100, Mehdi Djait wrote:
> > Make the clock producer reference lookup optional
> > 
> > Add support for ACPI-based platforms by parsing the 'clock-frequency'
> > property when no clock producer is available
> > 
> > Signed-off-by: Mehdi Djait <mehdi.djait@linux.intel.com>
> > ---
> >  drivers/media/i2c/imx219.c | 14 ++++++++++++--
> >  1 file changed, 12 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > index 2d54cea113e1..a876a6d80a47 100644
> > --- a/drivers/media/i2c/imx219.c
> > +++ b/drivers/media/i2c/imx219.c
> > @@ -1103,12 +1103,22 @@ static int imx219_probe(struct i2c_client *client)
> >  				     "failed to initialize CCI\n");
> >  
> >  	/* Get system clock (xclk) */
> > -	imx219->xclk = devm_clk_get(dev, NULL);
> > +	imx219->xclk = devm_clk_get_optional(dev, NULL);
> >  	if (IS_ERR(imx219->xclk))
> >  		return dev_err_probe(dev, PTR_ERR(imx219->xclk),
> >  				     "failed to get xclk\n");
> >  
> > -	imx219->xclk_freq = clk_get_rate(imx219->xclk);
> > +	if (imx219->xclk) {
> > +		imx219->xclk_freq = clk_get_rate(imx219->xclk);
> > +	} else {
> > +		ret = fwnode_property_read_u32(dev_fwnode(dev),
> > +					       "clock-frequency",
> > +					       &imx219->xclk_freq);
> > +		if (ret)
> > +			return dev_err_probe(dev, ret,
> > +					     "failed to get clock frequency");
> > +	}
> > +
> 
> This doesn't seem specific to the imx219 driver. Could you turn this
> into a generic V4L2 sensor helper that would take a struct device and a
> clock name, and return the frequency, either retrieved from the clock,
> or from the clock-frequency property as a fallback ?
> 
> Some drivers will also need to control the clock, so the clock should
> probably be returned too.
> 

Yes, I saw that many sensor drivers have the same issue.

I will try to make it into a generic V4L2 helper and send the patches.

--
Kind Regards
Mehdi Djait

Re: [PATCH] media: i2c: imx219: Add support for 'clock-frequency' parsing

[Sakari Ailus]

Hi Mehdi,

On Mon, Feb 24, 2025 at 08:59:34AM +0100, Mehdi Djait wrote:
> Hi Laurent,
> 
> On Mon, Feb 24, 2025 at 01:06:49AM +0200, Laurent Pinchart wrote:
> > Hi Mehdi,
> > 
> > Thank you for the patch.
> > 
> > On Thu, Feb 20, 2025 at 04:49:09PM +0100, Mehdi Djait wrote:
> > > Make the clock producer reference lookup optional
> > > 
> > > Add support for ACPI-based platforms by parsing the 'clock-frequency'
> > > property when no clock producer is available
> > > 
> > > Signed-off-by: Mehdi Djait <mehdi.djait@linux.intel.com>
> > > ---
> > >  drivers/media/i2c/imx219.c | 14 ++++++++++++--
> > >  1 file changed, 12 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > > index 2d54cea113e1..a876a6d80a47 100644
> > > --- a/drivers/media/i2c/imx219.c
> > > +++ b/drivers/media/i2c/imx219.c
> > > @@ -1103,12 +1103,22 @@ static int imx219_probe(struct i2c_client *client)
> > >  				     "failed to initialize CCI\n");
> > >  
> > >  	/* Get system clock (xclk) */
> > > -	imx219->xclk = devm_clk_get(dev, NULL);
> > > +	imx219->xclk = devm_clk_get_optional(dev, NULL);
> > >  	if (IS_ERR(imx219->xclk))
> > >  		return dev_err_probe(dev, PTR_ERR(imx219->xclk),
> > >  				     "failed to get xclk\n");
> > >  
> > > -	imx219->xclk_freq = clk_get_rate(imx219->xclk);
> > > +	if (imx219->xclk) {
> > > +		imx219->xclk_freq = clk_get_rate(imx219->xclk);
> > > +	} else {
> > > +		ret = fwnode_property_read_u32(dev_fwnode(dev),
> > > +					       "clock-frequency",
> > > +					       &imx219->xclk_freq);
> > > +		if (ret)
> > > +			return dev_err_probe(dev, ret,
> > > +					     "failed to get clock frequency");
> > > +	}
> > > +
> > 
> > This doesn't seem specific to the imx219 driver. Could you turn this
> > into a generic V4L2 sensor helper that would take a struct device and a
> > clock name, and return the frequency, either retrieved from the clock,
> > or from the clock-frequency property as a fallback ?
> > 
> > Some drivers will also need to control the clock, so the clock should
> > probably be returned too.
> > 
> 
> Yes, I saw that many sensor drivers have the same issue.
> 
> I will try to make it into a generic V4L2 helper and send the patches.

There are other such functions in drivers/media/v4l2-core/v4l2-common.c.
Perhaps this is where the new helper could be located as well?

-- 
Regards,

Sakari Ailus

Re: [PATCH] media: i2c: imx219: Add support for 'clock-frequency' parsing

[Mehdi Djait]

Hi Sakari,

On Mon, Feb 24, 2025 at 09:42:12AM +0000, Sakari Ailus wrote:
> Hi Mehdi,
> 
> On Mon, Feb 24, 2025 at 08:59:34AM +0100, Mehdi Djait wrote:
> > Hi Laurent,
> > 
> > On Mon, Feb 24, 2025 at 01:06:49AM +0200, Laurent Pinchart wrote:
> > > Hi Mehdi,
> > > 
> > > Thank you for the patch.
> > > 
> > > On Thu, Feb 20, 2025 at 04:49:09PM +0100, Mehdi Djait wrote:
> > > > Make the clock producer reference lookup optional
> > > > 
> > > > Add support for ACPI-based platforms by parsing the 'clock-frequency'
> > > > property when no clock producer is available
> > > > 
> > > > Signed-off-by: Mehdi Djait <mehdi.djait@linux.intel.com>
> > > > ---
> > > >  drivers/media/i2c/imx219.c | 14 ++++++++++++--
> > > >  1 file changed, 12 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > > > index 2d54cea113e1..a876a6d80a47 100644
> > > > --- a/drivers/media/i2c/imx219.c
> > > > +++ b/drivers/media/i2c/imx219.c
> > > > @@ -1103,12 +1103,22 @@ static int imx219_probe(struct i2c_client *client)
> > > >  				     "failed to initialize CCI\n");
> > > >  
> > > >  	/* Get system clock (xclk) */
> > > > -	imx219->xclk = devm_clk_get(dev, NULL);
> > > > +	imx219->xclk = devm_clk_get_optional(dev, NULL);
> > > >  	if (IS_ERR(imx219->xclk))
> > > >  		return dev_err_probe(dev, PTR_ERR(imx219->xclk),
> > > >  				     "failed to get xclk\n");
> > > >  
> > > > -	imx219->xclk_freq = clk_get_rate(imx219->xclk);
> > > > +	if (imx219->xclk) {
> > > > +		imx219->xclk_freq = clk_get_rate(imx219->xclk);
> > > > +	} else {
> > > > +		ret = fwnode_property_read_u32(dev_fwnode(dev),
> > > > +					       "clock-frequency",
> > > > +					       &imx219->xclk_freq);
> > > > +		if (ret)
> > > > +			return dev_err_probe(dev, ret,
> > > > +					     "failed to get clock frequency");
> > > > +	}
> > > > +
> > > 
> > > This doesn't seem specific to the imx219 driver. Could you turn this
> > > into a generic V4L2 sensor helper that would take a struct device and a
> > > clock name, and return the frequency, either retrieved from the clock,
> > > or from the clock-frequency property as a fallback ?
> > > 
> > > Some drivers will also need to control the clock, so the clock should
> > > probably be returned too.
> > > 
> > 
> > Yes, I saw that many sensor drivers have the same issue.
> > 
> > I will try to make it into a generic V4L2 helper and send the patches.
> 
> There are other such functions in drivers/media/v4l2-core/v4l2-common.c.
> Perhaps this is where the new helper could be located as well?
> 

I was thinking about drivers/media/v4l2-core/v4l2-fwnode.c but if
v4l2-common.c is more appropriate we can go with that.

--
Kind Regards
Mehdi Djait

Re: [PATCH] media: i2c: imx219: Add support for 'clock-frequency' parsing

[Sakari Ailus]

Hi Mehdi,

On Mon, Feb 24, 2025 at 12:54:16PM +0100, Mehdi Djait wrote:
> Hi Sakari,
> 
> On Mon, Feb 24, 2025 at 09:42:12AM +0000, Sakari Ailus wrote:
> > Hi Mehdi,
> > 
> > On Mon, Feb 24, 2025 at 08:59:34AM +0100, Mehdi Djait wrote:
> > > Hi Laurent,
> > > 
> > > On Mon, Feb 24, 2025 at 01:06:49AM +0200, Laurent Pinchart wrote:
> > > > Hi Mehdi,
> > > > 
> > > > Thank you for the patch.
> > > > 
> > > > On Thu, Feb 20, 2025 at 04:49:09PM +0100, Mehdi Djait wrote:
> > > > > Make the clock producer reference lookup optional
> > > > > 
> > > > > Add support for ACPI-based platforms by parsing the 'clock-frequency'
> > > > > property when no clock producer is available
> > > > > 
> > > > > Signed-off-by: Mehdi Djait <mehdi.djait@linux.intel.com>
> > > > > ---
> > > > >  drivers/media/i2c/imx219.c | 14 ++++++++++++--
> > > > >  1 file changed, 12 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > > > > index 2d54cea113e1..a876a6d80a47 100644
> > > > > --- a/drivers/media/i2c/imx219.c
> > > > > +++ b/drivers/media/i2c/imx219.c
> > > > > @@ -1103,12 +1103,22 @@ static int imx219_probe(struct i2c_client *client)
> > > > >  				     "failed to initialize CCI\n");
> > > > >  
> > > > >  	/* Get system clock (xclk) */
> > > > > -	imx219->xclk = devm_clk_get(dev, NULL);
> > > > > +	imx219->xclk = devm_clk_get_optional(dev, NULL);
> > > > >  	if (IS_ERR(imx219->xclk))
> > > > >  		return dev_err_probe(dev, PTR_ERR(imx219->xclk),
> > > > >  				     "failed to get xclk\n");
> > > > >  
> > > > > -	imx219->xclk_freq = clk_get_rate(imx219->xclk);
> > > > > +	if (imx219->xclk) {
> > > > > +		imx219->xclk_freq = clk_get_rate(imx219->xclk);
> > > > > +	} else {
> > > > > +		ret = fwnode_property_read_u32(dev_fwnode(dev),
> > > > > +					       "clock-frequency",
> > > > > +					       &imx219->xclk_freq);
> > > > > +		if (ret)
> > > > > +			return dev_err_probe(dev, ret,
> > > > > +					     "failed to get clock frequency");
> > > > > +	}
> > > > > +
> > > > 
> > > > This doesn't seem specific to the imx219 driver. Could you turn this
> > > > into a generic V4L2 sensor helper that would take a struct device and a
> > > > clock name, and return the frequency, either retrieved from the clock,
> > > > or from the clock-frequency property as a fallback ?
> > > > 
> > > > Some drivers will also need to control the clock, so the clock should
> > > > probably be returned too.
> > > > 
> > > 
> > > Yes, I saw that many sensor drivers have the same issue.
> > > 
> > > I will try to make it into a generic V4L2 helper and send the patches.
> > 
> > There are other such functions in drivers/media/v4l2-core/v4l2-common.c.
> > Perhaps this is where the new helper could be located as well?
> > 
> 
> I was thinking about drivers/media/v4l2-core/v4l2-fwnode.c but if
> v4l2-common.c is more appropriate we can go with that.

That's my suggestion, yes.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH] media: i2c: imx219: Add support for 'clock-frequency' parsing

[Tomi Valkeinen]

Hi,

On 24/02/2025 13:54, Mehdi Djait wrote:
> Hi Sakari,
> 
> On Mon, Feb 24, 2025 at 09:42:12AM +0000, Sakari Ailus wrote:
>> Hi Mehdi,
>>
>> On Mon, Feb 24, 2025 at 08:59:34AM +0100, Mehdi Djait wrote:
>>> Hi Laurent,
>>>
>>> On Mon, Feb 24, 2025 at 01:06:49AM +0200, Laurent Pinchart wrote:
>>>> Hi Mehdi,
>>>>
>>>> Thank you for the patch.
>>>>
>>>> On Thu, Feb 20, 2025 at 04:49:09PM +0100, Mehdi Djait wrote:
>>>>> Make the clock producer reference lookup optional
>>>>>
>>>>> Add support for ACPI-based platforms by parsing the 'clock-frequency'
>>>>> property when no clock producer is available
>>>>>
>>>>> Signed-off-by: Mehdi Djait <mehdi.djait@linux.intel.com>
>>>>> ---
>>>>>   drivers/media/i2c/imx219.c | 14 ++++++++++++--
>>>>>   1 file changed, 12 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
>>>>> index 2d54cea113e1..a876a6d80a47 100644
>>>>> --- a/drivers/media/i2c/imx219.c
>>>>> +++ b/drivers/media/i2c/imx219.c
>>>>> @@ -1103,12 +1103,22 @@ static int imx219_probe(struct i2c_client *client)
>>>>>   				     "failed to initialize CCI\n");
>>>>>   
>>>>>   	/* Get system clock (xclk) */
>>>>> -	imx219->xclk = devm_clk_get(dev, NULL);
>>>>> +	imx219->xclk = devm_clk_get_optional(dev, NULL);
>>>>>   	if (IS_ERR(imx219->xclk))
>>>>>   		return dev_err_probe(dev, PTR_ERR(imx219->xclk),
>>>>>   				     "failed to get xclk\n");
>>>>>   
>>>>> -	imx219->xclk_freq = clk_get_rate(imx219->xclk);
>>>>> +	if (imx219->xclk) {
>>>>> +		imx219->xclk_freq = clk_get_rate(imx219->xclk);
>>>>> +	} else {
>>>>> +		ret = fwnode_property_read_u32(dev_fwnode(dev),
>>>>> +					       "clock-frequency",
>>>>> +					       &imx219->xclk_freq);
>>>>> +		if (ret)
>>>>> +			return dev_err_probe(dev, ret,
>>>>> +					     "failed to get clock frequency");
>>>>> +	}
>>>>> +
>>>>
>>>> This doesn't seem specific to the imx219 driver. Could you turn this
>>>> into a generic V4L2 sensor helper that would take a struct device and a
>>>> clock name, and return the frequency, either retrieved from the clock,
>>>> or from the clock-frequency property as a fallback ?
>>>>
>>>> Some drivers will also need to control the clock, so the clock should
>>>> probably be returned too.
>>>>
>>>
>>> Yes, I saw that many sensor drivers have the same issue.
>>>
>>> I will try to make it into a generic V4L2 helper and send the patches.
>>
>> There are other such functions in drivers/media/v4l2-core/v4l2-common.c.
>> Perhaps this is where the new helper could be located as well?
>>
> 
> I was thinking about drivers/media/v4l2-core/v4l2-fwnode.c but if
> v4l2-common.c is more appropriate we can go with that.

I admit I have no clue about ACPI, but why is this v4l2 specific? Why 
doesn't clock framework do this for us?

  Tomi

Re: [PATCH] media: i2c: imx219: Add support for 'clock-frequency' parsing

[Sakari Ailus]

Moi,

On Mon, Feb 24, 2025 at 02:24:29PM +0200, Tomi Valkeinen wrote:
> Hi,
> 
> On 24/02/2025 13:54, Mehdi Djait wrote:
> > Hi Sakari,
> > 
> > On Mon, Feb 24, 2025 at 09:42:12AM +0000, Sakari Ailus wrote:
> > > Hi Mehdi,
> > > 
> > > On Mon, Feb 24, 2025 at 08:59:34AM +0100, Mehdi Djait wrote:
> > > > Hi Laurent,
> > > > 
> > > > On Mon, Feb 24, 2025 at 01:06:49AM +0200, Laurent Pinchart wrote:
> > > > > Hi Mehdi,
> > > > > 
> > > > > Thank you for the patch.
> > > > > 
> > > > > On Thu, Feb 20, 2025 at 04:49:09PM +0100, Mehdi Djait wrote:
> > > > > > Make the clock producer reference lookup optional
> > > > > > 
> > > > > > Add support for ACPI-based platforms by parsing the 'clock-frequency'
> > > > > > property when no clock producer is available
> > > > > > 
> > > > > > Signed-off-by: Mehdi Djait <mehdi.djait@linux.intel.com>
> > > > > > ---
> > > > > >   drivers/media/i2c/imx219.c | 14 ++++++++++++--
> > > > > >   1 file changed, 12 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > > > > > index 2d54cea113e1..a876a6d80a47 100644
> > > > > > --- a/drivers/media/i2c/imx219.c
> > > > > > +++ b/drivers/media/i2c/imx219.c
> > > > > > @@ -1103,12 +1103,22 @@ static int imx219_probe(struct i2c_client *client)
> > > > > >   				     "failed to initialize CCI\n");
> > > > > >   	/* Get system clock (xclk) */
> > > > > > -	imx219->xclk = devm_clk_get(dev, NULL);
> > > > > > +	imx219->xclk = devm_clk_get_optional(dev, NULL);
> > > > > >   	if (IS_ERR(imx219->xclk))
> > > > > >   		return dev_err_probe(dev, PTR_ERR(imx219->xclk),
> > > > > >   				     "failed to get xclk\n");
> > > > > > -	imx219->xclk_freq = clk_get_rate(imx219->xclk);
> > > > > > +	if (imx219->xclk) {
> > > > > > +		imx219->xclk_freq = clk_get_rate(imx219->xclk);
> > > > > > +	} else {
> > > > > > +		ret = fwnode_property_read_u32(dev_fwnode(dev),
> > > > > > +					       "clock-frequency",
> > > > > > +					       &imx219->xclk_freq);
> > > > > > +		if (ret)
> > > > > > +			return dev_err_probe(dev, ret,
> > > > > > +					     "failed to get clock frequency");
> > > > > > +	}
> > > > > > +
> > > > > 
> > > > > This doesn't seem specific to the imx219 driver. Could you turn this
> > > > > into a generic V4L2 sensor helper that would take a struct device and a
> > > > > clock name, and return the frequency, either retrieved from the clock,
> > > > > or from the clock-frequency property as a fallback ?
> > > > > 
> > > > > Some drivers will also need to control the clock, so the clock should
> > > > > probably be returned too.
> > > > > 
> > > > 
> > > > Yes, I saw that many sensor drivers have the same issue.
> > > > 
> > > > I will try to make it into a generic V4L2 helper and send the patches.
> > > 
> > > There are other such functions in drivers/media/v4l2-core/v4l2-common.c.
> > > Perhaps this is where the new helper could be located as well?
> > > 
> > 
> > I was thinking about drivers/media/v4l2-core/v4l2-fwnode.c but if
> > v4l2-common.c is more appropriate we can go with that.
> 
> I admit I have no clue about ACPI, but why is this v4l2 specific? Why
> doesn't clock framework do this for us?

The "clock-frequency" isn't really specific to ACPI but it's used on some
boards with DT, too, that precede the current clock bindings. Clocks aren't
generally available to OS in ACPI either but the sensor drivers still need
them. DisCo for Imaging uses "mipi-img-clock-frequency" which DisCo for
Imaging code deep down in the ACPI framework will offer to drivers as
"clock-frequency". A lot of this is actually specific to cameras. On top of
that, camera sensors tend to be devices that are used equally on both DT
and ACPI systems, it's quite uncommon elsewhere. Therefore I do think the
natural place for this code is actually the V4L2 framework.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH] media: i2c: imx219: Add support for 'clock-frequency' parsing

[Laurent Pinchart]

On Mon, Feb 24, 2025 at 12:49:48PM +0000, Sakari Ailus wrote:
> On Mon, Feb 24, 2025 at 02:24:29PM +0200, Tomi Valkeinen wrote:
> > On 24/02/2025 13:54, Mehdi Djait wrote:
> > > On Mon, Feb 24, 2025 at 09:42:12AM +0000, Sakari Ailus wrote:
> > > > On Mon, Feb 24, 2025 at 08:59:34AM +0100, Mehdi Djait wrote:
> > > > > On Mon, Feb 24, 2025 at 01:06:49AM +0200, Laurent Pinchart wrote:
> > > > > > On Thu, Feb 20, 2025 at 04:49:09PM +0100, Mehdi Djait wrote:
> > > > > > > Make the clock producer reference lookup optional
> > > > > > > 
> > > > > > > Add support for ACPI-based platforms by parsing the 'clock-frequency'
> > > > > > > property when no clock producer is available
> > > > > > > 
> > > > > > > Signed-off-by: Mehdi Djait <mehdi.djait@linux.intel.com>
> > > > > > > ---
> > > > > > >   drivers/media/i2c/imx219.c | 14 ++++++++++++--
> > > > > > >   1 file changed, 12 insertions(+), 2 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > > > > > > index 2d54cea113e1..a876a6d80a47 100644
> > > > > > > --- a/drivers/media/i2c/imx219.c
> > > > > > > +++ b/drivers/media/i2c/imx219.c
> > > > > > > @@ -1103,12 +1103,22 @@ static int imx219_probe(struct i2c_client *client)
> > > > > > >   				     "failed to initialize CCI\n");
> > > > > > >   	/* Get system clock (xclk) */
> > > > > > > -	imx219->xclk = devm_clk_get(dev, NULL);
> > > > > > > +	imx219->xclk = devm_clk_get_optional(dev, NULL);
> > > > > > >   	if (IS_ERR(imx219->xclk))
> > > > > > >   		return dev_err_probe(dev, PTR_ERR(imx219->xclk),
> > > > > > >   				     "failed to get xclk\n");
> > > > > > > -	imx219->xclk_freq = clk_get_rate(imx219->xclk);
> > > > > > > +	if (imx219->xclk) {
> > > > > > > +		imx219->xclk_freq = clk_get_rate(imx219->xclk);
> > > > > > > +	} else {
> > > > > > > +		ret = fwnode_property_read_u32(dev_fwnode(dev),
> > > > > > > +					       "clock-frequency",
> > > > > > > +					       &imx219->xclk_freq);
> > > > > > > +		if (ret)
> > > > > > > +			return dev_err_probe(dev, ret,
> > > > > > > +					     "failed to get clock frequency");
> > > > > > > +	}
> > > > > > > +
> > > > > > 
> > > > > > This doesn't seem specific to the imx219 driver. Could you turn this
> > > > > > into a generic V4L2 sensor helper that would take a struct device and a
> > > > > > clock name, and return the frequency, either retrieved from the clock,
> > > > > > or from the clock-frequency property as a fallback ?
> > > > > > 
> > > > > > Some drivers will also need to control the clock, so the clock should
> > > > > > probably be returned too.
> > > > > > 
> > > > > 
> > > > > Yes, I saw that many sensor drivers have the same issue.
> > > > > 
> > > > > I will try to make it into a generic V4L2 helper and send the patches.
> > > > 
> > > > There are other such functions in drivers/media/v4l2-core/v4l2-common.c.
> > > > Perhaps this is where the new helper could be located as well?
> > > > 
> > > 
> > > I was thinking about drivers/media/v4l2-core/v4l2-fwnode.c but if
> > > v4l2-common.c is more appropriate we can go with that.
> > 
> > I admit I have no clue about ACPI, but why is this v4l2 specific? Why
> > doesn't clock framework do this for us?
> 
> The "clock-frequency" isn't really specific to ACPI but it's used on some
> boards with DT, too, that precede the current clock bindings. Clocks aren't
> generally available to OS in ACPI either but the sensor drivers still need
> them. DisCo for Imaging uses "mipi-img-clock-frequency" which DisCo for
> Imaging code deep down in the ACPI framework will offer to drivers as
> "clock-frequency". A lot of this is actually specific to cameras. On top of
> that, camera sensors tend to be devices that are used equally on both DT
> and ACPI systems, it's quite uncommon elsewhere. Therefore I do think the
> natural place for this code is actually the V4L2 framework.

Can ACPI devices support programmable sensor clock frequency ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: i2c: imx219: Add support for 'clock-frequency' parsing

[Sakari Ailus]

Hi Laurent,

On Mon, Feb 24, 2025 at 03:25:36PM +0200, Laurent Pinchart wrote:
> On Mon, Feb 24, 2025 at 12:49:48PM +0000, Sakari Ailus wrote:
> > On Mon, Feb 24, 2025 at 02:24:29PM +0200, Tomi Valkeinen wrote:
> > > On 24/02/2025 13:54, Mehdi Djait wrote:
> > > > On Mon, Feb 24, 2025 at 09:42:12AM +0000, Sakari Ailus wrote:
> > > > > On Mon, Feb 24, 2025 at 08:59:34AM +0100, Mehdi Djait wrote:
> > > > > > On Mon, Feb 24, 2025 at 01:06:49AM +0200, Laurent Pinchart wrote:
> > > > > > > On Thu, Feb 20, 2025 at 04:49:09PM +0100, Mehdi Djait wrote:
> > > > > > > > Make the clock producer reference lookup optional
> > > > > > > > 
> > > > > > > > Add support for ACPI-based platforms by parsing the 'clock-frequency'
> > > > > > > > property when no clock producer is available
> > > > > > > > 
> > > > > > > > Signed-off-by: Mehdi Djait <mehdi.djait@linux.intel.com>
> > > > > > > > ---
> > > > > > > >   drivers/media/i2c/imx219.c | 14 ++++++++++++--
> > > > > > > >   1 file changed, 12 insertions(+), 2 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > > > > > > > index 2d54cea113e1..a876a6d80a47 100644
> > > > > > > > --- a/drivers/media/i2c/imx219.c
> > > > > > > > +++ b/drivers/media/i2c/imx219.c
> > > > > > > > @@ -1103,12 +1103,22 @@ static int imx219_probe(struct i2c_client *client)
> > > > > > > >   				     "failed to initialize CCI\n");
> > > > > > > >   	/* Get system clock (xclk) */
> > > > > > > > -	imx219->xclk = devm_clk_get(dev, NULL);
> > > > > > > > +	imx219->xclk = devm_clk_get_optional(dev, NULL);
> > > > > > > >   	if (IS_ERR(imx219->xclk))
> > > > > > > >   		return dev_err_probe(dev, PTR_ERR(imx219->xclk),
> > > > > > > >   				     "failed to get xclk\n");
> > > > > > > > -	imx219->xclk_freq = clk_get_rate(imx219->xclk);
> > > > > > > > +	if (imx219->xclk) {
> > > > > > > > +		imx219->xclk_freq = clk_get_rate(imx219->xclk);
> > > > > > > > +	} else {
> > > > > > > > +		ret = fwnode_property_read_u32(dev_fwnode(dev),
> > > > > > > > +					       "clock-frequency",
> > > > > > > > +					       &imx219->xclk_freq);
> > > > > > > > +		if (ret)
> > > > > > > > +			return dev_err_probe(dev, ret,
> > > > > > > > +					     "failed to get clock frequency");
> > > > > > > > +	}
> > > > > > > > +
> > > > > > > 
> > > > > > > This doesn't seem specific to the imx219 driver. Could you turn this
> > > > > > > into a generic V4L2 sensor helper that would take a struct device and a
> > > > > > > clock name, and return the frequency, either retrieved from the clock,
> > > > > > > or from the clock-frequency property as a fallback ?
> > > > > > > 
> > > > > > > Some drivers will also need to control the clock, so the clock should
> > > > > > > probably be returned too.
> > > > > > > 
> > > > > > 
> > > > > > Yes, I saw that many sensor drivers have the same issue.
> > > > > > 
> > > > > > I will try to make it into a generic V4L2 helper and send the patches.
> > > > > 
> > > > > There are other such functions in drivers/media/v4l2-core/v4l2-common.c.
> > > > > Perhaps this is where the new helper could be located as well?
> > > > > 
> > > > 
> > > > I was thinking about drivers/media/v4l2-core/v4l2-fwnode.c but if
> > > > v4l2-common.c is more appropriate we can go with that.
> > > 
> > > I admit I have no clue about ACPI, but why is this v4l2 specific? Why
> > > doesn't clock framework do this for us?
> > 
> > The "clock-frequency" isn't really specific to ACPI but it's used on some
> > boards with DT, too, that precede the current clock bindings. Clocks aren't
> > generally available to OS in ACPI either but the sensor drivers still need
> > them. DisCo for Imaging uses "mipi-img-clock-frequency" which DisCo for
> > Imaging code deep down in the ACPI framework will offer to drivers as
> > "clock-frequency". A lot of this is actually specific to cameras. On top of
> > that, camera sensors tend to be devices that are used equally on both DT
> > and ACPI systems, it's quite uncommon elsewhere. Therefore I do think the
> > natural place for this code is actually the V4L2 framework.
> 
> Can ACPI devices support programmable sensor clock frequency ?

Do you mean sensor's external clock or PLL? And do you mean programmable
as configured in system firmware or at runtime?

-- 
Regards,

Sakari Ailus

Re: [PATCH] media: i2c: imx219: Add support for 'clock-frequency' parsing

[Laurent Pinchart]

On Mon, Feb 24, 2025 at 02:38:35PM +0000, Sakari Ailus wrote:
> On Mon, Feb 24, 2025 at 03:25:36PM +0200, Laurent Pinchart wrote:
> > On Mon, Feb 24, 2025 at 12:49:48PM +0000, Sakari Ailus wrote:
> > > On Mon, Feb 24, 2025 at 02:24:29PM +0200, Tomi Valkeinen wrote:
> > > > On 24/02/2025 13:54, Mehdi Djait wrote:
> > > > > On Mon, Feb 24, 2025 at 09:42:12AM +0000, Sakari Ailus wrote:
> > > > > > On Mon, Feb 24, 2025 at 08:59:34AM +0100, Mehdi Djait wrote:
> > > > > > > On Mon, Feb 24, 2025 at 01:06:49AM +0200, Laurent Pinchart wrote:
> > > > > > > > On Thu, Feb 20, 2025 at 04:49:09PM +0100, Mehdi Djait wrote:
> > > > > > > > > Make the clock producer reference lookup optional
> > > > > > > > > 
> > > > > > > > > Add support for ACPI-based platforms by parsing the 'clock-frequency'
> > > > > > > > > property when no clock producer is available
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Mehdi Djait <mehdi.djait@linux.intel.com>
> > > > > > > > > ---
> > > > > > > > >   drivers/media/i2c/imx219.c | 14 ++++++++++++--
> > > > > > > > >   1 file changed, 12 insertions(+), 2 deletions(-)
> > > > > > > > > 
> > > > > > > > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > > > > > > > > index 2d54cea113e1..a876a6d80a47 100644
> > > > > > > > > --- a/drivers/media/i2c/imx219.c
> > > > > > > > > +++ b/drivers/media/i2c/imx219.c
> > > > > > > > > @@ -1103,12 +1103,22 @@ static int imx219_probe(struct i2c_client *client)
> > > > > > > > >   				     "failed to initialize CCI\n");
> > > > > > > > >   	/* Get system clock (xclk) */
> > > > > > > > > -	imx219->xclk = devm_clk_get(dev, NULL);
> > > > > > > > > +	imx219->xclk = devm_clk_get_optional(dev, NULL);
> > > > > > > > >   	if (IS_ERR(imx219->xclk))
> > > > > > > > >   		return dev_err_probe(dev, PTR_ERR(imx219->xclk),
> > > > > > > > >   				     "failed to get xclk\n");
> > > > > > > > > -	imx219->xclk_freq = clk_get_rate(imx219->xclk);
> > > > > > > > > +	if (imx219->xclk) {
> > > > > > > > > +		imx219->xclk_freq = clk_get_rate(imx219->xclk);
> > > > > > > > > +	} else {
> > > > > > > > > +		ret = fwnode_property_read_u32(dev_fwnode(dev),
> > > > > > > > > +					       "clock-frequency",
> > > > > > > > > +					       &imx219->xclk_freq);
> > > > > > > > > +		if (ret)
> > > > > > > > > +			return dev_err_probe(dev, ret,
> > > > > > > > > +					     "failed to get clock frequency");
> > > > > > > > > +	}
> > > > > > > > > +
> > > > > > > > 
> > > > > > > > This doesn't seem specific to the imx219 driver. Could you turn this
> > > > > > > > into a generic V4L2 sensor helper that would take a struct device and a
> > > > > > > > clock name, and return the frequency, either retrieved from the clock,
> > > > > > > > or from the clock-frequency property as a fallback ?
> > > > > > > > 
> > > > > > > > Some drivers will also need to control the clock, so the clock should
> > > > > > > > probably be returned too.
> > > > > > > > 
> > > > > > > 
> > > > > > > Yes, I saw that many sensor drivers have the same issue.
> > > > > > > 
> > > > > > > I will try to make it into a generic V4L2 helper and send the patches.
> > > > > > 
> > > > > > There are other such functions in drivers/media/v4l2-core/v4l2-common.c.
> > > > > > Perhaps this is where the new helper could be located as well?
> > > > > > 
> > > > > 
> > > > > I was thinking about drivers/media/v4l2-core/v4l2-fwnode.c but if
> > > > > v4l2-common.c is more appropriate we can go with that.
> > > > 
> > > > I admit I have no clue about ACPI, but why is this v4l2 specific? Why
> > > > doesn't clock framework do this for us?
> > > 
> > > The "clock-frequency" isn't really specific to ACPI but it's used on some
> > > boards with DT, too, that precede the current clock bindings. Clocks aren't
> > > generally available to OS in ACPI either but the sensor drivers still need
> > > them. DisCo for Imaging uses "mipi-img-clock-frequency" which DisCo for
> > > Imaging code deep down in the ACPI framework will offer to drivers as
> > > "clock-frequency". A lot of this is actually specific to cameras. On top of
> > > that, camera sensors tend to be devices that are used equally on both DT
> > > and ACPI systems, it's quite uncommon elsewhere. Therefore I do think the
> > > natural place for this code is actually the V4L2 framework.
> > 
> > Can ACPI devices support programmable sensor clock frequency ?
> 
> Do you mean sensor's external clock or PLL? And do you mean programmable
> as configured in system firmware or at runtime?

I mean external clock, and configurable at runtime.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: i2c: imx219: Add support for 'clock-frequency' parsing

[Sakari Ailus]

Hi Laurent,

On Mon, Feb 24, 2025 at 06:16:40PM +0200, Laurent Pinchart wrote:
> On Mon, Feb 24, 2025 at 02:38:35PM +0000, Sakari Ailus wrote:
> > On Mon, Feb 24, 2025 at 03:25:36PM +0200, Laurent Pinchart wrote:
> > > On Mon, Feb 24, 2025 at 12:49:48PM +0000, Sakari Ailus wrote:
> > > > On Mon, Feb 24, 2025 at 02:24:29PM +0200, Tomi Valkeinen wrote:
> > > > > On 24/02/2025 13:54, Mehdi Djait wrote:
> > > > > > On Mon, Feb 24, 2025 at 09:42:12AM +0000, Sakari Ailus wrote:
> > > > > > > On Mon, Feb 24, 2025 at 08:59:34AM +0100, Mehdi Djait wrote:
> > > > > > > > On Mon, Feb 24, 2025 at 01:06:49AM +0200, Laurent Pinchart wrote:
> > > > > > > > > On Thu, Feb 20, 2025 at 04:49:09PM +0100, Mehdi Djait wrote:
> > > > > > > > > > Make the clock producer reference lookup optional
> > > > > > > > > > 
> > > > > > > > > > Add support for ACPI-based platforms by parsing the 'clock-frequency'
> > > > > > > > > > property when no clock producer is available
> > > > > > > > > > 
> > > > > > > > > > Signed-off-by: Mehdi Djait <mehdi.djait@linux.intel.com>
> > > > > > > > > > ---
> > > > > > > > > >   drivers/media/i2c/imx219.c | 14 ++++++++++++--
> > > > > > > > > >   1 file changed, 12 insertions(+), 2 deletions(-)
> > > > > > > > > > 
> > > > > > > > > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > > > > > > > > > index 2d54cea113e1..a876a6d80a47 100644
> > > > > > > > > > --- a/drivers/media/i2c/imx219.c
> > > > > > > > > > +++ b/drivers/media/i2c/imx219.c
> > > > > > > > > > @@ -1103,12 +1103,22 @@ static int imx219_probe(struct i2c_client *client)
> > > > > > > > > >   				     "failed to initialize CCI\n");
> > > > > > > > > >   	/* Get system clock (xclk) */
> > > > > > > > > > -	imx219->xclk = devm_clk_get(dev, NULL);
> > > > > > > > > > +	imx219->xclk = devm_clk_get_optional(dev, NULL);
> > > > > > > > > >   	if (IS_ERR(imx219->xclk))
> > > > > > > > > >   		return dev_err_probe(dev, PTR_ERR(imx219->xclk),
> > > > > > > > > >   				     "failed to get xclk\n");
> > > > > > > > > > -	imx219->xclk_freq = clk_get_rate(imx219->xclk);
> > > > > > > > > > +	if (imx219->xclk) {
> > > > > > > > > > +		imx219->xclk_freq = clk_get_rate(imx219->xclk);
> > > > > > > > > > +	} else {
> > > > > > > > > > +		ret = fwnode_property_read_u32(dev_fwnode(dev),
> > > > > > > > > > +					       "clock-frequency",
> > > > > > > > > > +					       &imx219->xclk_freq);
> > > > > > > > > > +		if (ret)
> > > > > > > > > > +			return dev_err_probe(dev, ret,
> > > > > > > > > > +					     "failed to get clock frequency");
> > > > > > > > > > +	}
> > > > > > > > > > +
> > > > > > > > > 
> > > > > > > > > This doesn't seem specific to the imx219 driver. Could you turn this
> > > > > > > > > into a generic V4L2 sensor helper that would take a struct device and a
> > > > > > > > > clock name, and return the frequency, either retrieved from the clock,
> > > > > > > > > or from the clock-frequency property as a fallback ?
> > > > > > > > > 
> > > > > > > > > Some drivers will also need to control the clock, so the clock should
> > > > > > > > > probably be returned too.
> > > > > > > > > 
> > > > > > > > 
> > > > > > > > Yes, I saw that many sensor drivers have the same issue.
> > > > > > > > 
> > > > > > > > I will try to make it into a generic V4L2 helper and send the patches.
> > > > > > > 
> > > > > > > There are other such functions in drivers/media/v4l2-core/v4l2-common.c.
> > > > > > > Perhaps this is where the new helper could be located as well?
> > > > > > > 
> > > > > > 
> > > > > > I was thinking about drivers/media/v4l2-core/v4l2-fwnode.c but if
> > > > > > v4l2-common.c is more appropriate we can go with that.
> > > > > 
> > > > > I admit I have no clue about ACPI, but why is this v4l2 specific? Why
> > > > > doesn't clock framework do this for us?
> > > > 
> > > > The "clock-frequency" isn't really specific to ACPI but it's used on some
> > > > boards with DT, too, that precede the current clock bindings. Clocks aren't
> > > > generally available to OS in ACPI either but the sensor drivers still need
> > > > them. DisCo for Imaging uses "mipi-img-clock-frequency" which DisCo for
> > > > Imaging code deep down in the ACPI framework will offer to drivers as
> > > > "clock-frequency". A lot of this is actually specific to cameras. On top of
> > > > that, camera sensors tend to be devices that are used equally on both DT
> > > > and ACPI systems, it's quite uncommon elsewhere. Therefore I do think the
> > > > natural place for this code is actually the V4L2 framework.
> > > 
> > > Can ACPI devices support programmable sensor clock frequency ?
> > 
> > Do you mean sensor's external clock or PLL? And do you mean programmable
> > as configured in system firmware or at runtime?
> 
> I mean external clock, and configurable at runtime.

There's basically no standard API to change a clock's frequency at runtime
(as there is no API to handle clocks) -- which is why we have a property
instead of that presumed API.

-- 
Regards,

Sakari Ailus

Re: [PATCH] media: i2c: imx219: Add support for 'clock-frequency' parsing

[Laurent Pinchart]

On Mon, Feb 24, 2025 at 04:28:47PM +0000, Sakari Ailus wrote:
> On Mon, Feb 24, 2025 at 06:16:40PM +0200, Laurent Pinchart wrote:
> > On Mon, Feb 24, 2025 at 02:38:35PM +0000, Sakari Ailus wrote:
> > > On Mon, Feb 24, 2025 at 03:25:36PM +0200, Laurent Pinchart wrote:
> > > > On Mon, Feb 24, 2025 at 12:49:48PM +0000, Sakari Ailus wrote:
> > > > > On Mon, Feb 24, 2025 at 02:24:29PM +0200, Tomi Valkeinen wrote:
> > > > > > On 24/02/2025 13:54, Mehdi Djait wrote:
> > > > > > > On Mon, Feb 24, 2025 at 09:42:12AM +0000, Sakari Ailus wrote:
> > > > > > > > On Mon, Feb 24, 2025 at 08:59:34AM +0100, Mehdi Djait wrote:
> > > > > > > > > On Mon, Feb 24, 2025 at 01:06:49AM +0200, Laurent Pinchart wrote:
> > > > > > > > > > On Thu, Feb 20, 2025 at 04:49:09PM +0100, Mehdi Djait wrote:
> > > > > > > > > > > Make the clock producer reference lookup optional
> > > > > > > > > > > 
> > > > > > > > > > > Add support for ACPI-based platforms by parsing the 'clock-frequency'
> > > > > > > > > > > property when no clock producer is available
> > > > > > > > > > > 
> > > > > > > > > > > Signed-off-by: Mehdi Djait <mehdi.djait@linux.intel.com>
> > > > > > > > > > > ---
> > > > > > > > > > >   drivers/media/i2c/imx219.c | 14 ++++++++++++--
> > > > > > > > > > >   1 file changed, 12 insertions(+), 2 deletions(-)
> > > > > > > > > > > 
> > > > > > > > > > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > > > > > > > > > > index 2d54cea113e1..a876a6d80a47 100644
> > > > > > > > > > > --- a/drivers/media/i2c/imx219.c
> > > > > > > > > > > +++ b/drivers/media/i2c/imx219.c
> > > > > > > > > > > @@ -1103,12 +1103,22 @@ static int imx219_probe(struct i2c_client *client)
> > > > > > > > > > >   				     "failed to initialize CCI\n");
> > > > > > > > > > >   	/* Get system clock (xclk) */
> > > > > > > > > > > -	imx219->xclk = devm_clk_get(dev, NULL);
> > > > > > > > > > > +	imx219->xclk = devm_clk_get_optional(dev, NULL);
> > > > > > > > > > >   	if (IS_ERR(imx219->xclk))
> > > > > > > > > > >   		return dev_err_probe(dev, PTR_ERR(imx219->xclk),
> > > > > > > > > > >   				     "failed to get xclk\n");
> > > > > > > > > > > -	imx219->xclk_freq = clk_get_rate(imx219->xclk);
> > > > > > > > > > > +	if (imx219->xclk) {
> > > > > > > > > > > +		imx219->xclk_freq = clk_get_rate(imx219->xclk);
> > > > > > > > > > > +	} else {
> > > > > > > > > > > +		ret = fwnode_property_read_u32(dev_fwnode(dev),
> > > > > > > > > > > +					       "clock-frequency",
> > > > > > > > > > > +					       &imx219->xclk_freq);
> > > > > > > > > > > +		if (ret)
> > > > > > > > > > > +			return dev_err_probe(dev, ret,
> > > > > > > > > > > +					     "failed to get clock frequency");
> > > > > > > > > > > +	}
> > > > > > > > > > > +
> > > > > > > > > > 
> > > > > > > > > > This doesn't seem specific to the imx219 driver. Could you turn this
> > > > > > > > > > into a generic V4L2 sensor helper that would take a struct device and a
> > > > > > > > > > clock name, and return the frequency, either retrieved from the clock,
> > > > > > > > > > or from the clock-frequency property as a fallback ?
> > > > > > > > > > 
> > > > > > > > > > Some drivers will also need to control the clock, so the clock should
> > > > > > > > > > probably be returned too.
> > > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > Yes, I saw that many sensor drivers have the same issue.
> > > > > > > > > 
> > > > > > > > > I will try to make it into a generic V4L2 helper and send the patches.
> > > > > > > > 
> > > > > > > > There are other such functions in drivers/media/v4l2-core/v4l2-common.c.
> > > > > > > > Perhaps this is where the new helper could be located as well?
> > > > > > > > 
> > > > > > > 
> > > > > > > I was thinking about drivers/media/v4l2-core/v4l2-fwnode.c but if
> > > > > > > v4l2-common.c is more appropriate we can go with that.
> > > > > > 
> > > > > > I admit I have no clue about ACPI, but why is this v4l2 specific? Why
> > > > > > doesn't clock framework do this for us?
> > > > > 
> > > > > The "clock-frequency" isn't really specific to ACPI but it's used on some
> > > > > boards with DT, too, that precede the current clock bindings. Clocks aren't
> > > > > generally available to OS in ACPI either but the sensor drivers still need
> > > > > them. DisCo for Imaging uses "mipi-img-clock-frequency" which DisCo for
> > > > > Imaging code deep down in the ACPI framework will offer to drivers as
> > > > > "clock-frequency". A lot of this is actually specific to cameras. On top of
> > > > > that, camera sensors tend to be devices that are used equally on both DT
> > > > > and ACPI systems, it's quite uncommon elsewhere. Therefore I do think the
> > > > > natural place for this code is actually the V4L2 framework.
> > > > 
> > > > Can ACPI devices support programmable sensor clock frequency ?
> > > 
> > > Do you mean sensor's external clock or PLL? And do you mean programmable
> > > as configured in system firmware or at runtime?
> > 
> > I mean external clock, and configurable at runtime.
> 
> There's basically no standard API to change a clock's frequency at runtime
> (as there is no API to handle clocks) -- which is why we have a property
> instead of that presumed API.

I wonder, for ACPI devices that have a clock-frequency property, could
we automatically register a fixed-frequency clock ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: i2c: imx219: Add support for 'clock-frequency' parsing

[Sakari Ailus]

Hi Laurent,

On Mon, Feb 24, 2025 at 06:34:21PM +0200, Laurent Pinchart wrote:
> On Mon, Feb 24, 2025 at 04:28:47PM +0000, Sakari Ailus wrote:
> > On Mon, Feb 24, 2025 at 06:16:40PM +0200, Laurent Pinchart wrote:
> > > On Mon, Feb 24, 2025 at 02:38:35PM +0000, Sakari Ailus wrote:
> > > > On Mon, Feb 24, 2025 at 03:25:36PM +0200, Laurent Pinchart wrote:
> > > > > On Mon, Feb 24, 2025 at 12:49:48PM +0000, Sakari Ailus wrote:
> > > > > > On Mon, Feb 24, 2025 at 02:24:29PM +0200, Tomi Valkeinen wrote:
> > > > > > > On 24/02/2025 13:54, Mehdi Djait wrote:
> > > > > > > > On Mon, Feb 24, 2025 at 09:42:12AM +0000, Sakari Ailus wrote:
> > > > > > > > > On Mon, Feb 24, 2025 at 08:59:34AM +0100, Mehdi Djait wrote:
> > > > > > > > > > On Mon, Feb 24, 2025 at 01:06:49AM +0200, Laurent Pinchart wrote:
> > > > > > > > > > > On Thu, Feb 20, 2025 at 04:49:09PM +0100, Mehdi Djait wrote:
> > > > > > > > > > > > Make the clock producer reference lookup optional
> > > > > > > > > > > > 
> > > > > > > > > > > > Add support for ACPI-based platforms by parsing the 'clock-frequency'
> > > > > > > > > > > > property when no clock producer is available
> > > > > > > > > > > > 
> > > > > > > > > > > > Signed-off-by: Mehdi Djait <mehdi.djait@linux.intel.com>
> > > > > > > > > > > > ---
> > > > > > > > > > > >   drivers/media/i2c/imx219.c | 14 ++++++++++++--
> > > > > > > > > > > >   1 file changed, 12 insertions(+), 2 deletions(-)
> > > > > > > > > > > > 
> > > > > > > > > > > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > > > > > > > > > > > index 2d54cea113e1..a876a6d80a47 100644
> > > > > > > > > > > > --- a/drivers/media/i2c/imx219.c
> > > > > > > > > > > > +++ b/drivers/media/i2c/imx219.c
> > > > > > > > > > > > @@ -1103,12 +1103,22 @@ static int imx219_probe(struct i2c_client *client)
> > > > > > > > > > > >   				     "failed to initialize CCI\n");
> > > > > > > > > > > >   	/* Get system clock (xclk) */
> > > > > > > > > > > > -	imx219->xclk = devm_clk_get(dev, NULL);
> > > > > > > > > > > > +	imx219->xclk = devm_clk_get_optional(dev, NULL);
> > > > > > > > > > > >   	if (IS_ERR(imx219->xclk))
> > > > > > > > > > > >   		return dev_err_probe(dev, PTR_ERR(imx219->xclk),
> > > > > > > > > > > >   				     "failed to get xclk\n");
> > > > > > > > > > > > -	imx219->xclk_freq = clk_get_rate(imx219->xclk);
> > > > > > > > > > > > +	if (imx219->xclk) {
> > > > > > > > > > > > +		imx219->xclk_freq = clk_get_rate(imx219->xclk);
> > > > > > > > > > > > +	} else {
> > > > > > > > > > > > +		ret = fwnode_property_read_u32(dev_fwnode(dev),
> > > > > > > > > > > > +					       "clock-frequency",
> > > > > > > > > > > > +					       &imx219->xclk_freq);
> > > > > > > > > > > > +		if (ret)
> > > > > > > > > > > > +			return dev_err_probe(dev, ret,
> > > > > > > > > > > > +					     "failed to get clock frequency");
> > > > > > > > > > > > +	}
> > > > > > > > > > > > +
> > > > > > > > > > > 
> > > > > > > > > > > This doesn't seem specific to the imx219 driver. Could you turn this
> > > > > > > > > > > into a generic V4L2 sensor helper that would take a struct device and a
> > > > > > > > > > > clock name, and return the frequency, either retrieved from the clock,
> > > > > > > > > > > or from the clock-frequency property as a fallback ?
> > > > > > > > > > > 
> > > > > > > > > > > Some drivers will also need to control the clock, so the clock should
> > > > > > > > > > > probably be returned too.
> > > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > Yes, I saw that many sensor drivers have the same issue.
> > > > > > > > > > 
> > > > > > > > > > I will try to make it into a generic V4L2 helper and send the patches.
> > > > > > > > > 
> > > > > > > > > There are other such functions in drivers/media/v4l2-core/v4l2-common.c.
> > > > > > > > > Perhaps this is where the new helper could be located as well?
> > > > > > > > > 
> > > > > > > > 
> > > > > > > > I was thinking about drivers/media/v4l2-core/v4l2-fwnode.c but if
> > > > > > > > v4l2-common.c is more appropriate we can go with that.
> > > > > > > 
> > > > > > > I admit I have no clue about ACPI, but why is this v4l2 specific? Why
> > > > > > > doesn't clock framework do this for us?
> > > > > > 
> > > > > > The "clock-frequency" isn't really specific to ACPI but it's used on some
> > > > > > boards with DT, too, that precede the current clock bindings. Clocks aren't
> > > > > > generally available to OS in ACPI either but the sensor drivers still need
> > > > > > them. DisCo for Imaging uses "mipi-img-clock-frequency" which DisCo for
> > > > > > Imaging code deep down in the ACPI framework will offer to drivers as
> > > > > > "clock-frequency". A lot of this is actually specific to cameras. On top of
> > > > > > that, camera sensors tend to be devices that are used equally on both DT
> > > > > > and ACPI systems, it's quite uncommon elsewhere. Therefore I do think the
> > > > > > natural place for this code is actually the V4L2 framework.
> > > > > 
> > > > > Can ACPI devices support programmable sensor clock frequency ?
> > > > 
> > > > Do you mean sensor's external clock or PLL? And do you mean programmable
> > > > as configured in system firmware or at runtime?
> > > 
> > > I mean external clock, and configurable at runtime.
> > 
> > There's basically no standard API to change a clock's frequency at runtime
> > (as there is no API to handle clocks) -- which is why we have a property
> > instead of that presumed API.
> 
> I wonder, for ACPI devices that have a clock-frequency property, could
> we automatically register a fixed-frequency clock ?

That's an interesting idea.

It still could have potential to break something even if it may be
unlikely. I don't know what others are doing with clocks (if anything)
outside cameras on ACPI.

Maybe a case for an RFC? Doing this would be nice as we could get rid of
one of the few remaining DT/ACPI differences in sensor drivers.

That being said, this should not hold back merging the patch.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH] media: i2c: imx219: Add support for 'clock-frequency' parsing

[Laurent Pinchart]

On Mon, Feb 24, 2025 at 05:06:49PM +0000, Sakari Ailus wrote:
> Hi Laurent,
> 
> On Mon, Feb 24, 2025 at 06:34:21PM +0200, Laurent Pinchart wrote:
> > On Mon, Feb 24, 2025 at 04:28:47PM +0000, Sakari Ailus wrote:
> > > On Mon, Feb 24, 2025 at 06:16:40PM +0200, Laurent Pinchart wrote:
> > > > On Mon, Feb 24, 2025 at 02:38:35PM +0000, Sakari Ailus wrote:
> > > > > On Mon, Feb 24, 2025 at 03:25:36PM +0200, Laurent Pinchart wrote:
> > > > > > On Mon, Feb 24, 2025 at 12:49:48PM +0000, Sakari Ailus wrote:
> > > > > > > On Mon, Feb 24, 2025 at 02:24:29PM +0200, Tomi Valkeinen wrote:
> > > > > > > > On 24/02/2025 13:54, Mehdi Djait wrote:
> > > > > > > > > On Mon, Feb 24, 2025 at 09:42:12AM +0000, Sakari Ailus wrote:
> > > > > > > > > > On Mon, Feb 24, 2025 at 08:59:34AM +0100, Mehdi Djait wrote:
> > > > > > > > > > > On Mon, Feb 24, 2025 at 01:06:49AM +0200, Laurent Pinchart wrote:
> > > > > > > > > > > > On Thu, Feb 20, 2025 at 04:49:09PM +0100, Mehdi Djait wrote:
> > > > > > > > > > > > > Make the clock producer reference lookup optional
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Add support for ACPI-based platforms by parsing the 'clock-frequency'
> > > > > > > > > > > > > property when no clock producer is available
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Signed-off-by: Mehdi Djait <mehdi.djait@linux.intel.com>
> > > > > > > > > > > > > ---
> > > > > > > > > > > > >   drivers/media/i2c/imx219.c | 14 ++++++++++++--
> > > > > > > > > > > > >   1 file changed, 12 insertions(+), 2 deletions(-)
> > > > > > > > > > > > > 
> > > > > > > > > > > > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > > > > > > > > > > > > index 2d54cea113e1..a876a6d80a47 100644
> > > > > > > > > > > > > --- a/drivers/media/i2c/imx219.c
> > > > > > > > > > > > > +++ b/drivers/media/i2c/imx219.c
> > > > > > > > > > > > > @@ -1103,12 +1103,22 @@ static int imx219_probe(struct i2c_client *client)
> > > > > > > > > > > > >   				     "failed to initialize CCI\n");
> > > > > > > > > > > > >   	/* Get system clock (xclk) */
> > > > > > > > > > > > > -	imx219->xclk = devm_clk_get(dev, NULL);
> > > > > > > > > > > > > +	imx219->xclk = devm_clk_get_optional(dev, NULL);
> > > > > > > > > > > > >   	if (IS_ERR(imx219->xclk))
> > > > > > > > > > > > >   		return dev_err_probe(dev, PTR_ERR(imx219->xclk),
> > > > > > > > > > > > >   				     "failed to get xclk\n");
> > > > > > > > > > > > > -	imx219->xclk_freq = clk_get_rate(imx219->xclk);
> > > > > > > > > > > > > +	if (imx219->xclk) {
> > > > > > > > > > > > > +		imx219->xclk_freq = clk_get_rate(imx219->xclk);
> > > > > > > > > > > > > +	} else {
> > > > > > > > > > > > > +		ret = fwnode_property_read_u32(dev_fwnode(dev),
> > > > > > > > > > > > > +					       "clock-frequency",
> > > > > > > > > > > > > +					       &imx219->xclk_freq);
> > > > > > > > > > > > > +		if (ret)
> > > > > > > > > > > > > +			return dev_err_probe(dev, ret,
> > > > > > > > > > > > > +					     "failed to get clock frequency");
> > > > > > > > > > > > > +	}
> > > > > > > > > > > > > +
> > > > > > > > > > > > 
> > > > > > > > > > > > This doesn't seem specific to the imx219 driver. Could you turn this
> > > > > > > > > > > > into a generic V4L2 sensor helper that would take a struct device and a
> > > > > > > > > > > > clock name, and return the frequency, either retrieved from the clock,
> > > > > > > > > > > > or from the clock-frequency property as a fallback ?
> > > > > > > > > > > > 
> > > > > > > > > > > > Some drivers will also need to control the clock, so the clock should
> > > > > > > > > > > > probably be returned too.
> > > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > Yes, I saw that many sensor drivers have the same issue.
> > > > > > > > > > > 
> > > > > > > > > > > I will try to make it into a generic V4L2 helper and send the patches.
> > > > > > > > > > 
> > > > > > > > > > There are other such functions in drivers/media/v4l2-core/v4l2-common.c.
> > > > > > > > > > Perhaps this is where the new helper could be located as well?
> > > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > I was thinking about drivers/media/v4l2-core/v4l2-fwnode.c but if
> > > > > > > > > v4l2-common.c is more appropriate we can go with that.
> > > > > > > > 
> > > > > > > > I admit I have no clue about ACPI, but why is this v4l2 specific? Why
> > > > > > > > doesn't clock framework do this for us?
> > > > > > > 
> > > > > > > The "clock-frequency" isn't really specific to ACPI but it's used on some
> > > > > > > boards with DT, too, that precede the current clock bindings. Clocks aren't
> > > > > > > generally available to OS in ACPI either but the sensor drivers still need
> > > > > > > them. DisCo for Imaging uses "mipi-img-clock-frequency" which DisCo for
> > > > > > > Imaging code deep down in the ACPI framework will offer to drivers as
> > > > > > > "clock-frequency". A lot of this is actually specific to cameras. On top of
> > > > > > > that, camera sensors tend to be devices that are used equally on both DT
> > > > > > > and ACPI systems, it's quite uncommon elsewhere. Therefore I do think the
> > > > > > > natural place for this code is actually the V4L2 framework.
> > > > > > 
> > > > > > Can ACPI devices support programmable sensor clock frequency ?
> > > > > 
> > > > > Do you mean sensor's external clock or PLL? And do you mean programmable
> > > > > as configured in system firmware or at runtime?
> > > > 
> > > > I mean external clock, and configurable at runtime.
> > > 
> > > There's basically no standard API to change a clock's frequency at runtime
> > > (as there is no API to handle clocks) -- which is why we have a property
> > > instead of that presumed API.
> > 
> > I wonder, for ACPI devices that have a clock-frequency property, could
> > we automatically register a fixed-frequency clock ?
> 
> That's an interesting idea.
> 
> It still could have potential to break something even if it may be
> unlikely. I don't know what others are doing with clocks (if anything)
> outside cameras on ACPI.
> 
> Maybe a case for an RFC? Doing this would be nice as we could get rid of
> one of the few remaining DT/ACPI differences in sensor drivers.
> 
> That being said, this should not hold back merging the patch.

An RFC sounds good. For this specific patch, I think we agree we need a
helper function. I wonder if the helper could register a fixed clock if
there's no clocks property, that way we could easily change the
implementation later while minimizing changes to drivers. Some sort of

struct clk *devm_v4l2_sensor_clk_get(struct device *dev, const char *name);

that sensor drivers would call instead of devm_clk_get(). There may be
issues I don't foresee though, but maybe worth trying ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: i2c: imx219: Add support for 'clock-frequency' parsing

[Mehdi Djait]

Hi Laurent, Hi Sakari,

On Mon, Feb 24, 2025 at 08:19:35PM +0200, Laurent Pinchart wrote:
> On Mon, Feb 24, 2025 at 05:06:49PM +0000, Sakari Ailus wrote:
> > Hi Laurent,
> > 
> > On Mon, Feb 24, 2025 at 06:34:21PM +0200, Laurent Pinchart wrote:
> > > On Mon, Feb 24, 2025 at 04:28:47PM +0000, Sakari Ailus wrote:
> > > > On Mon, Feb 24, 2025 at 06:16:40PM +0200, Laurent Pinchart wrote:
> > > > > On Mon, Feb 24, 2025 at 02:38:35PM +0000, Sakari Ailus wrote:
> > > > > > On Mon, Feb 24, 2025 at 03:25:36PM +0200, Laurent Pinchart wrote:
> > > > > > > On Mon, Feb 24, 2025 at 12:49:48PM +0000, Sakari Ailus wrote:
> > > > > > > > On Mon, Feb 24, 2025 at 02:24:29PM +0200, Tomi Valkeinen wrote:
> > > > > > > > > On 24/02/2025 13:54, Mehdi Djait wrote:
> > > > > > > > > > On Mon, Feb 24, 2025 at 09:42:12AM +0000, Sakari Ailus wrote:
> > > > > > > > > > > On Mon, Feb 24, 2025 at 08:59:34AM +0100, Mehdi Djait wrote:
> > > > > > > > > > > > On Mon, Feb 24, 2025 at 01:06:49AM +0200, Laurent Pinchart wrote:
> > > > > > > > > > > > > On Thu, Feb 20, 2025 at 04:49:09PM +0100, Mehdi Djait wrote:
> > > > > > > > > > > > > > Make the clock producer reference lookup optional
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > Add support for ACPI-based platforms by parsing the 'clock-frequency'
> > > > > > > > > > > > > > property when no clock producer is available
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > Signed-off-by: Mehdi Djait <mehdi.djait@linux.intel.com>
> > > > > > > > > > > > > > ---
> > > > > > > > > > > > > >   drivers/media/i2c/imx219.c | 14 ++++++++++++--
> > > > > > > > > > > > > >   1 file changed, 12 insertions(+), 2 deletions(-)
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > > > > > > > > > > > > > index 2d54cea113e1..a876a6d80a47 100644
> > > > > > > > > > > > > > --- a/drivers/media/i2c/imx219.c
> > > > > > > > > > > > > > +++ b/drivers/media/i2c/imx219.c
> > > > > > > > > > > > > > @@ -1103,12 +1103,22 @@ static int imx219_probe(struct i2c_client *client)
> > > > > > > > > > > > > >   				     "failed to initialize CCI\n");
> > > > > > > > > > > > > >   	/* Get system clock (xclk) */
> > > > > > > > > > > > > > -	imx219->xclk = devm_clk_get(dev, NULL);
> > > > > > > > > > > > > > +	imx219->xclk = devm_clk_get_optional(dev, NULL);
> > > > > > > > > > > > > >   	if (IS_ERR(imx219->xclk))
> > > > > > > > > > > > > >   		return dev_err_probe(dev, PTR_ERR(imx219->xclk),
> > > > > > > > > > > > > >   				     "failed to get xclk\n");
> > > > > > > > > > > > > > -	imx219->xclk_freq = clk_get_rate(imx219->xclk);
> > > > > > > > > > > > > > +	if (imx219->xclk) {
> > > > > > > > > > > > > > +		imx219->xclk_freq = clk_get_rate(imx219->xclk);
> > > > > > > > > > > > > > +	} else {
> > > > > > > > > > > > > > +		ret = fwnode_property_read_u32(dev_fwnode(dev),
> > > > > > > > > > > > > > +					       "clock-frequency",
> > > > > > > > > > > > > > +					       &imx219->xclk_freq);
> > > > > > > > > > > > > > +		if (ret)
> > > > > > > > > > > > > > +			return dev_err_probe(dev, ret,
> > > > > > > > > > > > > > +					     "failed to get clock frequency");
> > > > > > > > > > > > > > +	}
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > 
> > > > > > > > > > > > > This doesn't seem specific to the imx219 driver. Could you turn this
> > > > > > > > > > > > > into a generic V4L2 sensor helper that would take a struct device and a
> > > > > > > > > > > > > clock name, and return the frequency, either retrieved from the clock,
> > > > > > > > > > > > > or from the clock-frequency property as a fallback ?
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Some drivers will also need to control the clock, so the clock should
> > > > > > > > > > > > > probably be returned too.
> > > > > > > > > > > > > 
> > > > > > > > > > > > 
> > > > > > > > > > > > Yes, I saw that many sensor drivers have the same issue.
> > > > > > > > > > > > 
> > > > > > > > > > > > I will try to make it into a generic V4L2 helper and send the patches.
> > > > > > > > > > > 
> > > > > > > > > > > There are other such functions in drivers/media/v4l2-core/v4l2-common.c.
> > > > > > > > > > > Perhaps this is where the new helper could be located as well?
> > > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > I was thinking about drivers/media/v4l2-core/v4l2-fwnode.c but if
> > > > > > > > > > v4l2-common.c is more appropriate we can go with that.
> > > > > > > > > 
> > > > > > > > > I admit I have no clue about ACPI, but why is this v4l2 specific? Why
> > > > > > > > > doesn't clock framework do this for us?
> > > > > > > > 
> > > > > > > > The "clock-frequency" isn't really specific to ACPI but it's used on some
> > > > > > > > boards with DT, too, that precede the current clock bindings. Clocks aren't
> > > > > > > > generally available to OS in ACPI either but the sensor drivers still need
> > > > > > > > them. DisCo for Imaging uses "mipi-img-clock-frequency" which DisCo for
> > > > > > > > Imaging code deep down in the ACPI framework will offer to drivers as
> > > > > > > > "clock-frequency". A lot of this is actually specific to cameras. On top of
> > > > > > > > that, camera sensors tend to be devices that are used equally on both DT
> > > > > > > > and ACPI systems, it's quite uncommon elsewhere. Therefore I do think the
> > > > > > > > natural place for this code is actually the V4L2 framework.
> > > > > > > 
> > > > > > > Can ACPI devices support programmable sensor clock frequency ?
> > > > > > 
> > > > > > Do you mean sensor's external clock or PLL? And do you mean programmable
> > > > > > as configured in system firmware or at runtime?
> > > > > 
> > > > > I mean external clock, and configurable at runtime.
> > > > 
> > > > There's basically no standard API to change a clock's frequency at runtime
> > > > (as there is no API to handle clocks) -- which is why we have a property
> > > > instead of that presumed API.
> > > 
> > > I wonder, for ACPI devices that have a clock-frequency property, could
> > > we automatically register a fixed-frequency clock ?
> > 
> > That's an interesting idea.
> > 
> > It still could have potential to break something even if it may be
> > unlikely. I don't know what others are doing with clocks (if anything)
> > outside cameras on ACPI.
> > 
> > Maybe a case for an RFC? Doing this would be nice as we could get rid of
> > one of the few remaining DT/ACPI differences in sensor drivers.
> > 
> > That being said, this should not hold back merging the patch.
> 
> An RFC sounds good. For this specific patch, I think we agree we need a
> helper function. I wonder if the helper could register a fixed clock if
> there's no clocks property, that way we could easily change the
> implementation later while minimizing changes to drivers. Some sort of
> 
> struct clk *devm_v4l2_sensor_clk_get(struct device *dev, const char *name);
> 
> that sensor drivers would call instead of devm_clk_get(). There may be
> issues I don't foresee though, but maybe worth trying ?
> 

I will give it a try and eventually send patches.

Thanks everyone for all the answers.

--
Kind Regards
Mehdi Djait

Re: [PATCH] media: i2c: imx219: Add support for 'clock-frequency' parsing

[Mehdi Djait]

Hi everyone,

On Mon, Feb 24, 2025 at 07:28:26PM +0100, Mehdi Djait wrote:
> Hi Laurent, Hi Sakari,
> 
> On Mon, Feb 24, 2025 at 08:19:35PM +0200, Laurent Pinchart wrote:
> > On Mon, Feb 24, 2025 at 05:06:49PM +0000, Sakari Ailus wrote:
> > > Hi Laurent,
> > > 
> > > On Mon, Feb 24, 2025 at 06:34:21PM +0200, Laurent Pinchart wrote:
> > > > On Mon, Feb 24, 2025 at 04:28:47PM +0000, Sakari Ailus wrote:
> > > > > On Mon, Feb 24, 2025 at 06:16:40PM +0200, Laurent Pinchart wrote:
> > > > > > On Mon, Feb 24, 2025 at 02:38:35PM +0000, Sakari Ailus wrote:
> > > > > > > On Mon, Feb 24, 2025 at 03:25:36PM +0200, Laurent Pinchart wrote:
> > > > > > > > On Mon, Feb 24, 2025 at 12:49:48PM +0000, Sakari Ailus wrote:
> > > > > > > > > On Mon, Feb 24, 2025 at 02:24:29PM +0200, Tomi Valkeinen wrote:
> > > > > > > > > > On 24/02/2025 13:54, Mehdi Djait wrote:
> > > > > > > > > > > On Mon, Feb 24, 2025 at 09:42:12AM +0000, Sakari Ailus wrote:
> > > > > > > > > > > > On Mon, Feb 24, 2025 at 08:59:34AM +0100, Mehdi Djait wrote:
> > > > > > > > > > > > > On Mon, Feb 24, 2025 at 01:06:49AM +0200, Laurent Pinchart wrote:
> > > > > > > > > > > > > > On Thu, Feb 20, 2025 at 04:49:09PM +0100, Mehdi Djait wrote:
> > > > > > > > > > > > > > > Make the clock producer reference lookup optional
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > Add support for ACPI-based platforms by parsing the 'clock-frequency'
> > > > > > > > > > > > > > > property when no clock producer is available
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > Signed-off-by: Mehdi Djait <mehdi.djait@linux.intel.com>
> > > > > > > > > > > > > > > ---
> > > > > > > > > > > > > > >   drivers/media/i2c/imx219.c | 14 ++++++++++++--
> > > > > > > > > > > > > > >   1 file changed, 12 insertions(+), 2 deletions(-)
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > > > > > > > > > > > > > > index 2d54cea113e1..a876a6d80a47 100644
> > > > > > > > > > > > > > > --- a/drivers/media/i2c/imx219.c
> > > > > > > > > > > > > > > +++ b/drivers/media/i2c/imx219.c
> > > > > > > > > > > > > > > @@ -1103,12 +1103,22 @@ static int imx219_probe(struct i2c_client *client)
> > > > > > > > > > > > > > >   				     "failed to initialize CCI\n");
> > > > > > > > > > > > > > >   	/* Get system clock (xclk) */
> > > > > > > > > > > > > > > -	imx219->xclk = devm_clk_get(dev, NULL);
> > > > > > > > > > > > > > > +	imx219->xclk = devm_clk_get_optional(dev, NULL);
> > > > > > > > > > > > > > >   	if (IS_ERR(imx219->xclk))
> > > > > > > > > > > > > > >   		return dev_err_probe(dev, PTR_ERR(imx219->xclk),
> > > > > > > > > > > > > > >   				     "failed to get xclk\n");
> > > > > > > > > > > > > > > -	imx219->xclk_freq = clk_get_rate(imx219->xclk);
> > > > > > > > > > > > > > > +	if (imx219->xclk) {
> > > > > > > > > > > > > > > +		imx219->xclk_freq = clk_get_rate(imx219->xclk);
> > > > > > > > > > > > > > > +	} else {
> > > > > > > > > > > > > > > +		ret = fwnode_property_read_u32(dev_fwnode(dev),
> > > > > > > > > > > > > > > +					       "clock-frequency",
> > > > > > > > > > > > > > > +					       &imx219->xclk_freq);
> > > > > > > > > > > > > > > +		if (ret)
> > > > > > > > > > > > > > > +			return dev_err_probe(dev, ret,
> > > > > > > > > > > > > > > +					     "failed to get clock frequency");
> > > > > > > > > > > > > > > +	}
> > > > > > > > > > > > > > > +
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > This doesn't seem specific to the imx219 driver. Could you turn this
> > > > > > > > > > > > > > into a generic V4L2 sensor helper that would take a struct device and a
> > > > > > > > > > > > > > clock name, and return the frequency, either retrieved from the clock,
> > > > > > > > > > > > > > or from the clock-frequency property as a fallback ?
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > Some drivers will also need to control the clock, so the clock should
> > > > > > > > > > > > > > probably be returned too.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Yes, I saw that many sensor drivers have the same issue.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > I will try to make it into a generic V4L2 helper and send the patches.
> > > > > > > > > > > > 
> > > > > > > > > > > > There are other such functions in drivers/media/v4l2-core/v4l2-common.c.
> > > > > > > > > > > > Perhaps this is where the new helper could be located as well?
> > > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > I was thinking about drivers/media/v4l2-core/v4l2-fwnode.c but if
> > > > > > > > > > > v4l2-common.c is more appropriate we can go with that.
> > > > > > > > > > 
> > > > > > > > > > I admit I have no clue about ACPI, but why is this v4l2 specific? Why
> > > > > > > > > > doesn't clock framework do this for us?
> > > > > > > > > 
> > > > > > > > > The "clock-frequency" isn't really specific to ACPI but it's used on some
> > > > > > > > > boards with DT, too, that precede the current clock bindings. Clocks aren't
> > > > > > > > > generally available to OS in ACPI either but the sensor drivers still need
> > > > > > > > > them. DisCo for Imaging uses "mipi-img-clock-frequency" which DisCo for
> > > > > > > > > Imaging code deep down in the ACPI framework will offer to drivers as
> > > > > > > > > "clock-frequency". A lot of this is actually specific to cameras. On top of
> > > > > > > > > that, camera sensors tend to be devices that are used equally on both DT
> > > > > > > > > and ACPI systems, it's quite uncommon elsewhere. Therefore I do think the
> > > > > > > > > natural place for this code is actually the V4L2 framework.
> > > > > > > > 
> > > > > > > > Can ACPI devices support programmable sensor clock frequency ?
> > > > > > > 
> > > > > > > Do you mean sensor's external clock or PLL? And do you mean programmable
> > > > > > > as configured in system firmware or at runtime?
> > > > > > 
> > > > > > I mean external clock, and configurable at runtime.
> > > > > 
> > > > > There's basically no standard API to change a clock's frequency at runtime
> > > > > (as there is no API to handle clocks) -- which is why we have a property
> > > > > instead of that presumed API.
> > > > 
> > > > I wonder, for ACPI devices that have a clock-frequency property, could
> > > > we automatically register a fixed-frequency clock ?
> > > 
> > > That's an interesting idea.
> > > 
> > > It still could have potential to break something even if it may be
> > > unlikely. I don't know what others are doing with clocks (if anything)
> > > outside cameras on ACPI.
> > > 
> > > Maybe a case for an RFC? Doing this would be nice as we could get rid of
> > > one of the few remaining DT/ACPI differences in sensor drivers.
> > > 
> > > That being said, this should not hold back merging the patch.
> > 
> > An RFC sounds good. For this specific patch, I think we agree we need a
> > helper function. I wonder if the helper could register a fixed clock if
> > there's no clocks property, that way we could easily change the
> > implementation later while minimizing changes to drivers. Some sort of
> > 
> > struct clk *devm_v4l2_sensor_clk_get(struct device *dev, const char *name);
> > 
> > that sensor drivers would call instead of devm_clk_get(). There may be
> > issues I don't foresee though, but maybe worth trying ?
> > 
> 
> I will give it a try and eventually send patches.
> 
> Thanks everyone for all the answers.

While looking around, I found the following:

- ClockInput resource added to ACPI v6.5: https://uefi.org/specs/ACPI/6.5/19_ASL_Reference.html#clockinput-clock-input-resource-descriptor-macro
- commit adding ClockInput resource to acpica: https://github.com/acpica/acpica/commit/661feab5ee01a34af95a389a18c82e79f1aba05a
- commit kernel upstream: 520d4a0ee5b6d9c7a1258ace6caa13a94ac35ef8 "ACPICA: add support
  for ClockInput resource (v6.5)"

this does not mean we can use it: I found this out-of-tree patch to supports fixed clock sources
https://github.com/niyas-sait/linux-acpi/blob/main/0001-acpi-add-clock-bindings-for-fixed-clock-resources.patch
it was not sent to the acpi mailing list. It was mentioned in this
dicussion: https://lore.kernel.org/linux-kernel/78763d69bae04204b2af37201b09f8b5@huawei.com/

Another interesting link: https://linaro.atlassian.net/wiki/spaces/CLIENTPC/pages/28822175758/ACPI+Clock+Input+Resources

Anyway, things to look out for as it seems that people are working on
this.

--
Kind Regards
Mehdi Djait

Re: [PATCH] media: i2c: imx219: Add support for 'clock-frequency' parsing

[Sakari Ailus]

Hi Mehdi,

On Tue, Feb 25, 2025 at 12:11:30PM +0100, Mehdi Djait wrote:
> Hi everyone,
> 
> On Mon, Feb 24, 2025 at 07:28:26PM +0100, Mehdi Djait wrote:
> > Hi Laurent, Hi Sakari,
> > 
> > On Mon, Feb 24, 2025 at 08:19:35PM +0200, Laurent Pinchart wrote:
> > > On Mon, Feb 24, 2025 at 05:06:49PM +0000, Sakari Ailus wrote:
> > > > Hi Laurent,
> > > > 
> > > > On Mon, Feb 24, 2025 at 06:34:21PM +0200, Laurent Pinchart wrote:
> > > > > On Mon, Feb 24, 2025 at 04:28:47PM +0000, Sakari Ailus wrote:
> > > > > > On Mon, Feb 24, 2025 at 06:16:40PM +0200, Laurent Pinchart wrote:
> > > > > > > On Mon, Feb 24, 2025 at 02:38:35PM +0000, Sakari Ailus wrote:
> > > > > > > > On Mon, Feb 24, 2025 at 03:25:36PM +0200, Laurent Pinchart wrote:
> > > > > > > > > On Mon, Feb 24, 2025 at 12:49:48PM +0000, Sakari Ailus wrote:
> > > > > > > > > > On Mon, Feb 24, 2025 at 02:24:29PM +0200, Tomi Valkeinen wrote:
> > > > > > > > > > > On 24/02/2025 13:54, Mehdi Djait wrote:
> > > > > > > > > > > > On Mon, Feb 24, 2025 at 09:42:12AM +0000, Sakari Ailus wrote:
> > > > > > > > > > > > > On Mon, Feb 24, 2025 at 08:59:34AM +0100, Mehdi Djait wrote:
> > > > > > > > > > > > > > On Mon, Feb 24, 2025 at 01:06:49AM +0200, Laurent Pinchart wrote:
> > > > > > > > > > > > > > > On Thu, Feb 20, 2025 at 04:49:09PM +0100, Mehdi Djait wrote:
> > > > > > > > > > > > > > > > Make the clock producer reference lookup optional
> > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > Add support for ACPI-based platforms by parsing the 'clock-frequency'
> > > > > > > > > > > > > > > > property when no clock producer is available
> > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > Signed-off-by: Mehdi Djait <mehdi.djait@linux.intel.com>
> > > > > > > > > > > > > > > > ---
> > > > > > > > > > > > > > > >   drivers/media/i2c/imx219.c | 14 ++++++++++++--
> > > > > > > > > > > > > > > >   1 file changed, 12 insertions(+), 2 deletions(-)
> > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > > > > > > > > > > > > > > > index 2d54cea113e1..a876a6d80a47 100644
> > > > > > > > > > > > > > > > --- a/drivers/media/i2c/imx219.c
> > > > > > > > > > > > > > > > +++ b/drivers/media/i2c/imx219.c
> > > > > > > > > > > > > > > > @@ -1103,12 +1103,22 @@ static int imx219_probe(struct i2c_client *client)
> > > > > > > > > > > > > > > >   				     "failed to initialize CCI\n");
> > > > > > > > > > > > > > > >   	/* Get system clock (xclk) */
> > > > > > > > > > > > > > > > -	imx219->xclk = devm_clk_get(dev, NULL);
> > > > > > > > > > > > > > > > +	imx219->xclk = devm_clk_get_optional(dev, NULL);
> > > > > > > > > > > > > > > >   	if (IS_ERR(imx219->xclk))
> > > > > > > > > > > > > > > >   		return dev_err_probe(dev, PTR_ERR(imx219->xclk),
> > > > > > > > > > > > > > > >   				     "failed to get xclk\n");
> > > > > > > > > > > > > > > > -	imx219->xclk_freq = clk_get_rate(imx219->xclk);
> > > > > > > > > > > > > > > > +	if (imx219->xclk) {
> > > > > > > > > > > > > > > > +		imx219->xclk_freq = clk_get_rate(imx219->xclk);
> > > > > > > > > > > > > > > > +	} else {
> > > > > > > > > > > > > > > > +		ret = fwnode_property_read_u32(dev_fwnode(dev),
> > > > > > > > > > > > > > > > +					       "clock-frequency",
> > > > > > > > > > > > > > > > +					       &imx219->xclk_freq);
> > > > > > > > > > > > > > > > +		if (ret)
> > > > > > > > > > > > > > > > +			return dev_err_probe(dev, ret,
> > > > > > > > > > > > > > > > +					     "failed to get clock frequency");
> > > > > > > > > > > > > > > > +	}
> > > > > > > > > > > > > > > > +
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > This doesn't seem specific to the imx219 driver. Could you turn this
> > > > > > > > > > > > > > > into a generic V4L2 sensor helper that would take a struct device and a
> > > > > > > > > > > > > > > clock name, and return the frequency, either retrieved from the clock,
> > > > > > > > > > > > > > > or from the clock-frequency property as a fallback ?
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > Some drivers will also need to control the clock, so the clock should
> > > > > > > > > > > > > > > probably be returned too.
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > Yes, I saw that many sensor drivers have the same issue.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > I will try to make it into a generic V4L2 helper and send the patches.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > There are other such functions in drivers/media/v4l2-core/v4l2-common.c.
> > > > > > > > > > > > > Perhaps this is where the new helper could be located as well?
> > > > > > > > > > > > > 
> > > > > > > > > > > > 
> > > > > > > > > > > > I was thinking about drivers/media/v4l2-core/v4l2-fwnode.c but if
> > > > > > > > > > > > v4l2-common.c is more appropriate we can go with that.
> > > > > > > > > > > 
> > > > > > > > > > > I admit I have no clue about ACPI, but why is this v4l2 specific? Why
> > > > > > > > > > > doesn't clock framework do this for us?
> > > > > > > > > > 
> > > > > > > > > > The "clock-frequency" isn't really specific to ACPI but it's used on some
> > > > > > > > > > boards with DT, too, that precede the current clock bindings. Clocks aren't
> > > > > > > > > > generally available to OS in ACPI either but the sensor drivers still need
> > > > > > > > > > them. DisCo for Imaging uses "mipi-img-clock-frequency" which DisCo for
> > > > > > > > > > Imaging code deep down in the ACPI framework will offer to drivers as
> > > > > > > > > > "clock-frequency". A lot of this is actually specific to cameras. On top of
> > > > > > > > > > that, camera sensors tend to be devices that are used equally on both DT
> > > > > > > > > > and ACPI systems, it's quite uncommon elsewhere. Therefore I do think the
> > > > > > > > > > natural place for this code is actually the V4L2 framework.
> > > > > > > > > 
> > > > > > > > > Can ACPI devices support programmable sensor clock frequency ?
> > > > > > > > 
> > > > > > > > Do you mean sensor's external clock or PLL? And do you mean programmable
> > > > > > > > as configured in system firmware or at runtime?
> > > > > > > 
> > > > > > > I mean external clock, and configurable at runtime.
> > > > > > 
> > > > > > There's basically no standard API to change a clock's frequency at runtime
> > > > > > (as there is no API to handle clocks) -- which is why we have a property
> > > > > > instead of that presumed API.
> > > > > 
> > > > > I wonder, for ACPI devices that have a clock-frequency property, could
> > > > > we automatically register a fixed-frequency clock ?
> > > > 
> > > > That's an interesting idea.
> > > > 
> > > > It still could have potential to break something even if it may be
> > > > unlikely. I don't know what others are doing with clocks (if anything)
> > > > outside cameras on ACPI.
> > > > 
> > > > Maybe a case for an RFC? Doing this would be nice as we could get rid of
> > > > one of the few remaining DT/ACPI differences in sensor drivers.
> > > > 
> > > > That being said, this should not hold back merging the patch.
> > > 
> > > An RFC sounds good. For this specific patch, I think we agree we need a
> > > helper function. I wonder if the helper could register a fixed clock if
> > > there's no clocks property, that way we could easily change the
> > > implementation later while minimizing changes to drivers. Some sort of
> > > 
> > > struct clk *devm_v4l2_sensor_clk_get(struct device *dev, const char *name);
> > > 
> > > that sensor drivers would call instead of devm_clk_get(). There may be
> > > issues I don't foresee though, but maybe worth trying ?
> > > 
> > 
> > I will give it a try and eventually send patches.
> > 
> > Thanks everyone for all the answers.
> 
> While looking around, I found the following:
> 
> - ClockInput resource added to ACPI v6.5: https://uefi.org/specs/ACPI/6.5/19_ASL_Reference.html#clockinput-clock-input-resource-descriptor-macro
> - commit adding ClockInput resource to acpica: https://github.com/acpica/acpica/commit/661feab5ee01a34af95a389a18c82e79f1aba05a
> - commit kernel upstream: 520d4a0ee5b6d9c7a1258ace6caa13a94ac35ef8 "ACPICA: add support
>   for ClockInput resource (v6.5)"
> 
> this does not mean we can use it: I found this out-of-tree patch to supports fixed clock sources
> https://github.com/niyas-sait/linux-acpi/blob/main/0001-acpi-add-clock-bindings-for-fixed-clock-resources.patch
> it was not sent to the acpi mailing list. It was mentioned in this
> dicussion: https://lore.kernel.org/linux-kernel/78763d69bae04204b2af37201b09f8b5@huawei.com/
> 
> Another interesting link: https://linaro.atlassian.net/wiki/spaces/CLIENTPC/pages/28822175758/ACPI+Clock+Input+Resources
> 
> Anyway, things to look out for as it seems that people are working on
> this.

Nice find. This doesn't answer to the existing needs but good to know
there's work being done to support this.

I still think what Laurent suggested should be doable. Clocks based on
properties just need to be parsed after resource descriptors, that's all it
should take for the two to co-exist.

-- 
Regards,

Sakari Ailus