[PATCH v8 00/13] max9286 v8 - modifications

[PATCH v8 00/13] max9286 v8 - modifications

[Kieran Bingham]

Hi,

These patches generate the current diff that I plan to squash into the current
MAX9286v7 driver, to prepare a v8 posting.

There are several key fixups from Sakari's review, along with new features and
functionality changes. As such I've kept them separate so that the changes are
clearly visible to the current interested parties :-)

If you have comments on any of these changes, please let me know - otherwise
they'll all get squashed directly into the max9286 driver in preparation for
the v8.


Kieran Bingham (13):
  squash! max9286: Update the bound_sources mask on unbind
  squash! max9286: convert probe kzalloc
  squash! max9286: Fix cleanup path from GPIO powerdown
  squash! max9286: cleanup GPIO device registration fail path
  squash! max9286: Convert to use devm_regulator_get()
  squash! max9286: Fit max9286_parse_dt print on one line
  squash! max9286: Move multi-device workarounds out of upstream
  squash! max9286: Remove I2C mod-table
  sqaush! max9286: Lock format changes
  squash! max9286: Implement Pixelrate control
  squash! max9286: Disable overlap window
  sqaush! max9286: Describe pad index usage
  sqaush! max9286: Remove poc_enabled workaround

 drivers/media/i2c/max9286.c | 156 ++++++++++++++++++++++++++----------
 1 file changed, 115 insertions(+), 41 deletions(-)

-- 
2.20.1

[PATCH v8 01/13] squash! max9286: Update the bound_sources mask on unbind

[Kieran Bingham]

The bound_sources bit mask tracks sources which have been successfully
bound through the v4l2 async notifier system.

Ensure that the mask is updated accordingly when unbinding.

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/media/i2c/max9286.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index cb58782e5143..b84d2daa6561 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -493,9 +493,12 @@ static void max9286_notify_unbind(struct v4l2_async_notifier *notifier,
 				  struct v4l2_subdev *subdev,
 				  struct v4l2_async_subdev *asd)
 {
+	struct max9286_priv *priv = sd_to_max9286(notifier->sd);
 	struct max9286_source *source = asd_to_max9286_source(asd);
+	unsigned int index = to_index(priv, source);
 
 	source->sd = NULL;
+	priv->bound_sources &= ~BIT(index);
 }
 
 static const struct v4l2_async_notifier_operations max9286_notify_ops = {
-- 
2.20.1

Re: [PATCH v8 01/13] squash! max9286: Update the bound_sources mask on unbind

[Jacopo Mondi]

Hi Kieran,

On Thu, Apr 09, 2020 at 01:11:50PM +0100, Kieran Bingham wrote:
> The bound_sources bit mask tracks sources which have been successfully
> bound through the v4l2 async notifier system.
>
> Ensure that the mask is updated accordingly when unbinding.
>

Indeed!

Acked-by: Jacopo Mondi <jacopo@jmondi.org>

Please squash in v8!

Thanks
  j

> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
>  drivers/media/i2c/max9286.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index cb58782e5143..b84d2daa6561 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -493,9 +493,12 @@ static void max9286_notify_unbind(struct v4l2_async_notifier *notifier,
>  				  struct v4l2_subdev *subdev,
>  				  struct v4l2_async_subdev *asd)
>  {
> +	struct max9286_priv *priv = sd_to_max9286(notifier->sd);
>  	struct max9286_source *source = asd_to_max9286_source(asd);
> +	unsigned int index = to_index(priv, source);
>
>  	source->sd = NULL;
> +	priv->bound_sources &= ~BIT(index);
>  }
>
>  static const struct v4l2_async_notifier_operations max9286_notify_ops = {
> --
> 2.20.1
>

[PATCH v8 02/13] squash! max9286: convert probe kzalloc

[Kieran Bingham]

v8:
 - Convert probe kzalloc usage to devm_ variant
---
 drivers/media/i2c/max9286.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index b84d2daa6561..0a43137b8112 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -1155,7 +1155,7 @@ static int max9286_probe(struct i2c_client *client)
 	unsigned int i;
 	int ret;
 
-	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
 
@@ -1232,7 +1232,6 @@ static int max9286_probe(struct i2c_client *client)
 	max9286_configure_i2c(priv, false);
 err_free:
 	max9286_cleanup_dt(priv);
-	kfree(priv);
 
 	return ret;
 }
@@ -1253,8 +1252,6 @@ static int max9286_remove(struct i2c_client *client)
 
 	gpiod_set_value_cansleep(priv->gpiod_pwdn, 0);
 
-	kfree(priv);
-
 	return 0;
 }
 
-- 
2.20.1

Re: [PATCH v8 02/13] squash! max9286: convert probe kzalloc

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Thu, Apr 09, 2020 at 01:11:51PM +0100, Kieran Bingham wrote:
> v8:
>  - Convert probe kzalloc usage to devm_ variant

This isn't worse than the existing code, but are you aware that devm_*
should not be used in this case ? The memory should be allocated with
kzalloc() and freed in the .release() operation.

> ---
>  drivers/media/i2c/max9286.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index b84d2daa6561..0a43137b8112 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -1155,7 +1155,7 @@ static int max9286_probe(struct i2c_client *client)
>  	unsigned int i;
>  	int ret;
>  
> -	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> +	priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
>  	if (!priv)
>  		return -ENOMEM;
>  
> @@ -1232,7 +1232,6 @@ static int max9286_probe(struct i2c_client *client)
>  	max9286_configure_i2c(priv, false);
>  err_free:
>  	max9286_cleanup_dt(priv);
> -	kfree(priv);
>  
>  	return ret;
>  }
> @@ -1253,8 +1252,6 @@ static int max9286_remove(struct i2c_client *client)
>  
>  	gpiod_set_value_cansleep(priv->gpiod_pwdn, 0);
>  
> -	kfree(priv);
> -
>  	return 0;
>  }
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH v8 02/13] squash! max9286: convert probe kzalloc

[Kieran Bingham]

Hi Laurent,

(+Sakari)

On 09/04/2020 17:33, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Thu, Apr 09, 2020 at 01:11:51PM +0100, Kieran Bingham wrote:
>> v8:
>>  - Convert probe kzalloc usage to devm_ variant
> 
> This isn't worse than the existing code, but are you aware that devm_*
> should not be used in this case ? The memory should be allocated with
> kzalloc() and freed in the .release() operation.

This change was at the request of a review comment from Sakari.

From:
https://lore.kernel.org/linux-media/4139f241-2fde-26ad-fe55-dcaeb76ad6cc@ideasonboard.com/
> 
>>> +
>>> +static int max9286_probe(struct i2c_client *client)
>>> +{
>>> +	struct max9286_priv *priv;
>>> +	unsigned int i;
>>> +	int ret;
>>> +
>>> +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
>>> +	if (!priv)
>>> +		return -ENOMEM;
>> 
>> You won't lose anything by using the devm_ variant here.
> 
> Indeed,
> 
>> 
>>> +
>>> +	priv->client = client;
>>> +	i2c_set_clientdata(client, priv);
>>> +
>>> +	for (i = 0; i < MAX9286_N_SINKS; i++)
>>> +		max9286_init_format(&priv->fmt[i]);
>>> +
>>> +	ret = max9286_parse_dt(priv);
>>> +	if (ret)
>>> +		return ret;
>> 
>> But you can avoid accidental memory leaks for nothing. :-)
> 
> It would be good not to leak indeed!


I understand that there are lifetime issues in V4L2 - but in my opinion
that needs to be handled by core V4l2 (and or support from driver core
framework).

Also - isn't it highly unlikely to affect the max9286? Isn't the
lifetime issue that the device can be unplugged while the file handle is
open?

I don't think anyone is going to 'unplug' the max9286 while it's active :-)



>> ---
>>  drivers/media/i2c/max9286.c | 5 +----
>>  1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
>> index b84d2daa6561..0a43137b8112 100644
>> --- a/drivers/media/i2c/max9286.c
>> +++ b/drivers/media/i2c/max9286.c
>> @@ -1155,7 +1155,7 @@ static int max9286_probe(struct i2c_client *client)
>>  	unsigned int i;
>>  	int ret;
>>  
>> -	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
>> +	priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
>>  	if (!priv)
>>  		return -ENOMEM;
>>  
>> @@ -1232,7 +1232,6 @@ static int max9286_probe(struct i2c_client *client)
>>  	max9286_configure_i2c(priv, false);
>>  err_free:
>>  	max9286_cleanup_dt(priv);
>> -	kfree(priv);
>>  
>>  	return ret;
>>  }
>> @@ -1253,8 +1252,6 @@ static int max9286_remove(struct i2c_client *client)
>>  
>>  	gpiod_set_value_cansleep(priv->gpiod_pwdn, 0);
>>  
>> -	kfree(priv);
>> -
>>  	return 0;
>>  }
>>  
> 

Re: [PATCH v8 02/13] squash! max9286: convert probe kzalloc

[Laurent Pinchart]

Hi Kieran,

On Fri, Apr 10, 2020 at 09:20:25AM +0100, Kieran Bingham wrote:
> On 09/04/2020 17:33, Laurent Pinchart wrote:
> > On Thu, Apr 09, 2020 at 01:11:51PM +0100, Kieran Bingham wrote:
> >> v8:
> >>  - Convert probe kzalloc usage to devm_ variant
> > 
> > This isn't worse than the existing code, but are you aware that devm_*
> > should not be used in this case ? The memory should be allocated with
> > kzalloc() and freed in the .release() operation.
> 
> This change was at the request of a review comment from Sakari.
> 
> From:
> https://lore.kernel.org/linux-media/4139f241-2fde-26ad-fe55-dcaeb76ad6cc@ideasonboard.com/
>
> >>> +
> >>> +static int max9286_probe(struct i2c_client *client)
> >>> +{
> >>> +	struct max9286_priv *priv;
> >>> +	unsigned int i;
> >>> +	int ret;
> >>> +
> >>> +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> >>> +	if (!priv)
> >>> +		return -ENOMEM;
> >> 
> >> You won't lose anything by using the devm_ variant here.
> > 
> > Indeed,
> > 
> >>> +
> >>> +	priv->client = client;
> >>> +	i2c_set_clientdata(client, priv);
> >>> +
> >>> +	for (i = 0; i < MAX9286_N_SINKS; i++)
> >>> +		max9286_init_format(&priv->fmt[i]);
> >>> +
> >>> +	ret = max9286_parse_dt(priv);
> >>> +	if (ret)
> >>> +		return ret;
> >> 
> >> But you can avoid accidental memory leaks for nothing. :-)
> > 
> > It would be good not to leak indeed!
> 
> I understand that there are lifetime issues in V4L2 - but in my opinion
> that needs to be handled by core V4l2 (and or support from driver core
> framework).

I'm afraid that's not possible. The V4L2 core can't delay remove().
There are helpers we could create to simplify correct memory management
for drivers, but in any case, devm_kzalloc() isn't correct.

There are also issues in the core that would make unbinding unsafe even
if correctly implemented in this driver, but a correct implementation in
drivers will be required in any case.

As I said before this patch isn't a regression as memory allocation is
already broken here, but it doesn't go in the right direction either.

> Also - isn't it highly unlikely to affect the max9286? Isn't the
> lifetime issue that the device can be unplugged while the file handle is
> open?
> 
> I don't think anyone is going to 'unplug' the max9286 while it's active :-)

No, but someone could unbind it through sysfs. In any case it's not an
excuse to not implement memory allocation correctly :-)

> >> ---
> >>  drivers/media/i2c/max9286.c | 5 +----
> >>  1 file changed, 1 insertion(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> >> index b84d2daa6561..0a43137b8112 100644
> >> --- a/drivers/media/i2c/max9286.c
> >> +++ b/drivers/media/i2c/max9286.c
> >> @@ -1155,7 +1155,7 @@ static int max9286_probe(struct i2c_client *client)
> >>  	unsigned int i;
> >>  	int ret;
> >>  
> >> -	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> >> +	priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
> >>  	if (!priv)
> >>  		return -ENOMEM;
> >>  
> >> @@ -1232,7 +1232,6 @@ static int max9286_probe(struct i2c_client *client)
> >>  	max9286_configure_i2c(priv, false);
> >>  err_free:
> >>  	max9286_cleanup_dt(priv);
> >> -	kfree(priv);
> >>  
> >>  	return ret;
> >>  }
> >> @@ -1253,8 +1252,6 @@ static int max9286_remove(struct i2c_client *client)
> >>  
> >>  	gpiod_set_value_cansleep(priv->gpiod_pwdn, 0);
> >>  
> >> -	kfree(priv);
> >> -
> >>  	return 0;
> >>  }
> >>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH v8 02/13] squash! max9286: convert probe kzalloc

[Sakari Ailus]

Hi Laurent, Kieran,

On Fri, Apr 10, 2020 at 02:15:19PM +0300, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Fri, Apr 10, 2020 at 09:20:25AM +0100, Kieran Bingham wrote:
> > On 09/04/2020 17:33, Laurent Pinchart wrote:
> > > On Thu, Apr 09, 2020 at 01:11:51PM +0100, Kieran Bingham wrote:
> > >> v8:
> > >>  - Convert probe kzalloc usage to devm_ variant
> > > 
> > > This isn't worse than the existing code, but are you aware that devm_*
> > > should not be used in this case ? The memory should be allocated with
> > > kzalloc() and freed in the .release() operation.
> > 
> > This change was at the request of a review comment from Sakari.
> > 
> > From:
> > https://lore.kernel.org/linux-media/4139f241-2fde-26ad-fe55-dcaeb76ad6cc@ideasonboard.com/
> >
> > >>> +
> > >>> +static int max9286_probe(struct i2c_client *client)
> > >>> +{
> > >>> +	struct max9286_priv *priv;
> > >>> +	unsigned int i;
> > >>> +	int ret;
> > >>> +
> > >>> +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> > >>> +	if (!priv)
> > >>> +		return -ENOMEM;
> > >> 
> > >> You won't lose anything by using the devm_ variant here.
> > > 
> > > Indeed,
> > > 
> > >>> +
> > >>> +	priv->client = client;
> > >>> +	i2c_set_clientdata(client, priv);
> > >>> +
> > >>> +	for (i = 0; i < MAX9286_N_SINKS; i++)
> > >>> +		max9286_init_format(&priv->fmt[i]);
> > >>> +
> > >>> +	ret = max9286_parse_dt(priv);
> > >>> +	if (ret)
> > >>> +		return ret;
> > >> 
> > >> But you can avoid accidental memory leaks for nothing. :-)
> > > 
> > > It would be good not to leak indeed!
> > 
> > I understand that there are lifetime issues in V4L2 - but in my opinion
> > that needs to be handled by core V4l2 (and or support from driver core
> > framework).
> 
> I'm afraid that's not possible. The V4L2 core can't delay remove().
> There are helpers we could create to simplify correct memory management
> for drivers, but in any case, devm_kzalloc() isn't correct.
> 
> There are also issues in the core that would make unbinding unsafe even
> if correctly implemented in this driver, but a correct implementation in
> drivers will be required in any case.
> 
> As I said before this patch isn't a regression as memory allocation is
> already broken here, but it doesn't go in the right direction either.
> 
> > Also - isn't it highly unlikely to affect the max9286? Isn't the
> > lifetime issue that the device can be unplugged while the file handle is
> > open?
> > 
> > I don't think anyone is going to 'unplug' the max9286 while it's active :-)
> 
> No, but someone could unbind it through sysfs. In any case it's not an
> excuse to not implement memory allocation correctly :-)

Properly refcounting the objects and being unbind-safe would require
rewriting the memory allocation in drivers. This can't be done as the
framework is broken.

Whether you use devm_* variant here makes no difference from that point of
view. But it makes it easier to find out driver does not do its object
refcounting properly later on when (or if) the framework is fixed.

-- 
Regards,

Sakari Ailus

Re: [PATCH v8 02/13] squash! max9286: convert probe kzalloc

[Laurent Pinchart]

Hi Sakari,

