[v4,3/7] media: i2c: ov9282: Add ov9281 compatible

  According to product brief they are identical from software point of view.
Differences are a different chief ray angle (CRA) and the package.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
Acked-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com>
---
 drivers/media/i2c/ov9282.c | 1 +
 1 file changed, 1 insertion(+)
  

On 28/07/2022 15:02, Alexander Stein wrote:
> According to product brief they are identical from software point of view.
> Differences are a different chief ray angle (CRA) and the package.
> 
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> Acked-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com>
> ---
>  drivers/media/i2c/ov9282.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index 8a252bf3b59f..c8d83a29f9bb 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -1113,6 +1113,7 @@ static const struct dev_pm_ops ov9282_pm_ops = {
>  };
>  
>  static const struct of_device_id ov9282_of_match[] = {
> +	{ .compatible = "ovti,ov9281" },

The devices seem entirely compatible, so why you add a new compatible
and not re-use existing?

The difference in lens does not explain this.


Best regards,
Krzysztof
  

Hi Krzysztof,

On Thu, Jul 28, 2022 at 03:13:11PM +0200, Krzysztof Kozlowski wrote:
> On 28/07/2022 15:02, Alexander Stein wrote:
> > According to product brief they are identical from software point of view.
> > Differences are a different chief ray angle (CRA) and the package.
> > 
> > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > Acked-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com>
> > ---
> >  drivers/media/i2c/ov9282.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > index 8a252bf3b59f..c8d83a29f9bb 100644
> > --- a/drivers/media/i2c/ov9282.c
> > +++ b/drivers/media/i2c/ov9282.c
> > @@ -1113,6 +1113,7 @@ static const struct dev_pm_ops ov9282_pm_ops = {
> >  };
> >  
> >  static const struct of_device_id ov9282_of_match[] = {
> > +	{ .compatible = "ovti,ov9281" },
> 
> The devices seem entirely compatible, so why you add a new compatible
> and not re-use existing?
> 
> The difference in lens does not explain this.

It is typically necessary to know what kind of related hardware can be
found in the system, beyond just the device's register interface. Apart
from USB cameras, less integrated cameras require low-level software
control in which specific device properties are important. In this case it
could be the lens shading table, among other things.

	https://www.ovt.com/sensor/ov9282/

Therefore I think adding a specific compatible string for this one is
justified.

Also cc Laurent.
  

Hi Sakari,

(Adding Dave and Naush to the CC list)

On Fri, Jul 29, 2022 at 10:07:36AM +0300, Sakari Ailus wrote:
> On Thu, Jul 28, 2022 at 03:13:11PM +0200, Krzysztof Kozlowski wrote:
> > On 28/07/2022 15:02, Alexander Stein wrote:
> > > According to product brief they are identical from software point of view.
> > > Differences are a different chief ray angle (CRA) and the package.
> > > 
> > > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > > Acked-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com>
> > > ---
> > >  drivers/media/i2c/ov9282.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > > index 8a252bf3b59f..c8d83a29f9bb 100644
> > > --- a/drivers/media/i2c/ov9282.c
> > > +++ b/drivers/media/i2c/ov9282.c
> > > @@ -1113,6 +1113,7 @@ static const struct dev_pm_ops ov9282_pm_ops = {
> > >  };
> > >  
> > >  static const struct of_device_id ov9282_of_match[] = {
> > > +	{ .compatible = "ovti,ov9281" },
> > 
> > The devices seem entirely compatible, so why you add a new compatible
> > and not re-use existing?
> > 
> > The difference in lens does not explain this.
> 
> It is typically necessary to know what kind of related hardware can be
> found in the system, beyond just the device's register interface. Apart
> from USB cameras, less integrated cameras require low-level software
> control in which specific device properties are important. In this case it
> could be the lens shading table, among other things.
> 
> 	https://www.ovt.com/sensor/ov9282/
> 
> Therefore I think adding a specific compatible string for this one is
> justified.
> 
> Also cc Laurent.

Interesting coincidence, we've talked about this topic (as part of a
broader discussion) no later than yesterday.

I agree with Sakari in that userspace needs to know the exact model of
the camera sensor. I don't see a good alternative to providing that
information through the platform firmware, so the device tree in this
case. The question is how it should be provided (the question of how it
should then be exposed to userspace is also important, but out of scope
in this discussion).

The compatible string is meant to indicate a device's compatibility with
"something", and that something is often considered from the point of
view of software support, and in particular to pick an appropriate
kernel driver and tune its behaviour for the device. Here, one could
argue that the exact model is also needed to ensure proper software
support, but in userspace this time, not in the kernel. I think using a
dedicated compatible string would be reasonable. An alternative would be
to use another DT property, which should then be standardized. I'm not
sure it's worth it.

Broadening the discussion, we also need to know detailed information
about the camera lens (I'm talking about the lens itself here, not the
lens controller IC that controls the motor that moves the focus lens).
The lens isn't described in the device tree with a dedicated device tree
node today, and I don't think it should (I'd have a hard time coming up
with a naming scheme for lenses that we could use in compatible strings,
and the lens-related data that a system requires can possibly vary based
not only on the lens itself but on the ISP that the camera sensor is
used with). Typical useful data are the lens movement range, the
hyperfocal distance, but also the lens shading tables. (Part of) that
information is sometimes stored in non-volatile memory in the camera
module (OTP in the camera sensor itself, or a separate EEPROM), but
that's not always the case. We have considered the possibility of
storing the information in the device tree, but I doubt that would be
accepted. We can store the information in userspace in configuration
files, but we will still need to device tree to provide lens
identification information to select the correct configuration file. I
don't know how that should be done.
  

On 29/07/2022 10:18, Laurent Pinchart wrote:
> Hi Sakari,
> 
> (Adding Dave and Naush to the CC list)
> 
> On Fri, Jul 29, 2022 at 10:07:36AM +0300, Sakari Ailus wrote:
>> On Thu, Jul 28, 2022 at 03:13:11PM +0200, Krzysztof Kozlowski wrote:
>>> On 28/07/2022 15:02, Alexander Stein wrote:
>>>> According to product brief they are identical from software point of view.
>>>> Differences are a different chief ray angle (CRA) and the package.
>>>>
>>>> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
>>>> Acked-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com>
>>>> ---
>>>>  drivers/media/i2c/ov9282.c | 1 +
>>>>  1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
>>>> index 8a252bf3b59f..c8d83a29f9bb 100644
>>>> --- a/drivers/media/i2c/ov9282.c
>>>> +++ b/drivers/media/i2c/ov9282.c
>>>> @@ -1113,6 +1113,7 @@ static const struct dev_pm_ops ov9282_pm_ops = {
>>>>  };
>>>>  
>>>>  static const struct of_device_id ov9282_of_match[] = {
>>>> +	{ .compatible = "ovti,ov9281" },
>>>
>>> The devices seem entirely compatible, so why you add a new compatible
>>> and not re-use existing?
>>>
>>> The difference in lens does not explain this.
>>
>> It is typically necessary to know what kind of related hardware can be
>> found in the system, beyond just the device's register interface. Apart
>> from USB cameras, less integrated cameras require low-level software
>> control in which specific device properties are important. In this case it
>> could be the lens shading table, among other things.
>>
>> 	https://www.ovt.com/sensor/ov9282/
>>
>> Therefore I think adding a specific compatible string for this one is
>> justified.

Specific compatible in binding is a requirement. No one discussed this.
However not in the driver. None of the arguments above justify adding
such binding, unless user-space depends on matching compatible, but not
real compatible?

>>
>> Also cc Laurent.
> 
> Interesting coincidence, we've talked about this topic (as part of a
> broader discussion) no later than yesterday.
> 
> I agree with Sakari in that userspace needs to know the exact model of
> the camera sensor. I don't see a good alternative to providing that
> information through the platform firmware, so the device tree in this
> case. The question is how it should be provided (the question of how it
> should then be exposed to userspace is also important, but out of scope
> in this discussion).
> 
> The compatible string is meant to indicate a device's compatibility with
> "something", and that something is often considered from the point of
> view of software support, and in particular to pick an appropriate
> kernel driver and tune its behaviour for the device. Here, one could
> argue that the exact model is also needed to ensure proper software
> support, but in userspace this time, not in the kernel. I think using a
> dedicated compatible string would be reasonable. An alternative would be
> to use another DT property, which should then be standardized. I'm not
> sure it's worth it.
> 
> Broadening the discussion, we also need to know detailed information
> about the camera lens (I'm talking about the lens itself here, not the
> lens controller IC that controls the motor that moves the focus lens).
> The lens isn't described in the device tree with a dedicated device tree
> node today, and I don't think it should (I'd have a hard time coming up
> with a naming scheme for lenses that we could use in compatible strings,
> and the lens-related data that a system requires can possibly vary based
> not only on the lens itself but on the ISP that the camera sensor is
> used with). Typical useful data are the lens movement range, the
> hyperfocal distance, but also the lens shading tables. (Part of) that
> information is sometimes stored in non-volatile memory in the camera
> module (OTP in the camera sensor itself, or a separate EEPROM), but
> that's not always the case. We have considered the possibility of
> storing the information in the device tree, but I doubt that would be
> accepted. We can store the information in userspace in configuration
> files, but we will still need to device tree to provide lens
> identification information to select the correct configuration file. I
> don't know how that should be done.

