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 | 536 +++++++++++++++++------------------
1 file changed, 257 insertions(+), 279 deletions(-)
base-commit: 00f4bc5184c19cb33f468f1ea409d70d19f8f502
--
2.39.2
The 104-QUAD-8 features several registers with various bitfields.
Utilize bitfield access macros such as u8_get_bits() and
u8_encode_bits() to make the code easier to read and the intent clearer.
Suggested-by: Jonathan Cameron <[email protected]>
Signed-off-by: William Breathitt Gray <[email protected]>
---
Changes in v2:
- Replace FIELD_PREP() and FIELD_GET() with u8_encode_bits() and
u8_get_bits()
drivers/counter/104-quad-8.c | 274 ++++++++++++++++++++++-------------
1 file changed, 172 insertions(+), 102 deletions(-)
diff --git a/drivers/counter/104-quad-8.c b/drivers/counter/104-quad-8.c
index d9cb937665cf..f07e4a9b946d 100644
--- a/drivers/counter/104-quad-8.c
+++ b/drivers/counter/104-quad-8.c
@@ -5,7 +5,8 @@
*
* This driver supports the ACCES 104-QUAD-8 and ACCES 104-QUAD-4.
*/
-#include <linux/bitops.h>
+#include <linux/bitfield.h>
+#include <linux/bits.h>
#include <linux/counter.h>
#include <linux/device.h>
#include <linux/errno.h>
@@ -98,36 +99,108 @@ struct quad8 {
};
/* Error flag */
-#define QUAD8_FLAG_E BIT(4)
+#define FLAG_E BIT(4)
/* Up/Down flag */
-#define QUAD8_FLAG_UD BIT(5)
+#define FLAG_UD BIT(5)
+
+#define REGISTER_SELECTION GENMASK(6, 5)
+
/* Reset and Load Signal Decoders */
-#define QUAD8_CTR_RLD 0x00
+#define SELECT_RLD u8_encode_bits(0x0, REGISTER_SELECTION)
/* Counter Mode Register */
-#define QUAD8_CTR_CMR 0x20
+#define SELECT_CMR u8_encode_bits(0x1, REGISTER_SELECTION)
/* Input / Output Control Register */
-#define QUAD8_CTR_IOR 0x40
+#define SELECT_IOR u8_encode_bits(0x2, REGISTER_SELECTION)
/* Index Control Register */
-#define QUAD8_CTR_IDR 0x60
+#define SELECT_IDR u8_encode_bits(0x3, REGISTER_SELECTION)
+
+/*
+ * Reset and Load Signal Decoders
+ */
+#define RESETS GENMASK(2, 1)
+#define LOADS GENMASK(4, 3)
/* Reset Byte Pointer (three byte data pointer) */
-#define QUAD8_RLD_RESET_BP 0x01
-/* Reset Counter */
-#define QUAD8_RLD_RESET_CNTR 0x02
-/* Reset Borrow Toggle, Carry Toggle, Compare Toggle, and Sign flags */
-#define QUAD8_RLD_RESET_FLAGS 0x04
+#define RESET_BP BIT(0)
+/* Reset Borrow Toggle, Carry toggle, Compare toggle, Sign, and Index flags */
+#define RESET_BT_CT_CPT_S_IDX u8_encode_bits(0x2, RESETS)
/* Reset Error flag */
-#define QUAD8_RLD_RESET_E 0x06
+#define RESET_E u8_encode_bits(0x3, RESETS)
/* Preset Register to Counter */
-#define QUAD8_RLD_PRESET_CNTR 0x08
+#define TRANSFER_PR_TO_CNTR u8_encode_bits(0x1, LOADS)
/* Transfer Counter to Output Latch */
-#define QUAD8_RLD_CNTR_OUT 0x10
+#define TRANSFER_CNTR_TO_OL u8_encode_bits(0x2, LOADS)
/* Transfer Preset Register LSB to FCK Prescaler */
-#define QUAD8_RLD_PRESET_PSC 0x18
-#define QUAD8_CHAN_OP_RESET_COUNTERS 0x01
-#define QUAD8_CHAN_OP_ENABLE_INTERRUPT_FUNC 0x04
-#define QUAD8_CMR_QUADRATURE_X1 0x08
-#define QUAD8_CMR_QUADRATURE_X2 0x10
-#define QUAD8_CMR_QUADRATURE_X4 0x18
+#define TRANSFER_PR0_TO_PSC u8_encode_bits(0x3, LOADS)
+
+/*
+ * Counter Mode Registers
+ */
+#define COUNT_ENCODING BIT(0)
+#define COUNT_MODE GENMASK(2, 1)
+#define QUADRATURE_MODE GENMASK(4, 3)
+/* Binary count */
+#define BINARY u8_encode_bits(0x0, COUNT_ENCODING)
+/* Normal count */
+#define NORMAL_COUNT 0x0
+/* Range Limit */
+#define RANGE_LIMIT 0x1
+/* Non-recycle count */
+#define NON_RECYCLE_COUNT 0x2
+/* Modulo-N */
+#define MODULO_N 0x3
+/* Non-quadrature */
+#define NON_QUADRATURE 0x0
+/* Quadrature X1 */
+#define QUADRATURE_X1 0x1
+/* Quadrature X2 */
+#define QUADRATURE_X2 0x2
+/* Quadrature X4 */
+#define QUADRATURE_X4 0x3
+
+/*
+ * Input/Output Control Register
+ */
+#define AB_GATE BIT(0)
+#define LOAD_PIN BIT(1)
+#define FLG_PINS GENMASK(4, 3)
+/* Disable inputs A and B */
+#define DISABLE_AB u8_encode_bits(0x0, AB_GATE)
+/* Load Counter input */
+#define LOAD_CNTR 0x0
+/* FLG1 = CARRY(active low); FLG2 = BORROW(active low) */
+#define FLG1_CARRY_FLG2_BORROW 0x0
+/* FLG1 = COMPARE(active low); FLG2 = BORROW(active low) */
+#define FLG1_COMPARE_FLG2_BORROW 0x1
+/* FLG1 = Carry(active low)/Borrow(active low); FLG2 = U/D(active low) flag */
+#define FLG1_CARRYBORROW_FLG2_UD 0x2
+/* FLG1 = INDX (low pulse at INDEX pin active level); FLG2 = E flag */
+#define FLG1_INDX_FLG2_E 0x3
+
+/*
+ * INDEX CONTROL REGISTERS
+ */
+#define INDEX_MODE BIT(0)
+#define INDEX_POLARITY BIT(1)
+/* Disable Index mode */
+#define DISABLE_INDEX_MODE 0x0
+/* Negative Index Polarity */
+#define NEGATIVE_INDEX_POLARITY 0x0
+
+/*
+ * Channel Operation Register
+ */
+#define COUNTERS_OPERATION BIT(0)
+#define INTERRUPT_FUNCTION BIT(2)
+/* Enable all Counters */
+#define ENABLE_COUNTERS u8_encode_bits(0x0, COUNTERS_OPERATION)
+/* Reset all Counters */
+#define RESET_COUNTERS u8_encode_bits(0x1, COUNTERS_OPERATION)
+/* Disable the interrupt function */
+#define DISABLE_INTERRUPT_FUNCTION u8_encode_bits(0x0, INTERRUPT_FUNCTION)
+/* Enable the interrupt function */
+#define ENABLE_INTERRUPT_FUNCTION u8_encode_bits(0x1, INTERRUPT_FUNCTION)
+/* Any write to the Channel Operation register clears any pending interrupts */
+#define CLEAR_PENDING_INTERRUPTS (ENABLE_COUNTERS | ENABLE_INTERRUPT_FUNCTION)
/* Each Counter is 24 bits wide */
#define LS7267_CNTR_MAX GENMASK(23, 0)
@@ -162,9 +235,7 @@ static int quad8_count_read(struct counter_device *counter,
spin_lock_irqsave(&priv->lock, irqflags);
- /* Reset Byte Pointer; transfer Counter to Output Latch */
- iowrite8(QUAD8_CTR_RLD | QUAD8_RLD_RESET_BP | QUAD8_RLD_CNTR_OUT,
- &chan->control);
+ 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);
@@ -187,28 +258,23 @@ static int quad8_count_write(struct counter_device *counter,
spin_lock_irqsave(&priv->lock, irqflags);
- /* Reset Byte Pointer */
- iowrite8(QUAD8_CTR_RLD | QUAD8_RLD_RESET_BP, &chan->control);
+ 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);
- /* Transfer Preset Register to Counter */
- iowrite8(QUAD8_CTR_RLD | QUAD8_RLD_PRESET_CNTR, &chan->control);
+ iowrite8(SELECT_RLD | TRANSFER_PR_TO_CNTR, &chan->control);
- /* Reset Byte Pointer */
- iowrite8(QUAD8_CTR_RLD | QUAD8_RLD_RESET_BP, &chan->control);
+ iowrite8(SELECT_RLD | RESET_BP, &chan->control);
/* 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(QUAD8_CTR_RLD | QUAD8_RLD_RESET_FLAGS, &chan->control);
- /* Reset Error flag */
- iowrite8(QUAD8_CTR_RLD | QUAD8_RLD_RESET_E, &chan->control);
+ iowrite8(SELECT_RLD | RESET_BT_CT_CPT_S_IDX, &chan->control);
+ iowrite8(SELECT_RLD | RESET_E, &chan->control);
spin_unlock_irqrestore(&priv->lock, irqflags);
@@ -279,8 +345,8 @@ static int quad8_function_write(struct counter_device *counter,
spin_lock_irqsave(&priv->lock, irqflags);
- mode_cfg = priv->count_mode[id] << 1;
- idr_cfg = priv->index_polarity[id] << 1;
+ mode_cfg = u8_encode_bits(priv->count_mode[id], COUNT_MODE);
+ idr_cfg = u8_encode_bits(priv->index_polarity[id], INDEX_POLARITY);
if (function == COUNTER_FUNCTION_PULSE_DIRECTION) {
*quadrature_mode = 0;
@@ -288,11 +354,14 @@ static int quad8_function_write(struct counter_device *counter,
/* Quadrature scaling only available in quadrature mode */
*scale = 0;
+ mode_cfg |= u8_encode_bits(NON_QUADRATURE, QUADRATURE_MODE);
+
/* Synchronous function not supported in non-quadrature mode */
if (*synchronous_mode) {
*synchronous_mode = 0;
/* Disable synchronous function mode */
- iowrite8(QUAD8_CTR_IDR | idr_cfg, control);
+ idr_cfg |= u8_encode_bits(*synchronous_mode, INDEX_MODE);
+ iowrite8(SELECT_IDR | idr_cfg, control);
}
} else {
*quadrature_mode = 1;
@@ -300,15 +369,15 @@ static int quad8_function_write(struct counter_device *counter,
switch (function) {
case COUNTER_FUNCTION_QUADRATURE_X1_A:
*scale = 0;
- mode_cfg |= QUAD8_CMR_QUADRATURE_X1;
+ mode_cfg |= u8_encode_bits(QUADRATURE_X1, QUADRATURE_MODE);
break;
case COUNTER_FUNCTION_QUADRATURE_X2_A:
*scale = 1;
- mode_cfg |= QUAD8_CMR_QUADRATURE_X2;
+ mode_cfg |= u8_encode_bits(QUADRATURE_X2, QUADRATURE_MODE);
break;
case COUNTER_FUNCTION_QUADRATURE_X4:
*scale = 2;
- mode_cfg |= QUAD8_CMR_QUADRATURE_X4;
+ mode_cfg |= u8_encode_bits(QUADRATURE_X4, QUADRATURE_MODE);
break;
default:
/* should never reach this path */
@@ -318,7 +387,7 @@ static int quad8_function_write(struct counter_device *counter,
}
/* Load mode configuration to Counter Mode Register */
- iowrite8(QUAD8_CTR_CMR | mode_cfg, control);
+ iowrite8(SELECT_CMR | mode_cfg, control);
spin_unlock_irqrestore(&priv->lock, irqflags);
@@ -332,9 +401,11 @@ static int quad8_direction_read(struct counter_device *counter,
const struct quad8 *const priv = counter_priv(counter);
unsigned int ud_flag;
u8 __iomem *const flag_addr = &priv->reg->channel[count->id].control;
+ u8 flag;
+ flag = ioread8(flag_addr);
/* U/D flag: nonzero = up, zero = down */
- ud_flag = ioread8(flag_addr) & QUAD8_FLAG_UD;
+ ud_flag = u8_get_bits(flag, FLAG_UD);
*direction = (ud_flag) ? COUNTER_COUNT_DIRECTION_FORWARD :
COUNTER_COUNT_DIRECTION_BACKWARD;
@@ -423,10 +494,10 @@ static int quad8_action_read(struct counter_device *counter,
}
enum {
- QUAD8_EVENT_CARRY = 0,
- QUAD8_EVENT_COMPARE = 1,
- QUAD8_EVENT_CARRY_BORROW = 2,
- QUAD8_EVENT_INDEX = 3,
+ QUAD8_EVENT_CARRY = FLG1_CARRY_FLG2_BORROW,
+ QUAD8_EVENT_COMPARE = FLG1_COMPARE_FLG2_BORROW,
+ QUAD8_EVENT_CARRY_BORROW = FLG1_CARRYBORROW_FLG2_UD,
+ QUAD8_EVENT_INDEX = FLG1_INDX_FLG2_E,
};
static int quad8_events_configure(struct counter_device *counter)
@@ -471,10 +542,10 @@ static int quad8_events_configure(struct counter_device *counter)
priv->irq_trigger[event_node->channel] = next_irq_trigger;
/* Load configuration to I/O Control Register */
- ior_cfg = priv->ab_enable[event_node->channel] |
- priv->preset_enable[event_node->channel] << 1 |
- priv->irq_trigger[event_node->channel] << 3;
- iowrite8(QUAD8_CTR_IOR | ior_cfg,
+ ior_cfg = u8_encode_bits(priv->ab_enable[event_node->channel], AB_GATE) |
+ u8_encode_bits(priv->preset_enable[event_node->channel], LOAD_PIN) |
+ u8_encode_bits(priv->irq_trigger[event_node->channel], FLG_PINS);
+ iowrite8(SELECT_IOR | ior_cfg,
&priv->reg->channel[event_node->channel].control);
}
@@ -544,16 +615,16 @@ static int quad8_index_polarity_set(struct counter_device *counter,
const size_t channel_id = signal->id - 16;
u8 __iomem *const control = &priv->reg->channel[channel_id].control;
unsigned long irqflags;
- unsigned int idr_cfg = index_polarity << 1;
+ unsigned int idr_cfg = u8_encode_bits(index_polarity, INDEX_POLARITY);
spin_lock_irqsave(&priv->lock, irqflags);
- idr_cfg |= priv->synchronous_mode[channel_id];
+ idr_cfg |= u8_encode_bits(priv->synchronous_mode[channel_id], INDEX_MODE);
priv->index_polarity[channel_id] = index_polarity;
/* Load Index Control configuration to Index Control Register */
- iowrite8(QUAD8_CTR_IDR | idr_cfg, control);
+ iowrite8(SELECT_IDR | idr_cfg, control);
spin_unlock_irqrestore(&priv->lock, irqflags);
@@ -611,11 +682,11 @@ static int quad8_synchronous_mode_set(struct counter_device *counter,
const size_t channel_id = signal->id - 16;
u8 __iomem *const control = &priv->reg->channel[channel_id].control;
unsigned long irqflags;
- unsigned int idr_cfg = synchronous_mode;
+ unsigned int idr_cfg = u8_encode_bits(synchronous_mode, INDEX_MODE);
spin_lock_irqsave(&priv->lock, irqflags);
- idr_cfg |= priv->index_polarity[channel_id] << 1;
+ idr_cfg |= u8_encode_bits(priv->index_polarity[channel_id], INDEX_POLARITY);
/* Index function must be non-synchronous in non-quadrature mode */
if (synchronous_mode && !priv->quadrature_mode[channel_id]) {
@@ -626,7 +697,7 @@ static int quad8_synchronous_mode_set(struct counter_device *counter,
priv->synchronous_mode[channel_id] = synchronous_mode;
/* Load Index Control configuration to Index Control Register */
- iowrite8(QUAD8_CTR_IDR | idr_cfg, control);
+ iowrite8(SELECT_IDR | idr_cfg, control);
spin_unlock_irqrestore(&priv->lock, irqflags);
@@ -648,18 +719,17 @@ static int quad8_count_mode_read(struct counter_device *counter,
{
const struct quad8 *const priv = counter_priv(counter);
- /* Map 104-QUAD-8 count mode to Generic Counter count mode */
switch (priv->count_mode[count->id]) {
- case 0:
+ case NORMAL_COUNT:
*cnt_mode = COUNTER_COUNT_MODE_NORMAL;
break;
- case 1:
+ case RANGE_LIMIT:
*cnt_mode = COUNTER_COUNT_MODE_RANGE_LIMIT;
break;
- case 2:
+ case NON_RECYCLE_COUNT:
*cnt_mode = COUNTER_COUNT_MODE_NON_RECYCLE;
break;
- case 3:
+ case MODULO_N:
*cnt_mode = COUNTER_COUNT_MODE_MODULO_N;
break;
}
@@ -677,19 +747,18 @@ static int quad8_count_mode_write(struct counter_device *counter,
u8 __iomem *const control = &priv->reg->channel[count->id].control;
unsigned long irqflags;
- /* Map Generic Counter count mode to 104-QUAD-8 count mode */
switch (cnt_mode) {
case COUNTER_COUNT_MODE_NORMAL:
- count_mode = 0;
+ count_mode = NORMAL_COUNT;
break;
case COUNTER_COUNT_MODE_RANGE_LIMIT:
- count_mode = 1;
+ count_mode = RANGE_LIMIT;
break;
case COUNTER_COUNT_MODE_NON_RECYCLE:
- count_mode = 2;
+ count_mode = NON_RECYCLE_COUNT;
break;
case COUNTER_COUNT_MODE_MODULO_N:
- count_mode = 3;
+ count_mode = MODULO_N;
break;
default:
/* should never reach this path */
@@ -701,14 +770,16 @@ static int quad8_count_mode_write(struct counter_device *counter,
priv->count_mode[count->id] = count_mode;
/* Set count mode configuration value */
- mode_cfg = count_mode << 1;
+ mode_cfg = u8_encode_bits(count_mode, COUNT_MODE);
/* Add quadrature mode configuration */
if (priv->quadrature_mode[count->id])
- mode_cfg |= (priv->quadrature_scale[count->id] + 1) << 3;
+ mode_cfg |= u8_encode_bits(priv->quadrature_scale[count->id] + 1, QUADRATURE_MODE);
+ else
+ mode_cfg |= u8_encode_bits(NON_QUADRATURE, QUADRATURE_MODE);
/* Load mode configuration to Counter Mode Register */
- iowrite8(QUAD8_CTR_CMR | mode_cfg, control);
+ iowrite8(SELECT_CMR | mode_cfg, control);
spin_unlock_irqrestore(&priv->lock, irqflags);
@@ -737,11 +808,12 @@ static int quad8_count_enable_write(struct counter_device *counter,
priv->ab_enable[count->id] = enable;
- ior_cfg = enable | priv->preset_enable[count->id] << 1 |
- priv->irq_trigger[count->id] << 3;
+ ior_cfg = u8_encode_bits(enable, AB_GATE) |
+ u8_encode_bits(priv->preset_enable[count->id], LOAD_PIN) |
+ u8_encode_bits(priv->irq_trigger[count->id], FLG_PINS);
/* Load I/O control configuration */
- iowrite8(QUAD8_CTR_IOR | ior_cfg, control);
+ iowrite8(SELECT_IOR | ior_cfg, control);
spin_unlock_irqrestore(&priv->lock, irqflags);
@@ -758,8 +830,10 @@ static int quad8_error_noise_get(struct counter_device *counter,
{
const struct quad8 *const priv = counter_priv(counter);
u8 __iomem *const flag_addr = &priv->reg->channel[count->id].control;
+ u8 flag;
- *noise_error = !!(ioread8(flag_addr) & QUAD8_FLAG_E);
+ flag = ioread8(flag_addr);
+ *noise_error = u8_get_bits(flag, FLAG_E);
return 0;
}
@@ -782,8 +856,7 @@ static void quad8_preset_register_set(struct quad8 *const priv, const int id,
priv->preset[id] = preset;
- /* Reset Byte Pointer */
- iowrite8(QUAD8_CTR_RLD | QUAD8_RLD_RESET_BP, &chan->control);
+ iowrite8(SELECT_RLD | RESET_BP, &chan->control);
/* Set Preset Register */
for (i = 0; i < 3; i++)
@@ -818,8 +891,8 @@ static int quad8_count_ceiling_read(struct counter_device *counter,
/* Range Limit and Modulo-N count modes use preset value as ceiling */
switch (priv->count_mode[count->id]) {
- case 1:
- case 3:
+ case RANGE_LIMIT:
+ case MODULO_N:
*ceiling = priv->preset[count->id];
break;
default:
@@ -845,8 +918,8 @@ static int quad8_count_ceiling_write(struct counter_device *counter,
/* Range Limit and Modulo-N count modes use preset value as ceiling */
switch (priv->count_mode[count->id]) {
- case 1:
- case 3:
+ case RANGE_LIMIT:
+ case MODULO_N:
quad8_preset_register_set(priv, count->id, ceiling);
spin_unlock_irqrestore(&priv->lock, irqflags);
return 0;
@@ -884,11 +957,12 @@ static int quad8_count_preset_enable_write(struct counter_device *counter,
priv->preset_enable[count->id] = preset_enable;
- ior_cfg = priv->ab_enable[count->id] | preset_enable << 1 |
- priv->irq_trigger[count->id] << 3;
+ ior_cfg = u8_encode_bits(priv->ab_enable[count->id], AB_GATE) |
+ u8_encode_bits(preset_enable, LOAD_PIN) |
+ u8_encode_bits(priv->irq_trigger[count->id], FLG_PINS);
/* Load I/O control configuration to Input / Output Control Register */
- iowrite8(QUAD8_CTR_IOR | ior_cfg, control);
+ iowrite8(SELECT_IOR | ior_cfg, control);
spin_unlock_irqrestore(&priv->lock, irqflags);
@@ -987,13 +1061,11 @@ static int quad8_signal_fck_prescaler_write(struct counter_device *counter,
priv->fck_prescaler[channel_id] = prescaler;
- /* Reset Byte Pointer */
- iowrite8(QUAD8_CTR_RLD | QUAD8_RLD_RESET_BP, &chan->control);
+ iowrite8(SELECT_RLD | RESET_BP, &chan->control);
/* Set filter clock factor */
iowrite8(prescaler, &chan->data);
- iowrite8(QUAD8_CTR_RLD | QUAD8_RLD_RESET_BP | QUAD8_RLD_PRESET_PSC,
- &chan->control);
+ iowrite8(SELECT_RLD | RESET_BP | TRANSFER_PR0_TO_PSC, &chan->control);
spin_unlock_irqrestore(&priv->lock, irqflags);
@@ -1183,7 +1255,7 @@ static irqreturn_t quad8_irq_handler(int irq, void *private)
}
/* Clear pending interrupts on device */
- iowrite8(QUAD8_CHAN_OP_ENABLE_INTERRUPT_FUNC, &priv->reg->channel_oper);
+ iowrite8(CLEAR_PENDING_INTERRUPTS, &priv->reg->channel_oper);
return IRQ_HANDLED;
}
@@ -1192,27 +1264,25 @@ static void quad8_init_counter(struct channel_reg __iomem *const chan)
{
unsigned long i;
- /* Reset Byte Pointer */
- iowrite8(QUAD8_CTR_RLD | QUAD8_RLD_RESET_BP, &chan->control);
+ iowrite8(SELECT_RLD | RESET_BP, &chan->control);
/* Reset filter clock factor */
iowrite8(0, &chan->data);
- iowrite8(QUAD8_CTR_RLD | QUAD8_RLD_RESET_BP | QUAD8_RLD_PRESET_PSC,
- &chan->control);
- /* Reset Byte Pointer */
- iowrite8(QUAD8_CTR_RLD | QUAD8_RLD_RESET_BP, &chan->control);
+ 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);
- /* Reset Borrow, Carry, Compare, and Sign flags */
- iowrite8(QUAD8_CTR_RLD | QUAD8_RLD_RESET_FLAGS, &chan->control);
- /* Reset Error flag */
- iowrite8(QUAD8_CTR_RLD | QUAD8_RLD_RESET_E, &chan->control);
+ iowrite8(SELECT_RLD | RESET_BT_CT_CPT_S_IDX, &chan->control);
+ iowrite8(SELECT_RLD | RESET_E, &chan->control);
/* Binary encoding; Normal count; non-quadrature mode */
- iowrite8(QUAD8_CTR_CMR, &chan->control);
+ iowrite8(SELECT_CMR | BINARY | u8_encode_bits(NORMAL_COUNT, COUNT_MODE) |
+ u8_encode_bits(NON_QUADRATURE, QUADRATURE_MODE), &chan->control);
/* Disable A and B inputs; preset on index; FLG1 as Carry */
- iowrite8(QUAD8_CTR_IOR, &chan->control);
+ iowrite8(SELECT_IOR | DISABLE_AB | u8_encode_bits(LOAD_CNTR, LOAD_PIN) |
+ u8_encode_bits(FLG1_CARRY_FLG2_BORROW, FLG_PINS), &chan->control);
/* Disable index function; negative index polarity */
- iowrite8(QUAD8_CTR_IDR, &chan->control);
+ iowrite8(SELECT_IDR | u8_encode_bits(DISABLE_INDEX_MODE, INDEX_MODE) |
+ u8_encode_bits(NEGATIVE_INDEX_POLARITY, INDEX_POLARITY), &chan->control);
}
static int quad8_probe(struct device *dev, unsigned int id)
@@ -1251,14 +1321,14 @@ static int quad8_probe(struct device *dev, unsigned int id)
/* Reset Index/Interrupt Register */
iowrite8(0x00, &priv->reg->index_interrupt);
/* Reset all counters and disable interrupt function */
- iowrite8(QUAD8_CHAN_OP_RESET_COUNTERS, &priv->reg->channel_oper);
+ iowrite8(RESET_COUNTERS | DISABLE_INTERRUPT_FUNCTION, &priv->reg->channel_oper);
/* Set initial configuration for all counters */
for (i = 0; i < QUAD8_NUM_COUNTERS; i++)
quad8_init_counter(priv->reg->channel + i);
/* Disable Differential Encoder Cable Status for all channels */
iowrite8(0xFF, &priv->reg->cable_status);
/* Enable all counters and enable interrupt function */
- iowrite8(QUAD8_CHAN_OP_ENABLE_INTERRUPT_FUNC, &priv->reg->channel_oper);
+ iowrite8(ENABLE_COUNTERS | ENABLE_INTERRUPT_FUNCTION, &priv->reg->channel_oper);
err = devm_request_irq(&counter->dev, irq[id], quad8_irq_handler,
IRQF_SHARED, counter->name, counter);
--
2.39.2
The 104-quad-8 driver buffers the device configuration states
separately, however each device has only three control registers: CMR,
IOR, and IDR. Refactoring to buffer the states of these control
registers rather than each configuration separately results in succinct
code that more closely matches what is happening on the device.
Signed-off-by: William Breathitt Gray <[email protected]>
---
Changes in v2:
- 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()
drivers/counter/104-quad-8.c | 287 +++++++++++++----------------------
1 file changed, 104 insertions(+), 183 deletions(-)
diff --git a/drivers/counter/104-quad-8.c b/drivers/counter/104-quad-8.c
index f07e4a9b946d..fe0887e6185d 100644
--- a/drivers/counter/104-quad-8.c
+++ b/drivers/counter/104-quad-8.c
@@ -68,32 +68,21 @@ struct quad8_reg {
/**
* struct quad8 - device private data structure
* @lock: lock to prevent clobbering device states during R/W ops
- * @counter: instance of the counter_device
+ * @cmr: array of Counter Mode Register states
+ * @ior: array of Input / Output Control Register states
+ * @idr: array of Index Control Register states
* @fck_prescaler: array of filter clock prescaler configurations
* @preset: array of preset values
- * @count_mode: array of count mode configurations
- * @quadrature_mode: array of quadrature mode configurations
- * @quadrature_scale: array of quadrature mode scale configurations
- * @ab_enable: array of A and B inputs enable configurations
- * @preset_enable: array of set_to_preset_on_index attribute configurations
- * @irq_trigger: array of current IRQ trigger function configurations
- * @synchronous_mode: array of index function synchronous mode configurations
- * @index_polarity: array of index function polarity configurations
* @cable_fault_enable: differential encoder cable status enable configurations
* @reg: I/O address offset for the device registers
*/
struct quad8 {
spinlock_t lock;
+ u8 cmr[QUAD8_NUM_COUNTERS];
+ u8 ior[QUAD8_NUM_COUNTERS];
+ u8 idr[QUAD8_NUM_COUNTERS];
unsigned int fck_prescaler[QUAD8_NUM_COUNTERS];
unsigned int preset[QUAD8_NUM_COUNTERS];
- unsigned int count_mode[QUAD8_NUM_COUNTERS];
- unsigned int quadrature_mode[QUAD8_NUM_COUNTERS];
- unsigned int quadrature_scale[QUAD8_NUM_COUNTERS];
- unsigned int ab_enable[QUAD8_NUM_COUNTERS];
- unsigned int preset_enable[QUAD8_NUM_COUNTERS];
- unsigned int irq_trigger[QUAD8_NUM_COUNTERS];
- unsigned int synchronous_mode[QUAD8_NUM_COUNTERS];
- unsigned int index_polarity[QUAD8_NUM_COUNTERS];
unsigned int cable_fault_enable;
struct quad8_reg __iomem *reg;
};
@@ -102,6 +91,8 @@ struct quad8 {
#define FLAG_E BIT(4)
/* Up/Down flag */
#define FLAG_UD BIT(5)
+/* Counting up */
+#define UP 0x1
#define REGISTER_SELECTION GENMASK(6, 5)
@@ -183,8 +174,12 @@ struct quad8 {
#define INDEX_POLARITY BIT(1)
/* Disable Index mode */
#define DISABLE_INDEX_MODE 0x0
+/* Enable Index mode */
+#define ENABLE_INDEX_MODE 0x1
/* Negative Index Polarity */
#define NEGATIVE_INDEX_POLARITY 0x0
+/* Positive Index Polarity */
+#define POSITIVE_INDEX_POLARITY 0x1
/*
* Channel Operation Register
@@ -205,6 +200,13 @@ struct quad8 {
/* Each Counter is 24 bits wide */
#define LS7267_CNTR_MAX GENMASK(23, 0)
+static void quad8_control_register_update(struct quad8 *const priv, u8 *const buf,
+ const size_t channel, const u8 val, const u8 field)
+{
+ u8p_replace_bits(&buf[channel], val, field);
+ iowrite8(buf[channel], &priv->reg->channel[channel].control);
+}
+
static int quad8_signal_read(struct counter_device *counter,
struct counter_signal *signal,
enum counter_signal_level *level)
@@ -291,19 +293,17 @@ static const enum counter_function quad8_count_functions_list[] = {
static int quad8_function_get(const struct quad8 *const priv, const size_t id,
enum counter_function *const function)
{
- if (!priv->quadrature_mode[id]) {
+ switch (u8_get_bits(priv->cmr[id], QUADRATURE_MODE)) {
+ case NON_QUADRATURE:
*function = COUNTER_FUNCTION_PULSE_DIRECTION;
return 0;
- }
-
- switch (priv->quadrature_scale[id]) {
- case 0:
+ case QUADRATURE_X1:
*function = COUNTER_FUNCTION_QUADRATURE_X1_A;
return 0;
- case 1:
+ case QUADRATURE_X2:
*function = COUNTER_FUNCTION_QUADRATURE_X2_A;
return 0;
- case 2:
+ case QUADRATURE_X4:
*function = COUNTER_FUNCTION_QUADRATURE_X4;
return 0;
default:
@@ -335,59 +335,36 @@ static int quad8_function_write(struct counter_device *counter,
{
struct quad8 *const priv = counter_priv(counter);
const int id = count->id;
- unsigned int *const quadrature_mode = priv->quadrature_mode + id;
- unsigned int *const scale = priv->quadrature_scale + id;
- unsigned int *const synchronous_mode = priv->synchronous_mode + id;
- u8 __iomem *const control = &priv->reg->channel[id].control;
unsigned long irqflags;
unsigned int mode_cfg;
- unsigned int idr_cfg;
-
- spin_lock_irqsave(&priv->lock, irqflags);
-
- mode_cfg = u8_encode_bits(priv->count_mode[id], COUNT_MODE);
- idr_cfg = u8_encode_bits(priv->index_polarity[id], INDEX_POLARITY);
+ bool synchronous_mode;
- if (function == COUNTER_FUNCTION_PULSE_DIRECTION) {
- *quadrature_mode = 0;
-
- /* Quadrature scaling only available in quadrature mode */
- *scale = 0;
-
- mode_cfg |= u8_encode_bits(NON_QUADRATURE, QUADRATURE_MODE);
+ switch (function) {
+ case COUNTER_FUNCTION_PULSE_DIRECTION:
+ mode_cfg = NON_QUADRATURE;
+ break;
+ case COUNTER_FUNCTION_QUADRATURE_X1_A:
+ mode_cfg = QUADRATURE_X1;
+ break;
+ case COUNTER_FUNCTION_QUADRATURE_X2_A:
+ mode_cfg = QUADRATURE_X2;
+ break;
+ case COUNTER_FUNCTION_QUADRATURE_X4:
+ mode_cfg = QUADRATURE_X4;
+ break;
+ default:
+ /* should never reach this path */
+ return -EINVAL;
+ }
- /* Synchronous function not supported in non-quadrature mode */
- if (*synchronous_mode) {
- *synchronous_mode = 0;
- /* Disable synchronous function mode */
- idr_cfg |= u8_encode_bits(*synchronous_mode, INDEX_MODE);
- iowrite8(SELECT_IDR | idr_cfg, control);
- }
- } else {
- *quadrature_mode = 1;
+ spin_lock_irqsave(&priv->lock, irqflags);
- switch (function) {
- case COUNTER_FUNCTION_QUADRATURE_X1_A:
- *scale = 0;
- mode_cfg |= u8_encode_bits(QUADRATURE_X1, QUADRATURE_MODE);
- break;
- case COUNTER_FUNCTION_QUADRATURE_X2_A:
- *scale = 1;
- mode_cfg |= u8_encode_bits(QUADRATURE_X2, QUADRATURE_MODE);
- break;
- case COUNTER_FUNCTION_QUADRATURE_X4:
- *scale = 2;
- mode_cfg |= u8_encode_bits(QUADRATURE_X4, QUADRATURE_MODE);
- break;
- default:
- /* should never reach this path */
- spin_unlock_irqrestore(&priv->lock, irqflags);
- return -EINVAL;
- }
- }
+ /* Synchronous function not supported in non-quadrature mode */
+ synchronous_mode = u8_get_bits(priv->idr[id], INDEX_MODE) == ENABLE_INDEX_MODE;
+ if (synchronous_mode && mode_cfg == NON_QUADRATURE)
+ quad8_control_register_update(priv, priv->idr, id, DISABLE_INDEX_MODE, INDEX_MODE);
- /* Load mode configuration to Counter Mode Register */
- iowrite8(SELECT_CMR | mode_cfg, control);
+ quad8_control_register_update(priv, priv->cmr, id, mode_cfg, QUADRATURE_MODE);
spin_unlock_irqrestore(&priv->lock, irqflags);
@@ -399,15 +376,11 @@ static int quad8_direction_read(struct counter_device *counter,
enum counter_count_direction *direction)
{
const struct quad8 *const priv = counter_priv(counter);
- unsigned int ud_flag;
u8 __iomem *const flag_addr = &priv->reg->channel[count->id].control;
u8 flag;
flag = ioread8(flag_addr);
- /* U/D flag: nonzero = up, zero = down */
- ud_flag = u8_get_bits(flag, FLAG_UD);
-
- *direction = (ud_flag) ? COUNTER_COUNT_DIRECTION_FORWARD :
+ *direction = (u8_get_bits(flag, FLAG_UD) == UP) ? COUNTER_COUNT_DIRECTION_FORWARD :
COUNTER_COUNT_DIRECTION_BACKWARD;
return 0;
@@ -437,13 +410,13 @@ static int quad8_action_read(struct counter_device *counter,
const size_t signal_a_id = count->synapses[0].signal->id;
enum counter_count_direction direction;
+ /* Default action mode */
+ *action = COUNTER_SYNAPSE_ACTION_NONE;
+
/* Handle Index signals */
if (synapse->signal->id >= 16) {
- if (!priv->preset_enable[count->id])
+ if (u8_get_bits(priv->ior[count->id], LOAD_PIN) == LOAD_CNTR)
*action = COUNTER_SYNAPSE_ACTION_RISING_EDGE;
- else
- *action = COUNTER_SYNAPSE_ACTION_NONE;
-
return 0;
}
@@ -463,9 +436,6 @@ static int quad8_action_read(struct counter_device *counter,
spin_unlock_irqrestore(&priv->lock, irqflags);
- /* Default action mode */
- *action = COUNTER_SYNAPSE_ACTION_NONE;
-
/* Determine action mode based on current count function mode */
switch (function) {
case COUNTER_FUNCTION_PULSE_DIRECTION:
@@ -493,37 +463,29 @@ static int quad8_action_read(struct counter_device *counter,
}
}
-enum {
- QUAD8_EVENT_CARRY = FLG1_CARRY_FLG2_BORROW,
- QUAD8_EVENT_COMPARE = FLG1_COMPARE_FLG2_BORROW,
- QUAD8_EVENT_CARRY_BORROW = FLG1_CARRYBORROW_FLG2_UD,
- QUAD8_EVENT_INDEX = FLG1_INDX_FLG2_E,
-};
-
static int quad8_events_configure(struct counter_device *counter)
{
struct quad8 *const priv = counter_priv(counter);
unsigned long irq_enabled = 0;
unsigned long irqflags;
struct counter_event_node *event_node;
- unsigned int next_irq_trigger;
- unsigned long ior_cfg;
+ u8 flg_pins;
spin_lock_irqsave(&priv->lock, irqflags);
list_for_each_entry(event_node, &counter->events_list, l) {
switch (event_node->event) {
case COUNTER_EVENT_OVERFLOW:
- next_irq_trigger = QUAD8_EVENT_CARRY;
+ flg_pins = FLG1_CARRY_FLG2_BORROW;
break;
case COUNTER_EVENT_THRESHOLD:
- next_irq_trigger = QUAD8_EVENT_COMPARE;
+ flg_pins = FLG1_COMPARE_FLG2_BORROW;
break;
case COUNTER_EVENT_OVERFLOW_UNDERFLOW:
- next_irq_trigger = QUAD8_EVENT_CARRY_BORROW;
+ flg_pins = FLG1_CARRYBORROW_FLG2_UD;
break;
case COUNTER_EVENT_INDEX:
- next_irq_trigger = QUAD8_EVENT_INDEX;
+ flg_pins = FLG1_INDX_FLG2_E;
break;
default:
/* should never reach this path */
@@ -535,18 +497,12 @@ static int quad8_events_configure(struct counter_device *counter)
irq_enabled |= BIT(event_node->channel);
/* Skip configuration if it is the same as previously set */
- if (priv->irq_trigger[event_node->channel] == next_irq_trigger)
+ if (flg_pins == u8_get_bits(priv->ior[event_node->channel], FLG_PINS))
continue;
/* Save new IRQ function configuration */
- priv->irq_trigger[event_node->channel] = next_irq_trigger;
-
- /* Load configuration to I/O Control Register */
- ior_cfg = u8_encode_bits(priv->ab_enable[event_node->channel], AB_GATE) |
- u8_encode_bits(priv->preset_enable[event_node->channel], LOAD_PIN) |
- u8_encode_bits(priv->irq_trigger[event_node->channel], FLG_PINS);
- iowrite8(SELECT_IOR | ior_cfg,
- &priv->reg->channel[event_node->channel].control);
+ quad8_control_register_update(priv, priv->ior, event_node->channel, flg_pins,
+ FLG_PINS);
}
iowrite8(irq_enabled, &priv->reg->index_interrupt);
@@ -602,7 +558,7 @@ static int quad8_index_polarity_get(struct counter_device *counter,
const struct quad8 *const priv = counter_priv(counter);
const size_t channel_id = signal->id - 16;
- *index_polarity = priv->index_polarity[channel_id];
+ *index_polarity = u8_get_bits(priv->idr[channel_id], INDEX_POLARITY);
return 0;
}
@@ -613,18 +569,11 @@ static int quad8_index_polarity_set(struct counter_device *counter,
{
struct quad8 *const priv = counter_priv(counter);
const size_t channel_id = signal->id - 16;
- u8 __iomem *const control = &priv->reg->channel[channel_id].control;
unsigned long irqflags;
- unsigned int idr_cfg = u8_encode_bits(index_polarity, INDEX_POLARITY);
spin_lock_irqsave(&priv->lock, irqflags);
- idr_cfg |= u8_encode_bits(priv->synchronous_mode[channel_id], INDEX_MODE);
-
- priv->index_polarity[channel_id] = index_polarity;
-
- /* Load Index Control configuration to Index Control Register */
- iowrite8(SELECT_IDR | idr_cfg, control);
+ quad8_control_register_update(priv, priv->idr, channel_id, index_polarity, INDEX_POLARITY);
spin_unlock_irqrestore(&priv->lock, irqflags);
@@ -642,7 +591,7 @@ static int quad8_polarity_read(struct counter_device *counter,
if (err)
return err;
- *polarity = (index_polarity) ? COUNTER_SIGNAL_POLARITY_POSITIVE :
+ *polarity = (index_polarity == POSITIVE_INDEX_POLARITY) ? COUNTER_SIGNAL_POLARITY_POSITIVE :
COUNTER_SIGNAL_POLARITY_NEGATIVE;
return 0;
@@ -652,7 +601,8 @@ static int quad8_polarity_write(struct counter_device *counter,
struct counter_signal *signal,
enum counter_signal_polarity polarity)
{
- const u32 pol = (polarity == COUNTER_SIGNAL_POLARITY_POSITIVE) ? 1 : 0;
+ const u32 pol = (polarity == COUNTER_SIGNAL_POLARITY_POSITIVE) ? POSITIVE_INDEX_POLARITY :
+ NEGATIVE_INDEX_POLARITY;
return quad8_index_polarity_set(counter, signal, pol);
}
@@ -669,7 +619,7 @@ static int quad8_synchronous_mode_get(struct counter_device *counter,
const struct quad8 *const priv = counter_priv(counter);
const size_t channel_id = signal->id - 16;
- *synchronous_mode = priv->synchronous_mode[channel_id];
+ *synchronous_mode = u8_get_bits(priv->idr[channel_id], INDEX_MODE);
return 0;
}
@@ -680,24 +630,19 @@ static int quad8_synchronous_mode_set(struct counter_device *counter,
{
struct quad8 *const priv = counter_priv(counter);
const size_t channel_id = signal->id - 16;
- u8 __iomem *const control = &priv->reg->channel[channel_id].control;
+ u8 quadrature_mode;
unsigned long irqflags;
- unsigned int idr_cfg = u8_encode_bits(synchronous_mode, INDEX_MODE);
spin_lock_irqsave(&priv->lock, irqflags);
- idr_cfg |= u8_encode_bits(priv->index_polarity[channel_id], INDEX_POLARITY);
-
/* Index function must be non-synchronous in non-quadrature mode */
- if (synchronous_mode && !priv->quadrature_mode[channel_id]) {
+ quadrature_mode = u8_get_bits(priv->idr[channel_id], QUADRATURE_MODE);
+ if (synchronous_mode && quadrature_mode == NON_QUADRATURE) {
spin_unlock_irqrestore(&priv->lock, irqflags);
return -EINVAL;
}
- priv->synchronous_mode[channel_id] = synchronous_mode;
-
- /* Load Index Control configuration to Index Control Register */
- iowrite8(SELECT_IDR | idr_cfg, control);
+ quad8_control_register_update(priv, priv->idr, channel_id, synchronous_mode, INDEX_MODE);
spin_unlock_irqrestore(&priv->lock, irqflags);
@@ -719,7 +664,7 @@ static int quad8_count_mode_read(struct counter_device *counter,
{
const struct quad8 *const priv = counter_priv(counter);
- switch (priv->count_mode[count->id]) {
+ switch (u8_get_bits(priv->cmr[count->id], COUNT_MODE)) {
case NORMAL_COUNT:
*cnt_mode = COUNTER_COUNT_MODE_NORMAL;
break;
@@ -743,8 +688,6 @@ static int quad8_count_mode_write(struct counter_device *counter,
{
struct quad8 *const priv = counter_priv(counter);
unsigned int count_mode;
- unsigned int mode_cfg;
- u8 __iomem *const control = &priv->reg->channel[count->id].control;
unsigned long irqflags;
switch (cnt_mode) {
@@ -767,19 +710,7 @@ static int quad8_count_mode_write(struct counter_device *counter,
spin_lock_irqsave(&priv->lock, irqflags);
- priv->count_mode[count->id] = count_mode;
-
- /* Set count mode configuration value */
- mode_cfg = u8_encode_bits(count_mode, COUNT_MODE);
-
- /* Add quadrature mode configuration */
- if (priv->quadrature_mode[count->id])
- mode_cfg |= u8_encode_bits(priv->quadrature_scale[count->id] + 1, QUADRATURE_MODE);
- else
- mode_cfg |= u8_encode_bits(NON_QUADRATURE, QUADRATURE_MODE);
-
- /* Load mode configuration to Counter Mode Register */
- iowrite8(SELECT_CMR | mode_cfg, control);
+ quad8_control_register_update(priv, priv->cmr, count->id, count_mode, COUNT_MODE);
spin_unlock_irqrestore(&priv->lock, irqflags);
@@ -791,7 +722,7 @@ static int quad8_count_enable_read(struct counter_device *counter,
{
const struct quad8 *const priv = counter_priv(counter);
- *enable = priv->ab_enable[count->id];
+ *enable = u8_get_bits(priv->ior[count->id], AB_GATE);
return 0;
}
@@ -800,20 +731,11 @@ static int quad8_count_enable_write(struct counter_device *counter,
struct counter_count *count, u8 enable)
{
struct quad8 *const priv = counter_priv(counter);
- u8 __iomem *const control = &priv->reg->channel[count->id].control;
unsigned long irqflags;
- unsigned int ior_cfg;
spin_lock_irqsave(&priv->lock, irqflags);
- priv->ab_enable[count->id] = enable;
-
- ior_cfg = u8_encode_bits(enable, AB_GATE) |
- u8_encode_bits(priv->preset_enable[count->id], LOAD_PIN) |
- u8_encode_bits(priv->irq_trigger[count->id], FLG_PINS);
-
- /* Load I/O control configuration */
- iowrite8(SELECT_IOR | ior_cfg, control);
+ quad8_control_register_update(priv, priv->ior, count->id, enable, AB_GATE);
spin_unlock_irqrestore(&priv->lock, irqflags);
@@ -890,7 +812,7 @@ static int quad8_count_ceiling_read(struct counter_device *counter,
spin_lock_irqsave(&priv->lock, irqflags);
/* Range Limit and Modulo-N count modes use preset value as ceiling */
- switch (priv->count_mode[count->id]) {
+ switch (u8_get_bits(priv->cmr[count->id], COUNT_MODE)) {
case RANGE_LIMIT:
case MODULO_N:
*ceiling = priv->preset[count->id];
@@ -917,7 +839,7 @@ static int quad8_count_ceiling_write(struct counter_device *counter,
spin_lock_irqsave(&priv->lock, irqflags);
/* Range Limit and Modulo-N count modes use preset value as ceiling */
- switch (priv->count_mode[count->id]) {
+ switch (u8_get_bits(priv->cmr[count->id], COUNT_MODE)) {
case RANGE_LIMIT:
case MODULO_N:
quad8_preset_register_set(priv, count->id, ceiling);
@@ -936,7 +858,8 @@ static int quad8_count_preset_enable_read(struct counter_device *counter,
{
const struct quad8 *const priv = counter_priv(counter);
- *preset_enable = !priv->preset_enable[count->id];
+ /* Preset enable is active low in Input/Output Control register */
+ *preset_enable = !u8_get_bits(priv->ior[count->id], LOAD_PIN);
return 0;
}
@@ -946,23 +869,12 @@ static int quad8_count_preset_enable_write(struct counter_device *counter,
u8 preset_enable)
{
struct quad8 *const priv = counter_priv(counter);
- u8 __iomem *const control = &priv->reg->channel[count->id].control;
unsigned long irqflags;
- unsigned int ior_cfg;
-
- /* Preset enable is active low in Input/Output Control register */
- preset_enable = !preset_enable;
spin_lock_irqsave(&priv->lock, irqflags);
- priv->preset_enable[count->id] = preset_enable;
-
- ior_cfg = u8_encode_bits(priv->ab_enable[count->id], AB_GATE) |
- u8_encode_bits(preset_enable, LOAD_PIN) |
- u8_encode_bits(priv->irq_trigger[count->id], FLG_PINS);
-
- /* Load I/O control configuration to Input / Output Control Register */
- iowrite8(SELECT_IOR | ior_cfg, control);
+ /* Preset enable is active low in Input/Output Control register */
+ quad8_control_register_update(priv, priv->ior, count->id, !preset_enable, LOAD_PIN);
spin_unlock_irqrestore(&priv->lock, irqflags);
@@ -1224,6 +1136,7 @@ static irqreturn_t quad8_irq_handler(int irq, void *private)
struct quad8 *const priv = counter_priv(counter);
unsigned long irq_status;
unsigned long channel;
+ unsigned int flg_pins;
u8 event;
irq_status = ioread8(&priv->reg->interrupt_status);
@@ -1231,23 +1144,24 @@ static irqreturn_t quad8_irq_handler(int irq, void *private)
return IRQ_NONE;
for_each_set_bit(channel, &irq_status, QUAD8_NUM_COUNTERS) {
- switch (priv->irq_trigger[channel]) {
- case QUAD8_EVENT_CARRY:
+ flg_pins = u8_get_bits(priv->ior[channel], FLG_PINS);
+ switch (flg_pins) {
+ case FLG1_CARRY_FLG2_BORROW:
event = COUNTER_EVENT_OVERFLOW;
break;
- case QUAD8_EVENT_COMPARE:
+ case FLG1_COMPARE_FLG2_BORROW:
event = COUNTER_EVENT_THRESHOLD;
break;
- case QUAD8_EVENT_CARRY_BORROW:
+ case FLG1_CARRYBORROW_FLG2_UD:
event = COUNTER_EVENT_OVERFLOW_UNDERFLOW;
break;
- case QUAD8_EVENT_INDEX:
+ case FLG1_INDX_FLG2_E:
event = COUNTER_EVENT_INDEX;
break;
default:
/* should never reach this path */
WARN_ONCE(true, "invalid interrupt trigger function %u configured for channel %lu\n",
- priv->irq_trigger[channel], channel);
+ flg_pins, channel);
continue;
}
@@ -1260,8 +1174,9 @@ static irqreturn_t quad8_irq_handler(int irq, void *private)
return IRQ_HANDLED;
}
-static void quad8_init_counter(struct channel_reg __iomem *const chan)
+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);
@@ -1274,15 +1189,21 @@ static void quad8_init_counter(struct channel_reg __iomem *const chan)
iowrite8(0x00, &chan->data);
iowrite8(SELECT_RLD | RESET_BT_CT_CPT_S_IDX, &chan->control);
iowrite8(SELECT_RLD | RESET_E, &chan->control);
+
/* Binary encoding; Normal count; non-quadrature mode */
- iowrite8(SELECT_CMR | BINARY | u8_encode_bits(NORMAL_COUNT, COUNT_MODE) |
- u8_encode_bits(NON_QUADRATURE, QUADRATURE_MODE), &chan->control);
+ priv->cmr[channel] = SELECT_CMR | BINARY | u8_encode_bits(NORMAL_COUNT, COUNT_MODE) |
+ u8_encode_bits(NON_QUADRATURE, QUADRATURE_MODE);
+ iowrite8(priv->cmr[channel], &chan->control);
+
/* Disable A and B inputs; preset on index; FLG1 as Carry */
- iowrite8(SELECT_IOR | DISABLE_AB | u8_encode_bits(LOAD_CNTR, LOAD_PIN) |
- u8_encode_bits(FLG1_CARRY_FLG2_BORROW, FLG_PINS), &chan->control);
+ priv->ior[channel] = SELECT_IOR | DISABLE_AB | u8_encode_bits(LOAD_CNTR, LOAD_PIN) |
+ u8_encode_bits(FLG1_CARRY_FLG2_BORROW, FLG_PINS);
+ iowrite8(priv->ior[channel], &chan->control);
+
/* Disable index function; negative index polarity */
- iowrite8(SELECT_IDR | u8_encode_bits(DISABLE_INDEX_MODE, INDEX_MODE) |
- u8_encode_bits(NEGATIVE_INDEX_POLARITY, INDEX_POLARITY), &chan->control);
+ priv->idr[channel] = SELECT_IDR | u8_encode_bits(DISABLE_INDEX_MODE, INDEX_MODE) |
+ u8_encode_bits(NEGATIVE_INDEX_POLARITY, INDEX_POLARITY);
+ iowrite8(priv->idr[channel], &chan->control);
}
static int quad8_probe(struct device *dev, unsigned int id)
@@ -1324,7 +1245,7 @@ static int quad8_probe(struct device *dev, unsigned int id)
iowrite8(RESET_COUNTERS | DISABLE_INTERRUPT_FUNCTION, &priv->reg->channel_oper);
/* Set initial configuration for all counters */
for (i = 0; i < QUAD8_NUM_COUNTERS; i++)
- quad8_init_counter(priv->reg->channel + i);
+ quad8_init_counter(priv, i);
/* Disable Differential Encoder Cable Status for all channels */
iowrite8(0xFF, &priv->reg->cable_status);
/* Enable all counters and enable interrupt function */
--
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 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 fe0887e6185d..02c5499378b6 100644
--- a/drivers/counter/104-quad-8.c
+++ b/drivers/counter/104-quad-8.c
@@ -231,52 +231,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);
@@ -770,21 +774,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)
{
@@ -796,6 +785,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);
@@ -842,6 +832,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;
@@ -960,24 +951,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);
@@ -1177,18 +1172,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
Hi William,
I love your patch! Yet something to improve:
[auto build test ERROR on 00f4bc5184c19cb33f468f1ea409d70d19f8f502]
url: https://github.com/intel-lab-lkp/linux/commits/William-Breathitt-Gray/counter-104-quad-8-Utilize-bitfield-access-macros/20230324-052655
base: 00f4bc5184c19cb33f468f1ea409d70d19f8f502
patch link: https://lore.kernel.org/r/c0e914e2b2d5876265494df3ca102edcf259f02a.1679605919.git.william.gray%40linaro.org
patch subject: [PATCH v2 3/3] counter: 104-quad-8: Utilize helper functions to handle PR, FLAG and PSC
config: riscv-allmodconfig (https://download.01.org/0day-ci/archive/20230324/[email protected]/config)
compiler: riscv64-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/5da9052d37541a9470e1f9b6621d41b52c148b34
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review William-Breathitt-Gray/counter-104-quad-8-Utilize-bitfield-access-macros/20230324-052655
git checkout 5da9052d37541a9470e1f9b6621d41b52c148b34
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=riscv olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash drivers/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
In file included from drivers/counter/104-quad-8.c:8:
In function 'field_multiplier',
inlined from 'field_mask' at include/linux/bitfield.h:170:17,
inlined from 'u8_encode_bits' at include/linux/bitfield.h:198:1,
inlined from 'u8p_replace_bits' at include/linux/bitfield.h:198:1,
inlined from 'quad8_control_register_update' at drivers/counter/104-quad-8.c:206:2:
>> include/linux/bitfield.h:165:17: error: call to '__bad_mask' declared with attribute error: bad bitfield mask
165 | __bad_mask();
| ^~~~~~~~~~~~
In function 'field_multiplier',
inlined from 'u8_encode_bits' at include/linux/bitfield.h:198:1,
inlined from 'u8p_replace_bits' at include/linux/bitfield.h:198:1,
inlined from 'quad8_control_register_update' at drivers/counter/104-quad-8.c:206:2:
>> include/linux/bitfield.h:165:17: error: call to '__bad_mask' declared with attribute error: bad bitfield mask
165 | __bad_mask();
| ^~~~~~~~~~~~
vim +/__bad_mask +165 include/linux/bitfield.h
e2192de59e457a Johannes Berg 2023-01-18 119
e2192de59e457a Johannes Berg 2023-01-18 120 /**
e2192de59e457a Johannes Berg 2023-01-18 121 * FIELD_PREP_CONST() - prepare a constant bitfield element
e2192de59e457a Johannes Berg 2023-01-18 122 * @_mask: shifted mask defining the field's length and position
e2192de59e457a Johannes Berg 2023-01-18 123 * @_val: value to put in the field
e2192de59e457a Johannes Berg 2023-01-18 124 *
e2192de59e457a Johannes Berg 2023-01-18 125 * FIELD_PREP_CONST() masks and shifts up the value. The result should
e2192de59e457a Johannes Berg 2023-01-18 126 * be combined with other fields of the bitfield using logical OR.
e2192de59e457a Johannes Berg 2023-01-18 127 *
e2192de59e457a Johannes Berg 2023-01-18 128 * Unlike FIELD_PREP() this is a constant expression and can therefore
e2192de59e457a Johannes Berg 2023-01-18 129 * be used in initializers. Error checking is less comfortable for this
e2192de59e457a Johannes Berg 2023-01-18 130 * version, and non-constant masks cannot be used.
e2192de59e457a Johannes Berg 2023-01-18 131 */
e2192de59e457a Johannes Berg 2023-01-18 132 #define FIELD_PREP_CONST(_mask, _val) \
e2192de59e457a Johannes Berg 2023-01-18 133 ( \
e2192de59e457a Johannes Berg 2023-01-18 134 /* mask must be non-zero */ \
e2192de59e457a Johannes Berg 2023-01-18 135 BUILD_BUG_ON_ZERO((_mask) == 0) + \
e2192de59e457a Johannes Berg 2023-01-18 136 /* check if value fits */ \
e2192de59e457a Johannes Berg 2023-01-18 137 BUILD_BUG_ON_ZERO(~((_mask) >> __bf_shf(_mask)) & (_val)) + \
e2192de59e457a Johannes Berg 2023-01-18 138 /* check if mask is contiguous */ \
e2192de59e457a Johannes Berg 2023-01-18 139 __BF_CHECK_POW2((_mask) + (1ULL << __bf_shf(_mask))) + \
e2192de59e457a Johannes Berg 2023-01-18 140 /* and create the value */ \
e2192de59e457a Johannes Berg 2023-01-18 141 (((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask)) \
e2192de59e457a Johannes Berg 2023-01-18 142 )
e2192de59e457a Johannes Berg 2023-01-18 143
3e9b3112ec74f1 Jakub Kicinski 2016-08-31 144 /**
3e9b3112ec74f1 Jakub Kicinski 2016-08-31 145 * FIELD_GET() - extract a bitfield element
3e9b3112ec74f1 Jakub Kicinski 2016-08-31 146 * @_mask: shifted mask defining the field's length and position
7240767450d6d8 Masahiro Yamada 2017-10-03 147 * @_reg: value of entire bitfield
3e9b3112ec74f1 Jakub Kicinski 2016-08-31 148 *
3e9b3112ec74f1 Jakub Kicinski 2016-08-31 149 * FIELD_GET() extracts the field specified by @_mask from the
3e9b3112ec74f1 Jakub Kicinski 2016-08-31 150 * bitfield passed in as @_reg by masking and shifting it down.
3e9b3112ec74f1 Jakub Kicinski 2016-08-31 151 */
3e9b3112ec74f1 Jakub Kicinski 2016-08-31 152 #define FIELD_GET(_mask, _reg) \
3e9b3112ec74f1 Jakub Kicinski 2016-08-31 153 ({ \
3e9b3112ec74f1 Jakub Kicinski 2016-08-31 154 __BF_FIELD_CHECK(_mask, _reg, 0U, "FIELD_GET: "); \
3e9b3112ec74f1 Jakub Kicinski 2016-08-31 155 (typeof(_mask))(((_reg) & (_mask)) >> __bf_shf(_mask)); \
3e9b3112ec74f1 Jakub Kicinski 2016-08-31 156 })
3e9b3112ec74f1 Jakub Kicinski 2016-08-31 157
e7d4a95da86e0b Johannes Berg 2018-06-20 158 extern void __compiletime_error("value doesn't fit into mask")
00b0c9b82663ac Al Viro 2017-12-14 159 __field_overflow(void);
00b0c9b82663ac Al Viro 2017-12-14 160 extern void __compiletime_error("bad bitfield mask")
00b0c9b82663ac Al Viro 2017-12-14 161 __bad_mask(void);
00b0c9b82663ac Al Viro 2017-12-14 162 static __always_inline u64 field_multiplier(u64 field)
00b0c9b82663ac Al Viro 2017-12-14 163 {
00b0c9b82663ac Al Viro 2017-12-14 164 if ((field | (field - 1)) & ((field | (field - 1)) + 1))
00b0c9b82663ac Al Viro 2017-12-14 @165 __bad_mask();
00b0c9b82663ac Al Viro 2017-12-14 166 return field & -field;
00b0c9b82663ac Al Viro 2017-12-14 167 }
00b0c9b82663ac Al Viro 2017-12-14 168 static __always_inline u64 field_mask(u64 field)
00b0c9b82663ac Al Viro 2017-12-14 169 {
00b0c9b82663ac Al Viro 2017-12-14 170 return field / field_multiplier(field);
00b0c9b82663ac Al Viro 2017-12-14 171 }
e31a50162feb35 Alex Elder 2020-03-12 172 #define field_max(field) ((typeof(field))field_mask(field))
00b0c9b82663ac Al Viro 2017-12-14 173 #define ____MAKE_OP(type,base,to,from) \
00b0c9b82663ac Al Viro 2017-12-14 174 static __always_inline __##type type##_encode_bits(base v, base field) \
00b0c9b82663ac Al Viro 2017-12-14 175 { \
e7d4a95da86e0b Johannes Berg 2018-06-20 176 if (__builtin_constant_p(v) && (v & ~field_mask(field))) \
00b0c9b82663ac Al Viro 2017-12-14 177 __field_overflow(); \
00b0c9b82663ac Al Viro 2017-12-14 178 return to((v & field_mask(field)) * field_multiplier(field)); \
00b0c9b82663ac Al Viro 2017-12-14 179 } \
00b0c9b82663ac Al Viro 2017-12-14 180 static __always_inline __##type type##_replace_bits(__##type old, \
00b0c9b82663ac Al Viro 2017-12-14 181 base val, base field) \
00b0c9b82663ac Al Viro 2017-12-14 182 { \
00b0c9b82663ac Al Viro 2017-12-14 183 return (old & ~to(field)) | type##_encode_bits(val, field); \
00b0c9b82663ac Al Viro 2017-12-14 184 } \
00b0c9b82663ac Al Viro 2017-12-14 185 static __always_inline void type##p_replace_bits(__##type *p, \
00b0c9b82663ac Al Viro 2017-12-14 186 base val, base field) \
00b0c9b82663ac Al Viro 2017-12-14 187 { \
00b0c9b82663ac Al Viro 2017-12-14 188 *p = (*p & ~to(field)) | type##_encode_bits(val, field); \
00b0c9b82663ac Al Viro 2017-12-14 189 } \
00b0c9b82663ac Al Viro 2017-12-14 190 static __always_inline base type##_get_bits(__##type v, base field) \
00b0c9b82663ac Al Viro 2017-12-14 191 { \
00b0c9b82663ac Al Viro 2017-12-14 192 return (from(v) & field)/field_multiplier(field); \
00b0c9b82663ac Al Viro 2017-12-14 193 }
00b0c9b82663ac Al Viro 2017-12-14 194 #define __MAKE_OP(size) \
00b0c9b82663ac Al Viro 2017-12-14 195 ____MAKE_OP(le##size,u##size,cpu_to_le##size,le##size##_to_cpu) \
00b0c9b82663ac Al Viro 2017-12-14 196 ____MAKE_OP(be##size,u##size,cpu_to_be##size,be##size##_to_cpu) \
00b0c9b82663ac Al Viro 2017-12-14 197 ____MAKE_OP(u##size,u##size,,)
37a3862e123826 Johannes Berg 2018-06-20 198 ____MAKE_OP(u8,u8,,)
00b0c9b82663ac Al Viro 2017-12-14 199 __MAKE_OP(16)
00b0c9b82663ac Al Viro 2017-12-14 200 __MAKE_OP(32)
00b0c9b82663ac Al Viro 2017-12-14 201 __MAKE_OP(64)
00b0c9b82663ac Al Viro 2017-12-14 202 #undef __MAKE_OP
00b0c9b82663ac Al Viro 2017-12-14 203 #undef ____MAKE_OP
00b0c9b82663ac Al Viro 2017-12-14 204
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
On Thu, Mar 23, 2023 at 05:25:28PM -0400, William Breathitt Gray wrote:
> The 104-quad-8 driver buffers the device configuration states
> separately, however each device has only three control registers: CMR,
> IOR, and IDR. Refactoring to buffer the states of these control
> registers rather than each configuration separately results in succinct
> code that more closely matches what is happening on the device.
...
> +static void quad8_control_register_update(struct quad8 *const priv, u8 *const buf,
> + const size_t channel, const u8 val, const u8 field)
> +{
> + u8p_replace_bits(&buf[channel], val, field);
> + iowrite8(buf[channel], &priv->reg->channel[channel].control);
> +}
How did you compile this?
Due to nature of *_replace_bits() this may only be a macro.
That's what LKP is telling about I think.
--
With Best Regards,
Andy Shevchenko
On Fri, Mar 24, 2023 at 01:48:43PM +0200, Andy Shevchenko wrote:
> On Thu, Mar 23, 2023 at 05:25:28PM -0400, William Breathitt Gray wrote:
...
> > +static void quad8_control_register_update(struct quad8 *const priv, u8 *const buf,
> > + const size_t channel, const u8 val, const u8 field)
> > +{
> > + u8p_replace_bits(&buf[channel], val, field);
> > + iowrite8(buf[channel], &priv->reg->channel[channel].control);
> > +}
>
> How did you compile this?
> Due to nature of *_replace_bits() this may only be a macro.
>
> That's what LKP is telling about I think.
Ah, no, that's because the last parameter is not constant in the last patch in
the series.
--
With Best Regards,
Andy Shevchenko
On Fri, Mar 24, 2023 at 01:50:07PM +0200, Andy Shevchenko wrote:
> On Fri, Mar 24, 2023 at 01:48:43PM +0200, Andy Shevchenko wrote:
> > On Thu, Mar 23, 2023 at 05:25:28PM -0400, William Breathitt Gray wrote:
>
> ...
>
> > > +static void quad8_control_register_update(struct quad8 *const priv, u8 *const buf,
> > > + const size_t channel, const u8 val, const u8 field)
> > > +{
> > > + u8p_replace_bits(&buf[channel], val, field);
> > > + iowrite8(buf[channel], &priv->reg->channel[channel].control);
> > > +}
> >
> > How did you compile this?
> > Due to nature of *_replace_bits() this may only be a macro.
> >
> > That's what LKP is telling about I think.
>
> Ah, no, that's because the last parameter is not constant in the last patch in
> the series.
>
> --
> With Best Regards,
> Andy Shevchenko
I'm having trouble cross-compiling for riscv, but I'm unable to recreate
the build error when I compile for x86_64. However, I'd like to
understand this error so I can fix it properly.
Is the problem here due to the "const u8 field" parameter? Instead of a
constant variable, does this need to be a constant literal value for
u8p_replace_bits()? I don't think that parameter changed in the last
patch of the series, so why is the build error occurring for the last
patch and not this penultimate patch here? Would qualifying the
quad8_control_register_update() function with "__always_inline" resolve
this issue?
William Breathitt Gray
On Fri, Mar 24, 2023 at 09:26:14AM -0400, William Breathitt Gray wrote:
> On Fri, Mar 24, 2023 at 01:50:07PM +0200, Andy Shevchenko wrote:
> > On Fri, Mar 24, 2023 at 01:48:43PM +0200, Andy Shevchenko wrote:
> > > On Thu, Mar 23, 2023 at 05:25:28PM -0400, William Breathitt Gray wrote:
...
> > > > +static void quad8_control_register_update(struct quad8 *const priv, u8 *const buf,
> > > > + const size_t channel, const u8 val, const u8 field)
> > > > +{
> > > > + u8p_replace_bits(&buf[channel], val, field);
> > > > + iowrite8(buf[channel], &priv->reg->channel[channel].control);
> > > > +}
> > >
> > > How did you compile this?
> > > Due to nature of *_replace_bits() this may only be a macro.
> > >
> > > That's what LKP is telling about I think.
> >
> > Ah, no, that's because the last parameter is not constant in the last patch in
> > the series.
> I'm having trouble cross-compiling for riscv, but I'm unable to recreate
> the build error when I compile for x86_64. However, I'd like to
> understand this error so I can fix it properly.
>
> Is the problem here due to the "const u8 field" parameter? Instead of a
> constant variable, does this need to be a constant literal value for
> u8p_replace_bits()? I don't think that parameter changed in the last
> patch of the series, so why is the build error occurring for the last
> patch and not this penultimate patch here?
Good question. Perhaps my understanding is incorrect.
> Would qualifying the
> quad8_control_register_update() function with "__always_inline" resolve
> this issue?
Hmm... Don't know. You can always download a toolchain specifically build for
building kernels: https://mirrors.edge.kernel.org/pub/tools/crosstool/.
--
With Best Regards,
Andy Shevchenko
On Fri, Mar 24, 2023 at 03:48:19PM +0200, Andy Shevchenko wrote:
> On Fri, Mar 24, 2023 at 09:26:14AM -0400, William Breathitt Gray wrote:
> > On Fri, Mar 24, 2023 at 01:50:07PM +0200, Andy Shevchenko wrote:
> > > On Fri, Mar 24, 2023 at 01:48:43PM +0200, Andy Shevchenko wrote:
> > > > On Thu, Mar 23, 2023 at 05:25:28PM -0400, William Breathitt Gray wrote:
>
> ...
>
> > > > > +static void quad8_control_register_update(struct quad8 *const priv, u8 *const buf,
> > > > > + const size_t channel, const u8 val, const u8 field)
> > > > > +{
> > > > > + u8p_replace_bits(&buf[channel], val, field);
> > > > > + iowrite8(buf[channel], &priv->reg->channel[channel].control);
> > > > > +}
> > > >
> > > > How did you compile this?
> > > > Due to nature of *_replace_bits() this may only be a macro.
> > > >
> > > > That's what LKP is telling about I think.
> > >
> > > Ah, no, that's because the last parameter is not constant in the last patch in
> > > the series.
>
> > I'm having trouble cross-compiling for riscv, but I'm unable to recreate
> > the build error when I compile for x86_64. However, I'd like to
> > understand this error so I can fix it properly.
> >
> > Is the problem here due to the "const u8 field" parameter? Instead of a
> > constant variable, does this need to be a constant literal value for
> > u8p_replace_bits()? I don't think that parameter changed in the last
> > patch of the series, so why is the build error occurring for the last
> > patch and not this penultimate patch here?
>
> Good question. Perhaps my understanding is incorrect.
>
> > Would qualifying the
> > quad8_control_register_update() function with "__always_inline" resolve
> > this issue?
>
> Hmm... Don't know. You can always download a toolchain specifically build for
> building kernels: https://mirrors.edge.kernel.org/pub/tools/crosstool/.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
I tried to build using crosstools, so I followed the reproduce sequence
from the kernel test robot message: download make.cross, fetch branch
and checkout commit, and then I executed the following (I'm on an arm64
system):
URL=https://cdn.kernel.org/pub/tools/crosstool/files/bin/arm64 COMPILER_INSTALL_PATH=$HOME/Projects/Linux/testwilliam/0day COMPILER=gcc-12.1.0 ../make.cross W=1 O=build_dir ARCH=riscv olddefconfig
URL=https://cdn.kernel.org/pub/tools/crosstool/files/bin/arm64 COMPILER_INSTALL_PATH=$HOME/Projects/Linux/testwilliam/0day COMPILER=gcc-12.1.0 ../make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash drivers/
I encountered the following errors regarding GCC plugins:
cc1: error: cannot load plugin ./scripts/gcc-plugins/randomize_layout_plugin.so: ./scripts/gcc-plugins/randomize_layout_plugin.so: undefined symbol: _ZN8opt_pass14set_pass_paramEjb
cc1: error: cannot load plugin ./scripts/gcc-plugins/latent_entropy_plugin.so: ./scripts/gcc-plugins/latent_entropy_plugin.so: undefined symbol: _ZN8opt_pass14set_pass_paramEjb
cc1: error: cannot load plugin ./scripts/gcc-plugins/randomize_layout_plugin.so: ./scripts/gcc-plugins/randomize_layout_plugin.so: undefined symbol: _ZN8opt_pass14set_pass_paramEjb
make[2]: *** [../scripts/Makefile.build:252: scripts/mod/empty.o] Error 1
cc1: error: cannot load plugin ./scripts/gcc-plugins/latent_entropy_plugin.so: ./scripts/gcc-plugins/latent_entropy_plugin.so: undefined symbol: _ZN8opt_pass14set_pass_paramEjb
I'm not quite sure how to resolve that plugin issue, but regardless I
continued investigating the original build error reported by the kernel
test robot.
There are eight calls to quad8_control_register_update() in 104-quad-8:
quad8_control_register_update(priv, priv->idr, id, DISABLE_INDEX_MODE, INDEX_MODE);
quad8_control_register_update(priv, priv->cmr, id, mode_cfg, QUADRATURE_MODE);
quad8_control_register_update(priv, priv->ior, event_node->channel, flg_pins, FLG_PINS);
quad8_control_register_update(priv, priv->idr, channel_id, index_polarity, INDEX_POLARITY);
quad8_control_register_update(priv, priv->idr, channel_id, synchronous_mode, INDEX_MODE);
quad8_control_register_update(priv, priv->cmr, count->id, count_mode, COUNT_MODE);
quad8_control_register_update(priv, priv->ior, count->id, enable, AB_GATE);
quad8_control_register_update(priv, priv->ior, count->id, !preset_enable, LOAD_PIN);
The field arguments for these calls are all preprocessor defines:
#define INDEX_MODE BIT(0)
#define QUADRATURE_MODE GENMASK(4, 3)
#define FLG_PINS GENMASK(4, 3)
#define INDEX_POLARITY BIT(1)
#define COUNT_MODE GENMASK(2, 1)
#define AB_GATE BIT(0)
#define LOAD_PIN BIT(1)
Removing the duplicates, we get the following four field masks:
BIT(0)
BIT(1)
GENMASK(2, 1)
GENMASK(4, 3)
I don't think there's a problem with these masks (unless there's
something broken in the BIT() or GENMASK() implementations for riscv) so
I'm suspecting something is wrong in bitfields.h. Here's the relevant
function:
extern void __compiletime_error("bad bitfield mask")
__bad_mask(void);
static __always_inline u64 field_multiplier(u64 field)
{
if ((field | (field - 1)) & ((field | (field - 1)) + 1))
__bad_mask();
return field & -field;
}
If I compute the conditional check by hand, it evaluates to false for
all four possible field masks. Is it possible the compiler is ignoring
the if statement evaluation and attempting the __bad_mask() compilation
regardless of the field passed in?
William Breathitt Gray
On Fri, Mar 24, 2023 at 11:35:02AM -0400, William Breathitt Gray wrote:
> There are eight calls to quad8_control_register_update() in 104-quad-8:
>
> quad8_control_register_update(priv, priv->idr, id, DISABLE_INDEX_MODE, INDEX_MODE);
> quad8_control_register_update(priv, priv->cmr, id, mode_cfg, QUADRATURE_MODE);
> quad8_control_register_update(priv, priv->ior, event_node->channel, flg_pins, FLG_PINS);
> quad8_control_register_update(priv, priv->idr, channel_id, index_polarity, INDEX_POLARITY);
> quad8_control_register_update(priv, priv->idr, channel_id, synchronous_mode, INDEX_MODE);
> quad8_control_register_update(priv, priv->cmr, count->id, count_mode, COUNT_MODE);
> quad8_control_register_update(priv, priv->ior, count->id, enable, AB_GATE);
> quad8_control_register_update(priv, priv->ior, count->id, !preset_enable, LOAD_PIN);
I attempted the cross-compiling using an x86-64 system and I was able to
recreate the build error. I tried to isolate the problem line by
commenting out quad8_control_register_update() calls and discover that
this appears to be an inline issue after all: if there are more than six
calls to quad8_control_register_update() are in the code, then the
'__bad_mask' build error occurs.
The build error doesn't occur if I force the inline via __always_inline,
so I'll add that to quad8_control_register_update() to resolve this
issue and submit a v3 patchset later this week.
William Breathitt Gray
+Cc clang (for the ideas you might have, while the issue seems related to GCC[?] )
On Sun, Mar 26, 2023 at 08:01:23PM -0400, William Breathitt Gray wrote:
> On Fri, Mar 24, 2023 at 11:35:02AM -0400, William Breathitt Gray wrote:
> > There are eight calls to quad8_control_register_update() in 104-quad-8:
> >
> > quad8_control_register_update(priv, priv->idr, id, DISABLE_INDEX_MODE, INDEX_MODE);
> > quad8_control_register_update(priv, priv->cmr, id, mode_cfg, QUADRATURE_MODE);
> > quad8_control_register_update(priv, priv->ior, event_node->channel, flg_pins, FLG_PINS);
> > quad8_control_register_update(priv, priv->idr, channel_id, index_polarity, INDEX_POLARITY);
> > quad8_control_register_update(priv, priv->idr, channel_id, synchronous_mode, INDEX_MODE);
> > quad8_control_register_update(priv, priv->cmr, count->id, count_mode, COUNT_MODE);
> > quad8_control_register_update(priv, priv->ior, count->id, enable, AB_GATE);
> > quad8_control_register_update(priv, priv->ior, count->id, !preset_enable, LOAD_PIN);
>
> I attempted the cross-compiling using an x86-64 system and I was able to
> recreate the build error. I tried to isolate the problem line by
> commenting out quad8_control_register_update() calls and discover that
> this appears to be an inline issue after all: if there are more than six
> calls to quad8_control_register_update() are in the code, then the
> '__bad_mask' build error occurs.
>
> The build error doesn't occur if I force the inline via __always_inline,
> so I'll add that to quad8_control_register_update() to resolve this
> issue and submit a v3 patchset later this week.
Doe it mean it's a compiler error? Or is it a code error?
I'm wondering if clang also fails here.
--
With Best Regards,
Andy Shevchenko
On Mon, Mar 27, 2023 at 12:55:04PM +0300, Andy Shevchenko wrote:
> +Cc clang (for the ideas you might have, while the issue seems related to GCC[?] )
>
> On Sun, Mar 26, 2023 at 08:01:23PM -0400, William Breathitt Gray wrote:
> > On Fri, Mar 24, 2023 at 11:35:02AM -0400, William Breathitt Gray wrote:
> > > There are eight calls to quad8_control_register_update() in 104-quad-8:
> > >
> > > quad8_control_register_update(priv, priv->idr, id, DISABLE_INDEX_MODE, INDEX_MODE);
> > > quad8_control_register_update(priv, priv->cmr, id, mode_cfg, QUADRATURE_MODE);
> > > quad8_control_register_update(priv, priv->ior, event_node->channel, flg_pins, FLG_PINS);
> > > quad8_control_register_update(priv, priv->idr, channel_id, index_polarity, INDEX_POLARITY);
> > > quad8_control_register_update(priv, priv->idr, channel_id, synchronous_mode, INDEX_MODE);
> > > quad8_control_register_update(priv, priv->cmr, count->id, count_mode, COUNT_MODE);
> > > quad8_control_register_update(priv, priv->ior, count->id, enable, AB_GATE);
> > > quad8_control_register_update(priv, priv->ior, count->id, !preset_enable, LOAD_PIN);
> >
> > I attempted the cross-compiling using an x86-64 system and I was able to
> > recreate the build error. I tried to isolate the problem line by
> > commenting out quad8_control_register_update() calls and discover that
> > this appears to be an inline issue after all: if there are more than six
> > calls to quad8_control_register_update() are in the code, then the
> > '__bad_mask' build error occurs.
> >
> > The build error doesn't occur if I force the inline via __always_inline,
> > so I'll add that to quad8_control_register_update() to resolve this
> > issue and submit a v3 patchset later this week.
>
> Doe it mean it's a compiler error? Or is it a code error?
>
> I'm wondering if clang also fails here.
>
> --
> With Best Regards,
> Andy Shevchenko
Al, I think you were the one who introduced the field_multiplier()
implementation in commit 00b0c9b82663ac ("Add primitives for
manipulating bitfields both in host- and fixed-endian."). Is this build
error [0] expected in your opinion?
I see that the field specification must be a constant according to the
commit description, so does that mean a "const u8 field" parameter is
valid? Does the field_multiplier() implementation have an expectation
that the condition check will be evaluated by the compiler during the
build and bypass the __bad_mask() compile time error so that it doesn't
appear?
William Breathitt Gray
[0] https://lore.kernel.org/all/[email protected]/