| 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 |
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)
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
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
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
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 --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)