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

[Alexander Stein <alexander.stein@ew.tq-group.com>]

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