On Tue, Apr 11, 2023 at 10:05:25AM -0400, William Breathitt Gray wrote: > On Tue, Apr 11, 2023 at 04:50:03PM +0300, Andy Shevchenko wrote: > > On Mon, Apr 10, 2023 at 10:03:13AM -0400, William Breathitt Gray wrote: > > > The Preset Register (PR), Flag Register (FLAG), and Filter Clock > > > Prescaler (PSC) have common usage patterns. Wrap up such usage into > > > dedicated functions to improve code clarity. ... > > > *val = 0; > > > > Is not needed now as always being initialized by below call. > > The regmap_noinc_read() call only reads the number of bytes requested. > Since we request 3 bytes, the upper bytes of the u64 val remain > uninitialized, so that is why we need to set *val = 0. This isn't > immediately clear in the code, so I can add a comment to make it > explicit. Hmm... Since we are using byte array for the value, can we actually use memset() to show that explicitly? Perhaps in that way it will be more visible? > > > spin_lock_irqsave(&priv->lock, irqflags); > > > > > > iowrite8(SELECT_RLD | RESET_BP | TRANSFER_CNTR_TO_OL, &chan->control); > > > - > > > - for (i = 0; i < 3; i++) > > > - *val |= (unsigned long)ioread8(&chan->data) << (8 * i); > > > + ioread8_rep(&chan->data, val, 3); But hold on, wouldn't this have an endianess issue? The call fills in LE, while here you use the CPU order. > > > spin_unlock_irqrestore(&priv->lock, irqflags); That said, I think you should have something like u8 value[3]; ioread8_rep(..., value, sizeof(value)); *val = get_unaligned_le24(value); -- With Best Regards, Andy Shevchenko
