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

From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
To: Alexander Stein <alexander.stein@ew.tq-group.com>,
	Sakari Ailus <sakari.ailus@iki.fi>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	"Paul J . Murphy" <paul.j.murphy@intel.com>,
	Daniele Alessandrelli <daniele.alessandrelli@intel.com>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	linux-media@vger.kernel.org, devicetree@vger.kernel.org,
	Dave Stevenson <dave.stevenson@raspberrypi.com>,
	Naushir Patuck <naush@raspberrypi.com>
Subject: Re: [PATCH v4 3/7] media: i2c: ov9282: Add ov9281 compatible
Date: Tue, 16 Aug 2022 10:16:44 +0300	[thread overview]
Message-ID: <ceb2a42d-0650-6e6f-3408-6347bcd8c5e2@linaro.org> (raw)
In-Reply-To: <2403639.ElGaqSPkdT@steina-w>

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