From: Alexander Stein <alexander.stein@ew.tq-group.com>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: linux-media@vger.kernel.org,
Manivannan Sadhasivam <mani@kernel.org>,
Sakari Ailus <sakari.ailus@iki.fi>
Subject: Re: Re: Re: [PATCH 07/19] media: i2c: imx290: Support variable-sized registers
Date: Thu, 21 Jul 2022 13:43:54 +0200 [thread overview]
Message-ID: <8007753.G0QQBjFxQf@steina-w> (raw)
In-Reply-To: <Ytk3nfwQ7eEQSTcV@pendragon.ideasonboard.com>
Am Donnerstag, 21. Juli 2022, 13:25:17 CEST schrieb Laurent Pinchart:
> Hi Alexander,
>
> On Thu, Jul 21, 2022 at 01:18:29PM +0200, Alexander Stein wrote:
> > Am Donnerstag, 21. Juli 2022, 12:54:51 CEST schrieb Laurent Pinchart:
> > > On Thu, Jul 21, 2022 at 11:43:47AM +0200, Alexander Stein wrote:
> > > > Am Donnerstag, 21. Juli 2022, 10:35:28 CEST schrieb Laurent Pinchart:
> > > > > The IMX290 has registers of different sizes. To simplify the code,
> > > > > handle this in the read/write functions instead of in the callers by
> > > > > encoding the register size in the symbolic name macros. All
> > > > > registers
> > > > > are defined as 8-bit for now, a subsequent change will move to
> > > > > larger
> > > > > registers where applicable.
> > > > >
> > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > ---
> > > > >
> > > > > drivers/media/i2c/imx290.c | 352
> > > > > +++++++++++++++++++------------------
> > > > > 1 file changed, 180 insertions(+), 172 deletions(-)
> > > > >
> > > > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > > > > index 711282126c34..ac58bfe6db7f 100644
> > > > > --- a/drivers/media/i2c/imx290.c
> > > > > +++ b/drivers/media/i2c/imx290.c
> > > > > @@ -22,22 +22,28 @@
> > > > >
> > > > > #include <media/v4l2-fwnode.h>
> > > > > #include <media/v4l2-subdev.h>
> > > > >
> > > > > -#define IMX290_STANDBY 0x3000
> > > > > -#define IMX290_REGHOLD 0x3001
> > > > > -#define IMX290_XMSTA 0x3002
> > > > > -#define IMX290_FR_FDG_SEL 0x3009
> > > > > -#define IMX290_BLKLEVEL_LOW 0x300a
> > > > > -#define IMX290_BLKLEVEL_HIGH 0x300b
> > > > > -#define IMX290_GAIN 0x3014
> > > > > -#define IMX290_HMAX_LOW 0x301c
> > > > > -#define IMX290_HMAX_HIGH 0x301d
> > > > > -#define IMX290_PGCTRL 0x308c
> > > > > -#define IMX290_PHY_LANE_NUM 0x3407
> > > > > -#define IMX290_CSI_LANE_MODE 0x3443
> > > > > +#define IMX290_REG_8BIT(n)
((1 << 16) |
> >
> > (n))
> >
> > > > > +#define IMX290_REG_16BIT(n)
> >
> > ((2 << 16) | (n))
> >
> > > > > +#define IMX290_REG_24BIT(n)
> >
> > ((3 << 16) | (n))
> >
> > > > IMX290_REG_SIZE_SHIFT instead of 16 here? Not exactly necessary, but
> > > > use
> > > > 1U, 2U and 3U instead.
> > >
> > > I'll fix that.
> > >
> > > > > +#define IMX290_REG_SIZE_SHIFT
> >
> > 16
> >
> > > > > +#define IMX290_REG_ADDR_MASK
> >
> > 0xffff
> >
> > > > > -#define IMX290_PGCTRL_REGEN BIT(0)
> > > > > -#define IMX290_PGCTRL_THRU BIT(1)
> > > > > -#define IMX290_PGCTRL_MODE(n) ((n) << 4)
> > > > > +#define IMX290_STANDBY
> >
> > IMX290_REG_8BIT(0x3000)
> >
> > > > > +#define IMX290_REGHOLD
> >
> > IMX290_REG_8BIT(0x3001)
> >
> > > > > +#define IMX290_XMSTA
> >
> > IMX290_REG_8BIT(0x3002)
> >
> > > > > +#define IMX290_FR_FDG_SEL
> >
> > IMX290_REG_8BIT(0x3009)
> >
> > > > > +#define IMX290_BLKLEVEL_LOW
> >
> > IMX290_REG_8BIT(0x300a)
> >
> > > > > +#define IMX290_BLKLEVEL_HIGH
> >
> > IMX290_REG_8BIT(0x300b)
> >
> > > > > +#define IMX290_GAIN
> >
> > IMX290_REG_8BIT(0x3014)
> >
> > > > > +#define IMX290_HMAX_LOW
> >
> > IMX290_REG_8BIT(0x301c)
> >
> > > > > +#define IMX290_HMAX_HIGH
> >
> > IMX290_REG_8BIT(0x301d)
> >
> > > > > +#define IMX290_PGCTRL
> >
> > IMX290_REG_8BIT(0x308c)
> >
> > > > > +#define IMX290_PHY_LANE_NUM
> >
> > IMX290_REG_8BIT(0x3407)
> >
> > > > > +#define IMX290_CSI_LANE_MODE
> >
> > IMX290_REG_8BIT(0x3443)
> >
> > > > > +
> > > > > +#define IMX290_PGCTRL_REGEN
> >
> > BIT(0)
> >
> > > > > +#define IMX290_PGCTRL_THRU
BIT(1)
> > > > > +#define IMX290_PGCTRL_MODE(n)
> >
> > ((n) << 4)
> >
> > > > > static const char * const imx290_supply_name[] = {
> > > > >
> > > > > "vdda",
> > > > >
> > > > > @@ -48,7 +54,7 @@ static const char * const imx290_supply_name[] = {
> > > > >
> > > > > #define IMX290_NUM_SUPPLIES ARRAY_SIZE(imx290_supply_name)
> > > > >
> > > > > struct imx290_regval {
> > > > >
> > > > > - u16 reg;
> > > > > + u32 reg;
> > > > >
> > > > > u8 val;
> > > > >
> > > > > };
> > > > >
> > > > > @@ -111,163 +117,163 @@ static const char * const
> > > > > imx290_test_pattern_menu[] = { };
> > > > >
> > > > > static const struct imx290_regval imx290_global_init_settings[] = {
> > > > >
> > > > > - { 0x3007, 0x00 },
> > > > > - { 0x3018, 0x65 },
> > > > > - { 0x3019, 0x04 },
> > > > > - { 0x301a, 0x00 },
> > > > > - { 0x3444, 0x20 },
> > > > > - { 0x3445, 0x25 },
> > > > > - { 0x303a, 0x0c },
> > > > > - { 0x3040, 0x00 },
> > > > > - { 0x3041, 0x00 },
> > > > > - { 0x303c, 0x00 },
> > > > > - { 0x303d, 0x00 },
> > > > > - { 0x3042, 0x9c },
> > > > > - { 0x3043, 0x07 },
> > > > > - { 0x303e, 0x49 },
> > > > > - { 0x303f, 0x04 },
> > > > > - { 0x304b, 0x0a },
> > > > > - { 0x300f, 0x00 },
> > > > > - { 0x3010, 0x21 },
> > > > > - { 0x3012, 0x64 },
> > > > > - { 0x3016, 0x09 },
> > > > > - { 0x3070, 0x02 },
> > > > > - { 0x3071, 0x11 },
> > > > > - { 0x309b, 0x10 },
> > > > > - { 0x309c, 0x22 },
> > > > > - { 0x30a2, 0x02 },
> > > > > - { 0x30a6, 0x20 },
> > > > > - { 0x30a8, 0x20 },
> > > > > - { 0x30aa, 0x20 },
> > > > > - { 0x30ac, 0x20 },
> > > > > - { 0x30b0, 0x43 },
> > > > > - { 0x3119, 0x9e },
> > > > > - { 0x311c, 0x1e },
> > > > > - { 0x311e, 0x08 },
> > > > > - { 0x3128, 0x05 },
> > > > > - { 0x313d, 0x83 },
> > > > > - { 0x3150, 0x03 },
> > > > > - { 0x317e, 0x00 },
> > > > > - { 0x32b8, 0x50 },
> > > > > - { 0x32b9, 0x10 },
> > > > > - { 0x32ba, 0x00 },
> > > > > - { 0x32bb, 0x04 },
> > > > > - { 0x32c8, 0x50 },
> > > > > - { 0x32c9, 0x10 },
> > > > > - { 0x32ca, 0x00 },
> > > > > - { 0x32cb, 0x04 },
> > > > > - { 0x332c, 0xd3 },
> > > > > - { 0x332d, 0x10 },
> > > > > - { 0x332e, 0x0d },
> > > > > - { 0x3358, 0x06 },
> > > > > - { 0x3359, 0xe1 },
> > > > > - { 0x335a, 0x11 },
> > > > > - { 0x3360, 0x1e },
> > > > > - { 0x3361, 0x61 },
> > > > > - { 0x3362, 0x10 },
> > > > > - { 0x33b0, 0x50 },
> > > > > - { 0x33b2, 0x1a },
> > > > > - { 0x33b3, 0x04 },
> > > > > + { IMX290_REG_8BIT(0x3007), 0x00 },
> > > > > + { IMX290_REG_8BIT(0x3018), 0x65 },
> > > > > + { IMX290_REG_8BIT(0x3019), 0x04 },
> > > > > + { IMX290_REG_8BIT(0x301a), 0x00 },
> > > > > + { IMX290_REG_8BIT(0x3444), 0x20 },
> > > > > + { IMX290_REG_8BIT(0x3445), 0x25 },
> > > > > + { IMX290_REG_8BIT(0x303a), 0x0c },
> > > > > + { IMX290_REG_8BIT(0x3040), 0x00 },
> > > > > + { IMX290_REG_8BIT(0x3041), 0x00 },
> > > > > + { IMX290_REG_8BIT(0x303c), 0x00 },
> > > > > + { IMX290_REG_8BIT(0x303d), 0x00 },
> > > > > + { IMX290_REG_8BIT(0x3042), 0x9c },
> > > > > + { IMX290_REG_8BIT(0x3043), 0x07 },
> > > > > + { IMX290_REG_8BIT(0x303e), 0x49 },
> > > > > + { IMX290_REG_8BIT(0x303f), 0x04 },
> > > > > + { IMX290_REG_8BIT(0x304b), 0x0a },
> > > > > + { IMX290_REG_8BIT(0x300f), 0x00 },
> > > > > + { IMX290_REG_8BIT(0x3010), 0x21 },
> > > > > + { IMX290_REG_8BIT(0x3012), 0x64 },
> > > > > + { IMX290_REG_8BIT(0x3016), 0x09 },
> > > > > + { IMX290_REG_8BIT(0x3070), 0x02 },
> > > > > + { IMX290_REG_8BIT(0x3071), 0x11 },
> > > > > + { IMX290_REG_8BIT(0x309b), 0x10 },
> > > > > + { IMX290_REG_8BIT(0x309c), 0x22 },
> > > > > + { IMX290_REG_8BIT(0x30a2), 0x02 },
> > > > > + { IMX290_REG_8BIT(0x30a6), 0x20 },
> > > > > + { IMX290_REG_8BIT(0x30a8), 0x20 },
> > > > > + { IMX290_REG_8BIT(0x30aa), 0x20 },
> > > > > + { IMX290_REG_8BIT(0x30ac), 0x20 },
> > > > > + { IMX290_REG_8BIT(0x30b0), 0x43 },
> > > > > + { IMX290_REG_8BIT(0x3119), 0x9e },
> > > > > + { IMX290_REG_8BIT(0x311c), 0x1e },
> > > > > + { IMX290_REG_8BIT(0x311e), 0x08 },
> > > > > + { IMX290_REG_8BIT(0x3128), 0x05 },
> > > > > + { IMX290_REG_8BIT(0x313d), 0x83 },
> > > > > + { IMX290_REG_8BIT(0x3150), 0x03 },
> > > > > + { IMX290_REG_8BIT(0x317e), 0x00 },
> > > > > + { IMX290_REG_8BIT(0x32b8), 0x50 },
> > > > > + { IMX290_REG_8BIT(0x32b9), 0x10 },
> > > > > + { IMX290_REG_8BIT(0x32ba), 0x00 },
> > > > > + { IMX290_REG_8BIT(0x32bb), 0x04 },
> > > > > + { IMX290_REG_8BIT(0x32c8), 0x50 },
> > > > > + { IMX290_REG_8BIT(0x32c9), 0x10 },
> > > > > + { IMX290_REG_8BIT(0x32ca), 0x00 },
> > > > > + { IMX290_REG_8BIT(0x32cb), 0x04 },
> > > > > + { IMX290_REG_8BIT(0x332c), 0xd3 },
> > > > > + { IMX290_REG_8BIT(0x332d), 0x10 },
> > > > > + { IMX290_REG_8BIT(0x332e), 0x0d },
> > > > > + { IMX290_REG_8BIT(0x3358), 0x06 },
> > > > > + { IMX290_REG_8BIT(0x3359), 0xe1 },
> > > > > + { IMX290_REG_8BIT(0x335a), 0x11 },
> > > > > + { IMX290_REG_8BIT(0x3360), 0x1e },
> > > > > + { IMX290_REG_8BIT(0x3361), 0x61 },
> > > > > + { IMX290_REG_8BIT(0x3362), 0x10 },
> > > > > + { IMX290_REG_8BIT(0x33b0), 0x50 },
> > > > > + { IMX290_REG_8BIT(0x33b2), 0x1a },
> > > > > + { IMX290_REG_8BIT(0x33b3), 0x04 },
> > > > >
> > > > > };
> > > > >
> > > > > static const struct imx290_regval imx290_1080p_settings[] = {
> > > > >
> > > > > /* mode settings */
> > > > >
> > > > > - { 0x3007, 0x00 },
> > > > > - { 0x303a, 0x0c },
> > > > > - { 0x3414, 0x0a },
> > > > > - { 0x3472, 0x80 },
> > > > > - { 0x3473, 0x07 },
> > > > > - { 0x3418, 0x38 },
> > > > > - { 0x3419, 0x04 },
> > > > > - { 0x3012, 0x64 },
> > > > > - { 0x3013, 0x00 },
> > > > > - { 0x305c, 0x18 },
> > > > > - { 0x305d, 0x03 },
> > > > > - { 0x305e, 0x20 },
> > > > > - { 0x305f, 0x01 },
> > > > > - { 0x315e, 0x1a },
> > > > > - { 0x3164, 0x1a },
> > > > > - { 0x3480, 0x49 },
> > > > > + { IMX290_REG_8BIT(0x3007), 0x00 },
> > > > > + { IMX290_REG_8BIT(0x303a), 0x0c },
> > > > > + { IMX290_REG_8BIT(0x3414), 0x0a },
> > > > > + { IMX290_REG_8BIT(0x3472), 0x80 },
> > > > > + { IMX290_REG_8BIT(0x3473), 0x07 },
> > > > > + { IMX290_REG_8BIT(0x3418), 0x38 },
> > > > > + { IMX290_REG_8BIT(0x3419), 0x04 },
> > > > > + { IMX290_REG_8BIT(0x3012), 0x64 },
> > > > > + { IMX290_REG_8BIT(0x3013), 0x00 },
> > > > > + { IMX290_REG_8BIT(0x305c), 0x18 },
> > > > > + { IMX290_REG_8BIT(0x305d), 0x03 },
> > > > > + { IMX290_REG_8BIT(0x305e), 0x20 },
> > > > > + { IMX290_REG_8BIT(0x305f), 0x01 },
> > > > > + { IMX290_REG_8BIT(0x315e), 0x1a },
> > > > > + { IMX290_REG_8BIT(0x3164), 0x1a },
> > > > > + { IMX290_REG_8BIT(0x3480), 0x49 },
> > > > >
> > > > > /* data rate settings */
> > > > >
> > > > > - { 0x3405, 0x10 },
> > > > > - { 0x3446, 0x57 },
> > > > > - { 0x3447, 0x00 },
> > > > > - { 0x3448, 0x37 },
> > > > > - { 0x3449, 0x00 },
> > > > > - { 0x344a, 0x1f },
> > > > > - { 0x344b, 0x00 },
> > > > > - { 0x344c, 0x1f },
> > > > > - { 0x344d, 0x00 },
> > > > > - { 0x344e, 0x1f },
> > > > > - { 0x344f, 0x00 },
> > > > > - { 0x3450, 0x77 },
> > > > > - { 0x3451, 0x00 },
> > > > > - { 0x3452, 0x1f },
> > > > > - { 0x3453, 0x00 },
> > > > > - { 0x3454, 0x17 },
> > > > > - { 0x3455, 0x00 },
> > > > > + { IMX290_REG_8BIT(0x3405), 0x10 },
> > > > > + { IMX290_REG_8BIT(0x3446), 0x57 },
> > > > > + { IMX290_REG_8BIT(0x3447), 0x00 },
> > > > > + { IMX290_REG_8BIT(0x3448), 0x37 },
> > > > > + { IMX290_REG_8BIT(0x3449), 0x00 },
> > > > > + { IMX290_REG_8BIT(0x344a), 0x1f },
> > > > > + { IMX290_REG_8BIT(0x344b), 0x00 },
> > > > > + { IMX290_REG_8BIT(0x344c), 0x1f },
> > > > > + { IMX290_REG_8BIT(0x344d), 0x00 },
> > > > > + { IMX290_REG_8BIT(0x344e), 0x1f },
> > > > > + { IMX290_REG_8BIT(0x344f), 0x00 },
> > > > > + { IMX290_REG_8BIT(0x3450), 0x77 },
> > > > > + { IMX290_REG_8BIT(0x3451), 0x00 },
> > > > > + { IMX290_REG_8BIT(0x3452), 0x1f },
> > > > > + { IMX290_REG_8BIT(0x3453), 0x00 },
> > > > > + { IMX290_REG_8BIT(0x3454), 0x17 },
> > > > > + { IMX290_REG_8BIT(0x3455), 0x00 },
> > > > >
> > > > > };
> > > > >
> > > > > static const struct imx290_regval imx290_720p_settings[] = {
> > > > >
> > > > > /* mode settings */
> > > > >
> > > > > - { 0x3007, 0x10 },
> > > > > - { 0x303a, 0x06 },
> > > > > - { 0x3414, 0x04 },
> > > > > - { 0x3472, 0x00 },
> > > > > - { 0x3473, 0x05 },
> > > > > - { 0x3418, 0xd0 },
> > > > > - { 0x3419, 0x02 },
> > > > > - { 0x3012, 0x64 },
> > > > > - { 0x3013, 0x00 },
> > > > > - { 0x305c, 0x20 },
> > > > > - { 0x305d, 0x00 },
> > > > > - { 0x305e, 0x20 },
> > > > > - { 0x305f, 0x01 },
> > > > > - { 0x315e, 0x1a },
> > > > > - { 0x3164, 0x1a },
> > > > > - { 0x3480, 0x49 },
> > > > > + { IMX290_REG_8BIT(0x3007), 0x10 },
> > > > > + { IMX290_REG_8BIT(0x303a), 0x06 },
> > > > > + { IMX290_REG_8BIT(0x3414), 0x04 },
> > > > > + { IMX290_REG_8BIT(0x3472), 0x00 },
> > > > > + { IMX290_REG_8BIT(0x3473), 0x05 },
> > > > > + { IMX290_REG_8BIT(0x3418), 0xd0 },
> > > > > + { IMX290_REG_8BIT(0x3419), 0x02 },
> > > > > + { IMX290_REG_8BIT(0x3012), 0x64 },
> > > > > + { IMX290_REG_8BIT(0x3013), 0x00 },
> > > > > + { IMX290_REG_8BIT(0x305c), 0x20 },
> > > > > + { IMX290_REG_8BIT(0x305d), 0x00 },
> > > > > + { IMX290_REG_8BIT(0x305e), 0x20 },
> > > > > + { IMX290_REG_8BIT(0x305f), 0x01 },
> > > > > + { IMX290_REG_8BIT(0x315e), 0x1a },
> > > > > + { IMX290_REG_8BIT(0x3164), 0x1a },
> > > > > + { IMX290_REG_8BIT(0x3480), 0x49 },
> > > > >
> > > > > /* data rate settings */
> > > > >
> > > > > - { 0x3405, 0x10 },
> > > > > - { 0x3446, 0x4f },
> > > > > - { 0x3447, 0x00 },
> > > > > - { 0x3448, 0x2f },
> > > > > - { 0x3449, 0x00 },
> > > > > - { 0x344a, 0x17 },
> > > > > - { 0x344b, 0x00 },
> > > > > - { 0x344c, 0x17 },
> > > > > - { 0x344d, 0x00 },
> > > > > - { 0x344e, 0x17 },
> > > > > - { 0x344f, 0x00 },
> > > > > - { 0x3450, 0x57 },
> > > > > - { 0x3451, 0x00 },
> > > > > - { 0x3452, 0x17 },
> > > > > - { 0x3453, 0x00 },
> > > > > - { 0x3454, 0x17 },
> > > > > - { 0x3455, 0x00 },
> > > > > + { IMX290_REG_8BIT(0x3405), 0x10 },
> > > > > + { IMX290_REG_8BIT(0x3446), 0x4f },
> > > > > + { IMX290_REG_8BIT(0x3447), 0x00 },
> > > > > + { IMX290_REG_8BIT(0x3448), 0x2f },
> > > > > + { IMX290_REG_8BIT(0x3449), 0x00 },
> > > > > + { IMX290_REG_8BIT(0x344a), 0x17 },
> > > > > + { IMX290_REG_8BIT(0x344b), 0x00 },
> > > > > + { IMX290_REG_8BIT(0x344c), 0x17 },
> > > > > + { IMX290_REG_8BIT(0x344d), 0x00 },
> > > > > + { IMX290_REG_8BIT(0x344e), 0x17 },
> > > > > + { IMX290_REG_8BIT(0x344f), 0x00 },
> > > > > + { IMX290_REG_8BIT(0x3450), 0x57 },
> > > > > + { IMX290_REG_8BIT(0x3451), 0x00 },
> > > > > + { IMX290_REG_8BIT(0x3452), 0x17 },
> > > > > + { IMX290_REG_8BIT(0x3453), 0x00 },
> > > > > + { IMX290_REG_8BIT(0x3454), 0x17 },
> > > > > + { IMX290_REG_8BIT(0x3455), 0x00 },
> > > > >
> > > > > };
> > > > >
> > > > > static const struct imx290_regval imx290_10bit_settings[] = {
> > > > >
> > > > > - { 0x3005, 0x00},
> > > > > - { 0x3046, 0x00},
> > > > > - { 0x3129, 0x1d},
> > > > > - { 0x317c, 0x12},
> > > > > - { 0x31ec, 0x37},
> > > > > - { 0x3441, 0x0a},
> > > > > - { 0x3442, 0x0a},
> > > > > - { 0x300a, 0x3c},
> > > > > - { 0x300b, 0x00},
> > > > > + { IMX290_REG_8BIT(0x3005), 0x00},
> > > > > + { IMX290_REG_8BIT(0x3046), 0x00},
> > > > > + { IMX290_REG_8BIT(0x3129), 0x1d},
> > > > > + { IMX290_REG_8BIT(0x317c), 0x12},
> > > > > + { IMX290_REG_8BIT(0x31ec), 0x37},
> > > > > + { IMX290_REG_8BIT(0x3441), 0x0a},
> > > > > + { IMX290_REG_8BIT(0x3442), 0x0a},
> > > > > + { IMX290_REG_8BIT(0x300a), 0x3c},
> > > > > + { IMX290_REG_8BIT(0x300b), 0x00},
> > > > >
> > > > > };
> > > > >
> > > > > static const struct imx290_regval imx290_12bit_settings[] = {
> > > > >
> > > > > - { 0x3005, 0x01 },
> > > > > - { 0x3046, 0x01 },
> > > > > - { 0x3129, 0x00 },
> > > > > - { 0x317c, 0x00 },
> > > > > - { 0x31ec, 0x0e },
> > > > > - { 0x3441, 0x0c },
> > > > > - { 0x3442, 0x0c },
> > > > > - { 0x300a, 0xf0 },
> > > > > - { 0x300b, 0x00 },
> > > > > + { IMX290_REG_8BIT(0x3005), 0x01 },
> > > > > + { IMX290_REG_8BIT(0x3046), 0x01 },
> > > > > + { IMX290_REG_8BIT(0x3129), 0x00 },
> > > > > + { IMX290_REG_8BIT(0x317c), 0x00 },
> > > > > + { IMX290_REG_8BIT(0x31ec), 0x0e },
> > > > > + { IMX290_REG_8BIT(0x3441), 0x0c },
> > > > > + { IMX290_REG_8BIT(0x3442), 0x0c },
> > > > > + { IMX290_REG_8BIT(0x300a), 0xf0 },
> > > > > + { IMX290_REG_8BIT(0x300b), 0x00 },
> > > > >
> > > > > };
> > > > >
> > > > > /* supported link frequencies */
> > > > >
> > > > > @@ -362,33 +368,35 @@ static inline struct imx290 *to_imx290(struct
> > > > > v4l2_subdev *_sd) return container_of(_sd, struct imx290, sd);
> > > > >
> > > > > }
> > > > >
> > > > > -static inline int __always_unused imx290_read_reg(struct imx290
> > > > > *imx290, u16 addr, u8 *value) +static int __always_unused
> > > > > imx290_read_reg(struct imx290 *imx290, u32 addr, u32 *value) {
> > > > > - unsigned int regval;
> > > > > + u8 data[3] = { 0, 0, 0 };
> > > > >
> > > > > int ret;
> > > > >
> > > > > - ret = regmap_read(imx290->regmap, addr, ®val);
> > > > > - if (ret) {
> > > > > - dev_err(imx290->dev, "Failed to read register
0x%04x: %d\n",
> > > > > - addr, ret);
> > > > > + ret = regmap_raw_read(imx290->regmap, addr &
IMX290_REG_ADDR_MASK,
> > > > > + data, (addr >>
IMX290_REG_SIZE_SHIFT) & 3);
> > > >
> > > > While I agree with the intention, it fails for some devices, e.g.
> > > > Vision
> > > > Components MIPI IMX327 C does not support auto-increment and most
> > > > probably
> > > > also valid for the IMX290 model.
> > > > This is similar to the OV9281 variant, see [1].
> > >
> > > *sigh* Can't they get their firmware right ? :-( Inserting a
> > > microcontroller between the SoC and sensor is bad enough in itself,
> > > making it mess up I2C access due to firmware bugs is worse.
> >
> > [1] indicates this is due to NDA compliance.
>
> I don't buy that, sorry. That's not the reason, and even if it were, it
> wouldn't excuse breaking auto-increment on read.
I expect other (valid) sensor registers can't be read as well. And to prevent
circumventing that by using auto-increment, it is blocked completely.
It's hassle I agree.
> > > How far do we need to go in sensor drivers to support messed up hardware
> > > designs that are not related to the sensor at all ? I don't want to
> > > patch all sensor drivers to make I2C read slower because someone
> > > somewhere has taken the sensor and decided to screw up. That doesn't
> > > scale, and hurt all users of these sensors in sane camera modules.
> >
> > A somewhat acceptable solution might be to add a property indicating that
> > auto-increment is not supported, so that each byte has to be fetched
> > individually.
>
> I would be fine with that. It should then be handled in the I2C core or
> regmap layer.
Nice the following snippet does the trick already:
---8<---
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -221,6 +221,7 @@ static const struct imx290_pixfmt imx290_formats[] = {
static const struct regmap_config imx290_regmap_config = {
.reg_bits = 16,
.val_bits = 8,
+ .use_single_read = true,
};
static const char * const imx290_test_pattern_menu[] = {
---8<---
As this affects the VC OV9281 as well, any suggestions for a common property?
Best regards,
Alexander
> > [1] https://github.com/raspberrypi/linux/commit/
> > 58deee7c917e1c3c5e37987c3a89ad19d791f58a
> >
> > > > Reading ID using 16bit register (using later patch) returns:
> > > > > imx290 2-001a: chip ID 0xffd0
> > > >
> > > > Using 2 8bit reads:
> > > > imx290 2-001a: chip ID 0x07d0
> > > >
> > > > Best regards,
> > > > Alexander
> > > >
> > > > [1]
> > > > https://patchwork.kernel.org/project/linux-media/patch/20220715074858.
> > > > 875
> > > > 808-7-alexander.stein@ew.tq-group.com/>
> > > >
> > > > > + if (ret < 0) {
> > > > > + dev_err(imx290->dev, "%u-bit read from 0x%04x
failed: %d\n",
> > > > > + ((addr >> IMX290_REG_SIZE_SHIFT)
& 3) * 8,
> > > > > + addr & IMX290_REG_ADDR_MASK,
ret);
> > > > >
> > > > > return ret;
> > > > >
> > > > > }
> > > > >
> > > > > - *value = regval & 0xff;
> > > > > -
> > > > > + *value = (data[2] << 16) | (data[1] << 8) | data[0];
> > > > >
> > > > > return 0;
> > > > >
> > > > > }
> > > > >
> > > > > -static int imx290_write_reg(struct imx290 *imx290, u16 addr, u8
> > > > > value)
> > > > > +static int imx290_write_reg(struct imx290 *imx290, u32 addr, u32
> > > > > value)
> > > > >
> > > > > {
> > > > >
> > > > > + u8 data[3] = { value & 0xff, (value >> 8) & 0xff, value
>> 16 };
> > > > >
> > > > > int ret;
> > > > >
> > > > > - ret = regmap_write(imx290->regmap, addr, value);
> > > > > - if (ret) {
> > > > > - dev_err(imx290->dev, "Failed to write
register 0x%04x: %d\n",
> > > > > - addr, ret);
> > > > > - return ret;
> > > > > - }
> > > > > + ret = regmap_raw_write(imx290->regmap, addr &
> > > > > IMX290_REG_ADDR_MASK,
> > > > > + data, (addr >>
IMX290_REG_SIZE_SHIFT) & 3);
> > > > > + if (ret < 0)
> > > > > + dev_err(imx290->dev, "%u-bit write to 0x%04x
failed: %d\n",
> > > > > + ((addr >> IMX290_REG_SIZE_SHIFT)
& 3) * 8,
> > > > > + addr & IMX290_REG_ADDR_MASK,
ret);
> > > > >
> > > > > return ret;
> > > > >
> > > > > }
next prev parent reply other threads:[~2022-07-21 11:44 UTC|newest]
Thread overview: 81+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-07-21 8:35 [PATCH 00/19] media: i2c: imx290: Miscellaneous improvements Laurent Pinchart
2022-07-21 8:35 ` [PATCH 01/19] media: i2c: imx290: Use device lock for the control handler Laurent Pinchart
2022-07-21 9:22 ` Alexander Stein
2022-07-21 8:35 ` [PATCH 02/19] media: i2c: imx290: Print error code when I2C transfer fails Laurent Pinchart
2022-07-21 9:23 ` Alexander Stein
2022-07-21 8:35 ` [PATCH 03/19] media: i2c: imx290: Specify HMAX values in decimal Laurent Pinchart
2022-07-21 9:18 ` Alexander Stein
2022-07-21 11:31 ` Laurent Pinchart
2022-07-21 11:54 ` Alexander Stein
2022-07-21 12:04 ` Laurent Pinchart
2022-07-21 8:35 ` [PATCH 04/19] media: i2c: imx290: Replace macro with explicit ARRAY_SIZE() Laurent Pinchart
2022-07-21 9:23 ` Alexander Stein
2022-07-21 8:35 ` [PATCH 05/19] media: i2c: imx290: Drop imx290_write_buffered_reg() Laurent Pinchart
2022-07-21 9:24 ` Alexander Stein
2022-07-21 8:35 ` [PATCH 06/19] media: i2c: imx290: Drop regmap cache Laurent Pinchart
2022-07-21 9:27 ` Alexander Stein
2022-07-21 8:35 ` [PATCH 07/19] media: i2c: imx290: Support variable-sized registers Laurent Pinchart
2022-07-21 9:43 ` Alexander Stein
2022-07-21 10:54 ` Laurent Pinchart
2022-07-21 11:18 ` Alexander Stein
2022-07-21 11:25 ` Laurent Pinchart
2022-07-21 11:43 ` Alexander Stein [this message]
2022-07-22 14:37 ` Sakari Ailus
2022-07-23 23:06 ` Laurent Pinchart
2022-07-25 6:49 ` Alexander Stein
2022-08-23 1:08 ` Laurent Pinchart
2022-08-23 2:51 ` Laurent Pinchart
2022-08-23 7:19 ` Alexander Stein
2022-10-16 5:36 ` Laurent Pinchart
2022-07-21 8:35 ` [PATCH 08/19] media: i2c: imx290: Correct register sizes Laurent Pinchart
2022-07-21 8:35 ` [PATCH 09/19] media: i2c: imx290: Simplify error handling when writing registers Laurent Pinchart
2022-07-21 9:50 ` Alexander Stein
2022-07-21 8:35 ` [PATCH 10/19] media: i2c: imx290: Define more register macros Laurent Pinchart
2022-07-21 10:00 ` Alexander Stein
2022-07-21 11:08 ` Laurent Pinchart
2022-07-21 11:28 ` Alexander Stein
2022-10-16 4:27 ` Laurent Pinchart
2022-07-21 8:35 ` [PATCH 11/19] media: i2c: imx290: Add exposure time control Laurent Pinchart
2022-07-21 10:01 ` Alexander Stein
2022-07-21 15:52 ` Dave Stevenson
2022-07-21 8:35 ` [PATCH 12/19] media: i2c: imx290: Fix max gain value Laurent Pinchart
2022-07-21 10:02 ` Alexander Stein
2022-07-21 16:08 ` Dave Stevenson
2022-10-16 4:51 ` Laurent Pinchart
2022-07-21 8:35 ` [PATCH 13/19] media: i2c: imx290: Split control initialization to separate function Laurent Pinchart
2022-07-21 10:03 ` Alexander Stein
2022-07-21 8:35 ` [PATCH 14/19] media: i2c: imx290: Implement HBLANK and VBLANK controls Laurent Pinchart
2022-07-21 10:05 ` Alexander Stein
2022-07-21 11:17 ` Laurent Pinchart
2022-07-21 11:32 ` Alexander Stein
2022-07-21 16:37 ` Dave Stevenson
2022-10-16 6:10 ` Laurent Pinchart
2022-10-17 13:46 ` Dave Stevenson
2022-07-21 8:35 ` [PATCH 15/19] media: i2c: imx290: Create controls for fwnode properties Laurent Pinchart
2022-07-21 10:06 ` Alexander Stein
2022-07-21 8:35 ` [PATCH 16/19] media: i2c: imx290: Move registers with fixed value to init array Laurent Pinchart
2022-07-21 10:08 ` Alexander Stein
2022-07-21 10:40 ` Laurent Pinchart
2022-07-21 11:08 ` Alexander Stein
2022-07-21 16:19 ` Dave Stevenson
2022-07-22 5:53 ` Alexander Stein
2022-07-22 9:10 ` Dave Stevenson
2022-07-21 8:35 ` [PATCH 17/19] media: i2c: imx290: Factor out format retrieval to separate function Laurent Pinchart
2022-07-21 10:11 ` Alexander Stein
2022-07-21 10:36 ` Laurent Pinchart
2022-07-21 11:12 ` Alexander Stein
2022-07-21 8:35 ` [PATCH 18/19] media: i2c: imx290: Add crop selection targets support Laurent Pinchart
2022-07-21 15:39 ` Dave Stevenson
2022-10-16 5:53 ` Laurent Pinchart
2022-07-21 8:35 ` [PATCH 19/19] media: i2c: imx290: Replace GAIN control with ANALOGUE_GAIN Laurent Pinchart
2022-07-21 10:11 ` Alexander Stein
2022-08-23 1:11 ` [PATCH 00/19] media: i2c: imx290: Miscellaneous improvements Laurent Pinchart
2022-10-10 10:31 ` Dave Stevenson
2022-10-16 5:37 ` Laurent Pinchart
2022-10-16 7:34 ` Dave Stevenson
2022-10-17 18:07 ` Dave Stevenson
2022-10-18 13:43 ` Dave Stevenson
2022-10-19 10:33 ` Sakari Ailus
2022-10-19 11:38 ` Dave Stevenson
2022-10-19 13:27 ` Sakari Ailus
2023-01-14 16:03 ` Laurent Pinchart
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8007753.G0QQBjFxQf@steina-w \
--to=alexander.stein@ew.tq-group.com \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-media@vger.kernel.org \
--cc=mani@kernel.org \
--cc=sakari.ailus@iki.fi \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.