diff mbox series

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

Message ID 71496f9295e68388ce07f3051bf5882177be83c5.1679149543.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 March 18, 2023, 2:59 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>
---
 drivers/counter/104-quad-8.c | 103 +++++++++++++++--------------------
 1 file changed, 45 insertions(+), 58 deletions(-)

Comments

Andy Shevchenko March 20, 2023, 12:28 p.m. UTC | #1 
On Sat, Mar 18, 2023 at 10:59:51AM -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.

...

> +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;
> +	int i;
> +
> +	/* Reset Byte Pointer */
> +	iowrite8(SELECT_RLD | RESET_BP, &chan->control);
> +
> +	/* Set Preset Register */
> +	for (i = 0; i < 3; i++)
> +		iowrite8(preset >> (8 * i), &chan->data);
> +}

May we add generic __iowrite8_copy() / __ioread8_copy() instead?

It seems that even current __ioread32_copy() and __iowrite32_copy() has to
be amended to support IO.
William Breathitt Gray March 20, 2023, 3:31 p.m. UTC | #2 
On Mon, Mar 20, 2023 at 02:28:31PM +0200, Andy Shevchenko wrote:
> On Sat, Mar 18, 2023 at 10:59:51AM -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.
> 
> ...
> 
> > +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;
> > +	int i;
> > +
> > +	/* Reset Byte Pointer */
> > +	iowrite8(SELECT_RLD | RESET_BP, &chan->control);
> > +
> > +	/* Set Preset Register */
> > +	for (i = 0; i < 3; i++)
> > +		iowrite8(preset >> (8 * i), &chan->data);
> > +}
> 
> May we add generic __iowrite8_copy() / __ioread8_copy() instead?
> 
> It seems that even current __ioread32_copy() and __iowrite32_copy() has to
> be amended to support IO.
> 
> -- 
> With Best Regards,
> Andy Shevchenko

Sure, I would use __iowrite8_copy() / __ioread8_copy() for these
situations if it were available.

Is something equivalent available for the regmap API? I'm planning to
migrate this driver to the regmap API soon after this patch series is
merged, so the *_copy() calls would need to migrated as well.

William Breathitt Gray
Andy Shevchenko March 20, 2023, 3:36 p.m. UTC | #3 
On Mon, Mar 20, 2023 at 11:31:07AM -0400, William Breathitt Gray wrote:
> On Mon, Mar 20, 2023 at 02:28:31PM +0200, Andy Shevchenko wrote:
> > On Sat, Mar 18, 2023 at 10:59:51AM -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.

...

> > > +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;
> > > +	int i;
> > > +
> > > +	/* Reset Byte Pointer */
> > > +	iowrite8(SELECT_RLD | RESET_BP, &chan->control);
> > > +
> > > +	/* Set Preset Register */
> > > +	for (i = 0; i < 3; i++)
> > > +		iowrite8(preset >> (8 * i), &chan->data);
> > > +}
> > 
> > May we add generic __iowrite8_copy() / __ioread8_copy() instead?
> > 
> > It seems that even current __ioread32_copy() and __iowrite32_copy() has to
> > be amended to support IO.

> Sure, I would use __iowrite8_copy() / __ioread8_copy() for these
> situations if it were available.

If needed, you may always introduce ones.

> Is something equivalent available for the regmap API? I'm planning to
> migrate this driver to the regmap API soon after this patch series is
> merged, so the *_copy() calls would need to migrated as well.

Yes. It's regmap bulk operations.
William Breathitt Gray March 20, 2023, 3:53 p.m. UTC | #4 
On Mon, Mar 20, 2023 at 05:36:17PM +0200, Andy Shevchenko wrote:
> On Mon, Mar 20, 2023 at 11:31:07AM -0400, William Breathitt Gray wrote:
> > On Mon, Mar 20, 2023 at 02:28:31PM +0200, Andy Shevchenko wrote:
> > > On Sat, Mar 18, 2023 at 10:59:51AM -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.
> 
> ...
> 
> > > > +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;
> > > > +	int i;
> > > > +
> > > > +	/* Reset Byte Pointer */
> > > > +	iowrite8(SELECT_RLD | RESET_BP, &chan->control);
> > > > +
> > > > +	/* Set Preset Register */
> > > > +	for (i = 0; i < 3; i++)
> > > > +		iowrite8(preset >> (8 * i), &chan->data);
> > > > +}
> > > 
> > > May we add generic __iowrite8_copy() / __ioread8_copy() instead?
> > > 
> > > It seems that even current __ioread32_copy() and __iowrite32_copy() has to
> > > be amended to support IO.
> 
> > Sure, I would use __iowrite8_copy() / __ioread8_copy() for these
> > situations if it were available.
> 
> If needed, you may always introduce ones.
> 
> > Is something equivalent available for the regmap API? I'm planning to
> > migrate this driver to the regmap API soon after this patch series is
> > merged, so the *_copy() calls would need to migrated as well.
> 
> Yes. It's regmap bulk operations.
> 
> -- 
> With Best Regards,
> Andy Shevchenko

