Re: [PATCH] media: staging: max96712: Add support for MAX96724

["Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>]

Hello Julien,

Sorry that I missed your later comments before sending out v2.

On 2024-05-28 14:04:03 +0200, Julien Massot wrote:

> > > > diff --git a/drivers/staging/media/max96712/max96712.c 
> > > > b/drivers/staging/media/max96712/max96712.c
> > > > index ea67bcf69c9d..69a0a6a16cf9 100644
> > > > --- a/drivers/staging/media/max96712/max96712.c
> > > > +++ b/drivers/staging/media/max96712/max96712.c
> > > > @@ -17,8 +17,10 @@
> > > >    #include <media/v4l2-subdev.h>
> > > >    #define MAX96712_ID 0x20
> > > > +#define MAX96724_ID 0xA7
> These identifiers seems incorrect
> #define MAX96712_ID 0xA0 (also valid for MAX96712B)
> #define MAX96724_ID 0xA2
> #define MAX96724F_ID 0xA3
> #define MAX96724R_ID 0xA4

> > > > @@ -419,8 +427,15 @@ static int max96712_probe(struct i2c_client 
> > > > *client)
> > > >    	if (priv->gpiod_pwdn)
> > > >    		usleep_range(4000, 5000);
> > > > -	if (max96712_read(priv, 0x4a) != MAX96712_ID)
> > > > +	switch (max96712_read(priv, 0x4a)) {
> 0x4a is a VDDCMP register you should read 0xd instead (DEV_ID)
> switch (max96712_read(priv, 0xd)) {

The datasheet I have for max96712 do unfortunately not document the 0x0d
register.

As Sakari suggested in the review of v2 the usage of device data and 
of_device_get_match_data() to store and fetch device specific 
differences this switch will not be needed. Instead I think I will drop 
trying to read the device id all together, it was left in since early 
development to make sure the driver could talk to the device and then I 
never remove it. Would this be OK for you?

-- 
Kind Regards,
Niklas Söderlund