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; >>>>> +} >