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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <85df7f30-e9ac-422e-8ab5-7c6b82774aaf@emfend.at>
Date: Mon, 3 Nov 2025 07:54:52 +0100
From: Matthias Fend <matthias.fend@...end.at>
To: Sakari Ailus <sakari.ailus@...ux.intel.com>
Cc: Mauro Carvalho Chehab <mchehab@...nel.org>, Rob Herring
 <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Hans Verkuil <hverkuil@...nel.org>,
 Hans de Goede <hansg@...nel.org>, Ricardo Ribalda <ribalda@...omium.org>,
 André Apitzsch <git@...tzsch.eu>,
 Tarang Raval <tarang.raval@...iconsignals.io>,
 Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
 Benjamin Mugnier <benjamin.mugnier@...s.st.com>,
 Sylvain Petinot <sylvain.petinot@...s.st.com>,
 Dongcheng Yan <dongcheng.yan@...el.com>,
 Bryan O'Donoghue <bryan.odonoghue@...aro.org>,
 Alan Stern <stern@...land.harvard.edu>,
 Jingjing Xiong <jingjing.xiong@...el.com>,
 Heimir Thor Sverrisson <heimir.sverrisson@...il.com>,
 Mehdi Djait <mehdi.djait@...ux.intel.com>,
 Vladimir Zapolskiy <vladimir.zapolskiy@...aro.org>,
 Laurent Pinchart <laurent.pinchart@...asonboard.com>,
 Hardevsinh Palaniya <hardevsinh.palaniya@...iconsignals.io>,
 linux-media@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, Hao Yao <hao.yao@...el.com>,
 bsp-development.geo@...ca-geosystems.com
Subject: Re: [PATCH v4 2/2] media: i2c: add Himax HM1246 image sensor driver

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?
> 
...
>>> +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?
> 
Thanks
  ~Matthias