Re: [PATCH v4 2/2] media: i2c: add Himax HM1246 image sensor driver

[Matthias Fend <matthias.fend@emfend.at>]

Hi Sakari,

Am 03.11.2025 um 12:02 schrieb Sakari Ailus:
> Hi Matthias,
> 
> Thanks for the ping.
> 
> On Mon, Nov 03, 2025 at 07:54:52AM +0100, Matthias Fend wrote:
>> Hi Sakari,
>>
>> Am 23.10.2025 um 11:00 schrieb Matthias Fend:
>>> Hi Sakari,
>>>
>>> thanks a lot for your feedback.
>>
>> I had two follow-up questions regarding your feedback, but I suspect they
>> got lost in all the code. I've cleaned up this mail a bit to make the
>> questions more visible.
>>
>>>>> +
>>>>> +static int hm1246_update_controls(struct hm1246 *hm1246,
>>>>> +                  const struct hm1246_mode *mode)
>>>>> +{
>>>>> +    s64 pixel_rate, exposure_max, vblank, hblank;
>>>>> +    int ret;
>>>>> +
>>>>> +    ret = __v4l2_ctrl_s_ctrl(hm1246->link_freq_ctrl, mode-
>>>>>> link_freq_index);
>>>>
>>>> Does this do something? There's only a single link frequency value (and
>>>> index) supported.
>>>
>>> You're right. Even though hm1246_update_controls() isn't exactly wrong,
>>> I could currently remove this function completely. The sensor supports
>>> various modes (which result in different clock rates), and I've already
>>> started implementing more of them. With multiple modes the controls need
>>> to be updated. However, since there were still some internal sensor
>>> issues to be addressed and I haven't been able to fully test them, I've
>>> decided to use only the presumably most common RAW mode for now.
>>>
>>> Should I remove the function now and add it back once more modes are
>>> implemented?
> 
> I think it'd be better to postpone adding this. I think you'll need further
> logic to support this and it'd be better to review this in conjunction with
> the additional features.

Okay, then I will remove hm1246_update_controls() for now.

> 
>>>
>> ...
>>>>> +static int hm1246_parse_fwnode(struct hm1246 *hm1246)
>>>>> +{
>>>>> +    struct fwnode_handle *endpoint;
>>>>> +    struct v4l2_fwnode_endpoint bus_cfg = {
>>>>> +        .bus_type = V4L2_MBUS_PARALLEL,
>>>>> +    };
>>>>> +    int ret;
>>>>> +
>>>>> +    endpoint =
>>>>> fwnode_graph_get_endpoint_by_id(dev_fwnode(hm1246- >dev), 0,
>>>>> +                           0,
>>>>> +                           FWNODE_GRAPH_ENDPOINT_NEXT);
>>>>> +    if (!endpoint)
>>>>> +        return dev_err_probe(hm1246->dev, -EINVAL,
>>>>> +                     "missing endpoint node\n");
>>>>> +
>>>>> +    ret = v4l2_fwnode_endpoint_parse(endpoint, &bus_cfg);
>>>>
>>>> What about validating the link frequencies? You can use
>>>> v4l2_link_freq_to_bitmap(), too.
>>>
>>> I was under the impression that for sensors with a parallel interface,
>>> no frequency information is provided in the device tree (because there's
>>> no need for it). Since there are no frequency entries, they can't be
>>> verified.
>>>
>>> Am I wrong, or did you perhaps mean something else?
> 
> The current documentation
> <URL:https://hverkuil.home.xs4all.nl/spec/driver-api/camera-sensor.html>
> doesn't distinguish CSI-2 and parallel interfaces in this respect. It's a
> good idea to ensure a safe frequency is used as the driver works the same
> way in all cases, whether or not using one is mandatory.

If I understand correctly, this means that in the bindings, the port 
property 'link-frequencies' should be marked as 'required', and the port 
in the example node should be extended with the line 'link-frequencies = 
/bits/ 64 <42174000>;'.
Then, during probe, it can be checked with v4l2_link_freq_to_bitmap() 
whether the link frequency entered in the device tree is supported (this 
also requires switching to v4l2_fwnode_endpoint_alloc_parse).

Does this describe the desired change?

Thanks for your help!
  ~Matthias

>