Re: [PATCH v2 1/2] media: v4l2-cci: Add support for little-endian encoded registers

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On 11/2/23 09:25, Sakari Ailus wrote:
> Hi Alexander,
> 
> On Thu, Nov 02, 2023 at 08:51:12AM +0100, Alexander Stein wrote:
>> Hi,
>>
>> thanks for the feedback.
>>
>> Am Donnerstag, 2. November 2023, 07:30:44 CET schrieb Sakari Ailus:
>>> Hi Laurent,
>>>
>>> On Thu, Nov 02, 2023 at 03:22:17AM +0200, Laurent Pinchart wrote:
>>>> Hi Alexander,
>>>>
>>>> Thank you for the patch.
>>>>
>>>> On Wed, Nov 01, 2023 at 01:23:53PM +0100, Alexander Stein wrote:
>>>>> Some sensors, e.g. Sony, are using little-endian registers. Add support
>>>>> for
>>>>
>>>> I would write Sony IMX290 here, as there are Sony sensors that use big
>>>> endian.
>>>
>>> Almost all of them. There are a few exceptions indeed. This seems to be a
>>> bug.
>>
>> Let's name IMX290 here as an actual example. No need to worry about other 
>> models here.
>>
>>>>> those by encoding the endianess into Bit 20 of the register address.
>>>>>
>>>>> Fixes: af73323b97702 ("media: imx290: Convert to new CCI register access
>>>>> helpers") Cc: stable@xxxxxxxxxxxxxxx
>>>>> Signed-off-by: Alexander Stein <alexander.stein@xxxxxxxxxxxxxxx>
>>>>> ---
>>>>>
>>>>>  drivers/media/v4l2-core/v4l2-cci.c | 44 ++++++++++++++++++++++++------
>>>>>  include/media/v4l2-cci.h           |  5 ++++
>>>>>  2 files changed, 41 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/drivers/media/v4l2-core/v4l2-cci.c
>>>>> b/drivers/media/v4l2-core/v4l2-cci.c index bc2dbec019b04..673637b67bf67
>>>>> 100644
>>>>> --- a/drivers/media/v4l2-core/v4l2-cci.c
>>>>> +++ b/drivers/media/v4l2-core/v4l2-cci.c
>>>>> @@ -18,6 +18,7 @@
>>>>>
>>>>>  int cci_read(struct regmap *map, u32 reg, u64 *val, int *err)
>>>>>  {
>>>>>
>>>>> +	bool little_endian;
>>>>>
>>>>>  	unsigned int len;
>>>>>  	u8 buf[8];
>>>>>  	int ret;
>>>>>
>>>>> @@ -25,6 +26,7 @@ int cci_read(struct regmap *map, u32 reg, u64 *val,
>>>>> int *err)> > 
>>>>>  	if (err && *err)
>>>>>  	
>>>>>  		return *err;
>>>>>
>>>>> +	little_endian = reg & CCI_REG_LE;
>>>>
>>>> You could initialize the variable when declaring it. Same below.
>>>
>>> I was thinking of the same, but then it'd be logical to move initialisation
>>> of all related variables there. reg is modified here though. I'd keep
>>> setting little_endian here. If someone wants to move it, that could be done
>>> in a separate patch.
>>>
>>>>>  	len = FIELD_GET(CCI_REG_WIDTH_MASK, reg);
>>>>>  	reg = FIELD_GET(CCI_REG_ADDR_MASK, reg);
>>>>>
>>>>> @@ -40,16 +42,28 @@ int cci_read(struct regmap *map, u32 reg, u64 *val,
>>>>> int *err)> > 
>>>>>  		*val = buf[0];
>>>>>  		break;
>>>>>  	
>>>>>  	case 2:
>>>>> -		*val = get_unaligned_be16(buf);
>>>>> +		if (little_endian)
>>>>> +			*val = get_unaligned_le16(buf);
>>>>> +		else
>>>>> +			*val = get_unaligned_be16(buf);
>>>>
>>>> Unrelated to this patch, isn't buf aligned to a 4 bytes boundary ?
>>>
>>> Very probably, as it's right after len that's an unsigned int. Adding
>>> __aligned(8) would ensure we don't need any of the unaligned variants, and
>>> most likely would keep the stack layout as-is.
>>
>> You mean something like this?
>>
>> u8 __aligned(8) buf[8];
>> [...]
>> if (little_endian)
>> 	*val = le64_to_cpup(buf);
>> else
>> 	*val = be64_to_cpup(buf);
>>
>> But what about 24 Bits? There is no le24_to_cpup. I would rather use the same 
>> API for all cases.
> 
> The aligned APIs are much better choice when you can use them. The 24 bit
> case can remain special IMO.
> 
>>
>>> Or... how about putting it in an union with a u64? That would mean it's
>>> accessible as u64 alignment-wise while the alignment itself is up to the
>>> ABI. A comment would be good to have probably.
>>
>> An additional union seems a bit too much here. Unless something suitable 
>> already exists for general usage.
> 
> I think it's nicer than using __aligned() as you get ABI alignment that
> way, not something you force manually --- that's a bit crude.
> 
> I wonder that others think.

I'm fine with adding the __aligned(8) and switching the non 24 bit
cases to helpers which assume alignment. The most important note
I have is that that is a separate improvement from this series though.

So this should be done in a follow-up patch which is not Cc: stable .

Regards,

Hans





[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux