Re: [PATCH v2 04/10] iio: adc: at91-sama5d2_adc: convert to platform specific data structures

[<Eugen.Hristev@microchip.com>]

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@microchip.com> 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@microchip.com>
>> ---
>>   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 !