Changes in v3:
- Add __always_inline attribute for quad8_control_register_update()
Changes in v2:
- Drop FIELD_MODIFY() macro introduction; u8p_replace_bits() is
utilized instead for the same purpose
- Replace FIELD_PREP() and FIELD_GET() with u8_encode_bits() and
u8_get_bits()
- Replace FIELD_MODIFY() with u8p_replace_bits()
- Wrap up control register update in quad8_control_register_update()
- Utilize ioread8_rep() and iowrite8_rep() to read and write counter
data
The 104-quad-8 driver was initially introduced to the IIO subsystem
where it didn't quite fit with the existing paradigm [0]; these
differences eventually led to the creation of the Counter subsystem[1].
As a result of its awkward beginnings, the design of the 104-quad-8
driver was structured around maintaining abstract state buffers that
would eventually be converted to match the actual device registers
states on-the-fly as needed.
The original design approach for the 104-quad-8 driver was neither
efficient nor easy to troubleshoot, but it did allow us to focus on
implementing and supporting necessary APIs for the nascent Counter
subsystem. Now that development for the 104-quad-8 driver has shifted
to maintenance, it is a good time to refactor and clean up the code to
match closer to what is actually happening on the device. This patchset
is an attempt to rectify the situation as such.
The primary change is a transition from maintaining individual
configuration states independently, to storing buffers of the device
register configurations. To that end, the bitfield API is leveraged to
access and retrieve field states. Some helper functions are introduced
as well to abstract the handling of the PR, FLAG, PSC, and control
registers.
[0] https://lore.kernel.org/r/b43e2942b763b87afc85bfa9fe36e5695cba4c44.1475079578.git.vilhelm.gray@gmail.com/
[1] https://lore.kernel.org/r/[email protected]/
William Breathitt Gray (3):
counter: 104-quad-8: Utilize bitfield access macros
counter: 104-quad-8: Refactor to buffer states for CMR, IOR, and IDR
counter: 104-quad-8: Utilize helper functions to handle PR, FLAG and
PSC
drivers/counter/104-quad-8.c | 537 +++++++++++++++++------------------
1 file changed, 258 insertions(+), 279 deletions(-)
base-commit: 09a9639e56c01c7a00d6c0ca63f4c7c41abe075d
--
2.39.2
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 <[email protected]>
---
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(-)
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) |
--
2.39.2
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];
--
With Best Regards,
Andy Shevchenko
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/[email protected]/
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
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