Hi,
On 8/15/23 15:15, Laurent Pinchart wrote:
> Hi Hans,
>
> 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?
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.
Regards,
Hans
>
>> -
>> - 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;
>> -}
>> -
>
> [snip]
>
