| Message ID | 20240902-imx290-avail-v3-1-b32a12799fed@skidata.com |
|---|---|
| State | New |
| Headers | show |
| Series | media: i2c: imx290: check for availability in probe() | expand |
Hi Benjamin, On Mon, Sep 02, 2024 at 05:57:26PM +0200, bbara93@gmail.com wrote: > From: Benjamin Bara <benjamin.bara@skidata.com> > > The imx290 datasheet states that the IMX290_STANDBY register has two > values: 0 for operating and 1 for standby. Define and use them. > > Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com> > --- > Changes since v2: > - new, split out from the previous 1/2 > --- > drivers/media/i2c/imx290.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c > index 4150e6e4b9a6..1c97f9650eb4 100644 > --- a/drivers/media/i2c/imx290.c > +++ b/drivers/media/i2c/imx290.c > @@ -29,6 +29,8 @@ > #include <media/v4l2-subdev.h> > > #define IMX290_STANDBY CCI_REG8(0x3000) > +#define IMX290_STANDBY_OPERATING 0x00 > +#define IMX290_STANDBY_STANDBY BIT(0) The convention, for single-bit fields, is to define a macro to describe the bit, but not a macro to describe the bit not being set. > #define IMX290_REGHOLD CCI_REG8(0x3001) > #define IMX290_XMSTA CCI_REG8(0x3002) > #define IMX290_ADBIT CCI_REG8(0x3005) > @@ -1016,7 +1018,8 @@ static int imx290_start_streaming(struct imx290 *imx290, > return ret; > } > > - cci_write(imx290->regmap, IMX290_STANDBY, 0x00, &ret); > + cci_write(imx290->regmap, IMX290_STANDBY, IMX290_STANDBY_OPERATING, > + &ret); I would thus rather drop this change. > > msleep(30); > > @@ -1029,7 +1032,7 @@ static int imx290_stop_streaming(struct imx290 *imx290) > { > int ret = 0; > > - cci_write(imx290->regmap, IMX290_STANDBY, 0x01, &ret); > + cci_write(imx290->regmap, IMX290_STANDBY, IMX290_STANDBY_STANDBY, &ret); And keep this one. > > msleep(30); >
Hi Laurent! Thanks for your feedback! On Mon, 2 Sept 2024 at 21:56, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Mon, Sep 02, 2024 at 05:57:26PM +0200, bbara93@gmail.com wrote: > > From: Benjamin Bara <benjamin.bara@skidata.com> > > > > The imx290 datasheet states that the IMX290_STANDBY register has two > > values: 0 for operating and 1 for standby. Define and use them. > > > > Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com> > > --- > > Changes since v2: > > - new, split out from the previous 1/2 > > --- > > drivers/media/i2c/imx290.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c > > index 4150e6e4b9a6..1c97f9650eb4 100644 > > --- a/drivers/media/i2c/imx290.c > > +++ b/drivers/media/i2c/imx290.c > > @@ -29,6 +29,8 @@ > > #include <media/v4l2-subdev.h> > > > > #define IMX290_STANDBY CCI_REG8(0x3000) > > +#define IMX290_STANDBY_OPERATING 0x00 > > +#define IMX290_STANDBY_STANDBY BIT(0) > > The convention, for single-bit fields, is to define a macro to describe > the bit, but not a macro to describe the bit not being set. > > > #define IMX290_REGHOLD CCI_REG8(0x3001) > > #define IMX290_XMSTA CCI_REG8(0x3002) > > #define IMX290_ADBIT CCI_REG8(0x3005) > > @@ -1016,7 +1018,8 @@ static int imx290_start_streaming(struct imx290 *imx290, > > return ret; > > } > > > > - cci_write(imx290->regmap, IMX290_STANDBY, 0x00, &ret); > > + cci_write(imx290->regmap, IMX290_STANDBY, IMX290_STANDBY_OPERATING, > > + &ret); > > I would thus rather drop this change. > > > > > msleep(30); > > > > @@ -1029,7 +1032,7 @@ static int imx290_stop_streaming(struct imx290 *imx290) > > { > > int ret = 0; > > > > - cci_write(imx290->regmap, IMX290_STANDBY, 0x01, &ret); > > + cci_write(imx290->regmap, IMX290_STANDBY, IMX290_STANDBY_STANDBY, &ret); > > And keep this one. > > > > > msleep(30); > > Thanks, will do in the next version. Kind regards Benjamin > > -- > Regards, > > Laurent Pinchart
diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c index 4150e6e4b9a6..1c97f9650eb4 100644 --- a/drivers/media/i2c/imx290.c +++ b/drivers/media/i2c/imx290.c @@ -29,6 +29,8 @@ #include <media/v4l2-subdev.h> #define IMX290_STANDBY CCI_REG8(0x3000) +#define IMX290_STANDBY_OPERATING 0x00 +#define IMX290_STANDBY_STANDBY BIT(0) #define IMX290_REGHOLD CCI_REG8(0x3001) #define IMX290_XMSTA CCI_REG8(0x3002) #define IMX290_ADBIT CCI_REG8(0x3005) @@ -1016,7 +1018,8 @@ static int imx290_start_streaming(struct imx290 *imx290, return ret; } - cci_write(imx290->regmap, IMX290_STANDBY, 0x00, &ret); + cci_write(imx290->regmap, IMX290_STANDBY, IMX290_STANDBY_OPERATING, + &ret); msleep(30); @@ -1029,7 +1032,7 @@ static int imx290_stop_streaming(struct imx290 *imx290) { int ret = 0; - cci_write(imx290->regmap, IMX290_STANDBY, 0x01, &ret); + cci_write(imx290->regmap, IMX290_STANDBY, IMX290_STANDBY_STANDBY, &ret); msleep(30);