[PATCH 4/4] iio: adc: at91-sama5d2: add support for sama7g5 device

Jonathan Cameron jic23 at kernel.org
Mon Mar 8 19:54:08 GMT 2021


On Mon, 8 Mar 2021 13:27:48 +0000
<Eugen.Hristev at microchip.com> wrote:

> On 06.03.2021 19:30, Jonathan Cameron wrote:
> > On Mon, 1 Mar 2021 16:32:56 +0200
> > Eugen Hristev <eugen.hristev at microchip.com> wrote:
> >   
> >> Add support to sama7g5 ADC which is similar with sama5d2/sam9x60 device.
> >> Differences are highlighted by compatible.
> >> Main differences include 16 channels instead of 12 and missing
> >> resistive touchscreen.
> >>
> >> Signed-off-by: Eugen Hristev <eugen.hristev at microchip.com>  
> > 
> > Hi Eugen,
> > 
> > What tends to end up cleaner than the many scattered places you have
> > had to bolt in part support here, is to use a per device type constant
> > structure.  That structure can contain register addresses where that
> > is all the differs between parts, and callbacks for more complex cases.
> > 
> > Along the lines of
> > 
> > static const struct sam5d2_chip_info sam5d2_inf {
> >          .eoc_register = 0x33,
> >          .max_chan_idx = bob,
> >          .ccr = bob_function,
> > ...
> > };
> > 
> > Then you can just put a pointer to this in the match_data and look that
> > up in probe  
> 
> Hi Jonathan,
> 
> 
> Could you have a look a little at this driver though:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/watchdog/sama5d4_wdt.c
> 
> The specific support for 'sam9x60' was added by me, and the driver does 
> not look so bad after all, having if/else clauses nearly everywhere, and 
> getting that 'sam9x60_supported' bit from the compatible string comparison.
> 
> That's the reason why I tried to have this ADC driver in a similar fashion.
> 
> I like your approach as well, I guess it's just a matter of preference. 
> I will start to change things to implement it as you suggested if you 
> don't say otherwise.

It works, but starts to go wrong once we get to a 3rd type and experience
says there will probably be a 3rd type sooner or later.

Once you hit that then the if / else everywhere begins to get really ugly.
I think I'd prefer to take the long term view and wrap it all up in a structure.

thanks,

Jonathan