On Thu, Apr 23, 2020 at 10:38:41AM +0300, Sakari Ailus wrote:
> On Fri, Apr 10, 2020 at 02:15:19PM +0300, Laurent Pinchart wrote:
> > On Fri, Apr 10, 2020 at 09:20:25AM +0100, Kieran Bingham wrote:
> > > On 09/04/2020 17:33, Laurent Pinchart wrote:
> > > > On Thu, Apr 09, 2020 at 01:11:51PM +0100, Kieran Bingham wrote:
> > > >> v8:
> > > >>  - Convert probe kzalloc usage to devm_ variant
> > > > 
> > > > This isn't worse than the existing code, but are you aware that devm_*
> > > > should not be used in this case ? The memory should be allocated with
> > > > kzalloc() and freed in the .release() operation.
> > > 
> > > This change was at the request of a review comment from Sakari.
> > > 
> > > From:
> > > https://lore.kernel.org/linux-media/4139f241-2fde-26ad-fe55-dcaeb76ad6cc@ideasonboard.com/
> > >
> > > >>> +
> > > >>> +static int max9286_probe(struct i2c_client *client)
> > > >>> +{
> > > >>> +	struct max9286_priv *priv;
> > > >>> +	unsigned int i;
> > > >>> +	int ret;
> > > >>> +
> > > >>> +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> > > >>> +	if (!priv)
> > > >>> +		return -ENOMEM;
> > > >> 
> > > >> You won't lose anything by using the devm_ variant here.
> > > > 
> > > > Indeed,
> > > > 
> > > >>> +
> > > >>> +	priv->client = client;
> > > >>> +	i2c_set_clientdata(client, priv);
> > > >>> +
> > > >>> +	for (i = 0; i < MAX9286_N_SINKS; i++)
> > > >>> +		max9286_init_format(&priv->fmt[i]);
> > > >>> +
> > > >>> +	ret = max9286_parse_dt(priv);
> > > >>> +	if (ret)
> > > >>> +		return ret;
> > > >> 
> > > >> But you can avoid accidental memory leaks for nothing. :-)
> > > > 
> > > > It would be good not to leak indeed!
> > > 
> > > I understand that there are lifetime issues in V4L2 - but in my opinion
> > > that needs to be handled by core V4l2 (and or support from driver core
> > > framework).
> > 
> > I'm afraid that's not possible. The V4L2 core can't delay remove().
> > There are helpers we could create to simplify correct memory management
> > for drivers, but in any case, devm_kzalloc() isn't correct.
> > 
> > There are also issues in the core that would make unbinding unsafe even
> > if correctly implemented in this driver, but a correct implementation in
> > drivers will be required in any case.
> > 
> > As I said before this patch isn't a regression as memory allocation is
> > already broken here, but it doesn't go in the right direction either.
> > 
> > > Also - isn't it highly unlikely to affect the max9286? Isn't the
> > > lifetime issue that the device can be unplugged while the file handle is
> > > open?
> > > 
> > > I don't think anyone is going to 'unplug' the max9286 while it's active :-)
> > 
> > No, but someone could unbind it through sysfs. In any case it's not an
> > excuse to not implement memory allocation correctly :-)
> 
> Properly refcounting the objects and being unbind-safe would require
> rewriting the memory allocation in drivers. This can't be done as the
> framework is broken.
> 
> Whether you use devm_* variant here makes no difference from that point of
> view. But it makes it easier to find out driver does not do its object
> refcounting properly later on when (or if) the framework is fixed.

It's an interesting point, "as it's broken, make it very visible" :-)
I'm fine with that.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v8 02/13] squash! max9286: convert probe kzalloc

[Kieran Bingham]

Hi Sakari,

On 23/04/2020 08:38, Sakari Ailus wrote:
> Hi Laurent, Kieran,
> 
> On Fri, Apr 10, 2020 at 02:15:19PM +0300, Laurent Pinchart wrote:
>> Hi Kieran,
>>
>> On Fri, Apr 10, 2020 at 09:20:25AM +0100, Kieran Bingham wrote:
>>> On 09/04/2020 17:33, Laurent Pinchart wrote:
>>>> On Thu, Apr 09, 2020 at 01:11:51PM +0100, Kieran Bingham wrote:
>>>>> v8:
>>>>>  - Convert probe kzalloc usage to devm_ variant
>>>>
>>>> This isn't worse than the existing code, but are you aware that devm_*
>>>> should not be used in this case ? The memory should be allocated with
>>>> kzalloc() and freed in the .release() operation.
>>>
>>> This change was at the request of a review comment from Sakari.
>>>
>>> From:
>>> https://lore.kernel.org/linux-media/4139f241-2fde-26ad-fe55-dcaeb76ad6cc@ideasonboard.com/
>>>
>>>>>> +
>>>>>> +static int max9286_probe(struct i2c_client *client)
>>>>>> +{
>>>>>> +	struct max9286_priv *priv;
>>>>>> +	unsigned int i;
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
>>>>>> +	if (!priv)
>>>>>> +		return -ENOMEM;
>>>>>
>>>>> You won't lose anything by using the devm_ variant here.
>>>>
>>>> Indeed,
>>>>
>>>>>> +
>>>>>> +	priv->client = client;
>>>>>> +	i2c_set_clientdata(client, priv);
>>>>>> +
>>>>>> +	for (i = 0; i < MAX9286_N_SINKS; i++)
>>>>>> +		max9286_init_format(&priv->fmt[i]);
>>>>>> +
>>>>>> +	ret = max9286_parse_dt(priv);
>>>>>> +	if (ret)
>>>>>> +		return ret;
>>>>>
>>>>> But you can avoid accidental memory leaks for nothing. :-)
>>>>
>>>> It would be good not to leak indeed!
>>>
>>> I understand that there are lifetime issues in V4L2 - but in my opinion
>>> that needs to be handled by core V4l2 (and or support from driver core
>>> framework).
>>
>> I'm afraid that's not possible. The V4L2 core can't delay remove().
>> There are helpers we could create to simplify correct memory management
>> for drivers, but in any case, devm_kzalloc() isn't correct.
>>
>> There are also issues in the core that would make unbinding unsafe even
>> if correctly implemented in this driver, but a correct implementation in
>> drivers will be required in any case.
>>
>> As I said before this patch isn't a regression as memory allocation is
>> already broken here, but it doesn't go in the right direction either.
>>
>>> Also - isn't it highly unlikely to affect the max9286? Isn't the
>>> lifetime issue that the device can be unplugged while the file handle is
>>> open?
>>>
>>> I don't think anyone is going to 'unplug' the max9286 while it's active :-)
>>
>> No, but someone could unbind it through sysfs. In any case it's not an
>> excuse to not implement memory allocation correctly :-)
> 
> Properly refcounting the objects and being unbind-safe would require
> rewriting the memory allocation in drivers. This can't be done as the
> framework is broken.


Which framework is broken?


> Whether you use devm_* variant here makes no difference from that point of
> view. But it makes it easier to find out driver does not do its object
> refcounting properly later on when (or if) the framework is fixed.

Is there anything *preventing* us from adding refcnting to the devm_
system so that we can take a reference on the struct device, thus
postponing all devm_ release actions while a file handle is open?

Then the reference handling can be done by V4l2 core - and we get a
whole bunch of drivers fixed ?

As it sounds like this is something which affects all subsystems which
are capable of exposing a device which gets opened (so presumably all
char/block devices?) this seems like it really needs to be fixed as low
as possible.

--
Kieran

Re: [PATCH v8 02/13] squash! max9286: convert probe kzalloc

[Sakari Ailus]

Hi Kieran,

On Thu, Apr 23, 2020 at 11:55:47AM +0100, Kieran Bingham wrote:
> Hi Sakari,
> 
> On 23/04/2020 08:38, Sakari Ailus wrote:
> > Hi Laurent, Kieran,
> > 
> > On Fri, Apr 10, 2020 at 02:15:19PM +0300, Laurent Pinchart wrote:
> >> Hi Kieran,
> >>
> >> On Fri, Apr 10, 2020 at 09:20:25AM +0100, Kieran Bingham wrote:
> >>> On 09/04/2020 17:33, Laurent Pinchart wrote:
> >>>> On Thu, Apr 09, 2020 at 01:11:51PM +0100, Kieran Bingham wrote:
> >>>>> v8:
> >>>>>  - Convert probe kzalloc usage to devm_ variant
> >>>>
> >>>> This isn't worse than the existing code, but are you aware that devm_*
> >>>> should not be used in this case ? The memory should be allocated with
> >>>> kzalloc() and freed in the .release() operation.
> >>>
> >>> This change was at the request of a review comment from Sakari.
> >>>
> >>> From:
> >>> https://lore.kernel.org/linux-media/4139f241-2fde-26ad-fe55-dcaeb76ad6cc@ideasonboard.com/
> >>>
> >>>>>> +
> >>>>>> +static int max9286_probe(struct i2c_client *client)
> >>>>>> +{
> >>>>>> +	struct max9286_priv *priv;
> >>>>>> +	unsigned int i;
> >>>>>> +	int ret;
> >>>>>> +
> >>>>>> +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> >>>>>> +	if (!priv)
> >>>>>> +		return -ENOMEM;
> >>>>>
> >>>>> You won't lose anything by using the devm_ variant here.
> >>>>
> >>>> Indeed,
> >>>>
> >>>>>> +
> >>>>>> +	priv->client = client;
> >>>>>> +	i2c_set_clientdata(client, priv);
> >>>>>> +
> >>>>>> +	for (i = 0; i < MAX9286_N_SINKS; i++)
> >>>>>> +		max9286_init_format(&priv->fmt[i]);
> >>>>>> +
> >>>>>> +	ret = max9286_parse_dt(priv);
> >>>>>> +	if (ret)
> >>>>>> +		return ret;
> >>>>>
> >>>>> But you can avoid accidental memory leaks for nothing. :-)
> >>>>
> >>>> It would be good not to leak indeed!
> >>>
> >>> I understand that there are lifetime issues in V4L2 - but in my opinion
> >>> that needs to be handled by core V4l2 (and or support from driver core
> >>> framework).
> >>
> >> I'm afraid that's not possible. The V4L2 core can't delay remove().
> >> There are helpers we could create to simplify correct memory management
> >> for drivers, but in any case, devm_kzalloc() isn't correct.
> >>
> >> There are also issues in the core that would make unbinding unsafe even
> >> if correctly implemented in this driver, but a correct implementation in
> >> drivers will be required in any case.
> >>
> >> As I said before this patch isn't a regression as memory allocation is
> >> already broken here, but it doesn't go in the right direction either.
> >>
> >>> Also - isn't it highly unlikely to affect the max9286? Isn't the
> >>> lifetime issue that the device can be unplugged while the file handle is
> >>> open?
> >>>
> >>> I don't think anyone is going to 'unplug' the max9286 while it's active :-)
> >>
> >> No, but someone could unbind it through sysfs. In any case it's not an
> >> excuse to not implement memory allocation correctly :-)
> > 
> > Properly refcounting the objects and being unbind-safe would require
> > rewriting the memory allocation in drivers. This can't be done as the
> > framework is broken.
> 
> 
> Which framework is broken?

V4L2 and MC. There is nothing that prevents unbinding a driver, and thus
freeing the resources related to it --- while a device is in use. This can
be done with V4L2 video nodes but not with subdev nodes nor MC. The same
applies all the graph objects in the same scope.

The original assumption was more or less that it's all mounted on the same
PCB, so why would it go away? Well, it turns out this is not so easy to
change afterwards.

> 
> 
> > Whether you use devm_* variant here makes no difference from that point of
> > view. But it makes it easier to find out driver does not do its object
> > refcounting properly later on when (or if) the framework is fixed.
> 
> Is there anything *preventing* us from adding refcnting to the devm_
> system so that we can take a reference on the struct device, thus
> postponing all devm_ release actions while a file handle is open?

That doesn't help really, they'll be freed at driver remove time AFAIR as
well. The effect is no different than freeing them in the remove callback
yourself.

> 
> Then the reference handling can be done by V4l2 core - and we get a
> whole bunch of drivers fixed ?

To address this, you'd need to keep reference to the relevant objects, so
the memory is eventually released.

> 
> As it sounds like this is something which affects all subsystems which
> are capable of exposing a device which gets opened (so presumably all
> char/block devices?) this seems like it really needs to be fixed as low
> as possible.

I think it's really everything that exposes some sort of interface, whether
it's kernel or user. I haven't studied other parts of the kernel this in
mind, but my understanding is that this is generally working as it should.

We just happen to have complex dependencies between the devices and thus
also drivers so the problem area is wider than elsewhere. I wonder how ALSA
has addressed similar issues.

-- 
Kind regards,

Sakari Ailus

[PATCH] squash! i2c: max9286: Put of node on error

[Jacopo Mondi]

Put the device of node in case of dt parsing error.

Fixes: 9eed4185c7a0 ("media: i2c: Add MAX9286 driver")
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/max9286.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 241deed0f270..bac9696f83b4 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -1121,6 +1121,7 @@ static int max9286_parse_dt(struct max9286_priv *priv)
 	i2c_mux = of_find_node_by_name(dev->of_node, "i2c-mux");
 	if (!i2c_mux) {
 		dev_err(dev, "Failed to find i2c-mux node\n");
+		of_node_put(dev->of_node);
 		return -EINVAL;
 	}

@@ -1168,6 +1169,7 @@ static int max9286_parse_dt(struct max9286_priv *priv)
 					of_fwnode_handle(node), &vep);
 			if (ret) {
 				of_node_put(node);
+				of_node_put(dev->of_node);
 				return ret;
 			}

@@ -1177,6 +1179,7 @@ static int max9286_parse_dt(struct max9286_priv *priv)
 					vep.bus_type);
 				v4l2_fwnode_endpoint_free(&vep);
 				of_node_put(node);
+				of_node_put(dev->of_node);
 				return -EINVAL;
 			}

@@ -1214,6 +1217,7 @@ static int max9286_parse_dt(struct max9286_priv *priv)
 		priv->nsources++;
 	}
 	of_node_put(node);
+	of_node_put(dev->of_node);

 	priv->route_mask = priv->source_mask;

--
2.26.0

Re: [PATCH] squash! i2c: max9286: Put of node on error

[Jacopo Mondi]

Dunno why, I formatted the patch in a way others interested parties
have been removed from the Cc list, just noticed :/

It's such a minor fix I won't bother resending I think

On Thu, Apr 09, 2020 at 07:08:06PM +0200, Jacopo Mondi wrote:
> Put the device of node in case of dt parsing error.
>
> Fixes: 9eed4185c7a0 ("media: i2c: Add MAX9286 driver")

Also this is a leftover from where this was intended to be sent out a
patch on it's own

> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/max9286.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 241deed0f270..bac9696f83b4 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -1121,6 +1121,7 @@ static int max9286_parse_dt(struct max9286_priv *priv)
>  	i2c_mux = of_find_node_by_name(dev->of_node, "i2c-mux");
>  	if (!i2c_mux) {
>  		dev_err(dev, "Failed to find i2c-mux node\n");
> +		of_node_put(dev->of_node);
>  		return -EINVAL;
>  	}
>
> @@ -1168,6 +1169,7 @@ static int max9286_parse_dt(struct max9286_priv *priv)
>  					of_fwnode_handle(node), &vep);
>  			if (ret) {
>  				of_node_put(node);
> +				of_node_put(dev->of_node);
>  				return ret;
>  			}
>
> @@ -1177,6 +1179,7 @@ static int max9286_parse_dt(struct max9286_priv *priv)
>  					vep.bus_type);
>  				v4l2_fwnode_endpoint_free(&vep);
>  				of_node_put(node);
> +				of_node_put(dev->of_node);
>  				return -EINVAL;
>  			}
>
> @@ -1214,6 +1217,7 @@ static int max9286_parse_dt(struct max9286_priv *priv)
>  		priv->nsources++;
>  	}
>  	of_node_put(node);
> +	of_node_put(dev->of_node);
>
>  	priv->route_mask = priv->source_mask;
>
> --
> 2.26.0
>

Re: [PATCH] squash! i2c: max9286: Put of node on error

[Kieran Bingham]

On 09/04/2020 21:20, Jacopo Mondi wrote:
> Dunno why, I formatted the patch in a way others interested parties
> have been removed from the Cc list, just noticed :/
> 
> It's such a minor fix I won't bother resending I think
> 
> On Thu, Apr 09, 2020 at 07:08:06PM +0200, Jacopo Mondi wrote:
>> Put the device of node in case of dt parsing error.
>>
>> Fixes: 9eed4185c7a0 ("media: i2c: Add MAX9286 driver")
> 
> Also this is a leftover from where this was intended to be sent out a
> patch on it's own
> 

Added to my branch, so it will be included in the next squash.

Thanks.

--
Kieran


