Re: [PATCH v5 0/2] media: Add Himax HM1092 mono NIR sensor driver

[Jake Steinman]

Ramshouriesh,

Good — and measuring your own link rate rather than pasting mine is the right
call. 24 MHz EXTCLK is exactly the sort of thing that would have made a
copied constant wrong in a new way, and my derivation only holds for a 19.2 MHz
part. I would rather your v6 carry a number you can defend than one of mine.

Your provenance is genuinely useful, by the way. Mine came out of Dell's
hm1092.sys, yours out of Qualcomm's com.qti.sensormodule.hm1092.bin in the ASUS
stack — two different vendors' Windows artifacts, extracted independently, and
the register sequences agree. That is much better evidence for whoever reviews
this than either of us had separately, and I think it belongs in the cover
letter.

If it would help, I can make that concrete rather than assertive: send me your
table as (addr, value) pairs in order and I will diff it against my 238 entries
and post the result — how many entries match exactly, which differ, and whether
the differences are all board-level defaults like the 0xb4/0xbe exposure you
already spotted. "Two independent extractions from two vendors' Windows stacks
agree on N of 238 entries, differing only in <list>" is a much stronger sentence
for a cover letter than either of us saying we trust our own dump. It also has a
real chance of catching a transcription error in one of them, which is worth
knowing before this lands rather than after.

## Exposure and gain

Here is what I have. Rather than send a patch against v5 that your respin will
immediately invalidate, take this as the implementation plus the notes that
matter, and I will send a proper patch against v6 once your geometry and
link-frequency changes have landed. Say the word if you would rather have it the
other way round.

Registers:

  0x0202 / 0x0203   exposure, 16-bit, high byte first
  0x0205            analogue gain, 8-bit
  0x020e / 0x020f   digital gain, 16-bit, high byte first
  0x0104            group hold: write 1, program the group, write 0

Ranges I use, on a 648x368 mode with VTS 741:

  exposure          min 2, max 720, step 1
  analogue gain     min 0, max 0xff, step 1
  digital gain      min 0x0100, max 0x0fff, step 1   (0x0100 = 1.0x)

The writes themselves, in CCI terms since you are already there:

  case V4L2_CID_EXPOSURE:
          cci_write(regmap, CCI_REG8(0x0202), ctrl->val >> 8, &ret);
          cci_write(regmap, CCI_REG8(0x0203), ctrl->val & 0xff, &ret);
          break;
  case V4L2_CID_ANALOGUE_GAIN:
          cci_write(regmap, CCI_REG8(0x0205), ctrl->val, &ret);
          break;
  case V4L2_CID_DIGITAL_GAIN:
          cci_write(regmap, CCI_REG8(0x020e), ctrl->val >> 8, &ret);
          cci_write(regmap, CCI_REG8(0x020f), ctrl->val & 0xff, &ret);
          break;

Four things worth knowing before you wire it up, because three of them cost me
time:

**1. Your control defaults must match what your init table leaves behind, not
mine.** This is the one I would most like you to check. The control handler
setup that runs at stream-on writes the defaults to the sensor, so if a default
differs from the value the init sequence already programmed, you silently stream
a configuration the part has never been shown to work in. My table ends with
0x0203 = 0xb4, so my exposure default is 180. **Yours ends with 0xbe, so your
default should be 190, not 180.** Same for gain: my table leaves 0x0205 = 0x00
so my default is 0. Check what yours leaves.

I hit this exactly: a refactor made the stream-on control setup a real hardware
write for the first time, and it started programming exposure 256 and gain 0x10
over a sequence that had only ever been validated at 180 and 0.

**2. Do not fold the 8-bit halves into CCI_REG16.** It is tempting — 0x0202/0x0203
and 0x020e/0x020f are adjacent — but a CCI_REG16 write becomes one
auto-incrementing burst rather than two transactions, and that no longer matches
the sequence the sensor is known to accept. I kept them as pairs deliberately.
Same reasoning applies to the 0x0340/0x0341 and 0x0342/0x0343 pairs in the init
table.

**3. Derive exposure max from VTS rather than hardcoding it.** Mine is 720
against VTS 741. Yours is VTS 750, so the equivalent is not the same number, and
a max above the frame length will just clip or misbehave. Whatever you choose,
worth confirming the sensor actually accepts it at the top of the range rather
than assuming.

**4. Group hold.** Bracket each group of exposure/gain writes with 0x0104 = 1
then 0x0104 = 0. You already have 0x0104 in your init table, so it is the same
mechanism; the vendor sequence uses it around every AE group.

I have exercised all of this on 648x368. It has never run against your 560x360
mode, and since that is a different crop of the binned window I would not assume
the exposure range transfers unchanged.

## The ipu-bridge patch, and testing

I will rebase the ipu-bridge HIMX1092 entry onto your v6 and send it as part of
your series rather than separately, unless you would rather I kept it standalone.

Once v6 is out I will build it on both x86/IPU7 machines — mine is a Dell XPS 16
DA16260, one lane on CSI-2 port 2 behind a Synaptics SVP7500; the other user is
on a different Dell, CSI-2 port 1, I2C bus 15, different bridge ACPI id — and
report back either way. If your driver does not come up on ACPI x86 I would
rather find that now than after it lands, and honestly it is the more likely
outcome for a first attempt: the fwnode graph on these machines is built by
ipu-bridge from ACPI SSDB data rather than described in DT, so the endpoint your
driver parses arrives by a different route than on your board.

Thanks for taking this the way you did.

Jake