It seems both you and Sakari suggested not to have specific compatible.
Such idea (not to have specific compatible) was not proposed by me.
Quite contrary - specific compatible is a requirement. However device
driver does no need it. Just use fallback for the driver.

Best regards,
Krzysztof
  

On 01/08/2022 20:07, Krzysztof Kozlowski wrote:
> On 29/07/2022 10:18, Laurent Pinchart wrote:
>> Hi Sakari,
>>
>> (Adding Dave and Naush to the CC list)
>>
>> On Fri, Jul 29, 2022 at 10:07:36AM +0300, Sakari Ailus wrote:
>>> On Thu, Jul 28, 2022 at 03:13:11PM +0200, Krzysztof Kozlowski wrote:
>>>> On 28/07/2022 15:02, Alexander Stein wrote:
>>>>> According to product brief they are identical from software point of view.
>>>>> Differences are a different chief ray angle (CRA) and the package.
>>>>>
>>>>> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
>>>>> Acked-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com>
>>>>> ---
>>>>>  drivers/media/i2c/ov9282.c | 1 +
>>>>>  1 file changed, 1 insertion(+)
>>>>>
>>>>> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
>>>>> index 8a252bf3b59f..c8d83a29f9bb 100644
>>>>> --- a/drivers/media/i2c/ov9282.c
>>>>> +++ b/drivers/media/i2c/ov9282.c
>>>>> @@ -1113,6 +1113,7 @@ static const struct dev_pm_ops ov9282_pm_ops = {
>>>>>  };
>>>>>  
>>>>>  static const struct of_device_id ov9282_of_match[] = {
>>>>> +	{ .compatible = "ovti,ov9281" },
>>>>
>>>> The devices seem entirely compatible, so why you add a new compatible
>>>> and not re-use existing?
>>>>
>>>> The difference in lens does not explain this.
>>>
>>> It is typically necessary to know what kind of related hardware can be
>>> found in the system, beyond just the device's register interface. Apart
>>> from USB cameras, less integrated cameras require low-level software
>>> control in which specific device properties are important. In this case it
>>> could be the lens shading table, among other things.
>>>
>>> 	https://www.ovt.com/sensor/ov9282/
>>>
>>> Therefore I think adding a specific compatible string for this one is
>>> justified.
> 
> Specific compatible in binding is a requirement. No one discussed this.
> However not in the driver. None of the arguments above justify adding
> such binding, unless user-space depends on matching compatible, but not
> real compatible?

Eh, now I used vague words. This should be instead:

"However not in the driver. None of the arguments above justify adding
such compatible to driver, unless user-space depends on matching
compatible, but not real compatible?"

> 
>>>
>>> Also cc Laurent.
>>
>> Interesting coincidence, we've talked about this topic (as part of a
>> broader discussion) no later than yesterday.
>>
>> I agree with Sakari in that userspace needs to know the exact model of
>> the camera sensor. I don't see a good alternative to providing that
>> information through the platform firmware, so the device tree in this
>> case. The question is how it should be provided (the question of how it
>> should then be exposed to userspace is also important, but out of scope
>> in this discussion).
>>
>> The compatible string is meant to indicate a device's compatibility with
>> "something", and that something is often considered from the point of
>> view of software support, and in particular to pick an appropriate
>> kernel driver and tune its behaviour for the device. Here, one could
>> argue that the exact model is also needed to ensure proper software
>> support, but in userspace this time, not in the kernel. I think using a
>> dedicated compatible string would be reasonable. An alternative would be
>> to use another DT property, which should then be standardized. I'm not
>> sure it's worth it.
>>
>> Broadening the discussion, we also need to know detailed information
>> about the camera lens (I'm talking about the lens itself here, not the
>> lens controller IC that controls the motor that moves the focus lens).
>> The lens isn't described in the device tree with a dedicated device tree
>> node today, and I don't think it should (I'd have a hard time coming up
>> with a naming scheme for lenses that we could use in compatible strings,
>> and the lens-related data that a system requires can possibly vary based
>> not only on the lens itself but on the ISP that the camera sensor is
>> used with). Typical useful data are the lens movement range, the
>> hyperfocal distance, but also the lens shading tables. (Part of) that
>> information is sometimes stored in non-volatile memory in the camera
>> module (OTP in the camera sensor itself, or a separate EEPROM), but
>> that's not always the case. We have considered the possibility of
>> storing the information in the device tree, but I doubt that would be
>> accepted. We can store the information in userspace in configuration
>> files, but we will still need to device tree to provide lens
>> identification information to select the correct configuration file. I
>> don't know how that should be done.
> 
> It seems both you and Sakari suggested not to have specific compatible.
> Such idea (not to have specific compatible) was not proposed by me.
> Quite contrary - specific compatible is a requirement. However device
> driver does no need it. Just use fallback for the driver.