>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>> ---
>>  drivers/media/i2c/max9286.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
>> index 241deed0f270..bac9696f83b4 100644
>> --- a/drivers/media/i2c/max9286.c
>> +++ b/drivers/media/i2c/max9286.c
>> @@ -1121,6 +1121,7 @@ static int max9286_parse_dt(struct max9286_priv *priv)
>>  	i2c_mux = of_find_node_by_name(dev->of_node, "i2c-mux");
>>  	if (!i2c_mux) {
>>  		dev_err(dev, "Failed to find i2c-mux node\n");
>> +		of_node_put(dev->of_node);
>>  		return -EINVAL;
>>  	}
>>
>> @@ -1168,6 +1169,7 @@ static int max9286_parse_dt(struct max9286_priv *priv)
>>  					of_fwnode_handle(node), &vep);
>>  			if (ret) {
>>  				of_node_put(node);
>> +				of_node_put(dev->of_node);
>>  				return ret;
>>  			}
>>
>> @@ -1177,6 +1179,7 @@ static int max9286_parse_dt(struct max9286_priv *priv)
>>  					vep.bus_type);
>>  				v4l2_fwnode_endpoint_free(&vep);
>>  				of_node_put(node);
>> +				of_node_put(dev->of_node);
>>  				return -EINVAL;
>>  			}
>>
>> @@ -1214,6 +1217,7 @@ static int max9286_parse_dt(struct max9286_priv *priv)
>>  		priv->nsources++;
>>  	}
>>  	of_node_put(node);
>> +	of_node_put(dev->of_node);
>>
>>  	priv->route_mask = priv->source_mask;
>>
>> --
>> 2.26.0
>>

[PATCH v8 03/13] squash! max9286: Fix cleanup path from GPIO powerdown

[Kieran Bingham]

 - Fix up cleanup path from GPIO PowerDown registration
---
 drivers/media/i2c/max9286.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 0a43137b8112..cc99740b34c5 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -1171,8 +1171,10 @@ static int max9286_probe(struct i2c_client *client)
 
 	priv->gpiod_pwdn = devm_gpiod_get_optional(&client->dev, "enable",
 						   GPIOD_OUT_HIGH);
-	if (IS_ERR(priv->gpiod_pwdn))
-		return PTR_ERR(priv->gpiod_pwdn);
+	if (IS_ERR(priv->gpiod_pwdn)) {
+		ret = PTR_ERR(priv->gpiod_pwdn);
+		goto err_cleanup_dt;
+	}
 
 	gpiod_set_consumer_name(priv->gpiod_pwdn, "max9286-pwdn");
 	gpiod_set_value_cansleep(priv->gpiod_pwdn, 1);
@@ -1193,7 +1195,7 @@ static int max9286_probe(struct i2c_client *client)
 				PTR_ERR(priv->regulator));
 		ret = PTR_ERR(priv->regulator);
 		priv->regulator = NULL;
-		goto err_free;
+		goto err_cleanup_dt;
 	}
 
 	/*
@@ -1230,7 +1232,7 @@ static int max9286_probe(struct i2c_client *client)
 	regulator_put(priv->regulator);
 	max9286_i2c_mux_close(priv);
 	max9286_configure_i2c(priv, false);
-err_free:
+err_cleanup_dt:
 	max9286_cleanup_dt(priv);
 
 	return ret;
@@ -1248,10 +1250,10 @@ static int max9286_remove(struct i2c_client *client)
 		regulator_disable(priv->regulator);
 	regulator_put(priv->regulator);
 
-	max9286_cleanup_dt(priv);
-
 	gpiod_set_value_cansleep(priv->gpiod_pwdn, 0);
 
+	max9286_cleanup_dt(priv);
+
 	return 0;
 }
 
-- 
2.20.1

Re: [PATCH v8 03/13] squash! max9286: Fix cleanup path from GPIO powerdown

[Jacopo Mondi]

Hi Kieran,
  slightly unrelated on this patch but

On Thu, Apr 09, 2020 at 01:11:52PM +0100, Kieran Bingham wrote:
>  - Fix up cleanup path from GPIO PowerDown registration
> ---
>  drivers/media/i2c/max9286.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 0a43137b8112..cc99740b34c5 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -1171,8 +1171,10 @@ static int max9286_probe(struct i2c_client *client)
>
>  	priv->gpiod_pwdn = devm_gpiod_get_optional(&client->dev, "enable",
>  						   GPIOD_OUT_HIGH);
> -	if (IS_ERR(priv->gpiod_pwdn))
> -		return PTR_ERR(priv->gpiod_pwdn);
> +	if (IS_ERR(priv->gpiod_pwdn)) {
> +		ret = PTR_ERR(priv->gpiod_pwdn);
> +		goto err_cleanup_dt;
> +	}
>
>  	gpiod_set_consumer_name(priv->gpiod_pwdn, "max9286-pwdn");
>  	gpiod_set_value_cansleep(priv->gpiod_pwdn, 1);

As we get_optional(), shouldn't this be protected by an
        if (priv->gpiod_pwdn)

 ?


Another point (sorry, I'm looking at gpio handling only now) we have
        ret = max9286_gpio(priv);
        if (ret)
                return ret;

That's not really a descriptive function name.. Could this be
        max9286_register_gpiochip()
?

One last point and then I'll stop.

We currently do

probe() {
        parse_dt()

        pwdn = devm_get_gpio_optional()
        if (err)
                goto err_cleanup_dt()
        set(pwdn, 1);

        register_gpiochip(); //uses devm
                goto err_cleanup_dt()

        get_regulator()
                goto err_cleanup_dt()

        ret = init()
        if (ret)
                goto err_regulator();

        return 0

err_regulator:
        regulator_put()
        mux_close()
        i2c_ack_disable()
err_cleanup_dt:
       cleanup_dt()

}

With patch 5 of this series this becomes

probe() {
        parse_dt()

        pwdn = devm_get_gpio_optional()
        if (err)
                goto err_cleanup_dt()
        set(pwdn, 1);

        register_gpiochip(); //uses devm
                goto err_cleanup_dt()

        devm_get_regulator()
                goto err_cleanup_dt()

        ret = init()
        if (ret)
                goto err_regulator();

        return 0

err_regulator:
        mux_close()
        i2c_ack_disable()
err_cleanup_dt:
       cleanup_dt()
}

as the i2c_mux is already closed at the end of init() (or never open
if we fail earlier) and i2c_ack can be disabled at the end of
max9286_setup() and in the error path there (as there are no more i2c
writes after that function returns), I think we could simplify all of
thise even more to:

probe() {
        pwdn = devm_get_gpio_optional()
        if (err)
                return;

        set(pwdn, 1);

        register_gpiochip(); //uses devm
                return;

        devm_get_regulator()
                return;

        parse_dt()

        ret = init()
        if (ret)
                goto cleanup_dt();

        return 0

err_cleanup_dt:
       cleanup_dt()
}

This could be done after 5/5 in this series if you want to keep fixups
separate for another review round.

What do you think ?

Thanks
   j



> @@ -1193,7 +1195,7 @@ static int max9286_probe(struct i2c_client *client)
>  				PTR_ERR(priv->regulator));
>  		ret = PTR_ERR(priv->regulator);
>  		priv->regulator = NULL;
> -		goto err_free;
> +		goto err_cleanup_dt;
>  	}
>
>  	/*
> @@ -1230,7 +1232,7 @@ static int max9286_probe(struct i2c_client *client)
>  	regulator_put(priv->regulator);
>  	max9286_i2c_mux_close(priv);
>  	max9286_configure_i2c(priv, false);
> -err_free:
> +err_cleanup_dt:
>  	max9286_cleanup_dt(priv);
>
>  	return ret;
> @@ -1248,10 +1250,10 @@ static int max9286_remove(struct i2c_client *client)
>  		regulator_disable(priv->regulator);
>  	regulator_put(priv->regulator);
>
> -	max9286_cleanup_dt(priv);
> -
>  	gpiod_set_value_cansleep(priv->gpiod_pwdn, 0);
>
> +	max9286_cleanup_dt(priv);
> +
>  	return 0;
>  }
>
> --
> 2.20.1
>

Re: [PATCH v8 03/13] squash! max9286: Fix cleanup path from GPIO powerdown

[Geert Uytterhoeven]

Hi Jacopo,

On Thu, Apr 9, 2020 at 7:07 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
> On Thu, Apr 09, 2020 at 01:11:52PM +0100, Kieran Bingham wrote:
> >  - Fix up cleanup path from GPIO PowerDown registration
> > ---
> >  drivers/media/i2c/max9286.c | 14 ++++++++------
> >  1 file changed, 8 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > index 0a43137b8112..cc99740b34c5 100644
> > --- a/drivers/media/i2c/max9286.c
> > +++ b/drivers/media/i2c/max9286.c
> > @@ -1171,8 +1171,10 @@ static int max9286_probe(struct i2c_client *client)
> >
> >       priv->gpiod_pwdn = devm_gpiod_get_optional(&client->dev, "enable",
> >                                                  GPIOD_OUT_HIGH);
> > -     if (IS_ERR(priv->gpiod_pwdn))
> > -             return PTR_ERR(priv->gpiod_pwdn);
> > +     if (IS_ERR(priv->gpiod_pwdn)) {
> > +             ret = PTR_ERR(priv->gpiod_pwdn);
> > +             goto err_cleanup_dt;
> > +     }
> >
> >       gpiod_set_consumer_name(priv->gpiod_pwdn, "max9286-pwdn");
> >       gpiod_set_value_cansleep(priv->gpiod_pwdn, 1);
>
> As we get_optional(), shouldn't this be protected by an
>         if (priv->gpiod_pwdn)
>
>  ?

When passed a NULL desc, validate_desc() just returns 0, without printing
an error message, so both functions become no-ops.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v8 03/13] squash! max9286: Fix cleanup path from GPIO powerdown

[Kieran Bingham]

On 10/04/2020 08:34, Geert Uytterhoeven wrote:
> Hi Jacopo,
> 
> On Thu, Apr 9, 2020 at 7:07 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
>> On Thu, Apr 09, 2020 at 01:11:52PM +0100, Kieran Bingham wrote:
>>>  - Fix up cleanup path from GPIO PowerDown registration
>>> ---
>>>  drivers/media/i2c/max9286.c | 14 ++++++++------
>>>  1 file changed, 8 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
>>> index 0a43137b8112..cc99740b34c5 100644
>>> --- a/drivers/media/i2c/max9286.c
>>> +++ b/drivers/media/i2c/max9286.c
>>> @@ -1171,8 +1171,10 @@ static int max9286_probe(struct i2c_client *client)
>>>
>>>       priv->gpiod_pwdn = devm_gpiod_get_optional(&client->dev, "enable",
>>>                                                  GPIOD_OUT_HIGH);
>>> -     if (IS_ERR(priv->gpiod_pwdn))
>>> -             return PTR_ERR(priv->gpiod_pwdn);
>>> +     if (IS_ERR(priv->gpiod_pwdn)) {
>>> +             ret = PTR_ERR(priv->gpiod_pwdn);
>>> +             goto err_cleanup_dt;
>>> +     }
>>>
>>>       gpiod_set_consumer_name(priv->gpiod_pwdn, "max9286-pwdn");
>>>       gpiod_set_value_cansleep(priv->gpiod_pwdn, 1);
>>
>> As we get_optional(), shouldn't this be protected by an
>>         if (priv->gpiod_pwdn)
>>
>>  ?
> 
> When passed a NULL desc, validate_desc() just returns 0, without printing
> an error message, so both functions become no-ops.

Aha! I knew I'd tested that code path :-)

Thanks for highlighting that, I missed it just 10 minutes ago when I
rechecked because:

 	VALIDATE_DESC(desc);

has a *HIDDEN RETURN* dammit:

	if (__valid <= 0) \
		return __valid; \
	} while (0)

Honestly - I thought we didn't do that in the kernel for exactly this
reason. Grrrrrrrrrr ... oh and grrrrr again !

They could have at least named it:

 VALIDATE_DESC_OR_RETURN(desc)



> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 

Re: [PATCH v8 03/13] squash! max9286: Fix cleanup path from GPIO powerdown

[Jacopo Mondi]

Hi Geert,

On Fri, Apr 10, 2020 at 09:34:28AM +0200, Geert Uytterhoeven wrote:
> Hi Jacopo,
>
> On Thu, Apr 9, 2020 at 7:07 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
> > On Thu, Apr 09, 2020 at 01:11:52PM +0100, Kieran Bingham wrote:
> > >  - Fix up cleanup path from GPIO PowerDown registration
> > > ---
> > >  drivers/media/i2c/max9286.c | 14 ++++++++------
> > >  1 file changed, 8 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > > index 0a43137b8112..cc99740b34c5 100644
> > > --- a/drivers/media/i2c/max9286.c
> > > +++ b/drivers/media/i2c/max9286.c
> > > @@ -1171,8 +1171,10 @@ static int max9286_probe(struct i2c_client *client)
> > >
> > >       priv->gpiod_pwdn = devm_gpiod_get_optional(&client->dev, "enable",
> > >                                                  GPIOD_OUT_HIGH);
> > > -     if (IS_ERR(priv->gpiod_pwdn))
> > > -             return PTR_ERR(priv->gpiod_pwdn);
> > > +     if (IS_ERR(priv->gpiod_pwdn)) {
> > > +             ret = PTR_ERR(priv->gpiod_pwdn);
> > > +             goto err_cleanup_dt;
> > > +     }
> > >
> > >       gpiod_set_consumer_name(priv->gpiod_pwdn, "max9286-pwdn");
> > >       gpiod_set_value_cansleep(priv->gpiod_pwdn, 1);
> >
> > As we get_optional(), shouldn't this be protected by an
> >         if (priv->gpiod_pwdn)
> >
> >  ?
>
> When passed a NULL desc, validate_desc() just returns 0, without printing
> an error message, so both functions become no-ops.
>

Ah indeed, sorry I have overlooked this!

Thanks for pointing this out and sorry Kieran for the noise!

> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Re: [PATCH v8 03/13] squash! max9286: Fix cleanup path from GPIO powerdown

[Kieran Bingham]

Hi Jacopo,

On 09/04/2020 17:22, Jacopo Mondi wrote:
> Hi Kieran,
>   slightly unrelated on this patch but

Everything's related :-) All comments welcome!

> 
> On Thu, Apr 09, 2020 at 01:11:52PM +0100, Kieran Bingham wrote:
>>  - Fix up cleanup path from GPIO PowerDown registration
>> ---
>>  drivers/media/i2c/max9286.c | 14 ++++++++------
>>  1 file changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
>> index 0a43137b8112..cc99740b34c5 100644
>> --- a/drivers/media/i2c/max9286.c
>> +++ b/drivers/media/i2c/max9286.c
>> @@ -1171,8 +1171,10 @@ static int max9286_probe(struct i2c_client *client)
>>
>>  	priv->gpiod_pwdn = devm_gpiod_get_optional(&client->dev, "enable",
>>  						   GPIOD_OUT_HIGH);
>> -	if (IS_ERR(priv->gpiod_pwdn))
>> -		return PTR_ERR(priv->gpiod_pwdn);
>> +	if (IS_ERR(priv->gpiod_pwdn)) {
>> +		ret = PTR_ERR(priv->gpiod_pwdn);
>> +		goto err_cleanup_dt;
>> +	}
>>
>>  	gpiod_set_consumer_name(priv->gpiod_pwdn, "max9286-pwdn");
>>  	gpiod_set_value_cansleep(priv->gpiod_pwdn, 1);
> 
> As we get_optional(), shouldn't this be protected by an
>         if (priv->gpiod_pwdn)
> 
>  ?
> 

Err - yes! That's odd - I was sure I had tested this without a gpio
specifier, and I thought those functions were null-safe, and were a
no-op if there was no desc. Clearly some wire got crossed in my head -
because I see no such null-checks now!


I've added a new squash patch on top to correct for this (including
checking priv->gpiod_pwdn on all uses).


> Another point (sorry, I'm looking at gpio handling only now) we have
>         ret = max9286_gpio(priv);
>         if (ret)
>                 return ret;
> 
> That's not really a descriptive function name.. Could this be
>         max9286_register_gpiochip()

Yup, I'm happy enough to rename that.

Patch added on top.



> ?
> 
> One last point and then I'll stop.
> 
> We currently do
> 
> probe() {
>         parse_dt()
> 
>         pwdn = devm_get_gpio_optional()
>         if (err)
>                 goto err_cleanup_dt()
>         set(pwdn, 1);
> 
>         register_gpiochip(); //uses devm
>                 goto err_cleanup_dt()
> 
>         get_regulator()
>                 goto err_cleanup_dt()
> 
>         ret = init()
>         if (ret)
>                 goto err_regulator();
> 
>         return 0
> 
> err_regulator:
>         regulator_put()
>         mux_close()
>         i2c_ack_disable()
> err_cleanup_dt:
>        cleanup_dt()
> 
> }
> 
> With patch 5 of this series this becomes
> 
> probe() {
>         parse_dt()
> 
>         pwdn = devm_get_gpio_optional()
>         if (err)
>                 goto err_cleanup_dt()
>         set(pwdn, 1);
> 
>         register_gpiochip(); //uses devm
>                 goto err_cleanup_dt()
> 
>         devm_get_regulator()
>                 goto err_cleanup_dt()
> 
>         ret = init()
>         if (ret)
>                 goto err_regulator();
> 
>         return 0
> 
> err_regulator:
>         mux_close()
>         i2c_ack_disable()
> err_cleanup_dt:
>        cleanup_dt()
> }
> 
> as the i2c_mux is already closed at the end of init() (or never open
> if we fail earlier) and i2c_ack can be disabled at the end of
> max9286_setup() and in the error path there (as there are no more i2c
> writes after that function returns), I think we could simplify all of
> thise even more to:
> 
> probe() {
>         pwdn = devm_get_gpio_optional()
>         if (err)
>                 return;
> 
>         set(pwdn, 1);
> 
>         register_gpiochip(); //uses devm
>                 return;
> 
>         devm_get_regulator()
>                 return;
> 
>         parse_dt()
> 
>         ret = init()
>         if (ret)
>                 goto cleanup_dt();
> 
>         return 0
> 
> err_cleanup_dt:
>        cleanup_dt()
> }
> 
> This could be done after 5/5 in this series if you want to keep fixups
> separate for another review round.
> 

Yes, indeed - it would make sense where possible to have the devm_
handled constructions before the manually managed ones.

I'll add a patch on top.


> What do you think ?
> 
> Thanks
>    j
> 
> 
> 
>> @@ -1193,7 +1195,7 @@ static int max9286_probe(struct i2c_client *client)
>>  				PTR_ERR(priv->regulator));
>>  		ret = PTR_ERR(priv->regulator);
>>  		priv->regulator = NULL;
>> -		goto err_free;
>> +		goto err_cleanup_dt;
>>  	}
>>
>>  	/*
>> @@ -1230,7 +1232,7 @@ static int max9286_probe(struct i2c_client *client)
>>  	regulator_put(priv->regulator);
>>  	max9286_i2c_mux_close(priv);
>>  	max9286_configure_i2c(priv, false);
>> -err_free:
>> +err_cleanup_dt:
>>  	max9286_cleanup_dt(priv);
>>
>>  	return ret;
>> @@ -1248,10 +1250,10 @@ static int max9286_remove(struct i2c_client *client)
>>  		regulator_disable(priv->regulator);
>>  	regulator_put(priv->regulator);
>>
>> -	max9286_cleanup_dt(priv);
>> -
>>  	gpiod_set_value_cansleep(priv->gpiod_pwdn, 0);
>>
>> +	max9286_cleanup_dt(priv);
>> +
>>  	return 0;
>>  }
>>
>> --
>> 2.20.1
>>

Re: [PATCH v8 03/13] squash! max9286: Fix cleanup path from GPIO powerdown

[Kieran Bingham]

On 10/04/2020 08:38, Kieran Bingham wrote:
> Hi Jacopo,
>>>
>>>  	priv->gpiod_pwdn = devm_gpiod_get_optional(&client->dev, "enable",
>>>  						   GPIOD_OUT_HIGH);
>>> -	if (IS_ERR(priv->gpiod_pwdn))
>>> -		return PTR_ERR(priv->gpiod_pwdn);
>>> +	if (IS_ERR(priv->gpiod_pwdn)) {
>>> +		ret = PTR_ERR(priv->gpiod_pwdn);
>>> +		goto err_cleanup_dt;
>>> +	}
>>>
>>>  	gpiod_set_consumer_name(priv->gpiod_pwdn, "max9286-pwdn");
>>>  	gpiod_set_value_cansleep(priv->gpiod_pwdn, 1);
>>
>> As we get_optional(), shouldn't this be protected by an
>>         if (priv->gpiod_pwdn)
>>
>>  ?
>>
> 
> Err - yes! That's odd - I was sure I had tested this without a gpio
> specifier, and I thought those functions were null-safe, and were a
> no-op if there was no desc. Clearly some wire got crossed in my head -
> because I see no such null-checks now!


Ahem, ok - so as highlighted by Geert, - these *are* NULL safe...

> 
> 
> I've added a new squash patch on top to correct for this (including
> checking priv->gpiod_pwdn on all uses).

now dropped :-)

Re: [PATCH v8 03/13] squash! max9286: Fix cleanup path from GPIO powerdown

[Jacopo Mondi]

HI Kieran,

On Thu, Apr 09, 2020 at 06:22:12PM +0200, Jacopo Mondi wrote:
> Hi Kieran,
>   slightly unrelated on this patch but
>
> On Thu, Apr 09, 2020 at 01:11:52PM +0100, Kieran Bingham wrote:
> >  - Fix up cleanup path from GPIO PowerDown registration
> > ---
> >  drivers/media/i2c/max9286.c | 14 ++++++++------
> >  1 file changed, 8 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > index 0a43137b8112..cc99740b34c5 100644
> > --- a/drivers/media/i2c/max9286.c
> > +++ b/drivers/media/i2c/max9286.c
> > @@ -1171,8 +1171,10 @@ static int max9286_probe(struct i2c_client *client)
> >
> >  	priv->gpiod_pwdn = devm_gpiod_get_optional(&client->dev, "enable",
> >  						   GPIOD_OUT_HIGH);
> > -	if (IS_ERR(priv->gpiod_pwdn))
> > -		return PTR_ERR(priv->gpiod_pwdn);
> > +	if (IS_ERR(priv->gpiod_pwdn)) {
> > +		ret = PTR_ERR(priv->gpiod_pwdn);
> > +		goto err_cleanup_dt;
> > +	}
> >
> >  	gpiod_set_consumer_name(priv->gpiod_pwdn, "max9286-pwdn");
> >  	gpiod_set_value_cansleep(priv->gpiod_pwdn, 1);
>
>

[snip]

> as the i2c_mux is already closed at the end of init() (or never open
> if we fail earlier) and i2c_ack can be disabled at the end of
> max9286_setup() and in the error path there (as there are no more i2c
> writes after that function returns), I think we could simplify all of

Knowing you're working on a new squash! series, I've just noticed I've
said somthing not correct here.

> i2c_ack can be disabled at the end of
> max9286_setup() and in the error path there (as there are no more i2c
> writes after that function returns)

That's not true, i2c auto ack should be disabled after registering the
i2c adapters for the remote ends. i2c_add_adapter probes the remote
ends, and so -a lot- of i2c writes take place.

Sorry for the confusion, I think i2c auto ack could be disabled at the
end of _init() not _setup().

Thanks
   j

> probe() {
>         pwdn = devm_get_gpio_optional()
>         if (err)
>                 return;
>
>         set(pwdn, 1);
>
>         register_gpiochip(); //uses devm
>                 return;
>
>         devm_get_regulator()
>                 return;
>
>         parse_dt()
>
>         ret = init()
>         if (ret)
>                 goto cleanup_dt();
>
>         return 0
>
> err_cleanup_dt:
>        cleanup_dt()
> }
>
> This could be done after 5/5 in this series if you want to keep fixups
> separate for another review round.
>
> What do you think ?
>
> Thanks
>    j
>
>
>
> > @@ -1193,7 +1195,7 @@ static int max9286_probe(struct i2c_client *client)
> >  				PTR_ERR(priv->regulator));
> >  		ret = PTR_ERR(priv->regulator);
> >  		priv->regulator = NULL;
> > -		goto err_free;
> > +		goto err_cleanup_dt;
> >  	}
> >
> >  	/*
> > @@ -1230,7 +1232,7 @@ static int max9286_probe(struct i2c_client *client)
> >  	regulator_put(priv->regulator);
> >  	max9286_i2c_mux_close(priv);
> >  	max9286_configure_i2c(priv, false);
> > -err_free:
> > +err_cleanup_dt:
> >  	max9286_cleanup_dt(priv);
> >
> >  	return ret;
> > @@ -1248,10 +1250,10 @@ static int max9286_remove(struct i2c_client *client)
> >  		regulator_disable(priv->regulator);
> >  	regulator_put(priv->regulator);
> >
> > -	max9286_cleanup_dt(priv);
> > -
> >  	gpiod_set_value_cansleep(priv->gpiod_pwdn, 0);
> >
> > +	max9286_cleanup_dt(priv);
> > +
> >  	return 0;
> >  }
> >
> > --
> > 2.20.1
> >

[PATCH v8 04/13] squash! max9286: cleanup GPIO device registration fail path

[Kieran Bingham]

 - cleanup GPIO device registration fail path
---
 drivers/media/i2c/max9286.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index cc99740b34c5..d0749c537152 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -1185,7 +1185,7 @@ static int max9286_probe(struct i2c_client *client)
 
 	ret = max9286_gpio(priv);
 	if (ret)
-		return ret;
+		goto err_powerdown;
 
 	priv->regulator = regulator_get(&client->dev, "poc");
 	if (IS_ERR(priv->regulator)) {
@@ -1194,8 +1194,7 @@ static int max9286_probe(struct i2c_client *client)
 				"Unable to get PoC regulator (%ld)\n",
 				PTR_ERR(priv->regulator));
 		ret = PTR_ERR(priv->regulator);
-		priv->regulator = NULL;
-		goto err_cleanup_dt;
+		goto err_powerdown;
 	}
 
 	/*
@@ -1232,6 +1231,8 @@ static int max9286_probe(struct i2c_client *client)
 	regulator_put(priv->regulator);
 	max9286_i2c_mux_close(priv);
 	max9286_configure_i2c(priv, false);
+err_powerdown:
+	gpiod_set_value_cansleep(priv->gpiod_pwdn, 0);
 err_cleanup_dt:
 	max9286_cleanup_dt(priv);
 
-- 
2.20.1

[PATCH v8 05/13] squash! max9286: Convert to use devm_regulator_get()

[Kieran Bingham]

 - Convert to use devm_regulator_get()
---
 drivers/media/i2c/max9286.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index d0749c537152..c374078c7001 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -1187,7 +1187,7 @@ static int max9286_probe(struct i2c_client *client)
 	if (ret)
 		goto err_powerdown;
 
-	priv->regulator = regulator_get(&client->dev, "poc");
+	priv->regulator = devm_regulator_get(&client->dev, "poc");
 	if (IS_ERR(priv->regulator)) {
 		if (PTR_ERR(priv->regulator) != -EPROBE_DEFER)
 			dev_err(&client->dev,
-- 
2.20.1

Re: [PATCH v8 05/13] squash! max9286: Convert to use devm_regulator_get()

[Kieran Bingham]

On 09/04/2020 13:11, Kieran Bingham wrote:
>  - Convert to use devm_regulator_get()

Oh dear, Sometimes things are just easier to see when they're in my inbox.

This is missing the removal of the regulator_puts() which are no longer
required ;-)

--
Kieran


> ---
>  drivers/media/i2c/max9286.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index d0749c537152..c374078c7001 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -1187,7 +1187,7 @@ static int max9286_probe(struct i2c_client *client)
>  	if (ret)
>  		goto err_powerdown;
>  
> -	priv->regulator = regulator_get(&client->dev, "poc");
> +	priv->regulator = devm_regulator_get(&client->dev, "poc");
>  	if (IS_ERR(priv->regulator)) {
>  		if (PTR_ERR(priv->regulator) != -EPROBE_DEFER)
>  			dev_err(&client->dev,
> 

[PATCH v8 06/13] squash! max9286: Fit max9286_parse_dt print on one line

[Kieran Bingham]

 - Fit max9286_parse_dt print on one line
---
 drivers/media/i2c/max9286.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index c374078c7001..4656a1027d81 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -1061,8 +1061,7 @@ static int max9286_parse_dt(struct max9286_priv *priv)
 			continue;
 
 		if (!of_device_is_available(node)) {
-			dev_dbg(dev, "Skipping disabled I2C bus port %u\n",
-				id);
+			dev_dbg(dev, "Skipping disabled I2C bus port %u\n", id);
 			continue;
 		}
 
-- 
2.20.1

[PATCH v8 07/13] squash! max9286: Move multi-device workarounds out of upstream

[Kieran Bingham]

 - Move multi-device workarounds out of upstream driver
---
 drivers/media/i2c/max9286.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 4656a1027d81..cdebee8a0a22 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -911,12 +911,6 @@ static int max9286_setup(struct max9286_priv *priv)
 	return 0;
 }
 
-static const struct of_device_id max9286_dt_ids[] = {
-	{ .compatible = "maxim,max9286" },
-	{},
-};
-MODULE_DEVICE_TABLE(of, max9286_dt_ids);
-
 static void max9286_gpio_set(struct gpio_chip *chip,
 			     unsigned int offset, int value)
 {
@@ -976,10 +970,6 @@ static int max9286_init(struct device *dev)
 	struct i2c_client *client;
 	int ret;
 
-	/* Skip non-max9286 devices. */
-	if (!dev->of_node || !of_match_node(max9286_dt_ids, dev->of_node))
-		return 0;
-
 	client = to_i2c_client(dev);
 	priv = i2c_get_clientdata(client);
 
