diff mbox series

[v3,3/3] counter: 104-quad-8: Utilize helper functions to handle PR, FLAG and PSC

Message ID 669c8f782f11fe27c4568e4fc3ba459c4f954874.1681134558.git.william.gray@linaro.org (mailing list archive)
State Handled Elsewhere
Headers show
Series Refactor 104-quad-8 to match device operations | expand

Commit Message

William Breathitt Gray April 10, 2023, 2:03 p.m. UTC 
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.

Signed-off-by: William Breathitt Gray <william.gray@linaro.org>
---
Changes in v3: none
Changes in v2:
 - Utilize ioread8_rep() and iowrite8_rep() to read and write counter
   data

 drivers/counter/104-quad-8.c | 87 +++++++++++++++---------------------
 1 file changed, 37 insertions(+), 50 deletions(-)

Comments

Andy Shevchenko April 11, 2023, 1:50 p.m. UTC | #1 
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.

>  	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];
William Breathitt Gray April 11, 2023, 2:05 p.m. UTC | #2 
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@linaro.org/
Andy Shevchenko April 11, 2023, 2:43 p.m. UTC | #3 
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);
William Breathitt Gray April 11, 2023, 2:58 p.m. UTC | #4 
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
diff mbox series

Patch

diff --git a/drivers/counter/104-quad-8.c b/drivers/counter/104-quad-8.c
index 0188c9c4e4cb..c171d0a80ef9 100644
--- a/drivers/counter/104-quad-8.c
+++ b/drivers/counter/104-quad-8.c
@@ -232,52 +232,56 @@  static int quad8_count_read(struct counter_device *counter,
 	struct quad8 *const priv = counter_priv(counter);
 	struct channel_reg __iomem *const chan = priv->reg->channel + count->id;
 	unsigned long irqflags;
-	int i;
 
 	*val = 0;
 
 	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);
 
 	return 0;
 }
 
