Re: [PATCH v2 04/10] iio: adc: at91-sama5d2_adc: convert to platform specific data structures
From: Eugen.Hristev
Date: Mon Aug 30 2021 - 08:28:34 EST
On 8/30/21 3:21 PM, Jonathan Cameron wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On Tue, 24 Aug 2021 14:54:35 +0300
> Eugen Hristev <eugen.hristev@xxxxxxxxxxxxx> wrote:
>
>> Convert the driver to platform specific structures. This means:
>> - create a register layout struct that will hold offsets for registers
>> - create a platform struct that will hold platform information (number of
>> channels, indexes for different channels and pointer to layout struct)
>> - convert specific macros that are platform dependent into platform variables
>>
>> This step is in fact a no-op, but allows the driver to be more flexible
>> and for future enhancement including adding new platforms that are partly
>> compatible with the current driver and differ slightly in register layout
>> or capabilities for example.
>>
>> Signed-off-by: Eugen Hristev <eugen.hristev@xxxxxxxxxxxxx>
>> ---
>> drivers/iio/adc/at91-sama5d2_adc.c | 410 +++++++++++++++++------------
>> 1 file changed, 247 insertions(+), 163 deletions(-)
>>
>> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
>> index 9d71dcffcf93..8ede18b8d789 100644
>> --- a/drivers/iio/adc/at91-sama5d2_adc.c
>> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
>> @@ -27,8 +27,9 @@
>> #include <linux/pinctrl/consumer.h>
>> #include <linux/regulator/consumer.h>
>>
>> +struct at91_adc_reg_layout {
>> /* Control Register */
>> -#define AT91_SAMA5D2_CR 0x00
>> + u16 CR;
>> /* Software Reset */
>> #define AT91_SAMA5D2_CR_SWRST BIT(0)
>> /* Start Conversion */
>> @@ -39,7 +40,7 @@
>> #define AT91_SAMA5D2_CR_CMPRST BIT(4)
>>
>> /* Mode Register */
>> -#define AT91_SAMA5D2_MR 0x04
>> + u16 MR;
>> /* Trigger Selection */
>> #define AT91_SAMA5D2_MR_TRGSEL(v) ((v) << 1)
>> /* ADTRG */
>> @@ -82,19 +83,19 @@
>> #define AT91_SAMA5D2_MR_USEQ BIT(31)
>>
>> /* Channel Sequence Register 1 */
>> -#define AT91_SAMA5D2_SEQR1 0x08
>> + u16 SEQR1;
>> /* Channel Sequence Register 2 */
>> -#define AT91_SAMA5D2_SEQR2 0x0c
>> + u16 SEQR2;
>> /* Channel Enable Register */
>> -#define AT91_SAMA5D2_CHER 0x10
>> + u16 CHER;
>> /* Channel Disable Register */
>> -#define AT91_SAMA5D2_CHDR 0x14
>> + u16 CHDR;
>> /* Channel Status Register */
>> -#define AT91_SAMA5D2_CHSR 0x18
>> + u16 CHSR;
>> /* Last Converted Data Register */
>> -#define AT91_SAMA5D2_LCDR 0x20
>> + u16 LCDR;
>> /* Interrupt Enable Register */
>> -#define AT91_SAMA5D2_IER 0x24
>> + u16 IER;
>> /* Interrupt Enable Register - TS X measurement ready */
>> #define AT91_SAMA5D2_IER_XRDY BIT(20)
>> /* Interrupt Enable Register - TS Y measurement ready */
>> @@ -109,22 +110,23 @@
>> #define AT91_SAMA5D2_IER_PEN BIT(29)
>> /* Interrupt Enable Register - No pen detect */
>> #define AT91_SAMA5D2_IER_NOPEN BIT(30)
>> +
>> /* Interrupt Disable Register */
>> -#define AT91_SAMA5D2_IDR 0x28
>> + u16 IDR;
>> /* Interrupt Mask Register */
>> -#define AT91_SAMA5D2_IMR 0x2c
>> + u16 IMR;
>> /* Interrupt Status Register */
>> -#define AT91_SAMA5D2_ISR 0x30
>> + u16 ISR;
>> /* Interrupt Status Register - Pen touching sense status */
>> #define AT91_SAMA5D2_ISR_PENS BIT(31)
>> /* Last Channel Trigger Mode Register */
>> -#define AT91_SAMA5D2_LCTMR 0x34
>> + u16 LCTMR;
>> /* Last Channel Compare Window Register */
>> -#define AT91_SAMA5D2_LCCWR 0x38
>> + u16 LCCWR;
>> /* Overrun Status Register */
>> -#define AT91_SAMA5D2_OVER 0x3c
>> + u16 OVER;
>> /* Extended Mode Register */
>> -#define AT91_SAMA5D2_EMR 0x40
>> + u16 EMR;
>> /* Extended Mode Register - Oversampling rate */
>> #define AT91_SAMA5D2_EMR_OSR(V) ((V) << 16)
>> #define AT91_SAMA5D2_EMR_OSR_MASK GENMASK(17, 16)
>> @@ -134,22 +136,22 @@
>>
>> /* Extended Mode Register - Averaging on single trigger event */
>> #define AT91_SAMA5D2_EMR_ASTE(V) ((V) << 20)
>> +
>> /* Compare Window Register */
>> -#define AT91_SAMA5D2_CWR 0x44
>> + u16 CWR;
>> /* Channel Gain Register */
>> -#define AT91_SAMA5D2_CGR 0x48
>> -
>> + u16 CGR;
>> /* Channel Offset Register */
>> -#define AT91_SAMA5D2_COR 0x4c
>> + u16 COR;
>> #define AT91_SAMA5D2_COR_DIFF_OFFSET 16
>>
>> /* Analog Control Register */
>> -#define AT91_SAMA5D2_ACR 0x94
>> + u16 ACR;
>> /* Analog Control Register - Pen detect sensitivity mask */
>> #define AT91_SAMA5D2_ACR_PENDETSENS_MASK GENMASK(1, 0)
>>
>> /* Touchscreen Mode Register */
>> -#define AT91_SAMA5D2_TSMR 0xb0
>> + u16 TSMR;
>> /* Touchscreen Mode Register - No touch mode */
>> #define AT91_SAMA5D2_TSMR_TSMODE_NONE 0
>> /* Touchscreen Mode Register - 4 wire screen, no pressure measurement */
>> @@ -178,13 +180,13 @@
>> #define AT91_SAMA5D2_TSMR_PENDET_ENA BIT(24)
>>
>> /* Touchscreen X Position Register */
>> -#define AT91_SAMA5D2_XPOSR 0xb4
>> + u16 XPOSR;
>> /* Touchscreen Y Position Register */
>> -#define AT91_SAMA5D2_YPOSR 0xb8
>> + u16 YPOSR;
>> /* Touchscreen Pressure Register */
>> -#define AT91_SAMA5D2_PRESSR 0xbc
>> + u16 PRESSR;
>> /* Trigger Register */
>> -#define AT91_SAMA5D2_TRGR 0xc0
>> + u16 TRGR;
>> /* Mask for TRGMOD field of TRGR register */
>> #define AT91_SAMA5D2_TRGR_TRGMOD_MASK GENMASK(2, 0)
>> /* No trigger, only software trigger can start conversions */
>> @@ -203,30 +205,52 @@
>> #define AT91_SAMA5D2_TRGR_TRGPER(x) ((x) << 16)
>>
>> /* Correction Select Register */
>> -#define AT91_SAMA5D2_COSR 0xd0
>> + u16 COSR;
>> /* Correction Value Register */
>> -#define AT91_SAMA5D2_CVR 0xd4
>> + u16 CVR;
>> /* Channel Error Correction Register */
>> -#define AT91_SAMA5D2_CECR 0xd8
>> + u16 CECR;
>> /* Write Protection Mode Register */
>> -#define AT91_SAMA5D2_WPMR 0xe4
>> + u16 WPMR;
>> /* Write Protection Status Register */
>> -#define AT91_SAMA5D2_WPSR 0xe8
>> + u16 WPSR;
>> /* Version Register */
>> -#define AT91_SAMA5D2_VERSION 0xfc
>> -
>> -#define AT91_SAMA5D2_HW_TRIG_CNT 3
>> -#define AT91_SAMA5D2_SINGLE_CHAN_CNT 12
>> -#define AT91_SAMA5D2_DIFF_CHAN_CNT 6
>> -
>> -#define AT91_SAMA5D2_TIMESTAMP_CHAN_IDX (AT91_SAMA5D2_SINGLE_CHAN_CNT + \
>> - AT91_SAMA5D2_DIFF_CHAN_CNT + 1)
>> + u16 VERSION;
>> +};
>>
>> -#define AT91_SAMA5D2_TOUCH_X_CHAN_IDX (AT91_SAMA5D2_SINGLE_CHAN_CNT + \
>> - AT91_SAMA5D2_DIFF_CHAN_CNT * 2)
>> -#define AT91_SAMA5D2_TOUCH_Y_CHAN_IDX (AT91_SAMA5D2_TOUCH_X_CHAN_IDX + 1)
>> -#define AT91_SAMA5D2_TOUCH_P_CHAN_IDX (AT91_SAMA5D2_TOUCH_Y_CHAN_IDX + 1)
>> -#define AT91_SAMA5D2_MAX_CHAN_IDX AT91_SAMA5D2_TOUCH_P_CHAN_IDX
>> +static const struct at91_adc_reg_layout sama5d2_layout = {
>> + .CR = 0x00,
>> + .MR = 0x04,
>> + .SEQR1 = 0x08,
>> + .SEQR2 = 0x0c,
>> + .CHER = 0x10,
>> + .CHDR = 0x14,
>> + .CHSR = 0x18,
>> + .LCDR = 0x20,
>> + .IER = 0x24,
>> + .IDR = 0x28,
>> + .IMR = 0x2c,
>> + .ISR = 0x30,
>> + .LCTMR = 0x34,
>> + .LCCWR = 0x38,
>> + .OVER = 0x3c,
>> + .EMR = 0x40,
>> + .CWR = 0x44,
>> + .CGR = 0x48,
>> + .COR = 0x4c,
>> + .ACR = 0x94,
>> + .TSMR = 0xb0,
>> + .XPOSR = 0xb4,
>> + .YPOSR = 0xb8,
>> + .PRESSR = 0xbc,
>> + .TRGR = 0xc0,
>> + .COSR = 0xd0,
>> + .CVR = 0xd4,
>> + .CECR = 0xd8,
>> + .WPMR = 0xe4,
>> + .WPSR = 0xe8,
>> + .VERSION = 0xfc,
>> +};
>>
>> #define AT91_SAMA5D2_TOUCH_SAMPLE_PERIOD_US 2000 /* 2ms */
>> #define AT91_SAMA5D2_TOUCH_PEN_DETECT_DEBOUNCE_US 200
>> @@ -235,18 +259,6 @@
>>
>> #define AT91_SAMA5D2_MAX_POS_BITS 12
>>
>> -/*
>> - * Maximum number of bytes to hold conversion from all channels
>> - * without the timestamp.
>> - */
>> -#define AT91_BUFFER_MAX_CONVERSION_BYTES ((AT91_SAMA5D2_SINGLE_CHAN_CNT + \
>> - AT91_SAMA5D2_DIFF_CHAN_CNT) * 2)
>> -
>> -/* This total must also include the timestamp */
>> -#define AT91_BUFFER_MAX_BYTES (AT91_BUFFER_MAX_CONVERSION_BYTES + 8)
>> -
>> -#define AT91_BUFFER_MAX_HWORDS (AT91_BUFFER_MAX_BYTES / 2)
>> -
>> #define AT91_HWFIFO_MAX_SIZE_STR "128"
>> #define AT91_HWFIFO_MAX_SIZE 128
>>
>> @@ -255,12 +267,12 @@
>> #define AT91_OSR_4SAMPLES 4
>> #define AT91_OSR_16SAMPLES 16
>>
>> -#define AT91_SAMA5D2_CHAN_SINGLE(num, addr) \
>> +#define AT91_SAMA5D2_CHAN_SINGLE(index, num, addr) \
>> { \
>> .type = IIO_VOLTAGE, \
>> .channel = num, \
>> .address = addr, \
>> - .scan_index = num, \
>> + .scan_index = index, \
>> .scan_type = { \
>> .sign = 'u', \
>> .realbits = 14, \
>> @@ -274,14 +286,14 @@
>> .indexed = 1, \
>> }
>>
>> -#define AT91_SAMA5D2_CHAN_DIFF(num, num2, addr) \
>> +#define AT91_SAMA5D2_CHAN_DIFF(index, num, num2, addr) \
>> { \
>> .type = IIO_VOLTAGE, \
>> .differential = 1, \
>> .channel = num, \
>> .channel2 = num2, \
>> .address = addr, \
>> - .scan_index = num + AT91_SAMA5D2_SINGLE_CHAN_CNT, \
>> + .scan_index = index, \
>> .scan_type = { \
>> .sign = 's', \
>> .realbits = 14, \
>> @@ -328,13 +340,48 @@
>> .datasheet_name = name, \
>> }
>>
>> -#define at91_adc_readl(st, reg) readl_relaxed(st->base + reg)
>> -#define at91_adc_writel(st, reg, val) writel_relaxed(val, st->base + reg)
>> +#define at91_adc_readl(st, reg) \
>> + readl_relaxed((st)->base + (st)->soc_info.platform->layout->reg)
> I was a bit in two minds about whether this should be in capitals to make it clear
> macro magic is going on, or whether lowercase is fine. In the end I couldn't make
> up my mind so will accept it like this! :)
>
>
> Jonathan
>
I was doubting as well when I wrote this... but what convinced me was
the much cleaner calls to the reg read and writes.
Has some drawbacks sometimes (like the XPOSR and YPOSR writes), but they
are not all over the code, so I guess it's fine.
Thanks for reviewing !