Re: [PATCH 2/5] iio: at91: Use different prescal, startup mask in MR for different IP

[Nicolas Ferre <nicolas.ferre@atmel.com>]

On 16/07/2013 10:35, Josh Wu :
> Hi, Maxime
>
> On 7/15/2013 8:58 PM, Maxime Ripard wrote:
>> Hi Josh,
>>
>> On Sun, Jul 14, 2013 at 04:04:26PM +0800, Josh Wu wrote:
>>> For at91 boards, there are different IPs for adc. Different IPs has
>>> different
>>> STARTUP & PRESCAL mask in ADC_MR.
>>>
>>> This patch can change the masks according to the different IP version.
>>>
>>> Signed-off-by: Josh Wu <josh.wu@atmel.com>
>>> ---
>>>   arch/arm/mach-at91/include/mach/at91_adc.h |    9 ++++--
>>>   drivers/iio/adc/at91_adc.c                 |   48
>>> ++++++++++++++++++++++++++--
>>>   2 files changed, 53 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-at91/include/mach/at91_adc.h
>>> b/arch/arm/mach-at91/include/mach/at91_adc.h
>>> index 8e7ed5c..ab273ee 100644
>>> --- a/arch/arm/mach-at91/include/mach/at91_adc.h
>>> +++ b/arch/arm/mach-at91/include/mach/at91_adc.h
>>> @@ -28,9 +28,12 @@
>>>   #define            AT91_ADC_TRGSEL_EXTERNAL    (6 << 1)
>>>   #define        AT91_ADC_LOWRES        (1 << 4)    /* Low Resolution */
>>>   #define        AT91_ADC_SLEEP        (1 << 5)    /* Sleep Mode */
>>> -#define        AT91_ADC_PRESCAL    (0x3f << 8)    /* Prescalar Rate
>>> Selection */
>>> +#define        AT91_ADC_PRESCAL    (0xff << 8)    /* Prescalar Rate
>>> Selection */
>>> +#define        AT91_ADC_PRESCAL_9260    (0x3f << 8)
>>>   #define            AT91_ADC_PRESCAL_(x)    ((x) << 8)
>>> -#define        AT91_ADC_STARTUP    (0x1f << 16)    /* Startup Up
>>> Time */
>>> +#define        AT91_ADC_STARTUP    (0xf << 16)    /* Startup Up Time */
>>> +#define        AT91_ADC_STARTUP_9260    (0x1f << 16)
>>> +#define        AT91_ADC_STARTUP_9G45    (0x7f << 16)
>>>   #define            AT91_ADC_STARTUP_(x)    ((x) << 16)
>>>   #define        AT91_ADC_SHTIM        (0xf  << 24)    /* Sample &
>>> Hold Time */
>>>   #define            AT91_ADC_SHTIM_(x)    ((x) << 24)
>>> @@ -58,4 +61,6 @@
>>>   #define AT91_ADC_CHR(n)        (0x30 + ((n) * 4))    /* Channel
>>> Data Register N */
>>>   #define        AT91_ADC_DATA        (0x3ff)
>>> +#define AT91_ADC_VERSION    0xFC
>>> +
>>>   #endif
>>> diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c
>>> index 18bd54f..14e99ba 100644
>>> --- a/drivers/iio/adc/at91_adc.c
>>> +++ b/drivers/iio/adc/at91_adc.c
>>> @@ -39,6 +39,12 @@
>>>   #define at91_adc_writel(st, reg, val) \
>>>       (writel_relaxed(val, st->reg_base + reg))
>>> +struct at91_adc_caps {
>>> +    bool    has_tsmr;    /* only at91sam9x5, sama5d3 have TSMR reg */
>>> +    u32    mr_prescal_mask;
>>> +    u32    mr_startup_mask;
>>> +};
>>> +
>>>   struct at91_adc_state {
>>>       struct clk        *adc_clk;
>>>       u16            *buffer;
>>> @@ -62,6 +68,7 @@ struct at91_adc_state {
>>>       u32            res;        /* resolution used for convertions */
>>>       bool            low_res;    /* the resolution corresponds to
>>> the lowest one */
>>>       wait_queue_head_t    wq_data_avail;
>>> +    struct at91_adc_caps    caps;
>>>   };
>>>   static irqreturn_t at91_adc_trigger_handler(int irq, void *p)
>>> @@ -580,6 +587,41 @@ static const struct iio_info at91_adc_info = {
>>>       .read_raw = &at91_adc_read_raw,
>>>   };
>>> +/*
>>> + * Since atmel adc support different ip for touchscreen mode.
>>> Through the
>>> + * IP check, we will know the touchscreen capbilities.
>>> + */
>>> +static void atmel_adc_get_cap(struct at91_adc_state *st)
>>> +{
>>> +    unsigned int version;
>>> +    struct iio_dev *idev = iio_priv_to_dev(st);
>>> +
>>> +    version = at91_adc_readl(st, AT91_ADC_VERSION);
>>> +    dev_dbg(&idev->dev, "version: 0x%x\n", version);
>>> +
>>> +    st->caps.mr_prescal_mask = AT91_ADC_PRESCAL_9260;
>>> +    st->caps.mr_startup_mask = AT91_ADC_STARTUP_9260;
>>> +
>>> +    /* keep only major version number */
>>> +    switch (version & 0xf00) {
>>> +    case 0x500:    /* SAMA5D3 */
>>> +    case 0x400:    /* AT91SAM9X5/9N12 */
>>> +        st->caps.has_tsmr = 1;
>>> +        st->caps.mr_startup_mask = AT91_ADC_STARTUP;
>>> +    case 0x200:    /* AT91SAM9M10/9G45 */
>>> +        st->caps.mr_prescal_mask = AT91_ADC_PRESCAL;
>>> +
>>> +        if ((version & 0xf00) == 0x200)
>>> +            st->caps.mr_startup_mask = AT91_ADC_STARTUP_9G45;
>>> +    case 0x100:    /* AT91SAM9260/9G20 */
>>> +        break;
>>> +    default:
>>> +        dev_warn(&idev->dev,
>>> +                "Unmanaged adc version, use minimal capabilities\n");
>>> +        break;
>>> +    };
>>> +}
>> Why don't you use different compatible names and derive your
>> capabilities from which compatible is declared.
>>
>> It seems safer.

I see it as handier in the sense that a different IP version can be 
compatible with an older IP version: so we do not need to modify the 
driver just to use another SoC.

On your side Maxime, what makes you say that it is "safer"?

> Ok, that make sense. I will use compatible names for the capabilities in
> next version. Thanks.

Hold on a little bit Josh, I know that Jean-Christophe is not in favor 
of the use of multiple compatible strings. So, as the code is already 
there, let's wait and see if we find another argument...

Bye,
-- 
Nicolas Ferre