Re: [PATCH 1/3] media: Add MIPI CCI register access helper functions

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 6/7/23 17:40, Andy Shevchenko wrote:
> On Wed, Jun 7, 2023 at 3:01 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
>> On Wed, Jun 07, 2023 at 10:40:34AM +0200, Hans de Goede wrote:
>>> On 6/6/23 22:43, Andy Shevchenko wrote:
>>>> On Tue, Jun 6, 2023 at 7:58 PM Hans de Goede <hdegoede@redhat.com> wrote:
> 
> ...
> 
>>>>> +int cci_read(struct regmap *map, u32 reg, u32 *val, int *err)
>>>>> +{
>>>>> +       int i, len, ret;
>>>>> +       u8 buf[4];
> 
>>>>> +       *val = 0;
>>>>> +       for (i = 0; i < len; i++) {
>>>>> +               *val <<= 8;
>>>>> +               *val |= buf[i];
>>>>> +       }
>>>>
>>>> I really prefer to see put_unaligned() here depending on the length.
>>>> Note, that on some CPUs it might be one assembly instruction or even
>>>> none, depending on how the result is going to be used.
>>>
>>> Ok, so you mean changing it to something like this:
>>>
>>>       switch (len)
>>>       case 1:
>>>               *val = buf[0];
>>>               break;
>>>       case 2:
>>>               *val = get_unaligned_be16(buf);
>>>               break;
>>>       case 3:
>>>               *val = __get_unaligned_be24(buf);
> 
> __without double underscore prefix

include/asm-generic/unaligned.h

defines __get_unaligned_be24() and not get_unaligned_be24(), I guess because 24bit is not a standard register width.



> 
>>>               break;
>>>       case 4:
>>>               *val = get_unaligned_be32(buf);
>>>               break;
>>>       }
>>
>> I think the loop looks nicer but I'm fine with this as well.
>>
>>> ?
> 
> But the loop hides what's going on there. And I believe code
> generation would be worse with a loop.
> Also note, that in case of switch-case we don't write to the pointed
> memory several times, which I think is also the win.

I understand, unless someone objects I'll move to the switch-case
approach for v2.

Regards,

Hans



> 
>>>>> +       return 0;
>>>>> +}
> 
> ...
> 
>>>>> +int cci_write(struct regmap *map, u32 reg, u32 val, int *err)
>>>>> +{
>>>>> +       int i, len, ret;
>>>>> +       u8 buf[4];
>>>>> +
>>>>> +       if (err && *err)
>>>>> +               return *err;
>>>>> +
>>>>> +       /* Set len to register width in bytes */
>>>>> +       len = ((reg & CCI_REG_WIDTH_MASK) >> CCI_REG_WIDTH_SHIFT) + 1;
>>>>> +       reg &= CCI_REG_ADDR_MASK;
>>>>> +
>>>>> +       for (i = 0; i < len; i++) {
>>>>> +               buf[len - i - 1] = val & 0xff;
>>>>> +               val >>= 8;
>>>>> +       }
> 
> Similar way here.
> 
>>>>> +
>>>>> +       ret = regmap_bulk_write(map, reg, buf, len);
>>>>> +       if (ret) {
>>>>> +               dev_err(regmap_get_device(map), "Error writing reg 0x%4x: %d\n", reg, ret);
>>>>> +               if (err)
>>>>> +                       *err = ret;
>>>>> +       }
>>>>> +
>>>>> +       return ret;
>>>>> +}
>