Best regards,
Krzysztof
  

On Mon, Aug 01, 2022 at 08:08:58PM +0200, Krzysztof Kozlowski wrote:
> On 01/08/2022 20:07, Krzysztof Kozlowski wrote:
> > On 29/07/2022 10:18, Laurent Pinchart wrote:
> >> Hi Sakari,
> >>
> >> (Adding Dave and Naush to the CC list)
> >>
> >> On Fri, Jul 29, 2022 at 10:07:36AM +0300, Sakari Ailus wrote:
> >>> On Thu, Jul 28, 2022 at 03:13:11PM +0200, Krzysztof Kozlowski wrote:
> >>>> On 28/07/2022 15:02, Alexander Stein wrote:
> >>>>> According to product brief they are identical from software point of view.
> >>>>> Differences are a different chief ray angle (CRA) and the package.
> >>>>>
> >>>>> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> >>>>> Acked-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com>
> >>>>> ---
> >>>>>  drivers/media/i2c/ov9282.c | 1 +
> >>>>>  1 file changed, 1 insertion(+)
> >>>>>
> >>>>> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> >>>>> index 8a252bf3b59f..c8d83a29f9bb 100644
> >>>>> --- a/drivers/media/i2c/ov9282.c
> >>>>> +++ b/drivers/media/i2c/ov9282.c
> >>>>> @@ -1113,6 +1113,7 @@ static const struct dev_pm_ops ov9282_pm_ops = {
> >>>>>  };
> >>>>>  
> >>>>>  static const struct of_device_id ov9282_of_match[] = {
> >>>>> +	{ .compatible = "ovti,ov9281" },
> >>>>
> >>>> The devices seem entirely compatible, so why you add a new compatible
> >>>> and not re-use existing?
> >>>>
> >>>> The difference in lens does not explain this.
> >>>
> >>> It is typically necessary to know what kind of related hardware can be
> >>> found in the system, beyond just the device's register interface. Apart
> >>> from USB cameras, less integrated cameras require low-level software
> >>> control in which specific device properties are important. In this case it
> >>> could be the lens shading table, among other things.
> >>>
> >>> 	https://www.ovt.com/sensor/ov9282/
> >>>
> >>> Therefore I think adding a specific compatible string for this one is
> >>> justified.
> > 
> > Specific compatible in binding is a requirement. No one discussed this.
> > However not in the driver. None of the arguments above justify adding
> > such binding, unless user-space depends on matching compatible, but not
> > real compatible?
> 
> Eh, now I used vague words. This should be instead:
> 
> "However not in the driver. None of the arguments above justify adding
> such compatible to driver, unless user-space depends on matching
> compatible, but not real compatible?"

If I understand you right, you'd put the more specific model name as well
as the more generic one to the compatible property and let the driver match
against the more generic one?

But in this case neither of these models is more generic than the other.
  

On 02/08/2022 10:23, Sakari Ailus wrote:
> On Mon, Aug 01, 2022 at 08:08:58PM +0200, Krzysztof Kozlowski wrote:
>> On 01/08/2022 20:07, Krzysztof Kozlowski wrote:
>>> On 29/07/2022 10:18, Laurent Pinchart wrote:
>>>> Hi Sakari,
>>>>
>>>> (Adding Dave and Naush to the CC list)
>>>>
>>>> On Fri, Jul 29, 2022 at 10:07:36AM +0300, Sakari Ailus wrote:
>>>>> On Thu, Jul 28, 2022 at 03:13:11PM +0200, Krzysztof Kozlowski wrote:
>>>>>> On 28/07/2022 15:02, Alexander Stein wrote:
>>>>>>> According to product brief they are identical from software point of view.
>>>>>>> Differences are a different chief ray angle (CRA) and the package.
>>>>>>>
>>>>>>> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
>>>>>>> Acked-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com>
>>>>>>> ---
>>>>>>>  drivers/media/i2c/ov9282.c | 1 +
>>>>>>>  1 file changed, 1 insertion(+)
>>>>>>>
>>>>>>> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
>>>>>>> index 8a252bf3b59f..c8d83a29f9bb 100644
>>>>>>> --- a/drivers/media/i2c/ov9282.c
>>>>>>> +++ b/drivers/media/i2c/ov9282.c
>>>>>>> @@ -1113,6 +1113,7 @@ static const struct dev_pm_ops ov9282_pm_ops = {
>>>>>>>  };
>>>>>>>  
>>>>>>>  static const struct of_device_id ov9282_of_match[] = {
>>>>>>> +	{ .compatible = "ovti,ov9281" },
>>>>>>
>>>>>> The devices seem entirely compatible, so why you add a new compatible
>>>>>> and not re-use existing?
>>>>>>
>>>>>> The difference in lens does not explain this.
>>>>>
>>>>> It is typically necessary to know what kind of related hardware can be
>>>>> found in the system, beyond just the device's register interface. Apart
>>>>> from USB cameras, less integrated cameras require low-level software
>>>>> control in which specific device properties are important. In this case it
>>>>> could be the lens shading table, among other things.
>>>>>
>>>>> 	https://www.ovt.com/sensor/ov9282/
>>>>>
>>>>> Therefore I think adding a specific compatible string for this one is
>>>>> justified.
>>>
>>> Specific compatible in binding is a requirement. No one discussed this.
>>> However not in the driver. None of the arguments above justify adding
>>> such binding, unless user-space depends on matching compatible, but not
>>> real compatible?
>>
>> Eh, now I used vague words. This should be instead:
>>
>> "However not in the driver. None of the arguments above justify adding
>> such compatible to driver, unless user-space depends on matching
>> compatible, but not real compatible?"
> 
> If I understand you right, you'd put the more specific model name as well
> as the more generic one to the compatible property and let the driver match
> against the more generic one?

Yes.

> 
> But in this case neither of these models is more generic than the other.

It's not a problem. Also the spec explains it similar way:
"They
 allow a device to express its compatibility with a family of similar
devices, potentially allowing a single
 device driver to match against several devices."

Of course the numbers would suggest that ov9281 should be the family (as
lower number usually means designed earlier), but it is a matter of
convention which here can be skipped. The point is that ov9281 and
ov9282 are compatible between each other, therefore they belong to
single family.

Best regards,
Krzysztof
  

Hello,

Am Dienstag, 2. August 2022, 10:30:40 CEST schrieb Krzysztof Kozlowski:
> On 02/08/2022 10:23, Sakari Ailus wrote:
> > On Mon, Aug 01, 2022 at 08:08:58PM +0200, Krzysztof Kozlowski wrote:
> >> On 01/08/2022 20:07, Krzysztof Kozlowski wrote:
> >>> On 29/07/2022 10:18, Laurent Pinchart wrote:
> >>>> Hi Sakari,
> >>>> 
> >>>> (Adding Dave and Naush to the CC list)
> >>>> 
> >>>> On Fri, Jul 29, 2022 at 10:07:36AM +0300, Sakari Ailus wrote:
> >>>>> On Thu, Jul 28, 2022 at 03:13:11PM +0200, Krzysztof Kozlowski wrote:
> >>>>>> On 28/07/2022 15:02, Alexander Stein wrote:
> >>>>>>> According to product brief they are identical from software point of
> >>>>>>> view.
> >>>>>>> Differences are a different chief ray angle (CRA) and the package.
> >>>>>>> 
> >>>>>>> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> >>>>>>> Acked-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com>
> >>>>>>> ---
> >>>>>>> 
> >>>>>>>  drivers/media/i2c/ov9282.c | 1 +
> >>>>>>>  1 file changed, 1 insertion(+)
> >>>>>>> 
> >>>>>>> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> >>>>>>> index 8a252bf3b59f..c8d83a29f9bb 100644
> >>>>>>> --- a/drivers/media/i2c/ov9282.c
> >>>>>>> +++ b/drivers/media/i2c/ov9282.c
> >>>>>>> @@ -1113,6 +1113,7 @@ static const struct dev_pm_ops ov9282_pm_ops =
> >>>>>>> {
> >>>>>>> 
> >>>>>>>  };
> >>>>>>>  
> >>>>>>>  static const struct of_device_id ov9282_of_match[] = {
> >>>>>>> 
> >>>>>>> +	{ .compatible = "ovti,ov9281" },
> >>>>>> 
> >>>>>> The devices seem entirely compatible, so why you add a new compatible
> >>>>>> and not re-use existing?
> >>>>>> 
> >>>>>> The difference in lens does not explain this.
> >>>>> 
> >>>>> It is typically necessary to know what kind of related hardware can be
> >>>>> found in the system, beyond just the device's register interface.
> >>>>> Apart
> >>>>> from USB cameras, less integrated cameras require low-level software
> >>>>> control in which specific device properties are important. In this
> >>>>> case it
> >>>>> could be the lens shading table, among other things.
> >>>>> 
> >>>>> 	https://www.ovt.com/sensor/ov9282/
> >>>>> 
> >>>>> Therefore I think adding a specific compatible string for this one is
> >>>>> justified.
> >>> 
> >>> Specific compatible in binding is a requirement. No one discussed this.
> >>> However not in the driver. None of the arguments above justify adding
> >>> such binding, unless user-space depends on matching compatible, but not
> >>> real compatible?
> >> 
> >> Eh, now I used vague words. This should be instead:
> >> 
> >> "However not in the driver. None of the arguments above justify adding
> >> such compatible to driver, unless user-space depends on matching
> >> compatible, but not real compatible?"
> > 
> > If I understand you right, you'd put the more specific model name as well
> > as the more generic one to the compatible property and let the driver
> > match
> > against the more generic one?
> 
> Yes.
> 
> > But in this case neither of these models is more generic than the other.
> 
> It's not a problem. Also the spec explains it similar way:
> "They
>  allow a device to express its compatibility with a family of similar
> devices, potentially allowing a single
>  device driver to match against several devices."
> 
> Of course the numbers would suggest that ov9281 should be the family (as
> lower number usually means designed earlier), but it is a matter of
> convention which here can be skipped. The point is that ov9281 and
> ov9282 are compatible between each other, therefore they belong to
> single family.
> 
> Best regards,
> Krzysztof

So what is the conclusion of this?
If using the "family" name there is no way for userspace to see the actual 
device name rather than the driver name. This might be confusing, especially 
of both ov9281 and ov9282 are attached to the same platform. The only 
difference would be the i2c-bus-address.
You can also go for ov928x but this is not a real improvement.

Best regards,
Alexander
  

On 15/08/2022 14:19, Alexander Stein wrote:
> Hello,
> 
> Am Dienstag, 2. August 2022, 10:30:40 CEST schrieb Krzysztof Kozlowski:
>> On 02/08/2022 10:23, Sakari Ailus wrote:
>>> On Mon, Aug 01, 2022 at 08:08:58PM +0200, Krzysztof Kozlowski wrote:
>>>> On 01/08/2022 20:07, Krzysztof Kozlowski wrote:
>>>>> On 29/07/2022 10:18, Laurent Pinchart wrote:
>>>>>> Hi Sakari,
>>>>>>
>>>>>> (Adding Dave and Naush to the CC list)
>>>>>>
>>>>>> On Fri, Jul 29, 2022 at 10:07:36AM +0300, Sakari Ailus wrote:
>>>>>>> On Thu, Jul 28, 2022 at 03:13:11PM +0200, Krzysztof Kozlowski wrote:
>>>>>>>> On 28/07/2022 15:02, Alexander Stein wrote:
>>>>>>>>> According to product brief they are identical from software point of
>>>>>>>>> view.
>>>>>>>>> Differences are a different chief ray angle (CRA) and the package.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
>>>>>>>>> Acked-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com>
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>>  drivers/media/i2c/ov9282.c | 1 +
>>>>>>>>>  1 file changed, 1 insertion(+)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
>>>>>>>>> index 8a252bf3b59f..c8d83a29f9bb 100644
>>>>>>>>> --- a/drivers/media/i2c/ov9282.c
>>>>>>>>> +++ b/drivers/media/i2c/ov9282.c
>>>>>>>>> @@ -1113,6 +1113,7 @@ static const struct dev_pm_ops ov9282_pm_ops =
>>>>>>>>> {
>>>>>>>>>
>>>>>>>>>  };
>>>>>>>>>  
>>>>>>>>>  static const struct of_device_id ov9282_of_match[] = {
>>>>>>>>>
>>>>>>>>> +	{ .compatible = "ovti,ov9281" },
>>>>>>>>
>>>>>>>> The devices seem entirely compatible, so why you add a new compatible
>>>>>>>> and not re-use existing?
>>>>>>>>
>>>>>>>> The difference in lens does not explain this.
>>>>>>>
>>>>>>> It is typically necessary to know what kind of related hardware can be
>>>>>>> found in the system, beyond just the device's register interface.
>>>>>>> Apart
>>>>>>> from USB cameras, less integrated cameras require low-level software
>>>>>>> control in which specific device properties are important. In this
>>>>>>> case it
>>>>>>> could be the lens shading table, among other things.
>>>>>>>
>>>>>>> 	https://www.ovt.com/sensor/ov9282/
>>>>>>>
>>>>>>> Therefore I think adding a specific compatible string for this one is
>>>>>>> justified.
>>>>>
>>>>> Specific compatible in binding is a requirement. No one discussed this.
>>>>> However not in the driver. None of the arguments above justify adding
>>>>> such binding, unless user-space depends on matching compatible, but not
>>>>> real compatible?
>>>>
>>>> Eh, now I used vague words. This should be instead:
>>>>
>>>> "However not in the driver. None of the arguments above justify adding
>>>> such compatible to driver, unless user-space depends on matching
>>>> compatible, but not real compatible?"
>>>
>>> If I understand you right, you'd put the more specific model name as well
>>> as the more generic one to the compatible property and let the driver
>>> match
>>> against the more generic one?
>>
>> Yes.
>>
>>> But in this case neither of these models is more generic than the other.
>>
>> It's not a problem. Also the spec explains it similar way:
>> "They
>>  allow a device to express its compatibility with a family of similar
>> devices, potentially allowing a single
>>  device driver to match against several devices."
>>
>> Of course the numbers would suggest that ov9281 should be the family (as
>> lower number usually means designed earlier), but it is a matter of
>> convention which here can be skipped. The point is that ov9281 and
>> ov9282 are compatible between each other, therefore they belong to
>> single family.
>>
>> Best regards,
>> Krzysztof
> 
> So what is the conclusion of this?
> If using the "family" name there is no way for userspace to see the actual 
> device name rather than the driver name. This might be confusing, especially 
> of both ov9281 and ov9282 are attached to the same platform. The only 
> difference would be the i2c-bus-address.
> You can also go for ov928x but this is not a real improvement.

I still don't understand. Why user-space cannot see this? I really
cannot find any trouble... Your 3/7 patch does nothing special here for
user-space...

Best regards,
Krzysztof
  

Am Dienstag, 16. August 2022, 09:16:44 CEST schrieb Krzysztof Kozlowski:
> On 15/08/2022 14:19, Alexander Stein wrote:
> > Hello,
> > 
> > Am Dienstag, 2. August 2022, 10:30:40 CEST schrieb Krzysztof Kozlowski:
> >> On 02/08/2022 10:23, Sakari Ailus wrote:
> >>> On Mon, Aug 01, 2022 at 08:08:58PM +0200, Krzysztof Kozlowski wrote:
> >>>> On 01/08/2022 20:07, Krzysztof Kozlowski wrote:
> >>>>> On 29/07/2022 10:18, Laurent Pinchart wrote:
> >>>>>> Hi Sakari,
> >>>>>> 
> >>>>>> (Adding Dave and Naush to the CC list)
> >>>>>> 
> >>>>>> On Fri, Jul 29, 2022 at 10:07:36AM +0300, Sakari Ailus wrote:
> >>>>>>> On Thu, Jul 28, 2022 at 03:13:11PM +0200, Krzysztof Kozlowski wrote:
> >>>>>>>> On 28/07/2022 15:02, Alexander Stein wrote:
> >>>>>>>>> According to product brief they are identical from software point
> >>>>>>>>> of
> >>>>>>>>> view.
> >>>>>>>>> Differences are a different chief ray angle (CRA) and the package.
> >>>>>>>>> 
> >>>>>>>>> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> >>>>>>>>> Acked-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com>
> >>>>>>>>> ---
> >>>>>>>>> 
> >>>>>>>>>  drivers/media/i2c/ov9282.c | 1 +
> >>>>>>>>>  1 file changed, 1 insertion(+)
> >>>>>>>>> 
> >>>>>>>>> diff --git a/drivers/media/i2c/ov9282.c
> >>>>>>>>> b/drivers/media/i2c/ov9282.c
> >>>>>>>>> index 8a252bf3b59f..c8d83a29f9bb 100644
> >>>>>>>>> --- a/drivers/media/i2c/ov9282.c
> >>>>>>>>> +++ b/drivers/media/i2c/ov9282.c
> >>>>>>>>> @@ -1113,6 +1113,7 @@ static const struct dev_pm_ops ov9282_pm_ops
> >>>>>>>>> =
> >>>>>>>>> {
> >>>>>>>>> 
> >>>>>>>>>  };
> >>>>>>>>>  
> >>>>>>>>>  static const struct of_device_id ov9282_of_match[] = {
> >>>>>>>>> 
> >>>>>>>>> +	{ .compatible = "ovti,ov9281" },
> >>>>>>>> 
> >>>>>>>> The devices seem entirely compatible, so why you add a new
> >>>>>>>> compatible
> >>>>>>>> and not re-use existing?
> >>>>>>>> 
> >>>>>>>> The difference in lens does not explain this.
> >>>>>>> 
> >>>>>>> It is typically necessary to know what kind of related hardware can
> >>>>>>> be
> >>>>>>> found in the system, beyond just the device's register interface.
> >>>>>>> Apart
> >>>>>>> from USB cameras, less integrated cameras require low-level software
> >>>>>>> control in which specific device properties are important. In this
> >>>>>>> case it
> >>>>>>> could be the lens shading table, among other things.
> >>>>>>> 
> >>>>>>> 	https://www.ovt.com/sensor/ov9282/
> >>>>>>> 
> >>>>>>> Therefore I think adding a specific compatible string for this one
> >>>>>>> is
> >>>>>>> justified.
> >>>>> 
> >>>>> Specific compatible in binding is a requirement. No one discussed
> >>>>> this.
> >>>>> However not in the driver. None of the arguments above justify adding
> >>>>> such binding, unless user-space depends on matching compatible, but
> >>>>> not
> >>>>> real compatible?
> >>>> 
> >>>> Eh, now I used vague words. This should be instead:
> >>>> 
> >>>> "However not in the driver. None of the arguments above justify adding
> >>>> such compatible to driver, unless user-space depends on matching
> >>>> compatible, but not real compatible?"
> >>> 
> >>> If I understand you right, you'd put the more specific model name as
> >>> well
> >>> as the more generic one to the compatible property and let the driver
> >>> match
> >>> against the more generic one?
> >> 
> >> Yes.
> >> 
> >>> But in this case neither of these models is more generic than the other.
> >> 
> >> It's not a problem. Also the spec explains it similar way:
> >> "They
> >> 
> >>  allow a device to express its compatibility with a family of similar
> >> 
> >> devices, potentially allowing a single
> >> 
> >>  device driver to match against several devices."
> >> 
> >> Of course the numbers would suggest that ov9281 should be the family (as
> >> lower number usually means designed earlier), but it is a matter of
> >> convention which here can be skipped. The point is that ov9281 and
> >> ov9282 are compatible between each other, therefore they belong to
> >> single family.
> >> 
> >> Best regards,
> >> Krzysztof
> > 
> > So what is the conclusion of this?
> > If using the "family" name there is no way for userspace to see the actual
> > device name rather than the driver name. This might be confusing,
> > especially of both ov9281 and ov9282 are attached to the same platform.
> > The only difference would be the i2c-bus-address.
> > You can also go for ov928x but this is not a real improvement.
> 
> I still don't understand. Why user-space cannot see this? I really
> cannot find any trouble... Your 3/7 patch does nothing special here for
> user-space...

3/7 itself does nothing for userspace, but 6/7 does, which relies on separate 
compatibles in the driver.

Best regards,
Alexander
  

On 16/08/2022 10:21, Alexander Stein wrote:
>>>
>>> So what is the conclusion of this?
>>> If using the "family" name there is no way for userspace to see the actual
>>> device name rather than the driver name. This might be confusing,
>>> especially of both ov9281 and ov9282 are attached to the same platform.
>>> The only difference would be the i2c-bus-address.
>>> You can also go for ov928x but this is not a real improvement.
>>
>> I still don't understand. Why user-space cannot see this? I really
>> cannot find any trouble... Your 3/7 patch does nothing special here for
>> user-space...
> 
> 3/7 itself does nothing for userspace, but 6/7 does, which relies on separate 
> compatibles in the driver.

Ah, that's so confusing... First adding new incomplete device support in
patch 3 and then in patch 6 fixing it. 6 and 3 should be squashed. They
really do not make any sense being separate and this just brings this
unnecessary confusion.

I would argue that the binding still should have devices compatible (in
one family), but now it is a bit less important.

Best regards,
Krzysztof
  

Hello Kieran,

Am Samstag, 12. November 2022, 00:48:24 CET schrieb Kieran Bingham:
> Hi All,
> 
> Quoting Alexander Stein (2022-07-28 14:02:33)
> 
> > According to product brief they are identical from software point of view.
> > Differences are a different chief ray angle (CRA) and the package.
> > 
> > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > Acked-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com>
> 
> Throwing my hat in the ring on this thread as I see it has been hanging
> around for a while and my attention was sent here from [0]
> 
> [0]
> https://lists.libcamera.org/pipermail/libcamera-devel/2022-November/035495.
> html

I postponed working on this change for a while, because there are (at least) 
two series from Dave pending which conflict a bit with this series.

> > ---
> > 
> >  drivers/media/i2c/ov9282.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > index 8a252bf3b59f..c8d83a29f9bb 100644
> > --- a/drivers/media/i2c/ov9282.c
> > +++ b/drivers/media/i2c/ov9282.c
> > @@ -1113,6 +1113,7 @@ static const struct dev_pm_ops ov9282_pm_ops = {
> > 
> >  };
> >  
> >  static const struct of_device_id ov9282_of_match[] = {
> > 
> > +       { .compatible = "ovti,ov9281" },
> 
> I believe from my existing understanding of how we would support
> existing sensors even with very similar parts is that a direct
> compatible lets the DT express this.
> 
> If there were a common name that we could apply, we could have a generic
> name here too, but I don't see anything specifically generic, and I
> haven't yet seen a clear pattern in the namings schemes from omnivision
> so ov928x wouldn't be appropriate as I couldn't be sure that an
> unrelated ov9289 wouldn't exist with very different properties ... so ..
> 
> >         { .compatible = "ovti,ov9282" },
> 
> Either squashed with the later 6/7 that adds the name or not: (I think
> it's fine either separated or squashed)
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> It does in turn bring questions into how we handle both the ov9281 and
> ov9282 together in libcamera, but I don't think that's an issue to solve
> here. Expressing the two separately to userspace also allows libcamera
> to make a distiction between the CRA should it need to.

Krzysztof is in favor of squashing so I'll respin this accordingly, removing 
the other changes from the ov9281 support series.

Best regards,
Alexander