Hi Hans,
On Tue, Aug 15, 2023 at 04:15:39PM +0200, Hans de Goede wrote:
> On 8/15/23 15:15, Laurent Pinchart wrote:
> > On Tue, Jun 27, 2023 at 02:51:06PM +0200, Hans de Goede wrote:
> >> Use the new comon CCI register access helpers to replace the private
> >> register access helpers in the imx290 driver.
> >>
> >> Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> >> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> >> ---
> >> Note:
> >> 1. This is untested
> >> 2. For reviewers: all the IMX290_REG_?BIT defines in both the register
> >> address defines as well as in various reg-sequences were automatically
> >> changed using search replace.
> >> ---
> >> Changes in v3:
> >> - Fixed a couple of lines > 80 chars
> >>
> >> Changes in v2:
> >> - New patch in v2 of this series
> >> ---
> >> drivers/media/i2c/Kconfig | 1 +
> >> drivers/media/i2c/imx290.c | 360 +++++++++++++++----------------------
> >> 2 files changed, 150 insertions(+), 211 deletions(-)
> >
> > [snip]
> >
> >> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> >> index b3f832e9d7e1..e78c7b91ae72 100644
> >> --- a/drivers/media/i2c/imx290.c
> >> +++ b/drivers/media/i2c/imx290.c
> >> @@ -21,91 +21,86 @@
> >
> > [snip]
> >
> >> @@ -615,63 +605,15 @@ imx290_format_info(const struct imx290 *imx290, u32 code)
> >> return NULL;
> >> }
> >>
> >> -/* -----------------------------------------------------------------------------
> >> - * Register access
> >> - */
> >> -
> >> -static int __always_unused imx290_read(struct imx290 *imx290, u32 addr, u32 *value)
> >> -{
> >> - u8 data[3] = { 0, 0, 0 };
> >> - int ret;
> >> -
> >> - ret = regmap_raw_read(imx290->regmap, addr & IMX290_REG_ADDR_MASK,
> >> - data, (addr >> IMX290_REG_SIZE_SHIFT) & 3);
> >> - 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 = get_unaligned_le24(data);
> >> - return 0;
> >> -}
> >> -
> >> -static int imx290_write(struct imx290 *imx290, u32 addr, u32 value, int *err)
> >> -{
> >> - u8 data[3];
> >> - int ret;
> >> -
> >> - if (err && *err)
> >> - return *err;
> >> -
> >> - put_unaligned_le24(value, data);
> >
> > We seem to be having a problem here, as the CCI helpers unconditionally
> > use big endian for the data :-(
>
> That is because that is what the specification says, from the MIPI CSI spec:
>
> """
> 6.3.2 The Transmission Byte Order for Multi-byte Register Values
>
> This is a normative section.
>
> The first byte of a CCI message is always the MS byte of a multi-byte
> register and the last byte is always the LS byte.
> """
>
> So it seems that the IMX sensors are special here and it might be best
> to just revert the conversion to the CCI helpers?
I'm fine with reverting for v6.6, as we're close to the release of v6.5.
> Alternative would be to make devm_cci_regmap_init_i2c() return a newly
> allocated struct which contains both a struct regmap * and a long
> flags and make the helpers take a pointer to that struct, combined
> with adding an endianess flag to the flags member.
I think I like the idea, it will probably help adding support for
various device-specific quirks in the future.
> >> -
> >> - 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);
> >> - if (err)
> >> - *err = ret;
> >> - }
> >> -
> >> - return ret;
> >> -}
> >> -
--
Regards,
Laurent Pinchart
