Hi,
On 6/7/23 17:40, Andy Shevchenko wrote:
> On Wed, Jun 7, 2023 at 3:01 PM Sakari Ailus
> <sakari.ailus@xxxxxxxxxxxxxxx> 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@xxxxxxxxxx> 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;
>>>>> +}
>
