diff mbox series

[v2,2/3] iio: adc: exynos-adc: Add support for ADC FSD-HW controller

Message ID 20220520145820.67667-3-m.shams@samsung.com (mailing list archive)
State New
Headers show
Series [v2,1/3] dt-bindings: iio: adc: Add FSD-HW variant | expand

Commit Message

Tamseel Shams May 20, 2022, 2:58 p.m. UTC
From: Alim Akhtar <alim.akhtar@samsung.com>

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 <alim.akhtar@samsung.com>
Signed-off-by: Tamseel Shams <m.shams@samsung.com>
---
- 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(+)

Comments

Jonathan Cameron May 22, 2022, 11:25 a.m. UTC | #1
On Fri, 20 May 2022 20:28:19 +0530
Tamseel Shams <m.shams@samsung.com> wrote:

> From: Alim Akhtar <alim.akhtar@samsung.com>
> 
> 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 <alim.akhtar@samsung.com>
> Signed-off-by: Tamseel Shams <m.shams@samsung.com>

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)
Krzysztof Kozlowski May 23, 2022, 10:20 a.m. UTC | #2
On 20/05/2022 16:58, Tamseel Shams wrote:
> From: Alim Akhtar <alim.akhtar@samsung.com>
> 
> 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 <alim.akhtar@samsung.com>
> Signed-off-by: Tamseel Shams <m.shams@samsung.com>

The compatible needs changing (as commented in bindings).

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof
Tamseel Shams May 31, 2022, 8:29 a.m. UTC | #3
Hi Krzysztof,

On 20/05/2022 16:58, Tamseel Shams wrote:
>> From: Alim Akhtar <alim.akhtar@samsung.com>
>> 
>> 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 <alim.akhtar@samsung.com>
>> Signed-off-by: Tamseel Shams <m.shams@samsung.com>

>The compatible needs changing (as commented in bindings).

Sure, will change in next version 


Thanks & Regards,
Tamseel Shams
Tamseel Shams May 31, 2022, 8:42 a.m. UTC | #4
Hi Jonathan,

On Fri, 20 May 2022 20:28:19 +0530
Tamseel Shams <m.shams@samsung.com> wrote:

>> From: Alim Akhtar <alim.akhtar@samsung.com>
>> 
>> 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 <alim.akhtar@samsung.com>
>> Signed-off-by: Tamseel Shams <m.shams@samsung.com>
>
> 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
Jonathan Cameron June 3, 2022, 3:10 p.m. UTC | #5
On Tue, 31 May 2022 14:12:46 +0530
"m.shams" <m.shams@samsung.com> wrote:

> Hi Jonathan,
> 
> On Fri, 20 May 2022 20:28:19 +0530
> Tamseel Shams <m.shams@samsung.com> wrote:
> 
> >> From: Alim Akhtar <alim.akhtar@samsung.com>
> >> 
> >> 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 <alim.akhtar@samsung.com>
> >> Signed-off-by: Tamseel Shams <m.shams@samsung.com>  
> >
> > 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
>
diff mbox series

Patch

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)