LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: <Eugen.Hristev@microchip.com>
To: <jic23@kernel.org>
Cc: <linux-iio@vger.kernel.org>, <devicetree@vger.kernel.org>,
	<Nicolas.Ferre@microchip.com>,
	<linux-arm-kernel@lists.infradead.org>,
	<linux-kernel@vger.kernel.org>, <robh+dt@kernel.org>,
	<Ludovic.Desroches@microchip.com>
Subject: Re: [PATCH v2 04/10] iio: adc: at91-sama5d2_adc: convert to platform specific data structures
Date: Mon, 30 Aug 2021 12:28:27 +0000	[thread overview]
Message-ID: <fa912a33-d264-f634-4f6b-ed17956a54c4@microchip.com> (raw)
In-Reply-To: <20210830132153.78f8dac5@jic23-huawei>

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 !

  reply	other threads:[~2021-08-30 12:28 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-24 11:54 [PATCH v2 00/10] iio: adc: at91-sama5d2_adc: add support for sama7g5 Eugen Hristev
2021-08-24 11:54 ` [PATCH v2 01/10] dt-bindings: iio: adc: at91-sama5d2: add compatible for sama7g5-adc Eugen Hristev
2021-08-24 11:54 ` [PATCH v2 02/10] iio: adc: at91-sama5d2_adc: initialize hardware after clock is started Eugen Hristev
2021-08-24 11:54 ` [PATCH v2 03/10] iio: adc: at91-sama5d2_adc: remove unused definition Eugen Hristev
2021-08-24 11:54 ` [PATCH v2 04/10] iio: adc: at91-sama5d2_adc: convert to platform specific data structures Eugen Hristev
2021-08-30 12:21   ` Jonathan Cameron
2021-08-30 12:28     ` Eugen.Hristev [this message]
2021-08-30 12:31   ` Eugen.Hristev
2021-08-30 14:44     ` Jonathan Cameron
2021-08-31 11:50       ` Eugen.Hristev
2021-09-05 10:08         ` Jonathan Cameron
2021-08-24 11:54 ` [PATCH v2 05/10] iio: adc: at91-sama5d2-adc: add support for separate end of conversion registers Eugen Hristev
2021-08-31  6:45   ` Eugen.Hristev
2021-09-05 10:12     ` Jonathan Cameron
2021-08-24 11:54 ` [PATCH v2 06/10] iio: adc: at91-sama5d2_adc: add helper for COR register Eugen Hristev
2021-08-30 12:18   ` Jonathan Cameron
2021-08-24 11:54 ` [PATCH v2 07/10] iio: adc: at91-sama5d2_adc: add support for sama7g5 device Eugen Hristev
2021-08-24 11:54 ` [PATCH v2 08/10] iio: adc: at91-sama5d2_adc: update copyright and authors information Eugen Hristev
2021-08-24 11:54 ` [PATCH v2 09/10] ARM: dts: at91: sama7g5: add node for the ADC Eugen Hristev
2021-08-24 11:54 ` [PATCH v2 10/10] ARM: dts: at91: sama7g5ek: enable ADC on the board Eugen Hristev
2021-08-30 12:26 ` [PATCH v2 00/10] iio: adc: at91-sama5d2_adc: add support for sama7g5 Jonathan Cameron
2021-09-01  9:47   ` Nicolas Ferre

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=fa912a33-d264-f634-4f6b-ed17956a54c4@microchip.com \
    --to=eugen.hristev@microchip.com \
    --cc=Ludovic.Desroches@microchip.com \
    --cc=Nicolas.Ferre@microchip.com \
    --cc=devicetree@vger.kernel.org \
    --cc=jic23@kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-iio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=robh+dt@kernel.org \
    --subject='Re: [PATCH v2 04/10] iio: adc: at91-sama5d2_adc: convert to platform specific data structures' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).