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

[Sakari Ailus <sakari.ailus@linux.intel.com>]

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.

> > 
> ...
> > > > +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.

-- 
Kind regards,

Sakari Ailus