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@xxxxxxxxxxxxxxx>
>>>>> Acked-by: Daniele Alessandrelli <daniele.alessandrelli@xxxxxxxxx>
>>>>> ---
>>>>> 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
