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
> > 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.
> > >> + 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;
> > >> +}
--
With Best Regards,
Andy Shevchenko
