Re: [PATCH v2 1/4] dt-bindings: media: Add Sony IMX585 CMOS image sensor

[Krzysztof Kozlowski <krzk@kernel.org>]

On 13/08/2025 06:30, Will Whang wrote:
> On Tue, Aug 12, 2025 at 2:56 AM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
>>
>> On Tue, Aug 12, 2025 at 08:47:12AM +0200, Krzysztof Kozlowski wrote:
>>> On 12/08/2025 08:31, Will Whang wrote:
>>>> On Mon, Aug 11, 2025 at 11:23 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>>>> On 12/08/2025 04:47, Will Whang wrote:
>>>>>> On Mon, Aug 11, 2025 at 1:01 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>>>>>> On Sun, Aug 10, 2025 at 11:09:18PM +0100, Will Whang wrote:
>>>>>>>> +description:
>>>>>>>> +  IMX585 sensor is a Sony CMOS sensor with 4K and FHD outputs.
>>>>>>>> +
>>>>>>>> +properties:
>>>>>>>> +  compatible:
>>>>>>>> +    enum:
>>>>>>>> +      - sony,imx585
>>>>>>>> +      - sony,imx585-mono
>>>>>>>
>>>>>>> I don't understand this second compatible. Is this different hardware?
>>>>>>> Can you point me to "mono" datasheet?
>>>>>>>
>>>>>>> Your description should explain this. Commit msg as well, instead of
>>>>>>> speaking about driver (in fact drop all driver related comments).
>>>>>>>
>>>>>> Mono version of this sensor is basically just removing the bayer
>>>>>> filter, so the sensor itself actually doesn't know if it is color or
>>>>>> mono and from my knowledge there are no registers programmed in the
>>>>>> factory that will show the variant and model number. (That is why when
>>>>>> the driver probing it only test blacklevel register because there are
>>>>>> no ID registers)
>>>>>> Originally in V1 patch I've made the switch between color and mono in
>>>>>> dtoverlay config but reviewer comments is to move it to compatible
>>>>>> string and not property.(https://lore.kernel.org/linux-media/20250703175121.GA17709@pendragon.ideasonboard.com/)
>>>>>
>>>>> You only partially answer and judging by mentioning driver below:
>>>>>
>>>>>> In this case, what would you recommend?
>>>>>>
>>>>>> compatible:
>>>>>>   enum:
>>>>>>     - sony,imx585
>>>>>>     - sony,imx585-mono
>>>>>>   description: IMX585 has two variants, color and mono which the
>>>>>> driver supports both.
>>>>>
>>>>> ... I still have doubts that you really understand what I am asking. Is
>>>>> this one device or two different devices?
>>>>
>>>> One device that has two variants: IMX585-AAMJ1 (Mono) and IMX585-AAQJ1
>>>> (Color). Silicon-wise the difference is just with or without bayer
>>>> filter.
>>>
>>> Then I would propose to use sony,imx585-aamj1 and -aaqj1 with short
>>> explanation either in comment or description about difference in RGB
>>> mosaic filter.
>>
>> Works for me. We could possibly omit the "j1" suffix too.
>>
> My thinking is that imx585 and imx585-mono are easier to comprehend
> than IMX585-AAM and IMX585-AAQ.
> Because in dtoverlay for the users/me they will have to know what is
> the exact name instead of easy to remember name.
> 
> dtoverlay=imx585-aam
> is not as nice as
> dtoverlay=imx585-mono

I have datasheet for AAQ, so how above is easier for me to figure out
which compatible I am using?

> 
> which is what it does, a mono variant of the sensor.
> 
> I really don't understand the standard for compatible string naming
> here, is there something I missed? Is it required to use the full name
> of the sensor parts number as a compatible string?

It's not part number. You have there different models. We don't add
prose to compatibles, but use device or model names.

Best regards,
Krzysztof