+static void quad8_preset_register_set(struct quad8 *const priv, const size_t id,
+				      const unsigned long preset)
+{
+	struct channel_reg __iomem *const chan = priv->reg->channel + id;
+
+	iowrite8(SELECT_RLD | RESET_BP, &chan->control);
+	iowrite8_rep(&chan->data, &preset, 3);
+}
+
+static void quad8_flag_register_reset(struct quad8 *const priv, const size_t id)
+{
+	struct channel_reg __iomem *const chan = priv->reg->channel + id;
+
+	iowrite8(SELECT_RLD | RESET_BT_CT_CPT_S_IDX, &chan->control);
+	iowrite8(SELECT_RLD | RESET_E, &chan->control);
+}
+
 static int quad8_count_write(struct counter_device *counter,
 			     struct counter_count *count, u64 val)
 {
 	struct quad8 *const priv = counter_priv(counter);
 	struct channel_reg __iomem *const chan = priv->reg->channel + count->id;
 	unsigned long irqflags;
-	int i;
 
 	if (val > LS7267_CNTR_MAX)
 		return -ERANGE;
 
 	spin_lock_irqsave(&priv->lock, irqflags);
 
-	iowrite8(SELECT_RLD | RESET_BP, &chan->control);
-
 	/* Counter can only be set via Preset Register */
-	for (i = 0; i < 3; i++)
-		iowrite8(val >> (8 * i), &chan->data);
-
+	quad8_preset_register_set(priv, count->id, val);
 	iowrite8(SELECT_RLD | TRANSFER_PR_TO_CNTR, &chan->control);
 
-	iowrite8(SELECT_RLD | RESET_BP, &chan->control);
+	quad8_flag_register_reset(priv, count->id);
 
 	/* Set Preset Register back to original value */
-	val = priv->preset[count->id];
-	for (i = 0; i < 3; i++)
-		iowrite8(val >> (8 * i), &chan->data);
-
-	iowrite8(SELECT_RLD | RESET_BT_CT_CPT_S_IDX, &chan->control);
-	iowrite8(SELECT_RLD | RESET_E, &chan->control);
+	quad8_preset_register_set(priv, count->id, priv->preset[count->id]);
 
 	spin_unlock_irqrestore(&priv->lock, irqflags);
 
@@ -771,21 +775,6 @@  static int quad8_count_preset_read(struct counter_device *counter,
 	return 0;
 }
 
-static void quad8_preset_register_set(struct quad8 *const priv, const int id,
-				      const unsigned int preset)
-{
-	struct channel_reg __iomem *const chan = priv->reg->channel + id;
-	int i;
-
-	priv->preset[id] = preset;
-
-	iowrite8(SELECT_RLD | RESET_BP, &chan->control);
-
-	/* Set Preset Register */
-	for (i = 0; i < 3; i++)
-		iowrite8(preset >> (8 * i), &chan->data);
-}
-
 static int quad8_count_preset_write(struct counter_device *counter,
 				    struct counter_count *count, u64 preset)
 {
@@ -797,6 +786,7 @@  static int quad8_count_preset_write(struct counter_device *counter,
 
 	spin_lock_irqsave(&priv->lock, irqflags);
 
+	priv->preset[count->id] = preset;
 	quad8_preset_register_set(priv, count->id, preset);
 
 	spin_unlock_irqrestore(&priv->lock, irqflags);
@@ -843,6 +833,7 @@  static int quad8_count_ceiling_write(struct counter_device *counter,
 	switch (u8_get_bits(priv->cmr[count->id], COUNT_MODE)) {
 	case RANGE_LIMIT:
 	case MODULO_N:
+		priv->preset[count->id] = ceiling;
 		quad8_preset_register_set(priv, count->id, ceiling);
 		spin_unlock_irqrestore(&priv->lock, irqflags);
 		return 0;
@@ -961,24 +952,28 @@  static int quad8_signal_fck_prescaler_read(struct counter_device *counter,
 	return 0;
 }
 
+static void quad8_filter_clock_prescaler_set(struct quad8 *const priv, const size_t id,
+					     const u8 prescaler)
+{
+	struct channel_reg __iomem *const chan = priv->reg->channel + id;
+
+	iowrite8(SELECT_RLD | RESET_BP, &chan->control);
+	iowrite8(prescaler, &chan->data);
+	iowrite8(SELECT_RLD | TRANSFER_PR0_TO_PSC, &chan->control);
+}
+
 static int quad8_signal_fck_prescaler_write(struct counter_device *counter,
 					    struct counter_signal *signal,
 					    u8 prescaler)
 {
 	struct quad8 *const priv = counter_priv(counter);
 	const size_t channel_id = signal->id / 2;
-	struct channel_reg __iomem *const chan = priv->reg->channel + channel_id;
 	unsigned long irqflags;
 
 	spin_lock_irqsave(&priv->lock, irqflags);
 
 	priv->fck_prescaler[channel_id] = prescaler;
-
-	iowrite8(SELECT_RLD | RESET_BP, &chan->control);
-
-	/* Set filter clock factor */
-	iowrite8(prescaler, &chan->data);
-	iowrite8(SELECT_RLD | RESET_BP | TRANSFER_PR0_TO_PSC, &chan->control);
+	quad8_filter_clock_prescaler_set(priv, channel_id, prescaler);
 
 	spin_unlock_irqrestore(&priv->lock, irqflags);
 
@@ -1178,18 +1173,10 @@  static irqreturn_t quad8_irq_handler(int irq, void *private)
 static void quad8_init_counter(struct quad8 *const priv, const size_t channel)
 {
 	struct channel_reg __iomem *const chan = priv->reg->channel + channel;
-	unsigned long i;
 
-	iowrite8(SELECT_RLD | RESET_BP, &chan->control);
-	/* Reset filter clock factor */
-	iowrite8(0, &chan->data);
-	iowrite8(SELECT_RLD | RESET_BP | TRANSFER_PR0_TO_PSC, &chan->control);
-	iowrite8(SELECT_RLD | RESET_BP, &chan->control);
-	/* Reset Preset Register */
-	for (i = 0; i < 3; i++)
-		iowrite8(0x00, &chan->data);
-	iowrite8(SELECT_RLD | RESET_BT_CT_CPT_S_IDX, &chan->control);
-	iowrite8(SELECT_RLD | RESET_E, &chan->control);
+	quad8_filter_clock_prescaler_set(priv, channel, 0);
+	quad8_preset_register_set(priv, channel, 0);
+	quad8_flag_register_reset(priv, channel);
 
 	/* Binary encoding; Normal count; non-quadrature mode */
 	priv->cmr[channel] = SELECT_CMR | BINARY | u8_encode_bits(NORMAL_COUNT, COUNT_MODE) |