[PATCH] media: i2c: ccs: Fix link frequency control range update

[PATCH] media: i2c: ccs: Fix link frequency control range update

[Laurent Pinchart]

When updating the link frequency control range in response to a format
change, the minimum value passed to the __v4l2_ctrl_modify_range()
function is hardcoded to 0, while there's no guarantee that the first
link frequency in the menu is valid for the selected format. Fix it by
getting using the index of the first bit set in the valid link
frequencies mask.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
I noticed this issue in the CCS driver while working on a different
sensor driver. I haven't tested this patch.
---
 drivers/media/i2c/ccs/ccs-core.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c
index e1ae0f9fad43..5257dc4912ae 100644
--- a/drivers/media/i2c/ccs/ccs-core.c
+++ b/drivers/media/i2c/ccs/ccs-core.c
@@ -2143,6 +2143,7 @@ static int ccs_set_format_source(struct v4l2_subdev *subdev,
 		*old_csi_format = sensor->csi_format;
 	unsigned long *valid_link_freqs;
 	u32 code = fmt->format.code;
+	unsigned int min, max;
 	unsigned int i;
 	int rval;
 
@@ -2179,10 +2180,13 @@ static int ccs_set_format_source(struct v4l2_subdev *subdev,
 		&sensor->valid_link_freqs[sensor->csi_format->compressed
 					  - sensor->compressed_min_bpp];
 
-	__v4l2_ctrl_modify_range(
-		sensor->link_freq, 0,
-		__fls(*valid_link_freqs), ~*valid_link_freqs,
-		__ffs(*valid_link_freqs));
+	min = __ffs(*valid_link_freqs);
+	man = __fls(*valid_link_freqs);
+
+	ret = __v4l2_ctrl_modify_range(sensor->link_freq, min, max,
+				       ~*valid_link_freqs, min);
+	if (ret)
+		return ret;
 
 	return ccs_pll_update(sensor);
 }

base-commit: afcd48134c58d6af45fb3fdb648f1260b20f2326
-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: i2c: ccs: Fix link frequency control range update

[Sakari Ailus]

Hi Laurent,

Thanks for the patch.

On Sat, Jun 29, 2024 at 12:26:03AM +0300, Laurent Pinchart wrote:
> When updating the link frequency control range in response to a format
> change, the minimum value passed to the __v4l2_ctrl_modify_range()
> function is hardcoded to 0, while there's no guarantee that the first
> link frequency in the menu is valid for the selected format. Fix it by
> getting using the index of the first bit set in the valid link
> frequencies mask.

Is this a problem? The bitmask does tell which ones are valid, doesn't it?

The minimum value will also be zero after control initialisation before
this function gets called. This should be also taken into account.

> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> I noticed this issue in the CCS driver while working on a different
> sensor driver. I haven't tested this patch.
> ---
>  drivers/media/i2c/ccs/ccs-core.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c
> index e1ae0f9fad43..5257dc4912ae 100644
> --- a/drivers/media/i2c/ccs/ccs-core.c
> +++ b/drivers/media/i2c/ccs/ccs-core.c
> @@ -2143,6 +2143,7 @@ static int ccs_set_format_source(struct v4l2_subdev *subdev,
>  		*old_csi_format = sensor->csi_format;
>  	unsigned long *valid_link_freqs;
>  	u32 code = fmt->format.code;
> +	unsigned int min, max;
>  	unsigned int i;
>  	int rval;
>  
> @@ -2179,10 +2180,13 @@ static int ccs_set_format_source(struct v4l2_subdev *subdev,
>  		&sensor->valid_link_freqs[sensor->csi_format->compressed
>  					  - sensor->compressed_min_bpp];
>  
> -	__v4l2_ctrl_modify_range(
> -		sensor->link_freq, 0,
> -		__fls(*valid_link_freqs), ~*valid_link_freqs,
> -		__ffs(*valid_link_freqs));
> +	min = __ffs(*valid_link_freqs);
> +	man = __fls(*valid_link_freqs);
> +
> +	ret = __v4l2_ctrl_modify_range(sensor->link_freq, min, max,
> +				       ~*valid_link_freqs, min);

As this doesn't effect any actual change the applying of which could fail,
you'd have to have an issue with the argument values themselves. I wouldn't
add a check here. Although if you do, the sensor configuration should be
returned to the state before the call which would probably be worth a new
patch.

> +	if (ret)
> +		return ret;
>  
>  	return ccs_pll_update(sensor);
>  }
> 
> base-commit: afcd48134c58d6af45fb3fdb648f1260b20f2326

-- 
Kind regards,

Sakari Ailus

Re: [PATCH] media: i2c: ccs: Fix link frequency control range update

[Laurent Pinchart]

On Sat, Jun 29, 2024 at 08:56:09AM +0000, Sakari Ailus wrote:
> Hi Laurent,
> 
> Thanks for the patch.
> 
> On Sat, Jun 29, 2024 at 12:26:03AM +0300, Laurent Pinchart wrote:
> > When updating the link frequency control range in response to a format
> > change, the minimum value passed to the __v4l2_ctrl_modify_range()
> > function is hardcoded to 0, while there's no guarantee that the first
> > link frequency in the menu is valid for the selected format. Fix it by
> > getting using the index of the first bit set in the valid link
> > frequencies mask.
> 
> Is this a problem? The bitmask does tell which ones are valid, doesn't it?

I noticed that the new range wasn't applied in my sensor driver when the
minimum was set to 0 and the mask didn't include that bit. However,
that's because I had the default value wrong, which caused
__v4l2_ctrl_modify_range() to error out. I thought the same applied to
the minimum, but that doesn't seem to be the case. Isn't it still
clearer to set the correct minimum, given that it is already computed
anyway, to be used as a default value ?

> The minimum value will also be zero after control initialisation before
> this function gets called. This should be also taken into account.
> 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > I noticed this issue in the CCS driver while working on a different
> > sensor driver. I haven't tested this patch.
> > ---
> >  drivers/media/i2c/ccs/ccs-core.c | 12 ++++++++----
> >  1 file changed, 8 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c
> > index e1ae0f9fad43..5257dc4912ae 100644
> > --- a/drivers/media/i2c/ccs/ccs-core.c
> > +++ b/drivers/media/i2c/ccs/ccs-core.c
> > @@ -2143,6 +2143,7 @@ static int ccs_set_format_source(struct v4l2_subdev *subdev,
> >  		*old_csi_format = sensor->csi_format;
> >  	unsigned long *valid_link_freqs;
> >  	u32 code = fmt->format.code;
> > +	unsigned int min, max;
> >  	unsigned int i;
> >  	int rval;
> >  
> > @@ -2179,10 +2180,13 @@ static int ccs_set_format_source(struct v4l2_subdev *subdev,
> >  		&sensor->valid_link_freqs[sensor->csi_format->compressed
> >  					  - sensor->compressed_min_bpp];
> >  
> > -	__v4l2_ctrl_modify_range(
> > -		sensor->link_freq, 0,
> > -		__fls(*valid_link_freqs), ~*valid_link_freqs,
> > -		__ffs(*valid_link_freqs));
> > +	min = __ffs(*valid_link_freqs);
> > +	man = __fls(*valid_link_freqs);
> > +
> > +	ret = __v4l2_ctrl_modify_range(sensor->link_freq, min, max,
> > +				       ~*valid_link_freqs, min);
> 
> As this doesn't effect any actual change the applying of which could fail,
> you'd have to have an issue with the argument values themselves. I wouldn't
> add a check here. Although if you do, the sensor configuration should be
> returned to the state before the call which would probably be worth a new
> patch.

The lack of a similar check caused my driver to silently keep the
current range, and it took me a while to debug that. I however agree
that, if the arguments are right, the check isn't needed. Maybe it can
be dropped, as the arguments are correct.

> > +	if (ret)
> > +		return ret;
> >  
> >  	return ccs_pll_update(sensor);
> >  }
> > 
> > base-commit: afcd48134c58d6af45fb3fdb648f1260b20f2326

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: i2c: ccs: Fix link frequency control range update

[Sakari Ailus]

Hi Laurent,

On Sat, Jun 29, 2024 at 01:52:22PM +0300, Laurent Pinchart wrote:
> On Sat, Jun 29, 2024 at 08:56:09AM +0000, Sakari Ailus wrote:
> > Hi Laurent,
> > 
> > Thanks for the patch.
> > 
> > On Sat, Jun 29, 2024 at 12:26:03AM +0300, Laurent Pinchart wrote:
> > > When updating the link frequency control range in response to a format
> > > change, the minimum value passed to the __v4l2_ctrl_modify_range()
> > > function is hardcoded to 0, while there's no guarantee that the first
> > > link frequency in the menu is valid for the selected format. Fix it by
> > > getting using the index of the first bit set in the valid link
> > > frequencies mask.
> > 
> > Is this a problem? The bitmask does tell which ones are valid, doesn't it?
> 
> I noticed that the new range wasn't applied in my sensor driver when the
> minimum was set to 0 and the mask didn't include that bit. However,
> that's because I had the default value wrong, which caused
> __v4l2_ctrl_modify_range() to error out. I thought the same applied to
> the minimum, but that doesn't seem to be the case. Isn't it still
> clearer to set the correct minimum, given that it is already computed
> anyway, to be used as a default value ?

I guess from user space point of view this could be helpful, yes. I'm fine
with changing this.

> 
> > The minimum value will also be zero after control initialisation before
> > this function gets called. This should be also taken into account.
> > 
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > > I noticed this issue in the CCS driver while working on a different
> > > sensor driver. I haven't tested this patch.
> > > ---
> > >  drivers/media/i2c/ccs/ccs-core.c | 12 ++++++++----
> > >  1 file changed, 8 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c
> > > index e1ae0f9fad43..5257dc4912ae 100644
> > > --- a/drivers/media/i2c/ccs/ccs-core.c
> > > +++ b/drivers/media/i2c/ccs/ccs-core.c
> > > @@ -2143,6 +2143,7 @@ static int ccs_set_format_source(struct v4l2_subdev *subdev,
> > >  		*old_csi_format = sensor->csi_format;
> > >  	unsigned long *valid_link_freqs;
> > >  	u32 code = fmt->format.code;
> > > +	unsigned int min, max;
> > >  	unsigned int i;
> > >  	int rval;
> > >  
> > > @@ -2179,10 +2180,13 @@ static int ccs_set_format_source(struct v4l2_subdev *subdev,
> > >  		&sensor->valid_link_freqs[sensor->csi_format->compressed
> > >  					  - sensor->compressed_min_bpp];
> > >  
> > > -	__v4l2_ctrl_modify_range(
> > > -		sensor->link_freq, 0,
> > > -		__fls(*valid_link_freqs), ~*valid_link_freqs,
> > > -		__ffs(*valid_link_freqs));
> > > +	min = __ffs(*valid_link_freqs);
> > > +	man = __fls(*valid_link_freqs);
> > > +
> > > +	ret = __v4l2_ctrl_modify_range(sensor->link_freq, min, max,
> > > +				       ~*valid_link_freqs, min);
> > 
> > As this doesn't effect any actual change the applying of which could fail,
> > you'd have to have an issue with the argument values themselves. I wouldn't
> > add a check here. Although if you do, the sensor configuration should be
> > returned to the state before the call which would probably be worth a new
> > patch.
> 
> The lack of a similar check caused my driver to silently keep the
> current range, and it took me a while to debug that. I however agree
> that, if the arguments are right, the check isn't needed. Maybe it can
> be dropped, as the arguments are correct.

Alternatively there should be a dev_warn(), too, that this is a driver bug.

> 
> > > +	if (ret)
> > > +		return ret;
> > >  
> > >  	return ccs_pll_update(sensor);
> > >  }
> > > 
> > > base-commit: afcd48134c58d6af45fb3fdb648f1260b20f2326
> 

-- 
Kind regards,

Sakari Ailus

Re: [PATCH] media: i2c: ccs: Fix link frequency control range update

[Laurent Pinchart]

On Sat, Jun 29, 2024 at 11:34:11AM +0000, Sakari Ailus wrote:
> On Sat, Jun 29, 2024 at 01:52:22PM +0300, Laurent Pinchart wrote:
> > On Sat, Jun 29, 2024 at 08:56:09AM +0000, Sakari Ailus wrote:
> > > Hi Laurent,
> > > 
> > > Thanks for the patch.
> > > 
> > > On Sat, Jun 29, 2024 at 12:26:03AM +0300, Laurent Pinchart wrote:
> > > > When updating the link frequency control range in response to a format
> > > > change, the minimum value passed to the __v4l2_ctrl_modify_range()
> > > > function is hardcoded to 0, while there's no guarantee that the first
> > > > link frequency in the menu is valid for the selected format. Fix it by
> > > > getting using the index of the first bit set in the valid link
> > > > frequencies mask.
> > > 
> > > Is this a problem? The bitmask does tell which ones are valid, doesn't it?
> > 
> > I noticed that the new range wasn't applied in my sensor driver when the
> > minimum was set to 0 and the mask didn't include that bit. However,
> > that's because I had the default value wrong, which caused
> > __v4l2_ctrl_modify_range() to error out. I thought the same applied to
> > the minimum, but that doesn't seem to be the case. Isn't it still
> > clearer to set the correct minimum, given that it is already computed
> > anyway, to be used as a default value ?
> 
> I guess from user space point of view this could be helpful, yes. I'm fine
> with changing this.

Another option would be for the control framework to adjust the minimum
and maximum based on the mask.

> > > The minimum value will also be zero after control initialisation before
> > > this function gets called. This should be also taken into account.
> > > 
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > ---
> > > > I noticed this issue in the CCS driver while working on a different
> > > > sensor driver. I haven't tested this patch.
> > > > ---
> > > >  drivers/media/i2c/ccs/ccs-core.c | 12 ++++++++----
> > > >  1 file changed, 8 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c
> > > > index e1ae0f9fad43..5257dc4912ae 100644
> > > > --- a/drivers/media/i2c/ccs/ccs-core.c
> > > > +++ b/drivers/media/i2c/ccs/ccs-core.c
> > > > @@ -2143,6 +2143,7 @@ static int ccs_set_format_source(struct v4l2_subdev *subdev,
> > > >  		*old_csi_format = sensor->csi_format;
> > > >  	unsigned long *valid_link_freqs;
> > > >  	u32 code = fmt->format.code;
> > > > +	unsigned int min, max;
> > > >  	unsigned int i;
> > > >  	int rval;
> > > >  
> > > > @@ -2179,10 +2180,13 @@ static int ccs_set_format_source(struct v4l2_subdev *subdev,
> > > >  		&sensor->valid_link_freqs[sensor->csi_format->compressed
> > > >  					  - sensor->compressed_min_bpp];
> > > >  
> > > > -	__v4l2_ctrl_modify_range(
> > > > -		sensor->link_freq, 0,
> > > > -		__fls(*valid_link_freqs), ~*valid_link_freqs,
> > > > -		__ffs(*valid_link_freqs));
> > > > +	min = __ffs(*valid_link_freqs);
> > > > +	man = __fls(*valid_link_freqs);
> > > > +
> > > > +	ret = __v4l2_ctrl_modify_range(sensor->link_freq, min, max,
> > > > +				       ~*valid_link_freqs, min);
> > > 
> > > As this doesn't effect any actual change the applying of which could fail,
> > > you'd have to have an issue with the argument values themselves. I wouldn't
> > > add a check here. Although if you do, the sensor configuration should be
> > > returned to the state before the call which would probably be worth a new
> > > patch.
> > 
> > The lack of a similar check caused my driver to silently keep the
> > current range, and it took me a while to debug that. I however agree
> > that, if the arguments are right, the check isn't needed. Maybe it can
> > be dropped, as the arguments are correct.
> 
> Alternatively there should be a dev_warn(), too, that this is a driver bug.

Do you think we should add the warning to the __v4l2_ctrl_modify_range()
function, or are there use cases where it could fail during normal
operation ?

> > > > +	if (ret)
> > > > +		return ret;
> > > >  
> > > >  	return ccs_pll_update(sensor);
> > > >  }
> > > > 
> > > > base-commit: afcd48134c58d6af45fb3fdb648f1260b20f2326

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: i2c: ccs: Fix link frequency control range update

[Sakari Ailus]

On Sat, Jun 29, 2024 at 02:52:04PM +0300, Laurent Pinchart wrote:
> On Sat, Jun 29, 2024 at 11:34:11AM +0000, Sakari Ailus wrote:
> > On Sat, Jun 29, 2024 at 01:52:22PM +0300, Laurent Pinchart wrote:
> > > On Sat, Jun 29, 2024 at 08:56:09AM +0000, Sakari Ailus wrote:
> > > > Hi Laurent,
> > > > 
> > > > Thanks for the patch.
> > > > 
> > > > On Sat, Jun 29, 2024 at 12:26:03AM +0300, Laurent Pinchart wrote:
> > > > > When updating the link frequency control range in response to a format
> > > > > change, the minimum value passed to the __v4l2_ctrl_modify_range()
> > > > > function is hardcoded to 0, while there's no guarantee that the first
> > > > > link frequency in the menu is valid for the selected format. Fix it by
> > > > > getting using the index of the first bit set in the valid link
> > > > > frequencies mask.
> > > > 
> > > > Is this a problem? The bitmask does tell which ones are valid, doesn't it?
> > > 
> > > I noticed that the new range wasn't applied in my sensor driver when the
> > > minimum was set to 0 and the mask didn't include that bit. However,
> > > that's because I had the default value wrong, which caused
> > > __v4l2_ctrl_modify_range() to error out. I thought the same applied to
> > > the minimum, but that doesn't seem to be the case. Isn't it still
> > > clearer to set the correct minimum, given that it is already computed
> > > anyway, to be used as a default value ?
> > 
> > I guess from user space point of view this could be helpful, yes. I'm fine
> > with changing this.
> 
> Another option would be for the control framework to adjust the minimum
> and maximum based on the mask.

I wonder what Hans (now cc'd) thinks. I think it's actually a good idea,
given that the minimum and maximum could change dynamically anyway.

> 
> > > > The minimum value will also be zero after control initialisation before
> > > > this function gets called. This should be also taken into account.
> > > > 
> > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > ---
> > > > > I noticed this issue in the CCS driver while working on a different
> > > > > sensor driver. I haven't tested this patch.
> > > > > ---
> > > > >  drivers/media/i2c/ccs/ccs-core.c | 12 ++++++++----
> > > > >  1 file changed, 8 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c
> > > > > index e1ae0f9fad43..5257dc4912ae 100644
> > > > > --- a/drivers/media/i2c/ccs/ccs-core.c
> > > > > +++ b/drivers/media/i2c/ccs/ccs-core.c
> > > > > @@ -2143,6 +2143,7 @@ static int ccs_set_format_source(struct v4l2_subdev *subdev,
> > > > >  		*old_csi_format = sensor->csi_format;
> > > > >  	unsigned long *valid_link_freqs;
> > > > >  	u32 code = fmt->format.code;
> > > > > +	unsigned int min, max;
> > > > >  	unsigned int i;
> > > > >  	int rval;
> > > > >  
> > > > > @@ -2179,10 +2180,13 @@ static int ccs_set_format_source(struct v4l2_subdev *subdev,
> > > > >  		&sensor->valid_link_freqs[sensor->csi_format->compressed
> > > > >  					  - sensor->compressed_min_bpp];
> > > > >  
> > > > > -	__v4l2_ctrl_modify_range(
> > > > > -		sensor->link_freq, 0,
> > > > > -		__fls(*valid_link_freqs), ~*valid_link_freqs,
> > > > > -		__ffs(*valid_link_freqs));
> > > > > +	min = __ffs(*valid_link_freqs);
> > > > > +	man = __fls(*valid_link_freqs);
> > > > > +
> > > > > +	ret = __v4l2_ctrl_modify_range(sensor->link_freq, min, max,
> > > > > +				       ~*valid_link_freqs, min);
> > > > 
> > > > As this doesn't effect any actual change the applying of which could fail,
> > > > you'd have to have an issue with the argument values themselves. I wouldn't
> > > > add a check here. Although if you do, the sensor configuration should be
> > > > returned to the state before the call which would probably be worth a new
> > > > patch.
> > > 
> > > The lack of a similar check caused my driver to silently keep the
> > > current range, and it took me a while to debug that. I however agree
> > > that, if the arguments are right, the check isn't needed. Maybe it can
> > > be dropped, as the arguments are correct.
> > 
> > Alternatively there should be a dev_warn(), too, that this is a driver bug.
> 
> Do you think we should add the warning to the __v4l2_ctrl_modify_range()
> function, or are there use cases where it could fail during normal
> operation ?

If modifying the range results in changing the control value, then this
results in (on I²C devices) an I²C write that can fail.

The CCS driver writes the configuration to the sensor when streaming starts
so there's no actual write operation resulting from this.

> 
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > >  
> > > > >  	return ccs_pll_update(sensor);
> > > > >  }
> > > > > 
> > > > > base-commit: afcd48134c58d6af45fb3fdb648f1260b20f2326
> 

-- 
Sakari Ailus

Re: [PATCH] media: i2c: ccs: Fix link frequency control range update

[Laurent Pinchart]

On Sat, Jun 29, 2024 at 11:58:35AM +0000, Sakari Ailus wrote:
> On Sat, Jun 29, 2024 at 02:52:04PM +0300, Laurent Pinchart wrote:
> > On Sat, Jun 29, 2024 at 11:34:11AM +0000, Sakari Ailus wrote:
> > > On Sat, Jun 29, 2024 at 01:52:22PM +0300, Laurent Pinchart wrote:
> > > > On Sat, Jun 29, 2024 at 08:56:09AM +0000, Sakari Ailus wrote:
> > > > > On Sat, Jun 29, 2024 at 12:26:03AM +0300, Laurent Pinchart wrote:
> > > > > > When updating the link frequency control range in response to a format
> > > > > > change, the minimum value passed to the __v4l2_ctrl_modify_range()
> > > > > > function is hardcoded to 0, while there's no guarantee that the first
> > > > > > link frequency in the menu is valid for the selected format. Fix it by
> > > > > > getting using the index of the first bit set in the valid link
> > > > > > frequencies mask.
> > > > > 
> > > > > Is this a problem? The bitmask does tell which ones are valid, doesn't it?
> > > > 
> > > > I noticed that the new range wasn't applied in my sensor driver when the
> > > > minimum was set to 0 and the mask didn't include that bit. However,
> > > > that's because I had the default value wrong, which caused
> > > > __v4l2_ctrl_modify_range() to error out. I thought the same applied to
> > > > the minimum, but that doesn't seem to be the case. Isn't it still
> > > > clearer to set the correct minimum, given that it is already computed
> > > > anyway, to be used as a default value ?
> > > 
> > > I guess from user space point of view this could be helpful, yes. I'm fine
> > > with changing this.
> > 
> > Another option would be for the control framework to adjust the minimum
> > and maximum based on the mask.
> 
> I wonder what Hans (now cc'd) thinks. I think it's actually a good idea,
> given that the minimum and maximum could change dynamically anyway.

Let's wait for comments before deciding what to do with this patch.

> > > > > The minimum value will also be zero after control initialisation before
> > > > > this function gets called. This should be also taken into account.
> > > > > 
> > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > > ---
> > > > > > I noticed this issue in the CCS driver while working on a different
> > > > > > sensor driver. I haven't tested this patch.
> > > > > > ---
> > > > > >  drivers/media/i2c/ccs/ccs-core.c | 12 ++++++++----
> > > > > >  1 file changed, 8 insertions(+), 4 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c
> > > > > > index e1ae0f9fad43..5257dc4912ae 100644
> > > > > > --- a/drivers/media/i2c/ccs/ccs-core.c
> > > > > > +++ b/drivers/media/i2c/ccs/ccs-core.c
> > > > > > @@ -2143,6 +2143,7 @@ static int ccs_set_format_source(struct v4l2_subdev *subdev,
> > > > > >  		*old_csi_format = sensor->csi_format;
> > > > > >  	unsigned long *valid_link_freqs;
> > > > > >  	u32 code = fmt->format.code;
> > > > > > +	unsigned int min, max;
> > > > > >  	unsigned int i;
> > > > > >  	int rval;
> > > > > >  
> > > > > > @@ -2179,10 +2180,13 @@ static int ccs_set_format_source(struct v4l2_subdev *subdev,
> > > > > >  		&sensor->valid_link_freqs[sensor->csi_format->compressed
> > > > > >  					  - sensor->compressed_min_bpp];
> > > > > >  
> > > > > > -	__v4l2_ctrl_modify_range(
> > > > > > -		sensor->link_freq, 0,
> > > > > > -		__fls(*valid_link_freqs), ~*valid_link_freqs,
> > > > > > -		__ffs(*valid_link_freqs));
> > > > > > +	min = __ffs(*valid_link_freqs);
> > > > > > +	man = __fls(*valid_link_freqs);
> > > > > > +
> > > > > > +	ret = __v4l2_ctrl_modify_range(sensor->link_freq, min, max,
> > > > > > +				       ~*valid_link_freqs, min);
> > > > > 
> > > > > As this doesn't effect any actual change the applying of which could fail,
> > > > > you'd have to have an issue with the argument values themselves. I wouldn't
> > > > > add a check here. Although if you do, the sensor configuration should be
> > > > > returned to the state before the call which would probably be worth a new
> > > > > patch.
> > > > 
> > > > The lack of a similar check caused my driver to silently keep the
> > > > current range, and it took me a while to debug that. I however agree
> > > > that, if the arguments are right, the check isn't needed. Maybe it can
> > > > be dropped, as the arguments are correct.
> > > 
> > > Alternatively there should be a dev_warn(), too, that this is a driver bug.
> > 
> > Do you think we should add the warning to the __v4l2_ctrl_modify_range()
> > function, or are there use cases where it could fail during normal
> > operation ?
> 
> If modifying the range results in changing the control value, then this
> results in (on I²C devices) an I²C write that can fail.

Good point.

> The CCS driver writes the configuration to the sensor when streaming starts
> so there's no actual write operation resulting from this.

I think all sensor drivers should do the same, it's not a legit use case
to change the link frequency during streaming, it shouldn't be
programmed from the .set_fmt() handler. Of course, the return value of
__v4l2_ctrl_modify_range() should still be checked in paths where
runtime updates are allowed (I'm thinking about updates to the vertical
blanking and exposure controls).

> > > > > > +	if (ret)
> > > > > > +		return ret;
> > > > > >  
> > > > > >  	return ccs_pll_update(sensor);
> > > > > >  }
> > > > > > 
> > > > > > base-commit: afcd48134c58d6af45fb3fdb648f1260b20f2326

-- 
Regards,

Laurent Pinchart