| Message ID | 1373789069-11604-3-git-send-email-josh.wu@atmel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. Maxime
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. Ok, that make sense. I will use compatible names for the capabilities in next version. Thanks. > > Maxime > Best Regards, Josh Wu
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,
On Tue, Jul 16, 2013 at 10:46:28AM +0200, Nicolas Ferre wrote: > >>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"? Well, the register holding the IP version seem to be not programmed in some cases (or, at least, the driver handles this case). So, what would happen if one SoC was in such case? You wouldn't be able to use the ADC/touchscreen, even though the IP in itself might very well work, which doesn't sound very nice, while the DT will always be there, and will always have a compatible property. > >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... And you know my feeling about this for quite some time already ;) Maxime
Dear Nicolas Ferre, On Tue, 16 Jul 2013 10:46:28 +0200, Nicolas Ferre wrote: > > 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... I've asked exactly this question last week at Linaro Connect during the ARM SoC consolidation panel/discussion, where Grant Likely, Arnd Bergmann, Olof and others were answering Device Tree related questions. My question, which precisely had the at91-adc DT binding in mind was precisely whether we should use different compatible properties to identify different revisions of an IP block and let the driver handle those differences, or whether the DT binding should provide sufficient properties (register offsets, bit numbers, etc.) to make the driver independent of the IP revisions. And clearly, the answer was that different compatible properties should be used to identify the different versions of the IP block, and the driver should abstract out the differences. I.e, was has been done for at91-adc is completely the opposite of the best practices for Device Tree on ARM. See http://www.youtube.com/watch?v=zF_AXLgkFy4&feature=player_detailpage#t=1581s where I ask exactly this question, and get answers from Olof Johansson and Grant Likely. They clearly say that the solution of having separate compatible properties and a driver that handles the differences is the way to go. So the way at91-adc (and possibly other at91 drivers) is using the Device Tree is wrong, there should have been multiple compatible properties. It's a shame because this is something we did say when we submitted at91-adc and during the reviews, but the maintainer wasn't listening to our comments... Best regards, Thomas
On 07/16/2013 12:30 PM, Thomas Petazzoni wrote: > Dear Nicolas Ferre, > > On Tue, 16 Jul 2013 10:46:28 +0200, Nicolas Ferre wrote: > >>> 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... > > I've asked exactly this question last week at Linaro Connect during the > ARM SoC consolidation panel/discussion, where Grant Likely, Arnd > Bergmann, Olof and others were answering Device Tree related questions. > > My question, which precisely had the at91-adc DT binding in mind was > precisely whether we should use different compatible properties to > identify different revisions of an IP block and let the driver handle > those differences, or whether the DT binding should provide sufficient > properties (register offsets, bit numbers, etc.) to make the driver > independent of the IP revisions. And clearly, the answer was that > different compatible properties should be used to identify the > different versions of the IP block, and the driver should abstract out > the differences. I.e, was has been done for at91-adc is completely the > opposite of the best practices for Device Tree on ARM. > > See > http://www.youtube.com/watch?v=zF_AXLgkFy4&feature=player_detailpage#t=1581s > where I ask exactly this question, and get answers from Olof Johansson > and Grant Likely. They clearly say that the solution of having separate > compatible properties and a driver that handles the differences is the > way to go. So the way at91-adc (and possibly other at91 drivers) is > using the Device Tree is wrong, there should have been multiple > compatible properties. It's a shame because this is something we did say > when we submitted at91-adc and during the reviews, but the maintainer > wasn't listening to our comments... > Thanks for getting some clarity on this Thomas. So I'll ask the somewhat obvious question - how do we unwind from where we are to where we want to be wrt to the bindings?
Dear Jonathan Cameron, On Tue, 16 Jul 2013 20:03:38 +0100, Jonathan Cameron wrote: > On 07/16/2013 12:30 PM, Thomas Petazzoni wrote: > > I've asked exactly this question last week at Linaro Connect during the > > ARM SoC consolidation panel/discussion, where Grant Likely, Arnd > > Bergmann, Olof and others were answering Device Tree related questions. > > > > My question, which precisely had the at91-adc DT binding in mind was > > precisely whether we should use different compatible properties to > > identify different revisions of an IP block and let the driver handle > > those differences, or whether the DT binding should provide sufficient > > properties (register offsets, bit numbers, etc.) to make the driver > > independent of the IP revisions. And clearly, the answer was that > > different compatible properties should be used to identify the > > different versions of the IP block, and the driver should abstract out > > the differences. I.e, was has been done for at91-adc is completely the > > opposite of the best practices for Device Tree on ARM. > > > > See > > http://www.youtube.com/watch?v=zF_AXLgkFy4&feature=player_detailpage#t=1581s > > where I ask exactly this question, and get answers from Olof Johansson > > and Grant Likely. They clearly say that the solution of having separate > > compatible properties and a driver that handles the differences is the > > way to go. So the way at91-adc (and possibly other at91 drivers) is > > using the Device Tree is wrong, there should have been multiple > > compatible properties. It's a shame because this is something we did say > > when we submitted at91-adc and during the reviews, but the maintainer > > wasn't listening to our comments... > > > > Thanks for getting some clarity on this Thomas. So I'll ask the somewhat obvious > question - how do we unwind from where we are to where we want to be wrt to the > bindings? During Linaro Connect last week, there was some discussion about marking DT bindings as unstable for a little while, once they get reviewed by a group of DT "experts" that mark them as stable. Until they are stable, the kernel does not offer any ABI guarantees, and we are free to change the DT bindings as needed. Now, since this unstable/stable thing is not in place at the moment, deciding whether to break or not existing bindings is something to be decided by the maintainer of this platform, judging what is the best option depending on whether there are already many users of the DT for this platform or not, for example. Best regards, Thomas
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@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 */ 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. > +#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.... > 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? > + 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... > + 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) >
On 15/07/2013 14:58, Maxime Ripard : > 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. Do not forget that we still have a handful of platforms that do not support DT (and never will). Josh, do you think that we can deal with this with different compatible string instead of checking the IP revision? Bye,
On 16/07/2013 21:17, Thomas Petazzoni : > Dear Jonathan Cameron, > > On Tue, 16 Jul 2013 20:03:38 +0100, Jonathan Cameron wrote: > >> On 07/16/2013 12:30 PM, Thomas Petazzoni wrote: >>> I've asked exactly this question last week at Linaro Connect during the >>> ARM SoC consolidation panel/discussion, where Grant Likely, Arnd >>> Bergmann, Olof and others were answering Device Tree related questions. >>> >>> My question, which precisely had the at91-adc DT binding in mind was >>> precisely whether we should use different compatible properties to >>> identify different revisions of an IP block and let the driver handle >>> those differences, or whether the DT binding should provide sufficient >>> properties (register offsets, bit numbers, etc.) to make the driver >>> independent of the IP revisions. And clearly, the answer was that >>> different compatible properties should be used to identify the >>> different versions of the IP block, and the driver should abstract out >>> the differences. I.e, was has been done for at91-adc is completely the >>> opposite of the best practices for Device Tree on ARM. >>> >>> See >>> http://www.youtube.com/watch?v=zF_AXLgkFy4&feature=player_detailpage#t=1581s >>> where I ask exactly this question, and get answers from Olof Johansson >>> and Grant Likely. They clearly say that the solution of having separate >>> compatible properties and a driver that handles the differences is the >>> way to go. So the way at91-adc (and possibly other at91 drivers) is >>> using the Device Tree is wrong, there should have been multiple >>> compatible properties. It's a shame because this is something we did say >>> when we submitted at91-adc and during the reviews, but the maintainer >>> wasn't listening to our comments... >>> >> >> Thanks for getting some clarity on this Thomas. So I'll ask the somewhat obvious >> question - how do we unwind from where we are to where we want to be wrt to the >> bindings? > > During Linaro Connect last week, there was some discussion about > marking DT bindings as unstable for a little while, once they get > reviewed by a group of DT "experts" that mark them as stable. Until > they are stable, the kernel does not offer any ABI guarantees, and we > are free to change the DT bindings as needed. > > Now, since this unstable/stable thing is not in place at the moment, > deciding whether to break or not existing bindings is something to be > decided by the maintainer of this platform, judging what is the best > option depending on whether there are already many users of the DT for > this platform or not, for example. I didn't had in mind that the current discussion about the addition of some properties could cast doubt on the entire at91-adc binding! The binding itself has several drawbacks and is kind of over engineered, I agree with that. Some register offsets in particular have nothing to do in a DT binding. On the other hand, some values are highly dependent on the SoC process itself and can't be stored in the driver because it would require to change the driver for each new SoC, depending on the electrical characteristics. In conclusion, we have to be cautious with this binding and make sure that we don't throw the baby out with the bath water. Moreover, at the time we are just beginning to be comfortable with DT on AT91 and beginning to overcome the difficulty of converting our platforms, I see this new step on the path to "mainline + DT stable" as another slowdown. Bye,
Hi, Nicolas On 7/17/2013 4:12 PM, Nicolas Ferre wrote: > On 15/07/2013 14:58, Maxime Ripard : >> 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. > > Do not forget that we still have a handful of platforms that do not > support DT (and never will). > > Josh, do you think that we can deal with this with different > compatible string instead of checking the IP revision? yes, the simplest way is define additional adc_cap structure for different compatible string. like following: static const struct of_device_id at91_adc_dt_ids[] = { { .compatible = "atmel,at91sam9260-adc", .data =&at91sam9260-adc-caps }, { .compatible = "atmel,at91sam9x5-adc", .data =&at91sam9x5-adc-caps }, {}, }; Then we can define the capability for different chip very easy. > > Bye, Best Regards, Josh Wu
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@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 */ > > 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
Hi Nicolas, On Wed, Jul 17, 2013 at 10:12:05AM +0200, Nicolas Ferre wrote: > On 15/07/2013 14:58, Maxime Ripard : > >On Sun, Jul 14, 2013 at 04:04:26PM +0800, Josh Wu wrote: > >>+/* > >>+ * 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. > > Do not forget that we still have a handful of platforms that do not > support DT (and never will). You can do pretty much the same thing with board-file-probed device drivers, using the id_table field of the platform_driver structure and use the driver_data field of the associated platform_device_id array to store the needed values. (see for example drivers/gpio/gpio-mxs.c) Maxime
On 07/17/2013 11:09 AM, Josh Wu wrote: > 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@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 */ >> >> 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 :) Indeed. Write the case statements out long hand without the full throughs. It'll take a couple more lines but be much easer 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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
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; + }; +} + 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)
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(-)