diff mbox series

[v2,6/6] media: i2c: ov9282: Fix device detection

Message ID 20220715074858.875808-7-alexander.stein@ew.tq-group.com (mailing list archive)
State New, archived
Headers show
Series OV9281 support | expand

Commit Message

Alexander Stein July 15, 2022, 7:48 a.m. UTC 
Apparently the Vision Components model (VC MIPI OV9281) does not support
address auto-increment, so probe fails with:
ov9282 2-0060: chip id mismatch: 9281!=92ff
Instead do two 1 byte reads and combine the result.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
Changes in v2:
* Fix commit message
* Add comment about prevented auto-increment
* Return early if reading ID register failed
* Reorder ID registers, smaller register number first

 drivers/media/i2c/ov9282.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

Comments

Sakari Ailus July 17, 2022, 2:22 p.m. UTC | #1 
Hi Alexander,

On Fri, Jul 15, 2022 at 09:48:58AM +0200, Alexander Stein wrote:
> Apparently the Vision Components model (VC MIPI OV9281) does not support
> address auto-increment, so probe fails with:
> ov9282 2-0060: chip id mismatch: 9281!=92ff
> Instead do two 1 byte reads and combine the result.
> 
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> ---
> Changes in v2:
> * Fix commit message
> * Add comment about prevented auto-increment
> * Return early if reading ID register failed
> * Reorder ID registers, smaller register number first
> 
>  drivers/media/i2c/ov9282.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index 263cdffc558f..532791304c3d 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -761,11 +761,24 @@ static int ov9282_set_stream(struct v4l2_subdev *sd, int enable)
>  static int ov9282_detect(struct ov9282 *ov9282)
>  {
>  	int ret;
> +	u32 id[2];
>  	u32 val;
>  
> -	ret = ov9282_read_reg(ov9282, OV9282_REG_ID, 2, &val);
> +	/*
> +	 * Some vendors prevent auto-increment, so each register has to
> +	 * be read separately
> +	 */

I suppose it still works for writes I guess? Would be nice to have this in
the comment.

> +	ret = ov9282_read_reg(ov9282, OV9282_REG_ID,
> +			      1, &id[0]);

Fits on one line. Same below.

>  	if (ret)
>  		return ret;
> +	ret = ov9282_read_reg(ov9282, OV9282_REG_ID + 1,
> +			      1, &id[1]);
> +	if (ret)
> +		return ret;
> +
> +	val = id[1];
> +	val |= (id[0] << 8);
>  
>  	if (val != OV9282_ID) {
>  		dev_err(ov9282->dev, "chip id mismatch: %x!=%x",
Alexander Stein July 18, 2022, 6:45 a.m. UTC | #2 
Hello Sakari,

thanks for the feedback.

Am Sonntag, 17. Juli 2022, 16:22:01 CEST schrieb Sakari Ailus:
> Hi Alexander,
> 
> On Fri, Jul 15, 2022 at 09:48:58AM +0200, Alexander Stein wrote:
> > Apparently the Vision Components model (VC MIPI OV9281) does not support
> > address auto-increment, so probe fails with:
> > ov9282 2-0060: chip id mismatch: 9281!=92ff
> > Instead do two 1 byte reads and combine the result.
> > 
> > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > ---
> > Changes in v2:
> > * Fix commit message
> > * Add comment about prevented auto-increment
> > * Return early if reading ID register failed
> > * Reorder ID registers, smaller register number first
> > 
> >  drivers/media/i2c/ov9282.c | 15 ++++++++++++++-
> >  1 file changed, 14 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > index 263cdffc558f..532791304c3d 100644
> > --- a/drivers/media/i2c/ov9282.c
> > +++ b/drivers/media/i2c/ov9282.c
> > @@ -761,11 +761,24 @@ static int ov9282_set_stream(struct v4l2_subdev *sd,
> > int enable)> 
> >  static int ov9282_detect(struct ov9282 *ov9282)
> >  {
> >  
> >  	int ret;
> > 
> > +	u32 id[2];
> > 
> >  	u32 val;
> > 
> > -	ret = ov9282_read_reg(ov9282, OV9282_REG_ID, 2, &val);
> > +	/*
> > +	 * Some vendors prevent auto-increment, so each register has to
> > +	 * be read separately
> > +	 */
> 
> I suppose it still works for writes I guess? Would be nice to have this in
> the comment.

Yes, I didn't notice any problems regarding writes. Will address this in the 
comment.

> > +	ret = ov9282_read_reg(ov9282, OV9282_REG_ID,
> > +			      1, &id[0]);
> 
> Fits on one line. Same below.

Sure, will be changed.

Thanks and best regards,
Alexander

> >  	if (ret)
> >  	
> >  		return ret;
> > 
> > +	ret = ov9282_read_reg(ov9282, OV9282_REG_ID + 1,
> > +			      1, &id[1]);
> > +	if (ret)
> > +		return ret;
> > +
> > +	val = id[1];
> > +	val |= (id[0] << 8);
> > 
> >  	if (val != OV9282_ID) {
> >  	
> >  		dev_err(ov9282->dev, "chip id mismatch: %x!=%x",
Alexander Stein July 21, 2022, 2:25 p.m. UTC | #3 
Hi,

Am Sonntag, 17. Juli 2022, 16:22:01 CEST schrieb Sakari Ailus:
> Hi Alexander,
> 
> On Fri, Jul 15, 2022 at 09:48:58AM +0200, Alexander Stein wrote:
> > Apparently the Vision Components model (VC MIPI OV9281) does not support
> > address auto-increment, so probe fails with:
> > ov9282 2-0060: chip id mismatch: 9281!=92ff
> > Instead do two 1 byte reads and combine the result.
> > 
> > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > ---
> > Changes in v2:
> > * Fix commit message
> > * Add comment about prevented auto-increment
> > * Return early if reading ID register failed
> > * Reorder ID registers, smaller register number first
> > 
> >  drivers/media/i2c/ov9282.c | 15 ++++++++++++++-
> >  1 file changed, 14 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > index 263cdffc558f..532791304c3d 100644
> > --- a/drivers/media/i2c/ov9282.c
> > +++ b/drivers/media/i2c/ov9282.c
> > @@ -761,11 +761,24 @@ static int ov9282_set_stream(struct v4l2_subdev *sd,
> > int enable)> 
> >  static int ov9282_detect(struct ov9282 *ov9282)
> >  {
> >  
> >  	int ret;
> > 
> > +	u32 id[2];
> > 
> >  	u32 val;
> > 
> > -	ret = ov9282_read_reg(ov9282, OV9282_REG_ID, 2, &val);
> > +	/*
> > +	 * Some vendors prevent auto-increment, so each register has to
> > +	 * be read separately
> > +	 */
> 
> I suppose it still works for writes I guess? Would be nice to have this in
> the comment.
> 
> > +	ret = ov9282_read_reg(ov9282, OV9282_REG_ID,
> > +			      1, &id[0]);
> 
> Fits on one line. Same below.
> 
> >  	if (ret)
> >  	
> >  		return ret;
> > 
> > +	ret = ov9282_read_reg(ov9282, OV9282_REG_ID + 1,
> > +			      1, &id[1]);
> > +	if (ret)
> > +		return ret;
> > +
> > +	val = id[1];
> > +	val |= (id[0] << 8);
> > 
> >  	if (val != OV9282_ID) {
> >  	
> >  		dev_err(ov9282->dev, "chip id mismatch: %x!=%x",

imx290 is affected by the same problem. This can be handles by using regmap's 
config option use_single_read=true.

Will convert to regmap to address this.

Best regards,
Alexander
diff mbox series

Patch

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index 263cdffc558f..532791304c3d 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -761,11 +761,24 @@  static int ov9282_set_stream(struct v4l2_subdev *sd, int enable)
 static int ov9282_detect(struct ov9282 *ov9282)
 {
 	int ret;
+	u32 id[2];
 	u32 val;
 
-	ret = ov9282_read_reg(ov9282, OV9282_REG_ID, 2, &val);
+	/*
+	 * Some vendors prevent auto-increment, so each register has to
+	 * be read separately
+	 */
+	ret = ov9282_read_reg(ov9282, OV9282_REG_ID,
+			      1, &id[0]);
 	if (ret)
 		return ret;
+	ret = ov9282_read_reg(ov9282, OV9282_REG_ID + 1,
+			      1, &id[1]);
+	if (ret)
+		return ret;
+
+	val = id[1];
+	val |= (id[0] << 8);
 
 	if (val != OV9282_ID) {
 		dev_err(ov9282->dev, "chip id mismatch: %x!=%x",