Re: [PATCH v2 04/10] iio: adc: at91-sama5d2_adc: convert to platform specific data structures
From: Eugen.Hristev
Date: Tue Aug 31 2021 - 07:50:07 EST
On 8/30/21 5:44 PM, Jonathan Cameron wrote:
> On Mon, 30 Aug 2021 12:31:46 +0000
> <Eugen.Hristev@xxxxxxxxxxxxx> wrote:
>
>> On 8/24/21 2:54 PM, Eugen Hristev 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)
>>
>> Hi Jonathan,
>>
>> While we are here, regarding the above line, I cannot tell why did I
>> multiply by two the differential channel count. This makes some gaps in
>> the number of channels when we put them all in the same table.
>>
>> I did not change this as it would break the ABI regarding the bindings
>> for the touchscreen #adc-cells phandle references.
>
> Really? Xlate is based off scan_index, not these values I think...
I figured it out.
We have 12 single channels [0..11] then 6 differential [12, 14, 16, 18,
20, 22] spaced by two, a timestamp [23], then we have the X channel
index at 24, Y at 25 and pressure are 26.
These are reflected in this file :
https://elixir.bootlin.com/linux/latest/source/include/dt-bindings/iio/adc/at91-sama5d2_adc.h
And the resistive touchscreen connects to the adc with these indexes.
And I realised why we have the padding/spaces.
It was the original design of the driver, in the line which I
highlighted now below in the patch : (and I never changed this since
working on this driver, because it was a break of the ABI in channel
numbers ...)
>
>>
>> However, I am thinking if there was a reason for it or it was a slip
>> when I initially wrote this.
>
> I've no idea :) I can't immediately see why we'd need the padding.
>
>>
>> Do you think there is any reason to change it and tighten the holes in
>> the indexes list ?
>
> I don't think it matters. As far as I can tell they are used mostly (possibly
> entirely) for internal management and not exposed to any of the ABIs etc.
>
> Jonathan
>
>>
>>
>> Eugen
>>
>>> -#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, \
Here it is: we always added num to the single channel count, but num
goes incrementally in steps of two : 0, 2, 4, 6, 8, 10
>>> + .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)
>>> +#define at91_adc_read_chan(st, reg) \
>>> + readl_relaxed((st)->base + reg)
>>> +#define at91_adc_writel(st, reg, val) \
>>> + writel_relaxed(val, (st)->base + (st)->soc_info.platform->layout->reg)
>>> +
>>> +/**
>>> + * struct at91_adc_platform - at91-sama5d2 platform information struct
>>> + * @layout: pointer to the reg layout struct
>>> + * @adc_channels: pointer to an array of channels for registering in
>>> + * the iio subsystem
>>> + * @nr_channels: number of physical channels available
>>> + * @touch_chan_x: index of the touchscreen X channel
>>> + * @touch_chan_y: index of the touchscreen Y channel
>>> + * @touch_chan_p: index of the touchscreen P channel
>>> + * @max_channels: number of total channels
>>> + * @hw_trig_cnt: number of possible hardware triggers
>>> + */
>>> +struct at91_adc_platform {
>>> + const struct at91_adc_reg_layout *layout;
>>> + const struct iio_chan_spec (*adc_channels)[];
>>> + unsigned int nr_channels;
>>> + unsigned int touch_chan_x;
>>> + unsigned int touch_chan_y;
>>> + unsigned int touch_chan_p;
>>> + unsigned int max_channels;
>>> + unsigned int hw_trig_cnt;
>>> +};
>>>
>>> +/**
>>> + * struct at91_adc_soc_info - at91-sama5d2 soc information struct
>>> + * @startup_time: device startup time
>>> + * @min_sample_rate: minimum sample rate in Hz
>>> + * @max_sample_rate: maximum sample rate in Hz
>>> + * @platform: pointer to the platform structure
>>> + */
>>> struct at91_adc_soc_info {
>>> unsigned startup_time;
>>> unsigned min_sample_rate;
>>> unsigned max_sample_rate;
>>> + const struct at91_adc_platform *platform;
>>> };
>>>
>>> struct at91_adc_trigger {
>>> @@ -382,6 +429,15 @@ struct at91_adc_touch {
>>> struct work_struct workq;
>>> };
>>>
>>> +/*
>>> + * Buffer size requirements:
>>> + * No channels * bytes_per_channel(2) + timestamp bytes (8)
>>> + * Divided by 2 because we need half words.
>>> + * We assume 32 channels for now, has to be increased if needed.
>>> + * Nobody minds a buffer being too big.
>>> + */
>>> +#define AT91_BUFFER_MAX_HWORDS ((32 * 2 + 8) / 2)
>>> +
>>> struct at91_adc_state {
>>> void __iomem *base;
>>> int irq;
>>> @@ -437,29 +493,49 @@ static const struct at91_adc_trigger at91_adc_trigger_list[] = {
>>> },
>>> };
>>>
>>> -static const struct iio_chan_spec at91_adc_channels[] = {
>>> - AT91_SAMA5D2_CHAN_SINGLE(0, 0x50),
>>> - AT91_SAMA5D2_CHAN_SINGLE(1, 0x54),
>>> - AT91_SAMA5D2_CHAN_SINGLE(2, 0x58),
>>> - AT91_SAMA5D2_CHAN_SINGLE(3, 0x5c),
>>> - AT91_SAMA5D2_CHAN_SINGLE(4, 0x60),
>>> - AT91_SAMA5D2_CHAN_SINGLE(5, 0x64),
>>> - AT91_SAMA5D2_CHAN_SINGLE(6, 0x68),
>>> - AT91_SAMA5D2_CHAN_SINGLE(7, 0x6c),
>>> - AT91_SAMA5D2_CHAN_SINGLE(8, 0x70),
>>> - AT91_SAMA5D2_CHAN_SINGLE(9, 0x74),
>>> - AT91_SAMA5D2_CHAN_SINGLE(10, 0x78),
>>> - AT91_SAMA5D2_CHAN_SINGLE(11, 0x7c),
>>> - AT91_SAMA5D2_CHAN_DIFF(0, 1, 0x50),
>>> - AT91_SAMA5D2_CHAN_DIFF(2, 3, 0x58),
>>> - AT91_SAMA5D2_CHAN_DIFF(4, 5, 0x60),
>>> - AT91_SAMA5D2_CHAN_DIFF(6, 7, 0x68),
>>> - AT91_SAMA5D2_CHAN_DIFF(8, 9, 0x70),
>>> - AT91_SAMA5D2_CHAN_DIFF(10, 11, 0x78),
you can see it here,
>>> - IIO_CHAN_SOFT_TIMESTAMP(AT91_SAMA5D2_TIMESTAMP_CHAN_IDX),
>>> - AT91_SAMA5D2_CHAN_TOUCH(AT91_SAMA5D2_TOUCH_X_CHAN_IDX, "x", IIO_MOD_X),
>>> - AT91_SAMA5D2_CHAN_TOUCH(AT91_SAMA5D2_TOUCH_Y_CHAN_IDX, "y", IIO_MOD_Y),
>>> - AT91_SAMA5D2_CHAN_PRESSURE(AT91_SAMA5D2_TOUCH_P_CHAN_IDX, "pressure"),
>>> +static const struct iio_chan_spec at91_sama5d2_adc_channels[] = {
>>> + AT91_SAMA5D2_CHAN_SINGLE(0, 0, 0x50),
>>> + AT91_SAMA5D2_CHAN_SINGLE(1, 1, 0x54),
>>> + AT91_SAMA5D2_CHAN_SINGLE(2, 2, 0x58),
>>> + AT91_SAMA5D2_CHAN_SINGLE(3, 3, 0x5c),
>>> + AT91_SAMA5D2_CHAN_SINGLE(4, 4, 0x60),
>>> + AT91_SAMA5D2_CHAN_SINGLE(5, 5, 0x64),
>>> + AT91_SAMA5D2_CHAN_SINGLE(6, 6, 0x68),
>>> + AT91_SAMA5D2_CHAN_SINGLE(7, 7, 0x6c),
>>> + AT91_SAMA5D2_CHAN_SINGLE(8, 8, 0x70),
>>> + AT91_SAMA5D2_CHAN_SINGLE(9, 9, 0x74),
>>> + AT91_SAMA5D2_CHAN_SINGLE(10, 10, 0x78),
>>> + AT91_SAMA5D2_CHAN_SINGLE(11, 11, 0x7c),
>>> + AT91_SAMA5D2_CHAN_DIFF(12, 0, 1, 0x50),
>>> + AT91_SAMA5D2_CHAN_DIFF(13, 2, 3, 0x58),
>>> + AT91_SAMA5D2_CHAN_DIFF(14, 4, 5, 0x60),
>>> + AT91_SAMA5D2_CHAN_DIFF(15, 6, 7, 0x68),
>>> + AT91_SAMA5D2_CHAN_DIFF(16, 8, 9, 0x70),
>>> + AT91_SAMA5D2_CHAN_DIFF(17, 10, 11, 0x78),
>>> + IIO_CHAN_SOFT_TIMESTAMP(18),
>>> + AT91_SAMA5D2_CHAN_TOUCH(19, "x", IIO_MOD_X),
>>> + AT91_SAMA5D2_CHAN_TOUCH(20, "y", IIO_MOD_Y),
>>> + AT91_SAMA5D2_CHAN_PRESSURE(21, "pressure"),
And I have to come up with a new version of the patch to fix this.
It should be like this:
>>> + AT91_SAMA5D2_CHAN_SINGLE(11, 11, 0x7c),
>>> + AT91_SAMA5D2_CHAN_DIFF(12, 0, 1, 0x50),
>>> + AT91_SAMA5D2_CHAN_DIFF(14, 2, 3, 0x58),
>>> + AT91_SAMA5D2_CHAN_DIFF(16, 4, 5, 0x60),
>>> + AT91_SAMA5D2_CHAN_DIFF(18, 6, 7, 0x68),
>>> + AT91_SAMA5D2_CHAN_DIFF(20, 8, 9, 0x70),
>>> + AT91_SAMA5D2_CHAN_DIFF(22, 10, 11, 0x78),
>>> + IIO_CHAN_SOFT_TIMESTAMP(23),
>>> + AT91_SAMA5D2_CHAN_TOUCH(24, "x", IIO_MOD_X),
>>> + AT91_SAMA5D2_CHAN_TOUCH(25, "y", IIO_MOD_Y),
>>> + AT91_SAMA5D2_CHAN_PRESSURE(26, "pressure"),
Sorry about this. Do you want me to resend the whole series together
with the fixes found by robots ?
I can redo the series, retest it, and send it tomorrow as a v3.
Thanks,
Eugen
>>> +};
>>> +
[snip]