After reading through the implementation for these functions I realized
they are actually doing something different than what's happening here.
The 104-QUAD-8 device exposes the 24-bit register by consecutive 8-bit
I/O operations on the same address; however, the iomap_copy and regmap
bulk functions operate on different addresses.

I'm not sure if there really is a way to make the 104-QUAD-8 operation
more generic for other drivers because it configures the current byte
pointer through a separate register from the data register (all of this
feel rather device specific), so I suspect keeping this function local
to 104-quad-8 is best for now.

William Breathitt Gray
Andy Shevchenko March 20, 2023, 4:54 p.m. UTC | #5 
On Mon, Mar 20, 2023 at 11:53:36AM -0400, William Breathitt Gray wrote:
> On Mon, Mar 20, 2023 at 05:36:17PM +0200, Andy Shevchenko wrote:

...

> After reading through the implementation for these functions I realized
> they are actually doing something different than what's happening here.
> The 104-QUAD-8 device exposes the 24-bit register by consecutive 8-bit
> I/O operations on the same address; however, the iomap_copy and regmap
> bulk functions operate on different addresses.

Ah, than we have ioreadXX_rep()/iowriteXX_rep() for that.

> I'm not sure if there really is a way to make the 104-QUAD-8 operation
> more generic for other drivers because it configures the current byte
> pointer through a separate register from the data register (all of this
> feel rather device specific), so I suspect keeping this function local
> to 104-quad-8 is best for now.
diff mbox series

Patch

diff --git a/drivers/counter/104-quad-8.c b/drivers/counter/104-quad-8.c
index 796f02fc53b8..27ec905ebe85 100644
--- a/drivers/counter/104-quad-8.c
+++ b/drivers/counter/104-quad-8.c
@@ -241,41 +241,49 @@  static int quad8_count_read(struct counter_device *counter,
 	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;
+	int i;
+
+	/* Reset Byte Pointer */
+	iowrite8(SELECT_RLD | RESET_BP, &chan->control);
+
+	/* Set Preset Register */
+	for (i = 0; i < 3; i++)
+		iowrite8(preset >> (8 * i), &chan->data);
+}
+
+static void quad8_flag_register_reset(struct quad8 *const priv, const size_t id)
+{
+	struct channel_reg __iomem *const chan = priv->reg->channel + id;
+
+	/* Reset Borrow, Carry, Compare, and Sign flags */
+	iowrite8(SELECT_RLD | RESET_BT_CT_CPT_S_IDX, &chan->control);
+	/* Reset Error flag */
+	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);
 
-	/* Reset Byte Pointer */
-	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);
 	/* Transfer Preset Register to Counter */
 	iowrite8(SELECT_RLD | TRANSFER_PR_TO_CNTR, &chan->control);
-
-	/* Reset Byte Pointer */
-	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);
-
-	/* Reset Borrow, Carry, Compare, and Sign flags */
-	iowrite8(SELECT_RLD | RESET_BT_CT_CPT_S_IDX, &chan->control);
-	/* Reset Error flag */
-	iowrite8(SELECT_RLD | RESET_E, &chan->control);
+	quad8_preset_register_set(priv, count->id, priv->preset[count->id]);
 
 	spin_unlock_irqrestore(&priv->lock, irqflags);
 
@@ -791,22 +799,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;
-
-	/* Reset Byte Pointer */
-	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)
 {
@@ -818,6 +810,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);
@@ -864,6 +857,7 @@  static int quad8_count_ceiling_write(struct counter_device *counter,
 	switch (FIELD_GET(COUNT_MODE, priv->cmr[count->id])) {
 	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;
@@ -985,25 +979,30 @@  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;
+
+	/* Reset Byte Pointer */
+	iowrite8(SELECT_RLD | RESET_BP, &chan->control);
+	/* Reset filter clock factor */
+	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;
-
-	/* Reset Byte Pointer */
-	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);
 
@@ -1203,22 +1202,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;
 
-	/* Reset Byte Pointer */
-	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);
-	/* Reset Byte Pointer */
-	iowrite8(SELECT_RLD | RESET_BP, &chan->control);
-	/* Reset Preset Register */
-	for (i = 0; i < 3; i++)
-		iowrite8(0x00, &chan->data);
-	/* Reset Borrow, Carry, Compare, and Sign flags */
-	iowrite8(SELECT_RLD | RESET_BT_CT_CPT_S_IDX, &chan->control);
-	/* Reset Error flag */
-	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 | FIELD_PREP(COUNT_MODE, NORMAL_COUNT) |