Re: [PATCH v4 1/2] dt-bindings: media: i2c: Add ov2735 sensor

[Krzysztof Kozlowski <krzk@kernel.org>]

On 17/07/2025 13:55, Hardevsinh Palaniya wrote:
>> On 17/07/2025 09:28, Hardevsinh Palaniya wrote:
>>>  Hi Krzysztof,
>>>
>>>> On Wed, Jul 16, 2025 at 07:14:16PM +0530, Hardevsinh Palaniya wrote:
>>>>> +        properties:
>>>>> +          data-lanes:
>>>>> +            items:
>>>>> +              - const: 1
>>>>> +              - const: 2
>>>>> +          link-frequencies: true
>>>>
>>>> Drop
>>>>
>>>> I don't understand why this appeared. I don't think anyone asked for it?
>>>
>>> Laurent suggested validating the link frequency in the Device Tree.
>>>
>>> Link[1]: https://lore.kernel.org/linux-media/20250710212131.GG22436@pendragon.ideasonboard.com/
>>
>> ... and I do not see here validation "that the link frequencies
>> specified in DT match".
>>
>> How do you validate that 1111 Hz is not / is a valid link frequency? How
>> did you exactly resolve the comment about validating?
> 
> In the ov2735_parse_endpoint() function, the driver validates the link frequency
> from DT using `v4l2_link_freq_to_bitmap()`. If an unsupported value like 1111 Hz is
> provided, it returns an error: 


But that's driver. You said that you added this property based on
feedback on validation. There is no validation here, thus the reason of
this property is not fulfilled and that's why I was asking. I know now
the answer...



> 
> ret = v4l2_link_freq_to_bitmap(ov2735->dev, bus_cfg.link_frequencies,
>                                bus_cfg.nr_of_link_frequencies,
>                                link_freq_menu_items,
>                                ARRAY_SIZE(link_freq_menu_items),
>                                &link_freq_bitmap);
> if (ret) {
>         ret = dev_err_probe(ov2735->dev,
>                             -EINVAL, "only 420MHz frequency is available\n");
>         goto error_out;
> }
> 
> So, validation is already done in the driver.
> 
> I understand that `link-frequencies: true` in the schema only declares the property
> without restricting values. Would you prefer the schema to explicitly allow only
> 420MHz like this?


You missed the last paragraph of my last email. I don't want to have
here what the driver is doing, but what the hardware is/needs/requires.
It can also be missing completely, because we don't enforce such rules
on other bindings.


Best regards,
Krzysztof