On Wed, Sep 27, 2023 at 02:23:02PM +0300, Laurent Pinchart wrote: > 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. Btw, this may be relevant: https://gitlab.com/ideasonboard/nxp/linux/-/commits/v6.6/sensors/vcmipi/ > > 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
