read ov9281 sensor-id in 2-steps in case the firmware locks

  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


Signed-off-by: Max Schulze <max.schulze@online.de>
Tested-by: Max Schulze <max.schulze@online.de>
Suggested-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
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

 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.
---
 drivers/media/i2c/ov9282.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)
  

Hi Max

On Wed, 27 Sept 2023 at 09:56, Max Schulze <max.schulze@online.de> 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
>
>
> Signed-off-by: Max Schulze <max.schulze@online.de>
> Tested-by: Max Schulze <max.schulze@online.de>
> Suggested-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
> 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.

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

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

  Dave

>                 return -ENXIO;
> --
> 2.30.2
>
  

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@online.de>
> > Tested-by: Max Schulze <max.schulze@online.de>
> > Suggested-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > ---
> > 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@raspberrypi.com>
> 
> > ---
> >  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;
  

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@online.de>
> > > Tested-by: Max Schulze <max.schulze@online.de>
> > > Suggested-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > ---
> > > 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@raspberrypi.com>
> > 
> > > ---
> > >  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;