From: Alim Akhtar <[email protected]>
Exynos's ADC-FSD-HW has some difference in registers set, number of
programmable channels (16 channel) etc. This patch adds support for
ADC-FSD-HW controller version.
Signed-off-by: Alim Akhtar <[email protected]>
Signed-off-by: Tamseel Shams <[email protected]>
---
- Changes since v1
* Addressed Jonathan's comment by using already provided isr handle
drivers/iio/adc/exynos_adc.c | 55 ++++++++++++++++++++++++++++++++++++
1 file changed, 55 insertions(+)
diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
index cff1ba57fb16..183ae591327a 100644
--- a/drivers/iio/adc/exynos_adc.c
+++ b/drivers/iio/adc/exynos_adc.c
@@ -55,6 +55,11 @@
#define ADC_V2_INT_ST(x) ((x) + 0x14)
#define ADC_V2_VER(x) ((x) + 0x20)
+/* ADC_FSD_HW register definitions */
+#define ADC_FSD_DAT(x) ((x) + 0x08)
+#define ADC_FSD_DAT_SUM(x) ((x) + 0x0C)
+#define ADC_FSD_DBG_DATA(x) ((x) + 0x1C)
+
/* Bit definitions for ADC_V1 */
#define ADC_V1_CON_RES (1u << 16)
#define ADC_V1_CON_PRSCEN (1u << 14)
@@ -92,6 +97,7 @@
/* Bit definitions for ADC_V2 */
#define ADC_V2_CON1_SOFT_RESET (1u << 2)
+#define ADC_V2_CON1_SOFT_NON_RESET (1u << 1)
#define ADC_V2_CON2_OSEL (1u << 10)
#define ADC_V2_CON2_ESEL (1u << 9)
@@ -100,6 +106,7 @@
#define ADC_V2_CON2_ACH_SEL(x) (((x) & 0xF) << 0)
#define ADC_V2_CON2_ACH_MASK 0xF
+#define MAX_ADC_FSD_CHANNELS 16
#define MAX_ADC_V2_CHANNELS 10
#define MAX_ADC_V1_CHANNELS 8
#define MAX_EXYNOS3250_ADC_CHANNELS 2
@@ -484,6 +491,43 @@ static const struct exynos_adc_data exynos7_adc_data = {
.start_conv = exynos_adc_v2_start_conv,
};
+static void exynos_adc_fsd_init_hw(struct exynos_adc *info)
+{
+ u32 con2;
+
+ writel(ADC_V2_CON1_SOFT_RESET, ADC_V2_CON1(info->regs));
+
+ writel(ADC_V2_CON1_SOFT_NON_RESET, ADC_V2_CON1(info->regs));
+
+ con2 = ADC_V2_CON2_C_TIME(6);
+ writel(con2, ADC_V2_CON2(info->regs));
+
+ /* Enable interrupts */
+ writel(1, ADC_V2_INT_EN(info->regs));
+}
+
+static void exynos_adc_fsd_exit_hw(struct exynos_adc *info)
+{
+ u32 con2;
+
+ con2 = readl(ADC_V2_CON2(info->regs));
+ con2 &= ~ADC_V2_CON2_C_TIME(7);
+ writel(con2, ADC_V2_CON2(info->regs));
+
+ /* Disable interrupts */
+ writel(0, ADC_V2_INT_EN(info->regs));
+}
+
+static const struct exynos_adc_data fsd_hw_adc_data = {
+ .num_channels = MAX_ADC_FSD_CHANNELS,
+ .mask = ADC_DATX_MASK, /* 12 bit ADC resolution */
+
+ .init_hw = exynos_adc_fsd_init_hw,
+ .exit_hw = exynos_adc_fsd_exit_hw,
+ .clear_irq = exynos_adc_v2_clear_irq,
+ .start_conv = exynos_adc_v2_start_conv,
+};
+
static const struct of_device_id exynos_adc_match[] = {
{
.compatible = "samsung,s3c2410-adc",
@@ -518,6 +562,9 @@ static const struct of_device_id exynos_adc_match[] = {
}, {
.compatible = "samsung,exynos7-adc",
.data = &exynos7_adc_data,
+ }, {
+ .compatible = "samsung,exynos-adc-fsd-hw",
+ .data = &fsd_hw_adc_data,
},
{},
};
@@ -626,6 +673,8 @@ static irqreturn_t exynos_adc_isr(int irq, void *dev_id)
info->ts_x = readl(ADC_V1_DATX(info->regs));
info->ts_y = readl(ADC_V1_DATY(info->regs));
writel(ADC_TSC_WAIT4INT | ADC_S3C2443_TSC_UD_SEN, ADC_V1_TSC(info->regs));
+ } else if (of_device_is_compatible(info->dev->of_node, "samsung,exynos-adc-fsd-hw")) {
+ info->value = readl(ADC_FSD_DAT(info->regs)) & mask;
} else {
info->value = readl(ADC_V1_DATX(info->regs)) & mask;
}
@@ -719,6 +768,12 @@ static const struct iio_chan_spec exynos_adc_iio_channels[] = {
ADC_CHANNEL(7, "adc7"),
ADC_CHANNEL(8, "adc8"),
ADC_CHANNEL(9, "adc9"),
+ ADC_CHANNEL(10, "adc10"),
+ ADC_CHANNEL(11, "adc11"),
+ ADC_CHANNEL(12, "adc12"),
+ ADC_CHANNEL(13, "adc13"),
+ ADC_CHANNEL(14, "adc14"),
+ ADC_CHANNEL(15, "adc15"),
};
static int exynos_adc_remove_devices(struct device *dev, void *c)
--
2.17.1
On Fri, 20 May 2022 20:28:19 +0530
Tamseel Shams <[email protected]> wrote:
> From: Alim Akhtar <[email protected]>
>
> Exynos's ADC-FSD-HW has some difference in registers set, number of
> programmable channels (16 channel) etc. This patch adds support for
> ADC-FSD-HW controller version.
>
> Signed-off-by: Alim Akhtar <[email protected]>
> Signed-off-by: Tamseel Shams <[email protected]>
Hi,
One suggestion inline, otherwise LGTM. Plenty of time to tidy this up as
this won't make the upcoming merge window - I'll be queuing it up for 5.20
Thanks,
Jonathan
> ---
> - Changes since v1
> * Addressed Jonathan's comment by using already provided isr handle
>
> drivers/iio/adc/exynos_adc.c | 55 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 55 insertions(+)
>
> diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
> index cff1ba57fb16..183ae591327a 100644
> --- a/drivers/iio/adc/exynos_adc.c
> +++ b/drivers/iio/adc/exynos_adc.c
> @@ -55,6 +55,11 @@
> #define ADC_V2_INT_ST(x) ((x) + 0x14)
> #define ADC_V2_VER(x) ((x) + 0x20)
>
> +/* ADC_FSD_HW register definitions */
> +#define ADC_FSD_DAT(x) ((x) + 0x08)
I mention this below, but these different register sets
should be in the struct exynos_adc_data to avoid the need
for an if "compatible" == check on each use of them.
> +#define ADC_FSD_DAT_SUM(x) ((x) + 0x0C)
> +#define ADC_FSD_DBG_DATA(x) ((x) + 0x1C)
> +
> /* Bit definitions for ADC_V1 */
> #define ADC_V1_CON_RES (1u << 16)
> #define ADC_V1_CON_PRSCEN (1u << 14)
> @@ -92,6 +97,7 @@
>
> /* Bit definitions for ADC_V2 */
> #define ADC_V2_CON1_SOFT_RESET (1u << 2)
> +#define ADC_V2_CON1_SOFT_NON_RESET (1u << 1)
>
> #define ADC_V2_CON2_OSEL (1u << 10)
> #define ADC_V2_CON2_ESEL (1u << 9)
> @@ -100,6 +106,7 @@
> #define ADC_V2_CON2_ACH_SEL(x) (((x) & 0xF) << 0)
> #define ADC_V2_CON2_ACH_MASK 0xF
>
> +#define MAX_ADC_FSD_CHANNELS 16
> #define MAX_ADC_V2_CHANNELS 10
> #define MAX_ADC_V1_CHANNELS 8
> #define MAX_EXYNOS3250_ADC_CHANNELS 2
> @@ -484,6 +491,43 @@ static const struct exynos_adc_data exynos7_adc_data = {
> .start_conv = exynos_adc_v2_start_conv,
> };
>
> +static void exynos_adc_fsd_init_hw(struct exynos_adc *info)
> +{
> + u32 con2;
> +
> + writel(ADC_V2_CON1_SOFT_RESET, ADC_V2_CON1(info->regs));
> +
> + writel(ADC_V2_CON1_SOFT_NON_RESET, ADC_V2_CON1(info->regs));
> +
> + con2 = ADC_V2_CON2_C_TIME(6);
> + writel(con2, ADC_V2_CON2(info->regs));
> +
> + /* Enable interrupts */
> + writel(1, ADC_V2_INT_EN(info->regs));
> +}
> +
> +static void exynos_adc_fsd_exit_hw(struct exynos_adc *info)
> +{
> + u32 con2;
> +
> + con2 = readl(ADC_V2_CON2(info->regs));
> + con2 &= ~ADC_V2_CON2_C_TIME(7);
> + writel(con2, ADC_V2_CON2(info->regs));
> +
> + /* Disable interrupts */
> + writel(0, ADC_V2_INT_EN(info->regs));
> +}
> +
> +static const struct exynos_adc_data fsd_hw_adc_data = {
> + .num_channels = MAX_ADC_FSD_CHANNELS,
> + .mask = ADC_DATX_MASK, /* 12 bit ADC resolution */
> +
> + .init_hw = exynos_adc_fsd_init_hw,
> + .exit_hw = exynos_adc_fsd_exit_hw,
> + .clear_irq = exynos_adc_v2_clear_irq,
> + .start_conv = exynos_adc_v2_start_conv,
> +};
> +
> static const struct of_device_id exynos_adc_match[] = {
> {
> .compatible = "samsung,s3c2410-adc",
> @@ -518,6 +562,9 @@ static const struct of_device_id exynos_adc_match[] = {
> }, {
> .compatible = "samsung,exynos7-adc",
> .data = &exynos7_adc_data,
> + }, {
> + .compatible = "samsung,exynos-adc-fsd-hw",
> + .data = &fsd_hw_adc_data,
> },
> {},
> };
> @@ -626,6 +673,8 @@ static irqreturn_t exynos_adc_isr(int irq, void *dev_id)
> info->ts_x = readl(ADC_V1_DATX(info->regs));
> info->ts_y = readl(ADC_V1_DATY(info->regs));
> writel(ADC_TSC_WAIT4INT | ADC_S3C2443_TSC_UD_SEN, ADC_V1_TSC(info->regs));
> + } else if (of_device_is_compatible(info->dev->of_node, "samsung,exynos-adc-fsd-hw")) {
Rather than a fairly expensive look up into a device tree node, why not add
the information to the struct exynos_adc_adc in some fashion? Maybe as an offset
for the register block?
> + info->value = readl(ADC_FSD_DAT(info->regs)) & mask;
> } else {
> info->value = readl(ADC_V1_DATX(info->regs)) & mask;
> }
> @@ -719,6 +768,12 @@ static const struct iio_chan_spec exynos_adc_iio_channels[] = {
> ADC_CHANNEL(7, "adc7"),
> ADC_CHANNEL(8, "adc8"),
> ADC_CHANNEL(9, "adc9"),
> + ADC_CHANNEL(10, "adc10"),
> + ADC_CHANNEL(11, "adc11"),
> + ADC_CHANNEL(12, "adc12"),
> + ADC_CHANNEL(13, "adc13"),
> + ADC_CHANNEL(14, "adc14"),
> + ADC_CHANNEL(15, "adc15"),
> };
>
> static int exynos_adc_remove_devices(struct device *dev, void *c)
On 20/05/2022 16:58, Tamseel Shams wrote:
> From: Alim Akhtar <[email protected]>
>
> Exynos's ADC-FSD-HW has some difference in registers set, number of
> programmable channels (16 channel) etc. This patch adds support for
> ADC-FSD-HW controller version.
>
> Signed-off-by: Alim Akhtar <[email protected]>
> Signed-off-by: Tamseel Shams <[email protected]>
The compatible needs changing (as commented in bindings).
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
Hi Krzysztof,
On 20/05/2022 16:58, Tamseel Shams wrote:
>> From: Alim Akhtar <[email protected]>
>>
>> Exynos's ADC-FSD-HW has some difference in registers set, number of
>> programmable channels (16 channel) etc. This patch adds support for
>> ADC-FSD-HW controller version.
>>
>> Signed-off-by: Alim Akhtar <[email protected]>
>> Signed-off-by: Tamseel Shams <[email protected]>
>The compatible needs changing (as commented in bindings).
Sure, will change in next version
Thanks & Regards,
Tamseel Shams
Hi Jonathan,
On Fri, 20 May 2022 20:28:19 +0530
Tamseel Shams <[email protected]> wrote:
>> From: Alim Akhtar <[email protected]>
>>
>> Exynos's ADC-FSD-HW has some difference in registers set, number of
>> programmable channels (16 channel) etc. This patch adds support for
>> ADC-FSD-HW controller version.
>>
>> Signed-off-by: Alim Akhtar <[email protected]>
>> Signed-off-by: Tamseel Shams <[email protected]>
>
> Hi,
>
> One suggestion inline, otherwise LGTM. Plenty of time to tidy this up as
this won't make the upcoming merge window - I'll be queuing it up for 5.20
>
> Thanks,
>
> Jonathan
>
Okay, Thanks for reviewing.
>> ---
>> - Changes since v1
>> * Addressed Jonathan's comment by using already provided isr handle
>>
>> drivers/iio/adc/exynos_adc.c | 55
>> ++++++++++++++++++++++++++++++++++++
>> 1 file changed, 55 insertions(+)
>>
>> diff --git a/drivers/iio/adc/exynos_adc.c
>> b/drivers/iio/adc/exynos_adc.c index cff1ba57fb16..183ae591327a 100644
>> --- a/drivers/iio/adc/exynos_adc.c
>> +++ b/drivers/iio/adc/exynos_adc.c
>> @@ -55,6 +55,11 @@
>> #define ADC_V2_INT_ST(x) ((x) + 0x14)
>> #define ADC_V2_VER(x) ((x) + 0x20)
>>
>> +/* ADC_FSD_HW register definitions */
>> +#define ADC_FSD_DAT(x) ((x) + 0x08)
>
> I mention this below, but these different register sets should be in the
struct exynos_adc_data to avoid the need for an if "compatible" == check on
each use of > them.
>
Can you clarify on how exactly you want me to add these register sets to
struct exynos_adc_data?
Do you mean just for these registers or other registers too which are
defined in this way only?
Thanks & Regards,
Tamseel Shams
On Tue, 31 May 2022 14:12:46 +0530
"m.shams" <[email protected]> wrote:
> Hi Jonathan,
>
> On Fri, 20 May 2022 20:28:19 +0530
> Tamseel Shams <[email protected]> wrote:
>
> >> From: Alim Akhtar <[email protected]>
> >>
> >> Exynos's ADC-FSD-HW has some difference in registers set, number of
> >> programmable channels (16 channel) etc. This patch adds support for
> >> ADC-FSD-HW controller version.
> >>
> >> Signed-off-by: Alim Akhtar <[email protected]>
> >> Signed-off-by: Tamseel Shams <[email protected]>
> >
> > Hi,
> >
> > One suggestion inline, otherwise LGTM. Plenty of time to tidy this up as
> this won't make the upcoming merge window - I'll be queuing it up for 5.20
> >
> > Thanks,
> >
> > Jonathan
> >
>
> Okay, Thanks for reviewing.
>
> >> ---
> >> - Changes since v1
> >> * Addressed Jonathan's comment by using already provided isr handle
> >>
> >> drivers/iio/adc/exynos_adc.c | 55
> >> ++++++++++++++++++++++++++++++++++++
> >> 1 file changed, 55 insertions(+)
> >>
> >> diff --git a/drivers/iio/adc/exynos_adc.c
> >> b/drivers/iio/adc/exynos_adc.c index cff1ba57fb16..183ae591327a 100644
> >> --- a/drivers/iio/adc/exynos_adc.c
> >> +++ b/drivers/iio/adc/exynos_adc.c
> >> @@ -55,6 +55,11 @@
> >> #define ADC_V2_INT_ST(x) ((x) + 0x14)
> >> #define ADC_V2_VER(x) ((x) + 0x20)
> >>
> >> +/* ADC_FSD_HW register definitions */
> >> +#define ADC_FSD_DAT(x) ((x) + 0x08)
> >
> > I mention this below, but these different register sets should be in the
> struct exynos_adc_data to avoid the need for an if "compatible" == check on
> each use of > them.
> >
>
> Can you clarify on how exactly you want me to add these register sets to
> struct exynos_adc_data?
> Do you mean just for these registers or other registers too which are
> defined in this way only?
Any registers addresses that are different for the different chip variants
supported by the driver.
In cases where the only difference between versions is a register address then
define something like
#define ADC_FSD_DAT_BASE 0x08
In the structure have a
dat_addr = ADC_FSD_DAT_BASE
and use dat_addr + x to access.
If things are more complex (and I haven't looked closely so that may apply to
the example give above, the wrap the different access sequence and register
addresses in a callback similar to already done for clear_irq.
Jonathan
>
>
> Thanks & Regards,
> Tamseel Shams
>