On Wed, Sep 27, 2023 at 11:38:21AM +0100, Dave Stevenson wrote: > On Wed, 27 Sept 2023 at 09:56, Max Schulze wrote: > > > > The ov9281 produced by vision-components does not support > > auto-incremented reads, so read the id in 2 separate steps > > to circumvent the error: > > kernel: ov9282 10-0060: chip id mismatch: 9281!=92ff > > kernel: ov9282 10-0060: failed to find sensor: -6 If I recall correctly, the issue isn't that it doesn't support auto-increment at all, but that it then will output one 0xff byte first. Or maybe that was with a different firmware version ? > > Signed-off-by: Max Schulze <max.schulze@xxxxxxxxx> > > Tested-by: Max Schulze <max.schulze@xxxxxxxxx> > > Suggested-by: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx> > > --- > > This was tested on rpi cm4 with two ov9281 at the same time, > > one from vc, one from inno-maker. > > > > The rpi kernel supported ov9281 out-of-tree until kernel 6.1. > > It carried this change originally made by Dave to support the vc > > sensor. Switching to mainline now broke support for it. > > > > I could not find a single-commit for the original change to which I > > could link, only squashed ones, i.e. > > > > https://github.com/raspberrypi/linux/commit/eb00efc993d8cd835221255b44e9019a31708abe > > The single commit was on the rpi-5.10.y branch > https://github.com/raspberrypi/linux/commit/e19e5fa998c7dfaa9942a494499e37788365ccec > > > media: i2c: ov9281: Read chip ID via 2 reads > > > > Vision Components have made an OV9281 module which blocks reading > > back the majority of registers to comply with NDAs, and in doing > > so doesn't allow auto-increment register reading as used when > > reading the chip ID. > > I know that Laurent has previously made comments on potentially nicer > ways to handle these annoying Vision Components sensors before, but in > the absence of anything solid then I have no issues with this patch. > It's not going to cause an issue with standard modules, and makes them > work with the VC ones. I'd rather handle this in a common layer, with a DT property to indicate that multi-reads are broken. A good candidate would be regmap, v4l2-cci being another possible option. I don't want to see this kind of change made in lots of sensor drivers because one vendor got it wrong. > Reviewed-by: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx> > > > --- > > drivers/media/i2c/ov9282.c | 13 ++++++++----- > > 1 file changed, 8 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c > > index 068c7449f50e..3b687f6d4581 100644 > > --- a/drivers/media/i2c/ov9282.c > > +++ b/drivers/media/i2c/ov9282.c > > @@ -1078,13 +1078,16 @@ static int ov9282_set_stream(struct v4l2_subdev *sd, int enable) > > static int ov9282_detect(struct ov9282 *ov9282) > > { > > int ret; > > - u32 val; > > + u32 val = 0, id_msb = 0; > > > > - ret = ov9282_read_reg(ov9282, OV9282_REG_ID, 2, &val); > > - if (ret) > > - return ret; > > + // some firmware limits auto-increment register writes, so do it separately > > + ret = ov9282_read_reg(ov9282, OV9282_REG_ID + 1, 1, &val); > > + if (!ret) > > + ret = ov9282_read_reg(ov9282, OV9282_REG_ID, 1, &id_msb); > > + > > + val |= (id_msb << 8); > > > > - if (val != OV9282_ID) { > > + if (ret || val != OV9282_ID) { > > dev_err(ov9282->dev, "chip id mismatch: %x!=%x", > > OV9282_ID, val); > > Minor nit: you'll print this error message if one of the transfers > failed, whereas previously it would have just returned the error code. > > > return -ENXIO; -- Regards, Laurent Pinchart
