On Tue, Apr 11, 2023 at 05:43:49PM +0300, Andy Shevchenko wrote: > 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 Yes, I think you're right, that solves both problems at once so I'll use get_aligned_le24() to set *val. William Breathitt Gray
Attachment:
signature.asc
Description: PGP signature
