Re: [PATCH 2/2] media: i2c: Add Himax HM1092 image sensor driver

From: Jake Steinman <[email protected]>

Hi Ramshouriesh,

Relevant to your series: the ipu-bridge entry for this sensor landed in
media/next on 2026-07-30 as commit 4fdb0342f05e ("media: ipu-bridge: Add
Himax HM1092 IR sensor"):

  IPU_SENSOR_CONFIG("HIMX1092", 2, 180000000, 180480000)

So on Intel platforms the sensor endpoint is built with both link
frequencies as soon as your driver lands. That is where comment 3 below
comes from.

I have not tested v7 on hardware -- my machine is Intel/x86 and, as below,
the driver cannot bind there in its current form. Everything I say about
ASUS behaviour I take on trust from your testing.


THREE THINGS THAT BLOCK x86

The driver is Qualcomm/DT-oriented, which is reasonable given what you can
test. It does mean it cannot probe on the Intel IPU6/IPU7 laptops this
sensor also ships in, and I would rather raise that now than after it lands.

1. The external clock rate is hard-rejected.

     #define HM1092_MCLK 24000000
     ...
     if (freq != HM1092_MCLK)
             return dev_err_probe(dev, -EINVAL, ...);

   Dell supplies 19.2 MHz, not 24. That part is measured rather than
   inferred:

     $ grep -A1 HIMX /sys/kernel/debug/clk/clk_summary
      INT3472:00-clk  1 1 0  19200000 ...

   The PLL is programmed to suit -- the Dell register table writes
   0x030d = 0x0a and 0x030f = 0x5e, which read as the low bytes of the CCS
   OP_PRE_PLL_CLK_DIV / OP_PLL_MULTIPLIER pair and give
   19200000 / 10 * 94 = 180480000. I have no datasheet, so please treat
   that decode as inference; note also that Dell writes 0x0310 = 0x00,
   which under CCS would be PLL_MODE_SINGLE, so this part is evidently not
   strictly CCS-compliant and I may be reading it wrong. The 19.2 MHz
   external clock itself is not in doubt.

   You already use devm_v4l2_sensor_clk_get(), which is the right thing --
   it is only the fixed-rate comparison that fails on Dell. Dropping the
   check, or deriving the expected link frequency from the rate, would make
   the driver portable without any other change.

2. There is no ACPI match.

   Only .of_match_table is set. On x86 the i2c client is enumerated from
   ACPI by i2c-core-acpi.c and client->name ends up as "HIMX1092:00". None
   of the three arms of i2c_device_match() can match that: the
   i2c_of_match_device_sysfs() fallback compares against "himax,hm1092" and
   "hm1092"; acpi_driver_match_device() without an acpi_match_table falls
   back to the DSD "compatible" property, which Dell does not supply
   (modalias is acpi:HIMX1092:HIMX1092:); and there is no id_table. So the
   driver needs an .acpi_match_table entry for "HIMX1092" or it will not
   bind at all.

3. The mode and link-frequency menu are ASUS-specific.

   hm1092_mode_560x360 with HTS 0x0650 / VTS 0x02ee, and a single-entry
   hm1092_link_freq_menu of 180000000. Dell runs 648x368 at nominally
   30 fps with HTS 1620 (0x0654) and VTS 741 (0x02e5), at 180480000.

   With the bridge entry above advertising both frequencies,
   v4l2_link_freq_to_bitmap() intersects the two lists, finds 180000000 in
   both and returns success -- no error, no warning. So on Dell the driver
   would simply advertise a link frequency 0.27% away from what the PLL is
   set to. That is unlikely to break the receiver; it is just wrong, and a
   second menu entry is cheap.

   One caveat on my numbers: my Dell table is a capture of a whole Windows
   Hello session rather than a static init sequence, so several registers
   are written more than once. 0x0340/0x0341 takes three distinct values in
   order -- 0x031e before the mode is selected, 0x02e4 with the mode, then
   0x02e5, which the AE loop then rewrites unchanged five more times.
   0x0342/0x0343 likewise goes 0x05e4 then 0x0654. So 741/1620 are the
   steady-state streaming values; reading only the first write gives
   798/1508, which is what I got wrong myself first time round.

None of this needs to hold up v7. I am happy to send a follow-up adding the
ACPI match, the second link frequency and the Dell mode, and to test it on
the hardware I have. Say whether you would rather have that on top of v7 or
rebased onto a v8, and I will work to whichever suits you.

One smaller note: the comment on the init table says the exposure default is
programmed at 0x0202/0x0203 so the hardware and control states agree. Worth
saying explicitly that this is why HM1092_EXPOSURE_DEFAULT is 500 rather
than a derived value -- it is the kind of thing a later cleanup would
"simplify" and quietly break.


ATTRIBUTION

You have Reported-by plus links and you credit the register work in the
commit body, which I appreciate. When you asked before v6 I said
Co-developed-by would fit better, and I still think so, though I want to be
precise about what I actually contributed rather than overstate it.

The exposure, analogue gain, digital gain and group-hold register
assignments came from my analysis of an HM1092 I2C bus capture taken during
a Windows Hello session, and I published a working driver using them. v7
implements those controls as I described them. Whether that is
Co-developed-by or Suggested-by is genuinely your call -- the difference is
whether you consider the control implementation co-authored or specified,
and you are better placed to judge that than I am.

If Co-developed-by:

  Co-developed-by: Jake Steinman <[email protected]>
  Signed-off-by: Jake Steinman <[email protected]>

One thing to flag either way: I use an AI coding assistant, and
Documentation/process/coding-assistants.rst asks that it be disclosed. My
ipu-bridge commit above carries "Assisted-by: Claude:claude-opus-5". If you
take any tag of mine, the same line should go on this patch for consistency.
The analysis and hardware testing are mine and the Signed-off-by would be
mine alone, but I would rather raise that than have it noticed later.

Regards,
Jake