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

From: [email-masked] (Josh Wu)
Date: Wed, 17 Jul 2013 18:09:34 +0800

Hi, Nicolas

Thanks for your review.

On 7/17/2013 3:58 PM, Nicolas Ferre wrote:
> On 14/07/2013 10:04, Josh Wu :
>> 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 <[email-masked]>
>> ---
>>   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 */
>
> Which product has this kind of startup mask: 0xf? Compared with 9260 
> and 9g45 values, it seems strange to me: maybe a little comment should 
> be added.

 From the datasheet, In at91sam9x5 and sama5d3x, the startup time mask 
is become 0xf. In meanwhile, the startup time calculation method is also 
changed.

Before at91sam9x5, Startup Time = (STARTUP+1) * 8/ADCCLK

Since at91sam9x5,  Startup Time = <find the following lookup table>
         0 SUT0 0 periods of ADCClock
         1 SUT8 8 periods of ADCClock
         2 SUT16 16 periods of ADCClock
         3 SUT24 24 periods of ADCClock
         4 SUT64 64 periods of ADCClock
         5 SUT80 80 periods of ADCClock
         ...    ...
         14 SUT896 896 periods of ADCClock
         15 SUT960 960 periods of ADCClock

so only 4bits, it still can define < 960's ADC clock.

In my 3/5 patch, it implement this calculation method.

>
>
>> +#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;
>> +};
>> +
>
> If we move to DT and compatible string for these values, maybe a 
> separate structure won't be needed....

yes, if we move the mask as DT property.
But another way is define above structure in driver. Driver will load 
the correct mask by checking the compatible string. In this way, we 
don't exposure all IP details out to DT.
I prefer the second way in the moment.

By answer this question, I just had a second thought of the ADC 
compatible implementation:
For touch ADC, there should have 3 catalogs:
   9260/9g20: not support touch.
   9g45: support touch, but no average, no TSMR. Need a software filter
   9x5, sama5: support touch, with average, TSMR. Sample data function 
changed a lot from 9g45.

>
>
>>   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;
>
> Here is where AT91_ADC_STARTUP may need another name: what about 
> AT91_ADC_STARTUP_9x5 to match the "compatibility" of this value?

ok.

>
>
>> +    case 0x200:    /* AT91SAM9M10/9G45 */
>> +        st->caps.mr_prescal_mask = AT91_ADC_PRESCAL;
>> +
>> +        if ((version & 0xf00) == 0x200)
>
> Sorry but I do not understand this test in a switch/case entry where 
> it should always be true...

Since the switch case is fall through, that means the higher IP 
(at91sam9x5, sama5) will also running this check, but this line only for 
9G45 chip.

a little bit hard to read  :)

>
>
>> +            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;
>> +    };
>> +}
>> +
>>   static int at91_adc_probe(struct platform_device *pdev)
>>   {
>>       unsigned int prsc, mstrclk, ticks, adc_clk, adc_clk_khz, shtim;
>> @@ -645,6 +687,8 @@ static int at91_adc_probe(struct platform_device 
>> *pdev)
>>           goto error_free_device;
>>       }
>>
>> +    atmel_adc_get_cap(st);
>> +
>>       st->clk = devm_clk_get(&pdev->dev, "adc_clk");
>>       if (IS_ERR(st->clk)) {
>>           dev_err(&pdev->dev, "Failed to get the clock.\n");
>> @@ -704,8 +748,8 @@ static int at91_adc_probe(struct platform_device 
>> *pdev)
>>       shtim = round_up((st->sample_hold_time * adc_clk_khz /
>>                 1000) - 1, 1);
>>
>> -    reg = AT91_ADC_PRESCAL_(prsc) & AT91_ADC_PRESCAL;
>> -    reg |= AT91_ADC_STARTUP_(ticks) & AT91_ADC_STARTUP;
>> +    reg = AT91_ADC_PRESCAL_(prsc) & st->caps.mr_prescal_mask;
>> +    reg |= AT91_ADC_STARTUP_(ticks) & st->caps.mr_startup_mask;
>>       if (st->low_res)
>>           reg |= AT91_ADC_LOWRES;
>>       if (st->sleep_mode)
>>
>
>

Thanks again.
Best Regards,
Josh Wu