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. > > > 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); > > > > spin_unlock_irqrestore(&priv->lock, irqflags); > > ... > > > + struct channel_reg __iomem *const chan = priv->reg->channel + id; > > Not sure if array representation will look better here and elsewhere. > > struct channel_reg __iomem *const chan = &priv->reg->channel[id]; Perhaps so, but all these struct channel_reg lines will go away in the next patch [0] migrating to the regmap API, so for the sake of stability of this patch I hesitate to change these lines. William Breathitt Gray [0] https://lore.kernel.org/all/20230410141252.143998-1-william.gray@xxxxxxxxxx/
Attachment:
signature.asc
Description: PGP signature