> 
> Thanks for reviewing,
> Eugen
> 
> >   
> >> ---
> >>   drivers/iio/adc/at91-sama5d2_adc.c | 287 +++++++++++++++++++++++------
> >>   1 file changed, 234 insertions(+), 53 deletions(-)
> >>
> >> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> >> index 066d0ad644ca..d61fa32ef294 100644
> >> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> >> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> >> @@ -1,9 +1,11 @@
> >>   // SPDX-License-Identifier: GPL-2.0-only
> >>   /*
> >> - * Atmel ADC driver for SAMA5D2 devices and compatible.
> >> + * Microchip ADC driver for SAMA5D2/SAMA7G5 devices and compatible.
> >>    *
> >>    * Copyright (C) 2015 Atmel,
> >> - *               2015 Ludovic Desroches <ludovic.desroches at atmel.com>
> >> + *               2015 Ludovic Desroches <ludovic.desroches at microchip.com>,
> >> + *            2021 Microchip Technology, Inc.,
> >> + *            2021 Eugen Hristev <eugen.hristev at microchip.com>
> >>    */
> >>
> >>   #include <linux/bitops.h>
> >> @@ -117,14 +119,26 @@
> >>   #define AT91_SAMA5D2_ISR     0x30
> >>   /* Interrupt Status Register - Pen touching sense status */
> >>   #define AT91_SAMA5D2_ISR_PENS   BIT(31)
> >> +
> >> +/* End of Conversion Interrupt Enable Register */
> >> +#define AT91_SAMA7G5_EOC_IER 0x34
> >> +/* End of Conversion Interrupt Disable Register */
> >> +#define AT91_SAMA7G5_EOC_IDR 0x38
> >> +/* End of Conversion Interrupt Mask Register */
> >> +#define AT91_SAMA7G5_EOC_IMR 0x3c
> >> +/* End of Conversion Interrupt Status Register */
> >> +#define AT91_SAMA7G5_EOC_ISR 0x40
> >> +
> >>   /* Last Channel Trigger Mode Register */
> >>   #define AT91_SAMA5D2_LCTMR   0x34
> >>   /* Last Channel Compare Window Register */
> >>   #define AT91_SAMA5D2_LCCWR   0x38
> >>   /* Overrun Status Register */
> >>   #define AT91_SAMA5D2_OVER    0x3c
> >> +#define AT91_SAMA7G5_OVER    0x4c
> >>   /* Extended Mode Register */
> >>   #define AT91_SAMA5D2_EMR     0x40
> >> +#define AT91_SAMA7G5_EMR     0x50
> >>   /* Extended Mode Register - Oversampling rate */
> >>   #define AT91_SAMA5D2_EMR_OSR(V)                      ((V) << 16)
> >>   #define AT91_SAMA5D2_EMR_OSR_MASK            GENMASK(17, 16)
> >> @@ -142,6 +156,9 @@
> >>   /* Channel Offset Register */
> >>   #define AT91_SAMA5D2_COR     0x4c
> >>   #define AT91_SAMA5D2_COR_DIFF_OFFSET 16
> >> +/* Channel Configuration Register */
> >> +#define AT91_SAMA7G5_CCR     0x5c
> >> +#define AT91_SAMA7G5_COR_DIFF_OFFSET 0
> >>
> >>   /* Analog Control Register */
> >>   #define AT91_SAMA5D2_ACR     0x94
> >> @@ -185,6 +202,7 @@
> >>   #define AT91_SAMA5D2_PRESSR  0xbc
> >>   /* Trigger Register */
> >>   #define AT91_SAMA5D2_TRGR    0xc0
> >> +#define AT91_SAMA7G5_TRGR    0x100
> >>   /* Mask for TRGMOD field of TRGR register */
> >>   #define AT91_SAMA5D2_TRGR_TRGMOD_MASK GENMASK(2, 0)
> >>   /* No trigger, only software trigger can start conversions */
> >> @@ -214,19 +232,26 @@
> >>   #define AT91_SAMA5D2_WPSR    0xe8
> >>   /* Version Register */
> >>   #define AT91_SAMA5D2_VERSION 0xfc
> >> +#define AT91_SAMA7G5_VERSION 0x130
> >>
> >>   #define AT91_SAMA5D2_HW_TRIG_CNT 3
> >>   #define AT91_SAMA5D2_SINGLE_CHAN_CNT 12
> >> +#define AT91_SAMA7G5_SINGLE_CHAN_CNT 16
> >>   #define AT91_SAMA5D2_DIFF_CHAN_CNT 6
> >> +#define AT91_SAMA7G5_DIFF_CHAN_CNT 8
> >>
> >>   #define AT91_SAMA5D2_TIMESTAMP_CHAN_IDX (AT91_SAMA5D2_SINGLE_CHAN_CNT + \
> >>                                         AT91_SAMA5D2_DIFF_CHAN_CNT + 1)
> >>
> >> +#define AT91_SAMA7G5_TIMESTAMP_CHAN_IDX (AT91_SAMA7G5_SINGLE_CHAN_CNT + \
> >> +                                      AT91_SAMA7G5_DIFF_CHAN_CNT + 1)
> >> +
> >>   #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
> >> +#define AT91_SAMA7G5_MAX_CHAN_IDX    AT91_SAMA7G5_TIMESTAMP_CHAN_IDX
> >>
> >>   #define AT91_SAMA5D2_TOUCH_SAMPLE_PERIOD_US          2000    /* 2ms */
> >>   #define AT91_SAMA5D2_TOUCH_PEN_DETECT_DEBOUNCE_US    200
> >> @@ -239,8 +264,19 @@
> >>    * 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)
> >> +#define AT91_SAMA5D2_BUFFER_MAX_CONVERSION_BYTES ( \
> >> +                                     (AT91_SAMA5D2_SINGLE_CHAN_CNT + \
> >> +                                     AT91_SAMA5D2_DIFF_CHAN_CNT) * 2)
> >> +
> >> +#define AT91_SAMA7G5_BUFFER_MAX_CONVERSION_BYTES ( \
> >> +                                     (AT91_SAMA7G5_SINGLE_CHAN_CNT + \
> >> +                                     AT91_SAMA7G5_DIFF_CHAN_CNT) * 2)
> >> +
> >> +#define AT91_BUFFER_MAX_CONVERSION_BYTES ( \
> >> +     (AT91_SAMA7G5_BUFFER_MAX_CONVERSION_BYTES > \
> >> +     AT91_SAMA5D2_BUFFER_MAX_CONVERSION_BYTES) ? \
> >> +     AT91_SAMA7G5_BUFFER_MAX_CONVERSION_BYTES : \
> >> +     AT91_SAMA5D2_BUFFER_MAX_CONVERSION_BYTES)
> >>
> >>   /* This total must also include the timestamp */
> >>   #define AT91_BUFFER_MAX_BYTES (AT91_BUFFER_MAX_CONVERSION_BYTES + 8)
> >> @@ -295,6 +331,27 @@
> >>                .indexed = 1,                                           \
> >>        }
> >>
> >> +#define AT91_SAMA7G5_CHAN_DIFF(num, num2, addr)                              \
> >> +     {                                                               \
> >> +             .type = IIO_VOLTAGE,                                    \
> >> +             .differential = 1,                                      \
> >> +             .channel = num,                                         \
> >> +             .channel2 = num2,                                       \
> >> +             .address = addr,                                        \
> >> +             .scan_index = num + AT91_SAMA7G5_SINGLE_CHAN_CNT,       \
> >> +             .scan_type = {                                          \
> >> +                     .sign = 's',                                    \
> >> +                     .realbits = 14,                                 \
> >> +                     .storagebits = 16,                              \
> >> +             },                                                      \
> >> +             .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),           \
> >> +             .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),   \
> >> +             .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ)|\
> >> +                             BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),  \
> >> +             .datasheet_name = "CH"#num"-CH"#num2,                   \
> >> +             .indexed = 1,                                           \
> >> +     }
> >> +
> >>   #define AT91_SAMA5D2_CHAN_TOUCH(num, name, mod)                              \
> >>        {                                                               \
> >>                .type = IIO_POSITIONRELATIVE,                           \
> >> @@ -335,6 +392,8 @@ struct at91_adc_soc_info {
> >>        unsigned                        startup_time;
> >>        unsigned                        min_sample_rate;
> >>        unsigned                        max_sample_rate;
> >> +#define AT91_ADC_SAMA7G5(st)         ((st)->soc_info.sama7g5)
> >> +     bool                            sama7g5;
> >>   };
> >>
> >>   struct at91_adc_trigger {
> >> @@ -436,7 +495,7 @@ static const struct at91_adc_trigger at91_adc_trigger_list[] = {
> >>        },
> >>   };
> >>
> >> -static const struct iio_chan_spec at91_adc_channels[] = {
> >> +static const struct iio_chan_spec at91_sama5d2_adc_channels[] = {
> >>        AT91_SAMA5D2_CHAN_SINGLE(0, 0x50),
> >>        AT91_SAMA5D2_CHAN_SINGLE(1, 0x54),
> >>        AT91_SAMA5D2_CHAN_SINGLE(2, 0x58),
> >> @@ -461,6 +520,42 @@ static const struct iio_chan_spec at91_adc_channels[] = {
> >>        AT91_SAMA5D2_CHAN_PRESSURE(AT91_SAMA5D2_TOUCH_P_CHAN_IDX, "pressure"),
> >>   };
> >>
> >> +static const struct iio_chan_spec at91_sama7g5_adc_channels[] = {
> >> +     AT91_SAMA5D2_CHAN_SINGLE(0, 0x60),
> >> +     AT91_SAMA5D2_CHAN_SINGLE(1, 0x64),
> >> +     AT91_SAMA5D2_CHAN_SINGLE(2, 0x68),
> >> +     AT91_SAMA5D2_CHAN_SINGLE(3, 0x6c),
> >> +     AT91_SAMA5D2_CHAN_SINGLE(4, 0x70),
> >> +     AT91_SAMA5D2_CHAN_SINGLE(5, 0x74),
> >> +     AT91_SAMA5D2_CHAN_SINGLE(6, 0x78),
> >> +     AT91_SAMA5D2_CHAN_SINGLE(7, 0x7c),
> >> +     AT91_SAMA5D2_CHAN_SINGLE(8, 0x80),
> >> +     AT91_SAMA5D2_CHAN_SINGLE(9, 0x84),
> >> +     AT91_SAMA5D2_CHAN_SINGLE(10, 0x88),
> >> +     AT91_SAMA5D2_CHAN_SINGLE(11, 0x8c),
> >> +     AT91_SAMA5D2_CHAN_SINGLE(12, 0x90),
> >> +     AT91_SAMA5D2_CHAN_SINGLE(13, 0x94),
> >> +     AT91_SAMA5D2_CHAN_SINGLE(14, 0x98),
> >> +     AT91_SAMA5D2_CHAN_SINGLE(15, 0x9c),
> >> +     AT91_SAMA7G5_CHAN_DIFF(0, 1, 0x60),
> >> +     AT91_SAMA7G5_CHAN_DIFF(2, 3, 0x68),
> >> +     AT91_SAMA7G5_CHAN_DIFF(4, 5, 0x70),
> >> +     AT91_SAMA7G5_CHAN_DIFF(6, 7, 0x78),
> >> +     AT91_SAMA7G5_CHAN_DIFF(8, 9, 0x80),
> >> +     AT91_SAMA7G5_CHAN_DIFF(10, 11, 0x88),
> >> +     AT91_SAMA7G5_CHAN_DIFF(12, 13, 0x90),
> >> +     AT91_SAMA7G5_CHAN_DIFF(14, 15, 0x98),
> >> +     IIO_CHAN_SOFT_TIMESTAMP(AT91_SAMA7G5_TIMESTAMP_CHAN_IDX),
> >> +};
> >> +
> >> +static unsigned int at91_adc_max_chan_idx(struct at91_adc_state *st)
> >> +{
> >> +     if (AT91_ADC_SAMA7G5(st))
> >> +             return AT91_SAMA7G5_MAX_CHAN_IDX;
> >> +     else
> >> +             return AT91_SAMA5D2_MAX_CHAN_IDX;
> >> +}
> >> +
> >>   static int at91_adc_chan_xlate(struct iio_dev *indio_dev, int chan)
> >>   {
> >>        int i;
> >> @@ -492,6 +587,7 @@ static unsigned int at91_adc_active_scan_mask_to_reg(struct iio_dev *indio_dev)
> >>   {
> >>        u32 mask = 0;
> >>        u8 bit;
> >> +     struct at91_adc_state *st = iio_priv(indio_dev);
> >>
> >>        for_each_set_bit(bit, indio_dev->active_scan_mask,
> >>                         indio_dev->num_channels) {
> >> @@ -500,13 +596,78 @@ static unsigned int at91_adc_active_scan_mask_to_reg(struct iio_dev *indio_dev)
> >>                mask |= BIT(chan->channel);
> >>        }
> >>
> >> -     return mask & GENMASK(11, 0);
> >> +     return mask & GENMASK(at91_adc_max_chan_idx(st), 0);
> >> +}
> >> +
> >> +static void at91_adc_ccr(struct at91_adc_state *st,
> >> +                      struct iio_chan_spec const *chan)
> >> +{
> >> +     u32 ccr, cur_ccr;
> >> +
> >> +     ccr = (BIT(chan->channel) | BIT(chan->channel2));
> >> +
> >> +     if (AT91_ADC_SAMA7G5(st)) {
> >> +             cur_ccr = at91_adc_readl(st, AT91_SAMA7G5_CCR);
> >> +             ccr <<= AT91_SAMA7G5_COR_DIFF_OFFSET;
> >> +             if (chan->differential)
> >> +                     at91_adc_writel(st, AT91_SAMA7G5_CCR, cur_ccr | ccr);
> >> +             else
> >> +                     at91_adc_writel(st, AT91_SAMA7G5_CCR, cur_ccr & ~ccr);
> >> +     } else {
> >> +             cur_ccr = at91_adc_readl(st, AT91_SAMA5D2_COR);
> >> +             ccr <<= AT91_SAMA5D2_COR_DIFF_OFFSET;
> >> +             if (chan->differential)
> >> +                     at91_adc_writel(st, AT91_SAMA5D2_COR, cur_ccr | ccr);
> >> +             else
> >> +                     at91_adc_writel(st, AT91_SAMA5D2_COR, cur_ccr & ~ccr);
> >> +     }
> >> +}
> >> +
> >> +static void at91_adc_irq_status(struct at91_adc_state *st, u32 *status,
> >> +                             u32 *eoc)
> >> +{
> >> +     if (AT91_ADC_SAMA7G5(st)) {
> >> +             *status = at91_adc_readl(st, AT91_SAMA5D2_ISR);
> >> +             *eoc = at91_adc_readl(st, AT91_SAMA7G5_EOC_ISR);
> >> +     } else {
> >> +             *status = *eoc = at91_adc_readl(st, AT91_SAMA5D2_ISR);
> >> +     }
> >> +}
> >> +
> >> +static void at91_adc_irq_mask(struct at91_adc_state *st, u32 *status, u32 *eoc)
> >> +{
> >> +     if (AT91_ADC_SAMA7G5(st)) {
> >> +             *status = at91_adc_readl(st, AT91_SAMA5D2_IMR);
> >> +             *eoc = at91_adc_readl(st, AT91_SAMA7G5_EOC_IMR);
> >> +     } else {
> >> +             *status = *eoc = at91_adc_readl(st, AT91_SAMA5D2_IMR);
> >> +     }
> >> +}
> >> +
> >> +static void at91_adc_eoc_dis(struct at91_adc_state *st, unsigned int channel)
> >> +{
> >> +     if (!AT91_ADC_SAMA7G5(st))
> >> +             at91_adc_writel(st, AT91_SAMA5D2_IDR, BIT(channel));
> >> +     /* for SAMA7G5, Errata recommends not writing to EOC_IDR register */
> >> +}
> >> +
> >> +static void at91_adc_eoc_ena(struct at91_adc_state *st, unsigned int channel)
> >> +{
> >> +     if (AT91_ADC_SAMA7G5(st))
> >> +             at91_adc_writel(st, AT91_SAMA7G5_EOC_IER, BIT(channel));
> >> +     else
> >> +             at91_adc_writel(st, AT91_SAMA5D2_IER, BIT(channel));
> >>   }
> >>
> >>   static void at91_adc_config_emr(struct at91_adc_state *st)
> >>   {
> >>        /* configure the extended mode register */
> >> -     unsigned int emr = at91_adc_readl(st, AT91_SAMA5D2_EMR);
> >> +     unsigned int emr;
> >> +
> >> +     if (AT91_ADC_SAMA7G5(st))
> >> +             emr = at91_adc_readl(st, AT91_SAMA7G5_EMR);
> >> +     else
> >> +             emr = at91_adc_readl(st, AT91_SAMA5D2_EMR);
> >>
> >>        /* select oversampling per single trigger event */
> >>        emr |= AT91_SAMA5D2_EMR_ASTE(1);
> >> @@ -530,7 +691,10 @@ static void at91_adc_config_emr(struct at91_adc_state *st)
> >>                break;
> >>        }
> >>
> >> -     at91_adc_writel(st, AT91_SAMA5D2_EMR, emr);
> >> +     if (AT91_ADC_SAMA7G5(st))
> >> +             at91_adc_writel(st, AT91_SAMA7G5_EMR, emr);
> >> +     else
> >> +             at91_adc_writel(st, AT91_SAMA5D2_EMR, emr);
> >>   }
> >>
> >>   static int at91_adc_adjust_val_osr(struct at91_adc_state *st, int *val)
> >> @@ -726,7 +890,12 @@ static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
> >>   {
> >>        struct iio_dev *indio = iio_trigger_get_drvdata(trig);
> >>        struct at91_adc_state *st = iio_priv(indio);
> >> -     u32 status = at91_adc_readl(st, AT91_SAMA5D2_TRGR);
> >> +     u32 status;
> >> +
> >> +     if (AT91_ADC_SAMA7G5(st))
> >> +             status = at91_adc_readl(st, AT91_SAMA7G5_TRGR);
> >> +     else
> >> +             status = at91_adc_readl(st, AT91_SAMA5D2_TRGR);
> >>
> >>        /* clear TRGMOD */
> >>        status &= ~AT91_SAMA5D2_TRGR_TRGMOD_MASK;
> >> @@ -735,7 +904,10 @@ static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
> >>                status |= st->selected_trig->trgmod_value;
> >>
> >>        /* set/unset hw trigger */
> >> -     at91_adc_writel(st, AT91_SAMA5D2_TRGR, status);
> >> +     if (AT91_ADC_SAMA7G5(st))
> >> +             at91_adc_writel(st, AT91_SAMA7G5_TRGR, status);
> >> +     else
> >> +             at91_adc_writel(st, AT91_SAMA5D2_TRGR, status);
> >>
> >>        return 0;
> >>   }
> >> @@ -877,7 +1049,7 @@ static bool at91_adc_current_chan_is_touch(struct iio_dev *indio_dev)
> >>
> >>        return !!bitmap_subset(indio_dev->active_scan_mask,
> >>                               &st->touch_st.channels_bitmask,
> >> -                            AT91_SAMA5D2_MAX_CHAN_IDX + 1);
> >> +                            at91_adc_max_chan_idx(st) + 1);
> >>   }
> >>
> >>   static int at91_adc_buffer_prepare(struct iio_dev *indio_dev)
> >> @@ -905,7 +1077,6 @@ static int at91_adc_buffer_prepare(struct iio_dev *indio_dev)
> >>                         indio_dev->num_channels) {
> >>                struct iio_chan_spec const *chan =
> >>                                        at91_adc_chan_get(indio_dev, bit);
> >> -             u32 cor;
> >>
> >>                if (!chan)
> >>                        continue;
> >> @@ -914,16 +1085,7 @@ static int at91_adc_buffer_prepare(struct iio_dev *indio_dev)
> >>                    chan->type == IIO_PRESSURE)
> >>                        continue;
> >>
> >> -             cor = at91_adc_readl(st, AT91_SAMA5D2_COR);
> >> -
> >> -             if (chan->differential)
> >> -                     cor |= (BIT(chan->channel) | BIT(chan->channel2)) <<
> >> -                             AT91_SAMA5D2_COR_DIFF_OFFSET;
> >> -             else
> >> -                     cor &= ~(BIT(chan->channel) <<
> >> -                            AT91_SAMA5D2_COR_DIFF_OFFSET);
> >> -
> >> -             at91_adc_writel(st, AT91_SAMA5D2_COR, cor);
> >> +             at91_adc_ccr(st, chan);
> >>
> >>                at91_adc_writel(st, AT91_SAMA5D2_CHER, BIT(chan->channel));
> >>        }
> >> @@ -975,7 +1137,10 @@ static int at91_adc_buffer_postdisable(struct iio_dev *indio_dev)
> >>                at91_adc_writel(st, AT91_SAMA5D2_IDR, AT91_SAMA5D2_IER_DRDY);
> >>
> >>        /* read overflow register to clear possible overflow status */
> >> -     at91_adc_readl(st, AT91_SAMA5D2_OVER);
> >> +     if (AT91_ADC_SAMA7G5(st))
> >> +             at91_adc_readl(st, AT91_SAMA7G5_OVER);
> >> +     else
> >> +             at91_adc_readl(st, AT91_SAMA5D2_OVER);
> >>
> >>        /* if we are using DMA we must clear registers and end DMA */
> >>        if (st->dma_st.dma_chan)
> >> @@ -1018,13 +1183,15 @@ static void at91_adc_trigger_handler_nodma(struct iio_dev *indio_dev,
> >>        u8 bit;
> >>        u32 mask = at91_adc_active_scan_mask_to_reg(indio_dev);
> >>        unsigned int timeout = 50;
> >> +     u32 status, imr, eoc = 0, eoc_imr;
> >>
> >>        /*
> >>         * Check if the conversion is ready. If not, wait a little bit, and
> >>         * in case of timeout exit with an error.
> >>         */
> >> -     while ((at91_adc_readl(st, AT91_SAMA5D2_ISR) & mask) != mask &&
> >> -            timeout) {
> >> +     while (((eoc & mask) != mask) && timeout) {
> >> +             at91_adc_irq_status(st, &status, &eoc);
> >> +             at91_adc_irq_mask(st, &imr, &eoc_imr);
> >>                usleep_range(50, 100);
> >>                timeout--;
> >>        }
> >> @@ -1195,7 +1362,7 @@ static void at91_adc_touch_data_handler(struct iio_dev *indio_dev)
> >>        int i = 0;
> >>
> >>        for_each_set_bit(bit, indio_dev->active_scan_mask,
> >> -                      AT91_SAMA5D2_MAX_CHAN_IDX + 1) {
> >> +                      at91_adc_max_chan_idx(st) + 1) {
> >>                struct iio_chan_spec const *chan =
> >>                                         at91_adc_chan_get(indio_dev, bit);
> >>
> >> @@ -1262,12 +1429,14 @@ static irqreturn_t at91_adc_interrupt(int irq, void *private)
> >>   {
> >>        struct iio_dev *indio = private;
> >>        struct at91_adc_state *st = iio_priv(indio);
> >> -     u32 status = at91_adc_readl(st, AT91_SAMA5D2_ISR);
> >> -     u32 imr = at91_adc_readl(st, AT91_SAMA5D2_IMR);
> >> +     u32 status, eoc, imr, eoc_imr;
> >>        u32 rdy_mask = AT91_SAMA5D2_IER_XRDY | AT91_SAMA5D2_IER_YRDY |
> >>                        AT91_SAMA5D2_IER_PRDY;
> >>
> >> -     if (!(status & imr))
> >> +     at91_adc_irq_status(st, &status, &eoc);
> >> +     at91_adc_irq_mask(st, &imr, &eoc_imr);
> >> +
> >> +     if (!(status & imr) && !(eoc & eoc_imr))
> >>                return IRQ_NONE;
> >>        if (status & AT91_SAMA5D2_IER_PEN) {
> >>                /* pen detected IRQ */
> >> @@ -1309,7 +1478,6 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
> >>                                  struct iio_chan_spec const *chan, int *val)
> >>   {
> >>        struct at91_adc_state *st = iio_priv(indio_dev);
> >> -     u32 cor = 0;
> >>        u16 tmp_val;
> >>        int ret;
> >>
> >> @@ -1355,13 +1523,9 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
> >>
> >>        st->chan = chan;
> >>
> >> -     if (chan->differential)
> >> -             cor = (BIT(chan->channel) | BIT(chan->channel2)) <<
> >> -                   AT91_SAMA5D2_COR_DIFF_OFFSET;
> >> -
> >> -     at91_adc_writel(st, AT91_SAMA5D2_COR, cor);
> >> +     at91_adc_ccr(st, chan);
> >>        at91_adc_writel(st, AT91_SAMA5D2_CHER, BIT(chan->channel));
> >> -     at91_adc_writel(st, AT91_SAMA5D2_IER, BIT(chan->channel));
> >> +     at91_adc_eoc_ena(st, chan->channel);
> >>        at91_adc_writel(st, AT91_SAMA5D2_CR, AT91_SAMA5D2_CR_START);
> >>
> >>        ret = wait_event_interruptible_timeout(st->wq_data_available,
> >> @@ -1378,7 +1542,7 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
> >>                st->conversion_done = false;
> >>        }
> >>
> >> -     at91_adc_writel(st, AT91_SAMA5D2_IDR, BIT(chan->channel));
> >> +     at91_adc_eoc_dis(st, st->chan->channel);
> >>        at91_adc_writel(st, AT91_SAMA5D2_CHDR, BIT(chan->channel));
> >>
> >>        /* Needed to ACK the DRDY interruption */
> >> @@ -1577,14 +1741,14 @@ static int at91_adc_update_scan_mode(struct iio_dev *indio_dev,
> >>        struct at91_adc_state *st = iio_priv(indio_dev);
> >>
> >>        if (bitmap_subset(scan_mask, &st->touch_st.channels_bitmask,
> >> -                       AT91_SAMA5D2_MAX_CHAN_IDX + 1))
> >> +                       at91_adc_max_chan_idx(st) + 1))
> >>                return 0;
> >>        /*
> >>         * if the new bitmap is a combination of touchscreen and regular
> >>         * channels, then we are not fine
> >>         */
> >>        if (bitmap_intersects(&st->touch_st.channels_bitmask, scan_mask,
> >> -                           AT91_SAMA5D2_MAX_CHAN_IDX + 1))
> >> +                           at91_adc_max_chan_idx(st) + 1))
> >>                return -EINVAL;
> >>        return 0;
> >>   }
> >> @@ -1594,6 +1758,8 @@ static void at91_adc_hw_init(struct iio_dev *indio_dev)
> >>        struct at91_adc_state *st = iio_priv(indio_dev);
> >>
> >>        at91_adc_writel(st, AT91_SAMA5D2_CR, AT91_SAMA5D2_CR_SWRST);
> >> +     if (AT91_ADC_SAMA7G5(st))
> >> +             at91_adc_writel(st, AT91_SAMA7G5_EOC_IDR, 0xffffffff);
> >>        at91_adc_writel(st, AT91_SAMA5D2_IDR, 0xffffffff);
> >>        /*
> >>         * Transfer field must be set to 2 according to the datasheet and
> >> @@ -1718,18 +1884,27 @@ static int at91_adc_probe(struct platform_device *pdev)
> >>        indio_dev->name = dev_name(&pdev->dev);
> >>        indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
> >>        indio_dev->info = &at91_adc_info;
> >> -     indio_dev->channels = at91_adc_channels;
> >> -     indio_dev->num_channels = ARRAY_SIZE(at91_adc_channels);
> >>
> >>        st = iio_priv(indio_dev);
> >>        st->indio_dev = indio_dev;
> >>
> >> -     bitmap_set(&st->touch_st.channels_bitmask,
> >> -                AT91_SAMA5D2_TOUCH_X_CHAN_IDX, 1);
> >> -     bitmap_set(&st->touch_st.channels_bitmask,
> >> -                AT91_SAMA5D2_TOUCH_Y_CHAN_IDX, 1);
> >> -     bitmap_set(&st->touch_st.channels_bitmask,
> >> -                AT91_SAMA5D2_TOUCH_P_CHAN_IDX, 1);
> >> +     st->soc_info.sama7g5 = of_device_is_compatible(pdev->dev.of_node,
> >> +                                                    "microchip,sama7g5-adc");  
> > 
> > Better to use match_data to get an enum value which is used to index
> > into a table of support part description structures.  Each of those
> > structures has all the registers etc + channel specs and callbacks
> > where needed for more complex handling.
> > 
> > Other advantage is you can introduce the structures in a precursor patch with
> > just one entry for existing behaviour.  The new device support is then added
> > in a second patch.  All we need to do then is check first patch is a noop
> > and that second much shorter patch makes sense.
> >   
> >> +
> >> +     if (AT91_ADC_SAMA7G5(st)) {
> >> +             indio_dev->channels = at91_sama7g5_adc_channels;
> >> +             indio_dev->num_channels = ARRAY_SIZE(at91_sama7g5_adc_channels);
> >> +     } else {
> >> +             indio_dev->channels = at91_sama5d2_adc_channels;
> >> +             indio_dev->num_channels = ARRAY_SIZE(at91_sama5d2_adc_channels);
> >> +
> >> +             bitmap_set(&st->touch_st.channels_bitmask,
> >> +                        AT91_SAMA5D2_TOUCH_X_CHAN_IDX, 1);
> >> +             bitmap_set(&st->touch_st.channels_bitmask,
> >> +                        AT91_SAMA5D2_TOUCH_Y_CHAN_IDX, 1);
> >> +             bitmap_set(&st->touch_st.channels_bitmask,
> >> +                        AT91_SAMA5D2_TOUCH_P_CHAN_IDX, 1);
> >> +     }
> >>
> >>        st->oversampling_ratio = AT91_OSR_1SAMPLES;
> >>
> >> @@ -1853,9 +2028,12 @@ static int at91_adc_probe(struct platform_device *pdev)
> >>                dev_info(&pdev->dev, "setting up trigger as %s\n",
> >>                         st->selected_trig->name);
> >>
> >> -     dev_info(&pdev->dev, "version: %x\n",
> >> -              readl_relaxed(st->base + AT91_SAMA5D2_VERSION));
> >> -
> >> +     if (AT91_ADC_SAMA7G5(st))  
> > 
> > We may be better off with a look up table of all the registers
> > (+ if needed some callback functions) rather than a whole bunch of
> > if statements.
> > 
> > You then just assign the right 'part number specific' structure
> > once in probe.
> >   
> >> +             dev_info(&pdev->dev, "version: %x\n",
> >> +                      at91_adc_readl(st, AT91_SAMA7G5_VERSION));
> >> +     else
> >> +             dev_info(&pdev->dev, "version: %x\n",
> >> +                      at91_adc_readl(st, AT91_SAMA5D2_VERSION));
> >>        return 0;
> >>
> >>   dma_disable:
> >> @@ -1957,6 +2135,8 @@ static SIMPLE_DEV_PM_OPS(at91_adc_pm_ops, at91_adc_suspend, at91_adc_resume);
> >>   static const struct of_device_id at91_adc_dt_match[] = {
> >>        {
> >>                .compatible = "atmel,sama5d2-adc",
> >> +     }, {
> >> +             .compatible = "microchip,sama7g5-adc",
> >>        }, {
> >>                /* sentinel */
> >>        }
> >> @@ -1967,13 +2147,14 @@ static struct platform_driver at91_adc_driver = {
> >>        .probe = at91_adc_probe,
> >>        .remove = at91_adc_remove,
> >>        .driver = {
> >> -             .name = "at91-sama5d2_adc",
> >> +             .name = "at91-sama5d2-sama7g5_adc",  
> > 
> > Please keep the driver name the same. It's common to have a name of one
> > random part a driver supports used even if there are lots of others.
> > This (or wild cards) never scales as more parts are added to a driver.
> >   
> >>                .of_match_table = at91_adc_dt_match,
> >>                .pm = &at91_adc_pm_ops,
> >>        },
> >>   };
> >>   module_platform_driver(at91_adc_driver)
> >>
> >> -MODULE_AUTHOR("Ludovic Desroches <ludovic.desroches at atmel.com>");
> >> -MODULE_DESCRIPTION("Atmel AT91 SAMA5D2 ADC");
> >> +MODULE_AUTHOR("Ludovic Desroches <ludovic.desroches at microchip.com>");
> >> +MODULE_AUTHOR("Eugen Hristev <eugen.hristev at microchip.com>");
> >> +MODULE_DESCRIPTION("Microchip AT91 SAMA5D2/SAMA7G5 ADC");
> >>   MODULE_LICENSE("GPL v2");  
> >   
> 




More information about the linux-arm-kernel mailing list