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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

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