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 <josh.wu@xxxxxxxxx>
---
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
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