@@ -1257,6 +1247,12 @@ static int max9286_remove(struct i2c_client *client)
 	return 0;
 }
 
+static const struct of_device_id max9286_dt_ids[] = {
+	{ .compatible = "maxim,max9286" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, max9286_dt_ids);
+
 static const struct i2c_device_id max9286_id[] = {
 	{ "max9286", 0 },
 	{ }
-- 
2.20.1

[PATCH v8 08/13] squash! max9286: Remove I2C mod-table

[Kieran Bingham]

'Technically' this table is still required for module autoloading, until
some solution such as [0] is merged:

[0] [PATCH RFC] modpost: Support I2C Aliases from OF tables

https://lore.kernel.org/lkml/20190710193918.31135-1-kieran.bingham+renesas@ideasonboard.com/

However in practice, I think the only restriction that gets lost is
device instantiation from sysfs, which wouldn't really work for this
driver anyway, because it requires a description of connected endpoints.

So - it should be quite safe to drop it.

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/media/i2c/max9286.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index cdebee8a0a22..911323d6d3c4 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -1253,12 +1253,6 @@ static const struct of_device_id max9286_dt_ids[] = {
 };
 MODULE_DEVICE_TABLE(of, max9286_dt_ids);
 
-static const struct i2c_device_id max9286_id[] = {
-	{ "max9286", 0 },
-	{ }
-};
-MODULE_DEVICE_TABLE(i2c, max9286_id);
-
 static struct i2c_driver max9286_i2c_driver = {
 	.driver	= {
 		.name		= "max9286",
@@ -1266,7 +1260,6 @@ static struct i2c_driver max9286_i2c_driver = {
 	},
 	.probe_new	= max9286_probe,
 	.remove		= max9286_remove,
-	.id_table	= max9286_id,
 };
 
 module_i2c_driver(max9286_i2c_driver);
-- 
2.20.1

[PATCH v8 09/13] sqaush! max9286: Lock format changes

[Kieran Bingham]

Provide a mutex to protect against format changes on the pads.  The
mutex can also be used to protect against control changes, or other
userspace facing interactions as necessary

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/media/i2c/max9286.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 911323d6d3c4..17830c362a50 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -18,6 +18,7 @@
 #include <linux/i2c.h>
 #include <linux/i2c-mux.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/of_graph.h>
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
@@ -157,6 +158,9 @@ struct max9286_priv {
 
 	struct v4l2_mbus_framefmt fmt[MAX9286_N_SINKS];
 
+	/* Protects controls and fmt structures */
+	struct mutex mutex;
+
 	unsigned int nsources;
 	unsigned int source_mask;
 	unsigned int route_mask;
@@ -680,7 +684,9 @@ static int max9286_set_fmt(struct v4l2_subdev *sd,
 	if (!cfg_fmt)
 		return -EINVAL;
 
+	mutex_lock(&priv->mutex);
 	*cfg_fmt = format->format;
+	mutex_unlock(&priv->mutex);
 
 	return 0;
 }
@@ -699,7 +705,9 @@ static int max9286_get_fmt(struct v4l2_subdev *sd,
 	if (!cfg_fmt)
 		return -EINVAL;
 
+	mutex_lock(&priv->mutex);
 	format->format = *cfg_fmt;
+	mutex_unlock(&priv->mutex);
 
 	return 0;
 }
@@ -1148,6 +1156,8 @@ static int max9286_probe(struct i2c_client *client)
 	if (!priv)
 		return -ENOMEM;
 
+	mutex_init(&priv->mutex);
+
 	priv->client = client;
 	i2c_set_clientdata(client, priv);
 
-- 
2.20.1

[PATCH v8 10/13] squash! max9286: Implement Pixelrate control

[Kieran Bingham]

Determine the (CSI2) pixel rate control by providing a control to read,
and checking the rate from the upstream camera sensors, and their
appropriate formats.

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/media/i2c/max9286.c | 44 ++++++++++++++++++++++++++++++++-----
 1 file changed, 38 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 17830c362a50..008a93910300 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -155,6 +155,7 @@ struct max9286_priv {
 	bool mux_open;
 
 	struct v4l2_ctrl_handler ctrls;
+	struct v4l2_ctrl *pixelrate;
 
 	struct v4l2_mbus_framefmt fmt[MAX9286_N_SINKS];
 
@@ -631,6 +632,16 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
 	return 0;
 }
 
+static int max9286_set_pixelrate(struct max9286_priv *priv, s64 rate)
+{
+	if (!priv->pixelrate)
+		return -EINVAL;
+
+	dev_err(&priv->client->dev, "Setting pixel rate to %lld\n", rate);
+
+	return v4l2_ctrl_s_ctrl_int64(priv->pixelrate, rate);
+}
+
 static int max9286_enum_mbus_code(struct v4l2_subdev *sd,
 				  struct v4l2_subdev_pad_config *cfg,
 				  struct v4l2_subdev_mbus_code_enum *code)
@@ -664,6 +675,7 @@ static int max9286_set_fmt(struct v4l2_subdev *sd,
 {
 	struct max9286_priv *priv = sd_to_max9286(sd);
 	struct v4l2_mbus_framefmt *cfg_fmt;
+	s64 pixelrate;
 
 	if (format->pad >= MAX9286_SRC_PAD)
 		return -EINVAL;
@@ -688,6 +700,12 @@ static int max9286_set_fmt(struct v4l2_subdev *sd,
 	*cfg_fmt = format->format;
 	mutex_unlock(&priv->mutex);
 
+	/* Update pixel rate for the CSI2 receiver */
+	pixelrate = cfg_fmt->width * cfg_fmt->height
+		  * priv->nsources * 30 /*FPS*/;
+
+	max9286_set_pixelrate(priv, pixelrate);
+
 	return 0;
 }
 
@@ -756,6 +774,20 @@ static const struct v4l2_subdev_internal_ops max9286_subdev_internal_ops = {
 	.open = max9286_open,
 };
 
+static int max9286_s_ctrl(struct v4l2_ctrl *ctrl)
+{
+	switch (ctrl->id) {
+	case V4L2_CID_PIXEL_RATE:
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct v4l2_ctrl_ops max9286_ctrl_ops = {
+	.s_ctrl	= max9286_s_ctrl,
+};
+
 static int max9286_v4l2_register(struct max9286_priv *priv)
 {
 	struct device *dev = &priv->client->dev;
@@ -777,12 +809,12 @@ static int max9286_v4l2_register(struct max9286_priv *priv)
 	priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
 
 	v4l2_ctrl_handler_init(&priv->ctrls, 1);
-	/*
-	 * FIXME: Compute the real pixel rate. The 50 MP/s value comes from the
-	 * hardcoded frequency in the BSP CSI-2 receiver driver.
-	 */
-	v4l2_ctrl_new_std(&priv->ctrls, NULL, V4L2_CID_PIXEL_RATE,
-			  50000000, 50000000, 1, 50000000);
+
+	priv->pixelrate = v4l2_ctrl_new_std(&priv->ctrls,
+					    &max9286_ctrl_ops,
+					    V4L2_CID_PIXEL_RATE,
+					    1, INT_MAX, 1, 50000000);
+
 	priv->sd.ctrl_handler = &priv->ctrls;
 	ret = priv->ctrls.error;
 	if (ret)
-- 
2.20.1

Re: [PATCH v8 10/13] squash! max9286: Implement Pixelrate control

[Jacopo Mondi]

Hi Kieran,

On Thu, Apr 09, 2020 at 01:11:59PM +0100, Kieran Bingham wrote:
> Determine the (CSI2) pixel rate control by providing a control to read,
> and checking the rate from the upstream camera sensors, and their
> appropriate formats.
>
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
>  drivers/media/i2c/max9286.c | 44 ++++++++++++++++++++++++++++++++-----
>  1 file changed, 38 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 17830c362a50..008a93910300 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -155,6 +155,7 @@ struct max9286_priv {
>  	bool mux_open;
>
>  	struct v4l2_ctrl_handler ctrls;
> +	struct v4l2_ctrl *pixelrate;
>
>  	struct v4l2_mbus_framefmt fmt[MAX9286_N_SINKS];
>
> @@ -631,6 +632,16 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
>  	return 0;
>  }
>
> +static int max9286_set_pixelrate(struct max9286_priv *priv, s64 rate)
> +{
> +	if (!priv->pixelrate)
> +		return -EINVAL;

Can this happen ?

> +
> +	dev_err(&priv->client->dev, "Setting pixel rate to %lld\n", rate);

Is this an error ?
> +
> +	return v4l2_ctrl_s_ctrl_int64(priv->pixelrate, rate);
> +}
> +
>  static int max9286_enum_mbus_code(struct v4l2_subdev *sd,
>  				  struct v4l2_subdev_pad_config *cfg,
>  				  struct v4l2_subdev_mbus_code_enum *code)
> @@ -664,6 +675,7 @@ static int max9286_set_fmt(struct v4l2_subdev *sd,
>  {
>  	struct max9286_priv *priv = sd_to_max9286(sd);
>  	struct v4l2_mbus_framefmt *cfg_fmt;
> +	s64 pixelrate;
>
>  	if (format->pad >= MAX9286_SRC_PAD)
>  		return -EINVAL;
> @@ -688,6 +700,12 @@ static int max9286_set_fmt(struct v4l2_subdev *sd,
>  	*cfg_fmt = format->format;
>  	mutex_unlock(&priv->mutex);
>
> +	/* Update pixel rate for the CSI2 receiver */
> +	pixelrate = cfg_fmt->width * cfg_fmt->height
> +		  * priv->nsources * 30 /*FPS*/;
> +
> +	max9286_set_pixelrate(priv, pixelrate);
> +
>  	return 0;
>  }
>
> @@ -756,6 +774,20 @@ static const struct v4l2_subdev_internal_ops max9286_subdev_internal_ops = {
>  	.open = max9286_open,
>  };
>
> +static int max9286_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	switch (ctrl->id) {
> +	case V4L2_CID_PIXEL_RATE:
> +		return 0;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct v4l2_ctrl_ops max9286_ctrl_ops = {
> +	.s_ctrl	= max9286_s_ctrl,
> +};
> +

After -years- I still don't get controls fully... Where is get? that's
the whole point of calculating the pixel rate to report it to the
receiver... I would not allow setting this from the extern but just
retrieve it after it has been updated by a set_format().

Am I getting controls wrong again ?

>  static int max9286_v4l2_register(struct max9286_priv *priv)
>  {
>  	struct device *dev = &priv->client->dev;
> @@ -777,12 +809,12 @@ static int max9286_v4l2_register(struct max9286_priv *priv)
>  	priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>
>  	v4l2_ctrl_handler_init(&priv->ctrls, 1);
> -	/*
> -	 * FIXME: Compute the real pixel rate. The 50 MP/s value comes from the
> -	 * hardcoded frequency in the BSP CSI-2 receiver driver.
> -	 */
> -	v4l2_ctrl_new_std(&priv->ctrls, NULL, V4L2_CID_PIXEL_RATE,
> -			  50000000, 50000000, 1, 50000000);
> +
> +	priv->pixelrate = v4l2_ctrl_new_std(&priv->ctrls,
> +					    &max9286_ctrl_ops,
> +					    V4L2_CID_PIXEL_RATE,
> +					    1, INT_MAX, 1, 50000000);
> +
>  	priv->sd.ctrl_handler = &priv->ctrls;
>  	ret = priv->ctrls.error;
>  	if (ret)
> --
> 2.20.1
>

Re: [PATCH v8 10/13] squash! max9286: Implement Pixelrate control

[Kieran Bingham]

On 09/04/2020 17:29, Jacopo Mondi wrote:
> Hi Kieran,
> 
> On Thu, Apr 09, 2020 at 01:11:59PM +0100, Kieran Bingham wrote:
>> Determine the (CSI2) pixel rate control by providing a control to read,
>> and checking the rate from the upstream camera sensors, and their
>> appropriate formats.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>> ---
>>  drivers/media/i2c/max9286.c | 44 ++++++++++++++++++++++++++++++++-----
>>  1 file changed, 38 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
>> index 17830c362a50..008a93910300 100644
>> --- a/drivers/media/i2c/max9286.c
>> +++ b/drivers/media/i2c/max9286.c
>> @@ -155,6 +155,7 @@ struct max9286_priv {
>>  	bool mux_open;
>>
>>  	struct v4l2_ctrl_handler ctrls;
>> +	struct v4l2_ctrl *pixelrate;
>>
>>  	struct v4l2_mbus_framefmt fmt[MAX9286_N_SINKS];
>>
>> @@ -631,6 +632,16 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
>>  	return 0;
>>  }
>>
>> +static int max9286_set_pixelrate(struct max9286_priv *priv, s64 rate)
>> +{
>> +	if (!priv->pixelrate)
>> +		return -EINVAL;
> 
> Can this happen ?

Hrm ... no because the control is registered when we register the V4L2
device, - so there can't be an occasion where we don't have it :-)

Removing and simplifying.

> 
>> +
>> +	dev_err(&priv->client->dev, "Setting pixel rate to %lld\n", rate);
> 
> Is this an error ?


Oops - debug print failure :-)

I can just drop this line.


>> +
>> +	return v4l2_ctrl_s_ctrl_int64(priv->pixelrate, rate);
>> +}
>> +
>>  static int max9286_enum_mbus_code(struct v4l2_subdev *sd,
>>  				  struct v4l2_subdev_pad_config *cfg,
>>  				  struct v4l2_subdev_mbus_code_enum *code)
>> @@ -664,6 +675,7 @@ static int max9286_set_fmt(struct v4l2_subdev *sd,
>>  {
>>  	struct max9286_priv *priv = sd_to_max9286(sd);
>>  	struct v4l2_mbus_framefmt *cfg_fmt;
>> +	s64 pixelrate;
>>
>>  	if (format->pad >= MAX9286_SRC_PAD)
>>  		return -EINVAL;
>> @@ -688,6 +700,12 @@ static int max9286_set_fmt(struct v4l2_subdev *sd,
>>  	*cfg_fmt = format->format;
>>  	mutex_unlock(&priv->mutex);
>>
>> +	/* Update pixel rate for the CSI2 receiver */
>> +	pixelrate = cfg_fmt->width * cfg_fmt->height
>> +		  * priv->nsources * 30 /*FPS*/;
>> +
>> +	max9286_set_pixelrate(priv, pixelrate);
>> +
>>  	return 0;
>>  }
>>
>> @@ -756,6 +774,20 @@ static const struct v4l2_subdev_internal_ops max9286_subdev_internal_ops = {
>>  	.open = max9286_open,
>>  };
>>
>> +static int max9286_s_ctrl(struct v4l2_ctrl *ctrl)
>> +{
>> +	switch (ctrl->id) {
>> +	case V4L2_CID_PIXEL_RATE:
>> +		return 0;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
>> +
>> +static const struct v4l2_ctrl_ops max9286_ctrl_ops = {
>> +	.s_ctrl	= max9286_s_ctrl,
>> +};
>> +
> 
> After -years- I still don't get controls fully... Where is get? that's
> the whole point of calculating the pixel rate to report it to the
> receiver... I would not allow setting this from the extern but just
> retrieve it after it has been updated by a set_format().
> 
> Am I getting controls wrong again ?

The control framework handles get. The value is stored in the
priv->pixelrate control structure or managed by the core.

The CSI2 receiver calls the get operation on this subdev to know what
the rate is for the CSI2 link, see:

https://git.kernel.org/pub/scm/linux/kernel/git/kbingham/rcar.git/tree/drivers/media/platform/rcar-vin/rcar-csi2.c?h=gmsl/dev#n449

> 
>>  static int max9286_v4l2_register(struct max9286_priv *priv)
>>  {
>>  	struct device *dev = &priv->client->dev;
>> @@ -777,12 +809,12 @@ static int max9286_v4l2_register(struct max9286_priv *priv)
>>  	priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>>
>>  	v4l2_ctrl_handler_init(&priv->ctrls, 1);
>> -	/*
>> -	 * FIXME: Compute the real pixel rate. The 50 MP/s value comes from the
>> -	 * hardcoded frequency in the BSP CSI-2 receiver driver.
>> -	 */
>> -	v4l2_ctrl_new_std(&priv->ctrls, NULL, V4L2_CID_PIXEL_RATE,
>> -			  50000000, 50000000, 1, 50000000);
>> +
>> +	priv->pixelrate = v4l2_ctrl_new_std(&priv->ctrls,
>> +					    &max9286_ctrl_ops,
>> +					    V4L2_CID_PIXEL_RATE,
>> +					    1, INT_MAX, 1, 50000000);
>> +
>>  	priv->sd.ctrl_handler = &priv->ctrls;
>>  	ret = priv->ctrls.error;
>>  	if (ret)
>> --
>> 2.20.1
>>

Re: [PATCH v8 10/13] squash! max9286: Implement Pixelrate control

[Jacopo Mondi]

Hi Kieran,

On Fri, Apr 10, 2020 at 08:51:21AM +0100, Kieran Bingham wrote:
> On 09/04/2020 17:29, Jacopo Mondi wrote:
> > Hi Kieran,
> >
> > On Thu, Apr 09, 2020 at 01:11:59PM +0100, Kieran Bingham wrote:
> >> Determine the (CSI2) pixel rate control by providing a control to read,
> >> and checking the rate from the upstream camera sensors, and their
> >> appropriate formats.
> >>
> >> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> >> ---
> >>  drivers/media/i2c/max9286.c | 44 ++++++++++++++++++++++++++++++++-----
> >>  1 file changed, 38 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> >> index 17830c362a50..008a93910300 100644
> >> --- a/drivers/media/i2c/max9286.c
> >> +++ b/drivers/media/i2c/max9286.c
> >> @@ -155,6 +155,7 @@ struct max9286_priv {
> >>  	bool mux_open;
> >>
> >>  	struct v4l2_ctrl_handler ctrls;
> >> +	struct v4l2_ctrl *pixelrate;
> >>
> >>  	struct v4l2_mbus_framefmt fmt[MAX9286_N_SINKS];
> >>
> >> @@ -631,6 +632,16 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
> >>  	return 0;
> >>  }
> >>
> >> +static int max9286_set_pixelrate(struct max9286_priv *priv, s64 rate)
> >> +{
> >> +	if (!priv->pixelrate)
> >> +		return -EINVAL;
> >
> > Can this happen ?
>
> Hrm ... no because the control is registered when we register the V4L2
> device, - so there can't be an occasion where we don't have it :-)
>
> Removing and simplifying.
>
> >
> >> +
> >> +	dev_err(&priv->client->dev, "Setting pixel rate to %lld\n", rate);
> >
> > Is this an error ?
>
>
> Oops - debug print failure :-)
>
> I can just drop this line.
>
>
> >> +
> >> +	return v4l2_ctrl_s_ctrl_int64(priv->pixelrate, rate);
> >> +}
> >> +
> >>  static int max9286_enum_mbus_code(struct v4l2_subdev *sd,
> >>  				  struct v4l2_subdev_pad_config *cfg,
> >>  				  struct v4l2_subdev_mbus_code_enum *code)
> >> @@ -664,6 +675,7 @@ static int max9286_set_fmt(struct v4l2_subdev *sd,
> >>  {
> >>  	struct max9286_priv *priv = sd_to_max9286(sd);
> >>  	struct v4l2_mbus_framefmt *cfg_fmt;
> >> +	s64 pixelrate;
> >>
> >>  	if (format->pad >= MAX9286_SRC_PAD)
> >>  		return -EINVAL;
> >> @@ -688,6 +700,12 @@ static int max9286_set_fmt(struct v4l2_subdev *sd,
> >>  	*cfg_fmt = format->format;
> >>  	mutex_unlock(&priv->mutex);
> >>
> >> +	/* Update pixel rate for the CSI2 receiver */
> >> +	pixelrate = cfg_fmt->width * cfg_fmt->height
> >> +		  * priv->nsources * 30 /*FPS*/;
> >> +
> >> +	max9286_set_pixelrate(priv, pixelrate);
> >> +
> >>  	return 0;
> >>  }
> >>
> >> @@ -756,6 +774,20 @@ static const struct v4l2_subdev_internal_ops max9286_subdev_internal_ops = {
> >>  	.open = max9286_open,
> >>  };
> >>
> >> +static int max9286_s_ctrl(struct v4l2_ctrl *ctrl)
> >> +{
> >> +	switch (ctrl->id) {
> >> +	case V4L2_CID_PIXEL_RATE:
> >> +		return 0;
> >> +	default:
> >> +		return -EINVAL;
> >> +	}
> >> +}
> >> +
> >> +static const struct v4l2_ctrl_ops max9286_ctrl_ops = {
> >> +	.s_ctrl	= max9286_s_ctrl,
> >> +};
> >> +
> >
> > After -years- I still don't get controls fully... Where is get? that's
> > the whole point of calculating the pixel rate to report it to the
> > receiver... I would not allow setting this from the extern but just
> > retrieve it after it has been updated by a set_format().
> >
> > Am I getting controls wrong again ?
>
> The control framework handles get. The value is stored in the
> priv->pixelrate control structure or managed by the core.

Ok, I keep being confused on which part are handled by the core for
controls...

>
> The CSI2 receiver calls the get operation on this subdev to know what
> the rate is for the CSI2 link, see:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/kbingham/rcar.git/tree/drivers/media/platform/rcar-vin/rcar-csi2.c?h=gmsl/dev#n449
>

Yeah that part's clear!

Thanks for clarifying this

> >
> >>  static int max9286_v4l2_register(struct max9286_priv *priv)
> >>  {
> >>  	struct device *dev = &priv->client->dev;
> >> @@ -777,12 +809,12 @@ static int max9286_v4l2_register(struct max9286_priv *priv)
> >>  	priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> >>
> >>  	v4l2_ctrl_handler_init(&priv->ctrls, 1);
> >> -	/*
> >> -	 * FIXME: Compute the real pixel rate. The 50 MP/s value comes from the
> >> -	 * hardcoded frequency in the BSP CSI-2 receiver driver.
> >> -	 */
> >> -	v4l2_ctrl_new_std(&priv->ctrls, NULL, V4L2_CID_PIXEL_RATE,
> >> -			  50000000, 50000000, 1, 50000000);
> >> +
> >> +	priv->pixelrate = v4l2_ctrl_new_std(&priv->ctrls,
> >> +					    &max9286_ctrl_ops,
> >> +					    V4L2_CID_PIXEL_RATE,
> >> +					    1, INT_MAX, 1, 50000000);
> >> +
> >>  	priv->sd.ctrl_handler = &priv->ctrls;
> >>  	ret = priv->ctrls.error;
> >>  	if (ret)
> >> --
> >> 2.20.1
> >>
>

Re: [PATCH v8 10/13] squash! max9286: Implement Pixelrate control

[Jacopo Mondi]

Hi Kieran,
   +Niklas and Laurent for R-Car CSI-2 question

sorry I now had the time to have a proper look to this one

On Thu, Apr 09, 2020 at 01:11:59PM +0100, Kieran Bingham wrote:
> Determine the (CSI2) pixel rate control by providing a control to read,
> and checking the rate from the upstream camera sensors, and their
> appropriate formats.
>
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
>  drivers/media/i2c/max9286.c | 44 ++++++++++++++++++++++++++++++++-----
>  1 file changed, 38 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 17830c362a50..008a93910300 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -155,6 +155,7 @@ struct max9286_priv {
>  	bool mux_open;
>
>  	struct v4l2_ctrl_handler ctrls;
> +	struct v4l2_ctrl *pixelrate;
>
>  	struct v4l2_mbus_framefmt fmt[MAX9286_N_SINKS];
>
> @@ -631,6 +632,16 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
>  	return 0;
>  }
>
> +static int max9286_set_pixelrate(struct max9286_priv *priv, s64 rate)
> +{
> +	if (!priv->pixelrate)
> +		return -EINVAL;
> +
> +	dev_err(&priv->client->dev, "Setting pixel rate to %lld\n", rate);
> +
> +	return v4l2_ctrl_s_ctrl_int64(priv->pixelrate, rate);
> +}
> +
>  static int max9286_enum_mbus_code(struct v4l2_subdev *sd,
>  				  struct v4l2_subdev_pad_config *cfg,
>  				  struct v4l2_subdev_mbus_code_enum *code)
> @@ -664,6 +675,7 @@ static int max9286_set_fmt(struct v4l2_subdev *sd,
>  {
>  	struct max9286_priv *priv = sd_to_max9286(sd);
>  	struct v4l2_mbus_framefmt *cfg_fmt;
> +	s64 pixelrate;
>
>  	if (format->pad >= MAX9286_SRC_PAD)
>  		return -EINVAL;
> @@ -688,6 +700,12 @@ static int max9286_set_fmt(struct v4l2_subdev *sd,
>  	*cfg_fmt = format->format;
>  	mutex_unlock(&priv->mutex);
>
> +	/* Update pixel rate for the CSI2 receiver */
> +	pixelrate = cfg_fmt->width * cfg_fmt->height
> +		  * priv->nsources * 30 /*FPS*/;

I would have calculated this differently, as this to me should come from
the CSI-2 link frequency.

In case of 4 * RDACM20 (1280*800, YUYV8_2X8, 30 FPS)

Tot Bandwidht = 4 * (1280 * 800 * 16 * 30) = 1,966,080,000 bits
CSI-2 Link Freq = TotBandwidth / lanes / DDR =
                = 1,966,080,000 / 4 / 2 = 245,760,000 Mhz
CSI-2 Lane Bandw = TotBandwith / lanes
                 = 491,520,00 Mbps
Pixel Rate = LaneBandwidth * lanes / BPP
           =  122,880,000 pixel/second

Which matches your calculation of
        (width * height * channesl * FPS)
        1280 * 800 * 4 * 30 = 122880000

So I think all of this just serves as validation of the above part.

Now, the PIXEL_RATE ctrl is queried by the CSI-2 receiver which use it
to compute again the bandwidth per lane in Mbps, in order to set the
PHTW value, but for H3 those values are available only up to 250Mbps,
while here we get a 491Mbps (and the CSI-2 driver which re-does the
calculations gets the same value).

Might this be because the total bandwidths of CSI-2 is 1Gbps ? (but in
that case the PHTW value should be calculated depending on the lane
nunmbers), so is our calculation of pixel rate in max9286 off ?

Also consider that the original value was set to 50MPixel/second, less
than the half of our calculation..

What do you think ?

Thanks
   j



> +
> +	max9286_set_pixelrate(priv, pixelrate);
> +
>  	return 0;
>  }
>
> @@ -756,6 +774,20 @@ static const struct v4l2_subdev_internal_ops max9286_subdev_internal_ops = {
>  	.open = max9286_open,
>  };
>
> +static int max9286_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	switch (ctrl->id) {
> +	case V4L2_CID_PIXEL_RATE:
> +		return 0;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct v4l2_ctrl_ops max9286_ctrl_ops = {
> +	.s_ctrl	= max9286_s_ctrl,
> +};
> +
>  static int max9286_v4l2_register(struct max9286_priv *priv)
>  {
>  	struct device *dev = &priv->client->dev;
> @@ -777,12 +809,12 @@ static int max9286_v4l2_register(struct max9286_priv *priv)
>  	priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>
>  	v4l2_ctrl_handler_init(&priv->ctrls, 1);
> -	/*
> -	 * FIXME: Compute the real pixel rate. The 50 MP/s value comes from the
> -	 * hardcoded frequency in the BSP CSI-2 receiver driver.
> -	 */
> -	v4l2_ctrl_new_std(&priv->ctrls, NULL, V4L2_CID_PIXEL_RATE,
> -			  50000000, 50000000, 1, 50000000);
> +
> +	priv->pixelrate = v4l2_ctrl_new_std(&priv->ctrls,
> +					    &max9286_ctrl_ops,
> +					    V4L2_CID_PIXEL_RATE,
> +					    1, INT_MAX, 1, 50000000);
> +
>  	priv->sd.ctrl_handler = &priv->ctrls;
>  	ret = priv->ctrls.error;
>  	if (ret)
> --
> 2.20.1
>

Re: [PATCH v8 10/13] squash! max9286: Implement Pixelrate control

[Laurent Pinchart]

Hi Jacopo,

On Tue, Apr 14, 2020 at 12:50:57PM +0200, Jacopo Mondi wrote:
> Hi Kieran,
>    +Niklas and Laurent for R-Car CSI-2 question
> 
> sorry I now had the time to have a proper look to this one
> 
> On Thu, Apr 09, 2020 at 01:11:59PM +0100, Kieran Bingham wrote:
> > Determine the (CSI2) pixel rate control by providing a control to read,
> > and checking the rate from the upstream camera sensors, and their
> > appropriate formats.
> >
> > Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > ---
> >  drivers/media/i2c/max9286.c | 44 ++++++++++++++++++++++++++++++++-----
> >  1 file changed, 38 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > index 17830c362a50..008a93910300 100644
> > --- a/drivers/media/i2c/max9286.c
> > +++ b/drivers/media/i2c/max9286.c
> > @@ -155,6 +155,7 @@ struct max9286_priv {
> >  	bool mux_open;
> >
> >  	struct v4l2_ctrl_handler ctrls;
> > +	struct v4l2_ctrl *pixelrate;
> >
> >  	struct v4l2_mbus_framefmt fmt[MAX9286_N_SINKS];
> >
> > @@ -631,6 +632,16 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
> >  	return 0;
> >  }
> >
> > +static int max9286_set_pixelrate(struct max9286_priv *priv, s64 rate)
> > +{
> > +	if (!priv->pixelrate)
> > +		return -EINVAL;
> > +
> > +	dev_err(&priv->client->dev, "Setting pixel rate to %lld\n", rate);
> > +
> > +	return v4l2_ctrl_s_ctrl_int64(priv->pixelrate, rate);
> > +}
> > +
> >  static int max9286_enum_mbus_code(struct v4l2_subdev *sd,
> >  				  struct v4l2_subdev_pad_config *cfg,
> >  				  struct v4l2_subdev_mbus_code_enum *code)
> > @@ -664,6 +675,7 @@ static int max9286_set_fmt(struct v4l2_subdev *sd,
> >  {
> >  	struct max9286_priv *priv = sd_to_max9286(sd);
> >  	struct v4l2_mbus_framefmt *cfg_fmt;
> > +	s64 pixelrate;
> >
> >  	if (format->pad >= MAX9286_SRC_PAD)
> >  		return -EINVAL;
> > @@ -688,6 +700,12 @@ static int max9286_set_fmt(struct v4l2_subdev *sd,
> >  	*cfg_fmt = format->format;
> >  	mutex_unlock(&priv->mutex);
> >
> > +	/* Update pixel rate for the CSI2 receiver */
> > +	pixelrate = cfg_fmt->width * cfg_fmt->height
> > +		  * priv->nsources * 30 /*FPS*/;
> 
> I would have calculated this differently, as this to me should come from
> the CSI-2 link frequency.
> 
> In case of 4 * RDACM20 (1280*800, YUYV8_2X8, 30 FPS)
> 
> Tot Bandwidht = 4 * (1280 * 800 * 16 * 30) = 1,966,080,000 bits

You're forgetting the blanking here.

> CSI-2 Link Freq = TotBandwidth / lanes / DDR =
>                 = 1,966,080,000 / 4 / 2 = 245,760,000 Mhz

And here you're not taking protocol overhead into account. You'll end up
with a too low frequency (and when I say you, it equally applies to this
patch).

The pixel rate should be computed based on the CSI-2 link frequency
instead, which itself should be based on the GMSL link frequency (unless
my understanding is wrong, the MAX9286 doesn't support retiming the
video signal to reduce or increase the clock frequency through increase
or reduction of horizontal blanking), itself based on the pixel clock
input of the serializer (with a few parameters taken into account, such
as double-input mode, high data-rate mode and 24-/32-bit mode).

> CSI-2 Lane Bandw = TotBandwith / lanes
>                  = 491,520,00 Mbps
> Pixel Rate = LaneBandwidth * lanes / BPP
>            =  122,880,000 pixel/second
> 
> Which matches your calculation of
>         (width * height * channesl * FPS)
>         1280 * 800 * 4 * 30 = 122880000
> 
> So I think all of this just serves as validation of the above part.
> 
> Now, the PIXEL_RATE ctrl is queried by the CSI-2 receiver which use it
> to compute again the bandwidth per lane in Mbps, in order to set the
> PHTW value, but for H3 those values are available only up to 250Mbps,
> while here we get a 491Mbps (and the CSI-2 driver which re-does the
> calculations gets the same value).
> 
> Might this be because the total bandwidths of CSI-2 is 1Gbps ? (but in
> that case the PHTW value should be calculated depending on the lane
> nunmbers), so is our calculation of pixel rate in max9286 off ?
> 
> Also consider that the original value was set to 50MPixel/second, less
> than the half of our calculation..
> 
> What do you think ?
> 
> > +
> > +	max9286_set_pixelrate(priv, pixelrate);
> > +
> >  	return 0;
> >  }
> >
> > @@ -756,6 +774,20 @@ static const struct v4l2_subdev_internal_ops max9286_subdev_internal_ops = {
> >  	.open = max9286_open,
> >  };
> >
> > +static int max9286_s_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > +	switch (ctrl->id) {
> > +	case V4L2_CID_PIXEL_RATE:
> > +		return 0;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static const struct v4l2_ctrl_ops max9286_ctrl_ops = {
> > +	.s_ctrl	= max9286_s_ctrl,
> > +};
> > +
> >  static int max9286_v4l2_register(struct max9286_priv *priv)
> >  {
> >  	struct device *dev = &priv->client->dev;
> > @@ -777,12 +809,12 @@ static int max9286_v4l2_register(struct max9286_priv *priv)
> >  	priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> >
> >  	v4l2_ctrl_handler_init(&priv->ctrls, 1);
> > -	/*
> > -	 * FIXME: Compute the real pixel rate. The 50 MP/s value comes from the
> > -	 * hardcoded frequency in the BSP CSI-2 receiver driver.
> > -	 */
> > -	v4l2_ctrl_new_std(&priv->ctrls, NULL, V4L2_CID_PIXEL_RATE,
> > -			  50000000, 50000000, 1, 50000000);
> > +
> > +	priv->pixelrate = v4l2_ctrl_new_std(&priv->ctrls,
> > +					    &max9286_ctrl_ops,
> > +					    V4L2_CID_PIXEL_RATE,
> > +					    1, INT_MAX, 1, 50000000);
> > +
> >  	priv->sd.ctrl_handler = &priv->ctrls;
> >  	ret = priv->ctrls.error;
> >  	if (ret)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v8 10/13] squash! max9286: Implement Pixelrate control

[Jacopo Mondi]

Hi Laurent,

On Wed, Apr 15, 2020 at 12:10:02AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Tue, Apr 14, 2020 at 12:50:57PM +0200, Jacopo Mondi wrote:
> > Hi Kieran,
> >    +Niklas and Laurent for R-Car CSI-2 question
> >
> > sorry I now had the time to have a proper look to this one
> >
> > On Thu, Apr 09, 2020 at 01:11:59PM +0100, Kieran Bingham wrote:
> > > Determine the (CSI2) pixel rate control by providing a control to read,
> > > and checking the rate from the upstream camera sensors, and their
> > > appropriate formats.
> > >
> > > Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > > ---
> > >  drivers/media/i2c/max9286.c | 44 ++++++++++++++++++++++++++++++++-----
> > >  1 file changed, 38 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > > index 17830c362a50..008a93910300 100644
> > > --- a/drivers/media/i2c/max9286.c
> > > +++ b/drivers/media/i2c/max9286.c
> > > @@ -155,6 +155,7 @@ struct max9286_priv {
> > >  	bool mux_open;
> > >
> > >  	struct v4l2_ctrl_handler ctrls;
> > > +	struct v4l2_ctrl *pixelrate;
> > >
> > >  	struct v4l2_mbus_framefmt fmt[MAX9286_N_SINKS];
> > >
> > > @@ -631,6 +632,16 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
> > >  	return 0;
> > >  }
> > >
> > > +static int max9286_set_pixelrate(struct max9286_priv *priv, s64 rate)
> > > +{
> > > +	if (!priv->pixelrate)
> > > +		return -EINVAL;
> > > +
> > > +	dev_err(&priv->client->dev, "Setting pixel rate to %lld\n", rate);
> > > +
> > > +	return v4l2_ctrl_s_ctrl_int64(priv->pixelrate, rate);
> > > +}
> > > +
> > >  static int max9286_enum_mbus_code(struct v4l2_subdev *sd,
> > >  				  struct v4l2_subdev_pad_config *cfg,
> > >  				  struct v4l2_subdev_mbus_code_enum *code)
> > > @@ -664,6 +675,7 @@ static int max9286_set_fmt(struct v4l2_subdev *sd,
> > >  {
> > >  	struct max9286_priv *priv = sd_to_max9286(sd);
> > >  	struct v4l2_mbus_framefmt *cfg_fmt;
> > > +	s64 pixelrate;
> > >
> > >  	if (format->pad >= MAX9286_SRC_PAD)
> > >  		return -EINVAL;
> > > @@ -688,6 +700,12 @@ static int max9286_set_fmt(struct v4l2_subdev *sd,
> > >  	*cfg_fmt = format->format;
> > >  	mutex_unlock(&priv->mutex);
> > >
> > > +	/* Update pixel rate for the CSI2 receiver */
> > > +	pixelrate = cfg_fmt->width * cfg_fmt->height
> > > +		  * priv->nsources * 30 /*FPS*/;
> >
> > I would have calculated this differently, as this to me should come from
> > the CSI-2 link frequency.
> >
> > In case of 4 * RDACM20 (1280*800, YUYV8_2X8, 30 FPS)
> >
> > Tot Bandwidht = 4 * (1280 * 800 * 16 * 30) = 1,966,080,000 bits
>
> You're forgetting the blanking here.
>

Am I ? I understand the full line lenght should be taken into account
when programming the PLL cirtcuitry of a sensor to tune it's CSI-2
transmitter output, but, being the CSI-2 transmission happening after
several translations, I am not sure about it.

The serializer receives frames on a serial bus, driven by the sensor
reference signals (HREF, VSYNC and PCLK). As in regular parallel capture
operations (if I'm not mistaken), data are sampled during the active HREF
periods, so either the serializer sends active data to be scrambled
and encoded on the GMSL bus, or it does the same transmitting
'blankings' perdiods as specially encoded packets on the GSML bus.

I'm actually not sure how what happens there, and I assumed only valid
data gets decoded and sent but the fact that we enable HS/VS encoding
in the GMSL stream makes me wonder if blankings are not actually sent
and then encoded in the output CSI-2 stream as well.


> > CSI-2 Link Freq = TotBandwidth / lanes / DDR =
> >                 = 1,966,080,000 / 4 / 2 = 245,760,000 Mhz
>
> And here you're not taking protocol overhead into account. You'll end up
> with a too low frequency (and when I say you, it equally applies to this
> patch).
>
> The pixel rate should be computed based on the CSI-2 link frequency
> instead, which itself should be based on the GMSL link frequency (unless
> my understanding is wrong, the MAX9286 doesn't support retiming the
> video signal to reduce or increase the clock frequency through increase
> or reduction of horizontal blanking), itself based on the pixel clock
> input of the serializer (with a few parameters taken into account, such
> as double-input mode, high data-rate mode and 24-/32-bit mode).
>

The deserializer manual presents formulas to calculate the CSI-2
output bit rate per lane unrelated to the GMSL link frequency at page
47 of the manual.

Could the deserializer be capable of deducing the frequency to program its
CSI-2 transmitter to using the GSML link frequency, knowing the GMSL bus
protocol overheads?

I think we could do the same and deduce the CSI-2 frequency from the
sensor's pixel rate leaving the GMSL link overhead out of the picture.


> > CSI-2 Lane Bandw = TotBandwith / lanes
> >                  = 491,520,00 Mbps
> > Pixel Rate = LaneBandwidth * lanes / BPP
> >            =  122,880,000 pixel/second
> >
> > Which matches your calculation of
> >         (width * height * channesl * FPS)
> >         1280 * 800 * 4 * 30 = 122880000
> >
> > So I think all of this just serves as validation of the above part.
> >
> > Now, the PIXEL_RATE ctrl is queried by the CSI-2 receiver which use it
> > to compute again the bandwidth per lane in Mbps, in order to set the
> > PHTW value, but for H3 those values are available only up to 250Mbps,
> > while here we get a 491Mbps (and the CSI-2 driver which re-does the
> > calculations gets the same value).
> >
> > Might this be because the total bandwidths of CSI-2 is 1Gbps ? (but in
> > that case the PHTW value should be calculated depending on the lane
> > nunmbers), so is our calculation of pixel rate in max9286 off ?
> >
> > Also consider that the original value was set to 50MPixel/second, less
> > than the half of our calculation..
> >
> > What do you think ?
> >
> > > +
> > > +	max9286_set_pixelrate(priv, pixelrate);
> > > +
> > >  	return 0;
> > >  }
> > >
> > > @@ -756,6 +774,20 @@ static const struct v4l2_subdev_internal_ops max9286_subdev_internal_ops = {
> > >  	.open = max9286_open,
> > >  };
> > >
> > > +static int max9286_s_ctrl(struct v4l2_ctrl *ctrl)
> > > +{
> > > +	switch (ctrl->id) {
> > > +	case V4L2_CID_PIXEL_RATE:
> > > +		return 0;
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> > > +}
> > > +
> > > +static const struct v4l2_ctrl_ops max9286_ctrl_ops = {
> > > +	.s_ctrl	= max9286_s_ctrl,
> > > +};
> > > +
> > >  static int max9286_v4l2_register(struct max9286_priv *priv)
> > >  {
> > >  	struct device *dev = &priv->client->dev;
> > > @@ -777,12 +809,12 @@ static int max9286_v4l2_register(struct max9286_priv *priv)
> > >  	priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> > >
> > >  	v4l2_ctrl_handler_init(&priv->ctrls, 1);
> > > -	/*
> > > -	 * FIXME: Compute the real pixel rate. The 50 MP/s value comes from the
> > > -	 * hardcoded frequency in the BSP CSI-2 receiver driver.
> > > -	 */
> > > -	v4l2_ctrl_new_std(&priv->ctrls, NULL, V4L2_CID_PIXEL_RATE,
> > > -			  50000000, 50000000, 1, 50000000);
> > > +
> > > +	priv->pixelrate = v4l2_ctrl_new_std(&priv->ctrls,
> > > +					    &max9286_ctrl_ops,
> > > +					    V4L2_CID_PIXEL_RATE,
> > > +					    1, INT_MAX, 1, 50000000);
> > > +
> > >  	priv->sd.ctrl_handler = &priv->ctrls;
> > >  	ret = priv->ctrls.error;
> > >  	if (ret)
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH v8 10/13] squash! max9286: Implement Pixelrate control

[Laurent Pinchart]

Hi Jacopo,

On Wed, Apr 15, 2020 at 11:13:57AM +0200, Jacopo Mondi wrote:
> On Wed, Apr 15, 2020 at 12:10:02AM +0300, Laurent Pinchart wrote:
> > On Tue, Apr 14, 2020 at 12:50:57PM +0200, Jacopo Mondi wrote:
> >> Hi Kieran,
> >>    +Niklas and Laurent for R-Car CSI-2 question
> >>
> >> sorry I now had the time to have a proper look to this one
> >>
> >> On Thu, Apr 09, 2020 at 01:11:59PM +0100, Kieran Bingham wrote:
> >>> Determine the (CSI2) pixel rate control by providing a control to read,
> >>> and checking the rate from the upstream camera sensors, and their
> >>> appropriate formats.
> >>>
> >>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> >>> ---
> >>>  drivers/media/i2c/max9286.c | 44 ++++++++++++++++++++++++++++++++-----
> >>>  1 file changed, 38 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> >>> index 17830c362a50..008a93910300 100644
> >>> --- a/drivers/media/i2c/max9286.c
> >>> +++ b/drivers/media/i2c/max9286.c
> >>> @@ -155,6 +155,7 @@ struct max9286_priv {
> >>>  	bool mux_open;
> >>>
> >>>  	struct v4l2_ctrl_handler ctrls;
> >>> +	struct v4l2_ctrl *pixelrate;
> >>>
> >>>  	struct v4l2_mbus_framefmt fmt[MAX9286_N_SINKS];
> >>>
> >>> @@ -631,6 +632,16 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
> >>>  	return 0;
> >>>  }
> >>>
> >>> +static int max9286_set_pixelrate(struct max9286_priv *priv, s64 rate)
> >>> +{
> >>> +	if (!priv->pixelrate)
> >>> +		return -EINVAL;
> >>> +
> >>> +	dev_err(&priv->client->dev, "Setting pixel rate to %lld\n", rate);
> >>> +
> >>> +	return v4l2_ctrl_s_ctrl_int64(priv->pixelrate, rate);
> >>> +}
> >>> +
> >>>  static int max9286_enum_mbus_code(struct v4l2_subdev *sd,
> >>>  				  struct v4l2_subdev_pad_config *cfg,
> >>>  				  struct v4l2_subdev_mbus_code_enum *code)
> >>> @@ -664,6 +675,7 @@ static int max9286_set_fmt(struct v4l2_subdev *sd,
> >>>  {
> >>>  	struct max9286_priv *priv = sd_to_max9286(sd);
> >>>  	struct v4l2_mbus_framefmt *cfg_fmt;
> >>> +	s64 pixelrate;
> >>>
> >>>  	if (format->pad >= MAX9286_SRC_PAD)
> >>>  		return -EINVAL;
> >>> @@ -688,6 +700,12 @@ static int max9286_set_fmt(struct v4l2_subdev *sd,
> >>>  	*cfg_fmt = format->format;
> >>>  	mutex_unlock(&priv->mutex);
> >>>
> >>> +	/* Update pixel rate for the CSI2 receiver */
> >>> +	pixelrate = cfg_fmt->width * cfg_fmt->height
> >>> +		  * priv->nsources * 30 /*FPS*/;
> >>
> >> I would have calculated this differently, as this to me should come from
> >> the CSI-2 link frequency.
> >>
> >> In case of 4 * RDACM20 (1280*800, YUYV8_2X8, 30 FPS)
> >>
> >> Tot Bandwidht = 4 * (1280 * 800 * 16 * 30) = 1,966,080,000 bits
> >
> > You're forgetting the blanking here.
> 
> Am I ? I understand the full line lenght should be taken into account
> when programming the PLL cirtcuitry of a sensor to tune it's CSI-2
> transmitter output, but, being the CSI-2 transmission happening after
> several translations, I am not sure about it.
> 
> The serializer receives frames on a serial bus, driven by the sensor
> reference signals (HREF, VSYNC and PCLK). As in regular parallel capture
> operations (if I'm not mistaken), data are sampled during the active HREF
> periods, so either the serializer sends active data to be scrambled
> and encoded on the GMSL bus, or it does the same transmitting
> 'blankings' perdiods as specially encoded packets on the GSML bus.
> 
> I'm actually not sure how what happens there, and I assumed only valid
> data gets decoded and sent but the fact that we enable HS/VS encoding
> in the GMSL stream makes me wonder if blankings are not actually sent
> and then encoded in the output CSI-2 stream as well.

I would be surprised if the serializer or deserializer performed any
kind of clock adaptation to drop the blanking. That would require
spreading the active data across the duration of a whole line, requiring
an internal line buffer, and circuitry to handle the spreading. As the
serializer or deserializer are not programmed with the line length, they
would have to detect it automatically, which I really don't think they
do.

This is even more improbable for vertical blanking, as the lines would
need to be spread over the whole image, requiring an internal buffer of
multiple lines.

> >> CSI-2 Link Freq = TotBandwidth / lanes / DDR =
> >>                 = 1,966,080,000 / 4 / 2 = 245,760,000 Mhz
> >
> > And here you're not taking protocol overhead into account. You'll end up
> > with a too low frequency (and when I say you, it equally applies to this
> > patch).
> >
> > The pixel rate should be computed based on the CSI-2 link frequency
> > instead, which itself should be based on the GMSL link frequency (unless
> > my understanding is wrong, the MAX9286 doesn't support retiming the
> > video signal to reduce or increase the clock frequency through increase
> > or reduction of horizontal blanking), itself based on the pixel clock
> > input of the serializer (with a few parameters taken into account, such
> > as double-input mode, high data-rate mode and 24-/32-bit mode).
> 
> The deserializer manual presents formulas to calculate the CSI-2
> output bit rate per lane unrelated to the GMSL link frequency at page
> 47 of the manual.
> 
> Could the deserializer be capable of deducing the frequency to program its
> CSI-2 transmitter to using the GSML link frequency, knowing the GMSL bus
> protocol overheads?

I expect the MAX9286 to slave its PLLs to the incoming clock (recovered
from the signal on the coax cable), with divisors and multipliers to
take DDR, the number of lanes, the number of channels and similar
parameters into account, but without any kind of protocol overhead
calculation.

> I think we could do the same and deduce the CSI-2 frequency from the
> sensor's pixel rate leaving the GMSL link overhead out of the picture.

The CSI-2 bandwidth should be equal to the bandwidth per camera
multiplied by the number of cameras. The bandwidth per camera is equal
to the camera pixel rate multiplied by the number of bits per pixel. It
shouldn't need to be more complicated than that. If you need the pixel
rate on the CSI-2 bus, it's the pixel rate per camera multipled by the
number of cameras. There's no need to perform any calculation here that
would take the width and height into account, just get the pixel rate
from the serializer, which should get it from the camera, and multiply
it by the number of active channels. You may want, as a sanity check, to
verify that all serializers report the same pixel rate.

> >> CSI-2 Lane Bandw = TotBandwith / lanes
> >>                  = 491,520,00 Mbps
> >> Pixel Rate = LaneBandwidth * lanes / BPP
> >>            =  122,880,000 pixel/second
> >>
> >> Which matches your calculation of
> >>         (width * height * channesl * FPS)
> >>         1280 * 800 * 4 * 30 = 122880000
> >>
> >> So I think all of this just serves as validation of the above part.
> >>
> >> Now, the PIXEL_RATE ctrl is queried by the CSI-2 receiver which use it
> >> to compute again the bandwidth per lane in Mbps, in order to set the
> >> PHTW value, but for H3 those values are available only up to 250Mbps,
> >> while here we get a 491Mbps (and the CSI-2 driver which re-does the
> >> calculations gets the same value).
> >>
> >> Might this be because the total bandwidths of CSI-2 is 1Gbps ? (but in
> >> that case the PHTW value should be calculated depending on the lane
> >> nunmbers), so is our calculation of pixel rate in max9286 off ?
> >>
> >> Also consider that the original value was set to 50MPixel/second, less
> >> than the half of our calculation..
> >>
> >> What do you think ?
> >>
> >>> +
> >>> +	max9286_set_pixelrate(priv, pixelrate);
> >>> +
> >>>  	return 0;
> >>>  }
> >>>
> >>> @@ -756,6 +774,20 @@ static const struct v4l2_subdev_internal_ops max9286_subdev_internal_ops = {
> >>>  	.open = max9286_open,
> >>>  };
> >>>
> >>> +static int max9286_s_ctrl(struct v4l2_ctrl *ctrl)
> >>> +{
> >>> +	switch (ctrl->id) {
> >>> +	case V4L2_CID_PIXEL_RATE:
> >>> +		return 0;
> >>> +	default:
> >>> +		return -EINVAL;
> >>> +	}
> >>> +}
> >>> +
> >>> +static const struct v4l2_ctrl_ops max9286_ctrl_ops = {
> >>> +	.s_ctrl	= max9286_s_ctrl,
> >>> +};
> >>> +
> >>>  static int max9286_v4l2_register(struct max9286_priv *priv)
> >>>  {
> >>>  	struct device *dev = &priv->client->dev;
> >>> @@ -777,12 +809,12 @@ static int max9286_v4l2_register(struct max9286_priv *priv)
> >>>  	priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> >>>
> >>>  	v4l2_ctrl_handler_init(&priv->ctrls, 1);
> >>> -	/*
> >>> -	 * FIXME: Compute the real pixel rate. The 50 MP/s value comes from the
> >>> -	 * hardcoded frequency in the BSP CSI-2 receiver driver.
> >>> -	 */
> >>> -	v4l2_ctrl_new_std(&priv->ctrls, NULL, V4L2_CID_PIXEL_RATE,
> >>> -			  50000000, 50000000, 1, 50000000);
> >>> +
> >>> +	priv->pixelrate = v4l2_ctrl_new_std(&priv->ctrls,
> >>> +					    &max9286_ctrl_ops,
> >>> +					    V4L2_CID_PIXEL_RATE,
> >>> +					    1, INT_MAX, 1, 50000000);
> >>> +
> >>>  	priv->sd.ctrl_handler = &priv->ctrls;
> >>>  	ret = priv->ctrls.error;
> >>>  	if (ret)

-- 
Regards,

Laurent Pinchart

[PATCH v8 11/13] squash! max9286: Disable overlap window

[Kieran Bingham]

Provide a function to control setting of the overlap window, but disable
it by default.

The function will allow the value to be easily updated in the future,
either statically in the code, or via an external control mechanism.

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/media/i2c/max9286.c | 42 +++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 008a93910300..61178ae363d6 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -118,6 +118,9 @@
 #define MAX9286_REV_FLEN(n)		((n) - 20)
 /* Register 0x49 */
 #define MAX9286_VIDEO_DETECT_MASK	0x0f
+/* Register 0x64 */
+#define MAX9286_ENFSINLAST		BIT(5)
+#define MAX9286_OVLP_WINDOWH_MASK	GENMASK(4, 0)
 /* Register 0x69 */
 #define MAX9286_LFLTBMONMASKED		BIT(7)
 #define MAX9286_LOCKMONMASKED		BIT(6)
@@ -632,6 +635,34 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
 	return 0;
 }
 
+/*
+ * The overlap window is a 13 bit value with the low byte in register 0x63 and
+ * the high byte(5bits) stored in the least significant bits of register 0x64.
+ */
+static int max9286_set_overlap_window(struct max9286_priv *priv, u16 window)
+{
+	int ret;
+	u8 val;
+
+	ret = max9286_read(priv, 0x64);
+	if (ret < 0)
+		return -EIO;
+
+	max9286_write(priv, 0x63, window & 0xff);
+
+	/*
+	 * Process the high byte, while preserve existing bits set in 0x64.
+	 * TODO: Convert this all to regmap so we can utilise regmap_update_bits
+	 */
+	window >>= 8;
+	val = ret & ~MAX9286_OVLP_WINDOWH_MASK;
+	val |= window & MAX9286_OVLP_WINDOWH_MASK;
+
+	max9286_write(priv, 0x64, val);
+
+	return 0;
+}
+
 static int max9286_set_pixelrate(struct max9286_priv *priv, s64 rate)
 {
 	if (!priv->pixelrate)
@@ -942,6 +973,17 @@ static int max9286_setup(struct max9286_priv *priv)
 	max9286_write(priv, 0x0c, MAX9286_HVEN | MAX9286_INVVS |
 		      MAX9286_HVSRC_D14);
 
+	/*
+	 * The overlap window seems to provide additional validation by tracking
+	 * the delay between vsync and frame sync, generating an error if the
+	 * delay is bigger than the programmed window, though it's not yet clear
+	 * what value should be set.
+	 *
+	 * As it's an optional value and can be disabled, we do so by setting
+	 * a 0 overlap value.
+	 */
+	max9286_set_overlap_window(priv, 0);
+
 	/*
 	 * Wait for 2ms to allow the link to resynchronize after the
 	 * configuration change.
-- 
2.20.1

Re: [PATCH v8 11/13] squash! max9286: Disable overlap window

[Jacopo Mondi]

Hi Kieran,

On Thu, Apr 09, 2020 at 01:12:00PM +0100, Kieran Bingham wrote:
> Provide a function to control setting of the overlap window, but disable
> it by default.
>
> The function will allow the value to be easily updated in the future,
> either statically in the code, or via an external control mechanism.
>
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
>  drivers/media/i2c/max9286.c | 42 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
>
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 008a93910300..61178ae363d6 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -118,6 +118,9 @@
>  #define MAX9286_REV_FLEN(n)		((n) - 20)
>  /* Register 0x49 */
>  #define MAX9286_VIDEO_DETECT_MASK	0x0f
> +/* Register 0x64 */
> +#define MAX9286_ENFSINLAST		BIT(5)
> +#define MAX9286_OVLP_WINDOWH_MASK	GENMASK(4, 0)
>  /* Register 0x69 */
>  #define MAX9286_LFLTBMONMASKED		BIT(7)
>  #define MAX9286_LOCKMONMASKED		BIT(6)
> @@ -632,6 +635,34 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
>  	return 0;
>  }
>
> +/*
> + * The overlap window is a 13 bit value with the low byte in register 0x63 and
> + * the high byte(5bits) stored in the least significant bits of register 0x64.
                   ^ space

But I'm not sure this is useful comment. The register layout is
actually the only documented thing of this register :)

> + */
> +static int max9286_set_overlap_window(struct max9286_priv *priv, u16 window)
> +{
> +	int ret;
> +	u8 val;
> +
> +	ret = max9286_read(priv, 0x64);
> +	if (ret < 0)
> +		return -EIO;
> +
> +	max9286_write(priv, 0x63, window & 0xff);
> +
> +	/*
> +	 * Process the high byte, while preserve existing bits set in 0x64.
> +	 * TODO: Convert this all to regmap so we can utilise regmap_update_bits
> +	 */
> +	window >>= 8;
> +	val = ret & ~MAX9286_OVLP_WINDOWH_MASK;
> +	val |= window & MAX9286_OVLP_WINDOWH_MASK;
> +
> +	max9286_write(priv, 0x64, val);
> +
> +	return 0;
> +}
> +
>  static int max9286_set_pixelrate(struct max9286_priv *priv, s64 rate)
>  {
>  	if (!priv->pixelrate)
> @@ -942,6 +973,17 @@ static int max9286_setup(struct max9286_priv *priv)
>  	max9286_write(priv, 0x0c, MAX9286_HVEN | MAX9286_INVVS |
>  		      MAX9286_HVSRC_D14);
>
> +	/*
> +	 * The overlap window seems to provide additional validation by tracking
> +	 * the delay between vsync and frame sync, generating an error if the
> +	 * delay is bigger than the programmed window, though it's not yet clear
> +	 * what value should be set.
> +	 *
> +	 * As it's an optional value and can be disabled, we do so by setting
> +	 * a 0 overlap value.
> +	 */

This is useful instead :)


> +	max9286_set_overlap_window(priv, 0);
> +
>  	/*
>  	 * Wait for 2ms to allow the link to resynchronize after the
>  	 * configuration change.
> --
> 2.20.1
>

Re: [PATCH v8 11/13] squash! max9286: Disable overlap window

[Kieran Bingham]

Hi Jacopo,

On 09/04/2020 17:32, Jacopo Mondi wrote:
> Hi Kieran,
> 
> On Thu, Apr 09, 2020 at 01:12:00PM +0100, Kieran Bingham wrote:
>> Provide a function to control setting of the overlap window, but disable
>> it by default.
>>
>> The function will allow the value to be easily updated in the future,
>> either statically in the code, or via an external control mechanism.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>> ---
>>  drivers/media/i2c/max9286.c | 42 +++++++++++++++++++++++++++++++++++++
>>  1 file changed, 42 insertions(+)
>>
>> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
>> index 008a93910300..61178ae363d6 100644
>> --- a/drivers/media/i2c/max9286.c
>> +++ b/drivers/media/i2c/max9286.c
>> @@ -118,6 +118,9 @@
>>  #define MAX9286_REV_FLEN(n)		((n) - 20)
>>  /* Register 0x49 */
>>  #define MAX9286_VIDEO_DETECT_MASK	0x0f
>> +/* Register 0x64 */
>> +#define MAX9286_ENFSINLAST		BIT(5)
>> +#define MAX9286_OVLP_WINDOWH_MASK	GENMASK(4, 0)
>>  /* Register 0x69 */
>>  #define MAX9286_LFLTBMONMASKED		BIT(7)
>>  #define MAX9286_LOCKMONMASKED		BIT(6)
>> @@ -632,6 +635,34 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
>>  	return 0;
>>  }
>>
>> +/*
>> + * The overlap window is a 13 bit value with the low byte in register 0x63 and
>> + * the high byte(5bits) stored in the least significant bits of register 0x64.
>                    ^ space
> 
> But I'm not sure this is useful comment. The register layout is
> actually the only documented thing of this register :)

Sure, but readers won't necessarily have the documentation :-) I think I
was just trying to explain what was happening because we have so many
unidentified registers (i.e. they're all just numbers, so we can't do,

  max9286_write(priv, MAX9286_OVERLAP_LOW, overlap & 0xff);
  max9286_write(priv, MAX9286_OVERLAP_HIGH, (overlap >> SHIFT) & MASK);

I can drop this comment all the same, although I'd like to document
clearly that it's a 13-bit value as an input, (not a full 16 bit)

Will this be better suited?


/*
 * The overlap window is a 13 bit value stored across two registers.
 * The definition and units of the value is undocumented.
 */



>> + */
>> +static int max9286_set_overlap_window(struct max9286_priv *priv, u16 window)
>> +{
>> +	int ret;
>> +	u8 val;
>> +
>> +	ret = max9286_read(priv, 0x64);
>> +	if (ret < 0)
>> +		return -EIO;
>> +
>> +	max9286_write(priv, 0x63, window & 0xff);
>> +
>> +	/*
>> +	 * Process the high byte, while preserve existing bits set in 0x64.
>> +	 * TODO: Convert this all to regmap so we can utilise regmap_update_bits
>> +	 */
>> +	window >>= 8;
>> +	val = ret & ~MAX9286_OVLP_WINDOWH_MASK;
>> +	val |= window & MAX9286_OVLP_WINDOWH_MASK;
>> +
>> +	max9286_write(priv, 0x64, val);
>> +
>> +	return 0;
>> +}
>> +
>>  static int max9286_set_pixelrate(struct max9286_priv *priv, s64 rate)
>>  {
>>  	if (!priv->pixelrate)
>> @@ -942,6 +973,17 @@ static int max9286_setup(struct max9286_priv *priv)
>>  	max9286_write(priv, 0x0c, MAX9286_HVEN | MAX9286_INVVS |
>>  		      MAX9286_HVSRC_D14);
>>
>> +	/*
>> +	 * The overlap window seems to provide additional validation by tracking
>> +	 * the delay between vsync and frame sync, generating an error if the
>> +	 * delay is bigger than the programmed window, though it's not yet clear
>> +	 * what value should be set.
>> +	 *
>> +	 * As it's an optional value and can be disabled, we do so by setting
>> +	 * a 0 overlap value.
>> +	 */
> 
> This is useful instead :)


:-)


>> +	max9286_set_overlap_window(priv, 0);
>> +
>>  	/*
>>  	 * Wait for 2ms to allow the link to resynchronize after the
>>  	 * configuration change.
>> --
>> 2.20.1
>>

Re: [PATCH v8 11/13] squash! max9286: Disable overlap window

[Jacopo Mondi]

Hi Kieran,

On Fri, Apr 10, 2020 at 08:14:18AM +0100, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 09/04/2020 17:32, Jacopo Mondi wrote:
> > Hi Kieran,
> >
> > On Thu, Apr 09, 2020 at 01:12:00PM +0100, Kieran Bingham wrote:
> >> Provide a function to control setting of the overlap window, but disable
> >> it by default.
> >>
> >> The function will allow the value to be easily updated in the future,
> >> either statically in the code, or via an external control mechanism.
> >>
> >> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> >> ---
> >>  drivers/media/i2c/max9286.c | 42 +++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 42 insertions(+)
> >>
> >> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> >> index 008a93910300..61178ae363d6 100644
> >> --- a/drivers/media/i2c/max9286.c
> >> +++ b/drivers/media/i2c/max9286.c
> >> @@ -118,6 +118,9 @@
> >>  #define MAX9286_REV_FLEN(n)		((n) - 20)
> >>  /* Register 0x49 */
> >>  #define MAX9286_VIDEO_DETECT_MASK	0x0f
> >> +/* Register 0x64 */
> >> +#define MAX9286_ENFSINLAST		BIT(5)
> >> +#define MAX9286_OVLP_WINDOWH_MASK	GENMASK(4, 0)
> >>  /* Register 0x69 */
> >>  #define MAX9286_LFLTBMONMASKED		BIT(7)
> >>  #define MAX9286_LOCKMONMASKED		BIT(6)
> >> @@ -632,6 +635,34 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
> >>  	return 0;
> >>  }
> >>
> >> +/*
> >> + * The overlap window is a 13 bit value with the low byte in register 0x63 and
> >> + * the high byte(5bits) stored in the least significant bits of register 0x64.
> >                    ^ space
> >
> > But I'm not sure this is useful comment. The register layout is
> > actually the only documented thing of this register :)
>
> Sure, but readers won't necessarily have the documentation :-) I think I
> was just trying to explain what was happening because we have so many
> unidentified registers (i.e. they're all just numbers, so we can't do,
>
>   max9286_write(priv, MAX9286_OVERLAP_LOW, overlap & 0xff);
>   max9286_write(priv, MAX9286_OVERLAP_HIGH, (overlap >> SHIFT) & MASK);
>
> I can drop this comment all the same, although I'd like to document
> clearly that it's a 13-bit value as an input, (not a full 16 bit)
>
> Will this be better suited?
>
>
> /*
>  * The overlap window is a 13 bit value stored across two registers.
>  * The definition and units of the value is undocumented.
>  */

Oh no worries, if you want to keep that comment the first version is
perfectly fine.

[PATCH v8 12/13] sqaush! max9286: Describe pad index usage

[Kieran Bingham]

Sakari commented that the sink/source pad indexes are not the same as
the OF port numbers. We define them such that they match, so that they
can be used interchangably - but of course they are not the 'same
thing'. Document this in a comment at the definition of the pad sizings.

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/media/i2c/max9286.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 61178ae363d6..6f114756a1e2 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -128,6 +128,10 @@
 #define MAX9286_AUTOMASKEN		BIT(4)
 #define MAX9286_MASKLINK(n)		((n) << 0)
 
+/*
+ * The sink and source pads are created to match the OF graph port numbers so
+ * that their indexes can be used interchangeably.
+ */
 #define MAX9286_NUM_GMSL		4
 #define MAX9286_N_SINKS			4
 #define MAX9286_N_PADS			5
-- 
2.20.1

[PATCH v8 13/13] sqaush! max9286: Remove poc_enabled workaround

[Kieran Bingham]

This boolean is a flag used to handle the regulator when our
multi-max9286 workaround is in place.  It shouldn't be in the upstream
driver, and is moved out.

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/media/i2c/max9286.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 6f114756a1e2..022f4cfaf294 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -152,7 +152,6 @@ struct max9286_priv {
 	struct v4l2_subdev sd;
 	struct media_pad pads[MAX9286_N_PADS];
 	struct regulator *regulator;
-	bool poc_enabled;
 
 	struct gpio_chip gpio;
 	u8 gpio_state;
@@ -1066,8 +1065,6 @@ static int max9286_init(struct device *dev)
 		return ret;
 	}
 
-	priv->poc_enabled = true;
-
 	ret = max9286_setup(priv);
 	if (ret) {
 		dev_err(dev, "Unable to setup max9286\n");
@@ -1099,7 +1096,6 @@ static int max9286_init(struct device *dev)
 	max9286_v4l2_unregister(priv);
 err_regulator:
 	regulator_disable(priv->regulator);
-	priv->poc_enabled = false;
 
 	return ret;
 }
@@ -1324,8 +1320,7 @@ static int max9286_remove(struct i2c_client *client)
 
 	max9286_v4l2_unregister(priv);
 
-	if (priv->poc_enabled)
-		regulator_disable(priv->regulator);
+	regulator_disable(priv->regulator);
 	regulator_put(priv->regulator);
 
 	gpiod_set_value_cansleep(priv->gpiod_pwdn, 0);
-- 
2.20.1

Re: [PATCH v8 13/13] sqaush! max9286: Remove poc_enabled workaround

[Jacopo Mondi]

Hi Kieran,

On Thu, Apr 09, 2020 at 01:12:02PM +0100, Kieran Bingham wrote:
> This boolean is a flag used to handle the regulator when our
> multi-max9286 workaround is in place.  It shouldn't be in the upstream
> driver, and is moved out.
>

For this and all the other patches where I didn't have comments on,
please consider Acked-by: Jacopo Mondi <jacopo@jmondi.org> and feel
free to squash!

Thanks, this is looking much better now!


> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
>  drivers/media/i2c/max9286.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 6f114756a1e2..022f4cfaf294 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -152,7 +152,6 @@ struct max9286_priv {
>  	struct v4l2_subdev sd;
>  	struct media_pad pads[MAX9286_N_PADS];
>  	struct regulator *regulator;
> -	bool poc_enabled;
>
>  	struct gpio_chip gpio;
>  	u8 gpio_state;
> @@ -1066,8 +1065,6 @@ static int max9286_init(struct device *dev)
>  		return ret;
>  	}
>
> -	priv->poc_enabled = true;
> -
>  	ret = max9286_setup(priv);
>  	if (ret) {
>  		dev_err(dev, "Unable to setup max9286\n");
> @@ -1099,7 +1096,6 @@ static int max9286_init(struct device *dev)
>  	max9286_v4l2_unregister(priv);
>  err_regulator:
>  	regulator_disable(priv->regulator);
> -	priv->poc_enabled = false;
>
>  	return ret;
>  }
> @@ -1324,8 +1320,7 @@ static int max9286_remove(struct i2c_client *client)
>
>  	max9286_v4l2_unregister(priv);
>
> -	if (priv->poc_enabled)
> -		regulator_disable(priv->regulator);
> +	regulator_disable(priv->regulator);
>  	regulator_put(priv->regulator);
>
>  	gpiod_set_value_cansleep(priv->gpiod_pwdn, 0);
> --
> 2.20.1
>