| Message ID | 20220113121143.22280-22-alim.akhtar@samsung.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [01/23] dt-bindings: clock: Document FSD CMU bindings | expand |
On Thu, 13 Jan 2022 17:41:41 +0530 Alim Akhtar <alim.akhtar@samsung.com> wrote: > Exynos's ADC-V3 has some difference in registers set, number of > programmable channels (16 channel) etc. This patch adds support for ADC-V3 > controller version. > > Cc: linux-fsd@tesla.com > Cc: jic23@kernel.org > Cc: linux-iio@vger.kernel.org > Signed-off-by: Tamseel Shams <m.shams@samsung.com> > Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com> Hi Alim, A few minor suggestions below. I'm not seeing a binding update though... I'd also suggest that it would be more appropriate to break this out as a separate mini series from the main support so that it can be reviewed and merge separately. It's not ideal when a list just gets patch 21 of 23 with no cover letter etc sent to it. Jonathan > --- > drivers/iio/adc/exynos_adc.c | 74 +++++++++++++++++++++++++++++++++++- > 1 file changed, 72 insertions(+), 2 deletions(-) > > diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c > index 3b3868aa2533..61752e798fd6 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_V3 register definitions */ > +#define ADC_V3_DAT(x) ((x) + 0x08) > +#define ADC_V3_DAT_SUM(x) ((x) + 0x0C) > +#define ADC_V3_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_V3_CHANNELS 16 > #define MAX_ADC_V2_CHANNELS 10 > #define MAX_ADC_V1_CHANNELS 8 > #define MAX_EXYNOS3250_ADC_CHANNELS 2 Given we have a mixture of required an unrequired elements in this structure it might be a good idea to add some documentation. Kernel-doc for the whole structure preferred. Note this isn't necessarily something that needs to be in this patch given the lack of docs predates this and with the change to make adc_isr() required that I suggest below things aren't made worse by this patch. > @@ -164,6 +171,7 @@ struct exynos_adc_data { > void (*exit_hw)(struct exynos_adc *info); > void (*clear_irq)(struct exynos_adc *info); > void (*start_conv)(struct exynos_adc *info, unsigned long addr); > + irqreturn_t (*adc_isr)(int irq, void *dev_id); > }; > > static void exynos_adc_unprepare_clk(struct exynos_adc *info) > @@ -484,6 +492,59 @@ static const struct exynos_adc_data exynos7_adc_data = { > .start_conv = exynos_adc_v2_start_conv, > }; > > +static void exynos_adc_v3_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_v3_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 irqreturn_t exynos_adc_v3_isr(int irq, void *dev_id) > +{ > + struct exynos_adc *info = (struct exynos_adc *)dev_id; Shouldn't need the cast as cast from void * to another pointer is always valid in C without the explicit cast. > + u32 mask = info->data->mask; > + > + info->value = readl(ADC_V3_DAT(info->regs)) & mask; > + > + if (info->data->clear_irq) > + info->data->clear_irq(info); Don't need this currently as v3_isr() is always matched with clear_isr() being provided. Having the check implies otherwise which is probably not a good thing to do until some future device support (maybe) needs it. > + > + complete(&info->completion); > + > + return IRQ_HANDLED; > +} > + > +static const struct exynos_adc_data exynos_adc_v3_adc_data = { > + .num_channels = MAX_ADC_V3_CHANNELS, > + .mask = ADC_DATX_MASK, /* 12 bit ADC resolution */ > + > + .init_hw = exynos_adc_v3_init_hw, > + .exit_hw = exynos_adc_v3_exit_hw, > + .clear_irq = exynos_adc_v2_clear_irq, > + .start_conv = exynos_adc_v2_start_conv, > + .adc_isr = exynos_adc_v3_isr, > +}; > + > static const struct of_device_id exynos_adc_match[] = { > { > .compatible = "samsung,s3c2410-adc", > @@ -518,6 +579,9 @@ static const struct of_device_id exynos_adc_match[] = { > }, { > .compatible = "samsung,exynos7-adc", > .data = &exynos7_adc_data, > + }, { > + .compatible = "samsung,exynos-adc-v3", > + .data = &exynos_adc_v3_adc_data, > }, > {}, > }; > @@ -719,6 +783,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) > @@ -885,8 +955,8 @@ static int exynos_adc_probe(struct platform_device *pdev) > > mutex_init(&info->lock); > > - ret = request_irq(info->irq, exynos_adc_isr, > - 0, dev_name(&pdev->dev), info); > + ret = request_irq(info->irq, info->data->adc_isr ? info->data->adc_isr : > + exynos_adc_isr, 0, dev_name(&pdev->dev), info); I'd rather see the slightly larger change of providing adc_isr for existing parts and the conditional part here going away. Jonathan > if (ret < 0) { > dev_err(&pdev->dev, "failed requesting irq, irq = %d\n", > info->irq);
Hi Jonathan >-----Original Message----- >From: Jonathan Cameron [mailto:jic23@kernel.org] >Sent: Sunday, January 16, 2022 4:50 PM >To: Alim Akhtar <alim.akhtar@samsung.com> >Cc: linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; >soc@kernel.org; linux-clk@vger.kernel.org; devicetree@vger.kernel.org; >olof@lixom.net; linus.walleij@linaro.org; catalin.marinas@arm.com; >robh+dt@kernel.org; krzysztof.kozlowski@canonical.com; >s.nawrocki@samsung.com; linux-samsung-soc@vger.kernel.org; >pankaj.dubey@samsung.com; linux-fsd@tesla.com; linux- >iio@vger.kernel.org; Tamseel Shams <m.shams@samsung.com> >Subject: Re: [PATCH 21/23] iio: adc: exynos-adc: Add support for ADC V3 >controller > >On Thu, 13 Jan 2022 17:41:41 +0530 >Alim Akhtar <alim.akhtar@samsung.com> wrote: > >> Exynos's ADC-V3 has some difference in registers set, number of >> programmable channels (16 channel) etc. This patch adds support for >> ADC-V3 controller version. >> >> Cc: linux-fsd@tesla.com >> Cc: jic23@kernel.org >> Cc: linux-iio@vger.kernel.org >> Signed-off-by: Tamseel Shams <m.shams@samsung.com> >> Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com> > >Hi Alim, > >A few minor suggestions below. I'm not seeing a binding update though... > >I'd also suggest that it would be more appropriate to break this out as a >separate mini series from the main support so that it can be reviewed and >merge separately. It's not ideal when a list just gets patch 21 of >23 with no cover letter etc sent to it. > Thanks for the detailed review, I agree, will send as a separate patch set only related with ADC support. And addressing rest of your comments in this patch. >Jonathan > >> --- >> drivers/iio/adc/exynos_adc.c | 74 >> +++++++++++++++++++++++++++++++++++- >> 1 file changed, 72 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/iio/adc/exynos_adc.c >> b/drivers/iio/adc/exynos_adc.c index 3b3868aa2533..61752e798fd6 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_V3 register definitions */ >> +#define ADC_V3_DAT(x) ((x) + 0x08) >> +#define ADC_V3_DAT_SUM(x) ((x) + 0x0C) >> +#define ADC_V3_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_V3_CHANNELS 16 >> #define MAX_ADC_V2_CHANNELS 10 >> #define MAX_ADC_V1_CHANNELS 8 >> #define MAX_EXYNOS3250_ADC_CHANNELS 2 > >Given we have a mixture of required an unrequired elements in this structure >it might be a good idea to add some documentation. Kernel-doc for the >whole structure preferred. Note this isn't necessarily something that needs >to be in this patch given the lack of docs predates this and with the change to >make >adc_isr() required that I suggest below things aren't made worse by this >patch. > >> @@ -164,6 +171,7 @@ struct exynos_adc_data { >> void (*exit_hw)(struct exynos_adc *info); >> void (*clear_irq)(struct exynos_adc *info); >> void (*start_conv)(struct exynos_adc *info, unsigned long addr); >> + irqreturn_t (*adc_isr)(int irq, void *dev_id); >> }; >> >> static void exynos_adc_unprepare_clk(struct exynos_adc *info) @@ >> -484,6 +492,59 @@ static const struct exynos_adc_data exynos7_adc_data = >{ >> .start_conv = exynos_adc_v2_start_conv, >> }; >> >> +static void exynos_adc_v3_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_v3_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 irqreturn_t exynos_adc_v3_isr(int irq, void *dev_id) { >> + struct exynos_adc *info = (struct exynos_adc *)dev_id; > >Shouldn't need the cast as cast from void * to another pointer is always valid >in C without the explicit cast. > >> + u32 mask = info->data->mask; >> + >> + info->value = readl(ADC_V3_DAT(info->regs)) & mask; >> + >> + if (info->data->clear_irq) >> + info->data->clear_irq(info); > >Don't need this currently as v3_isr() is always matched with clear_isr() being >provided. Having the check implies otherwise which is probably not a good >thing to do until some future device support (maybe) needs it. > >> + >> + complete(&info->completion); >> + >> + return IRQ_HANDLED; >> +} >> + >> +static const struct exynos_adc_data exynos_adc_v3_adc_data = { >> + .num_channels = MAX_ADC_V3_CHANNELS, >> + .mask = ADC_DATX_MASK, /* 12 bit ADC resolution */ >> + >> + .init_hw = exynos_adc_v3_init_hw, >> + .exit_hw = exynos_adc_v3_exit_hw, >> + .clear_irq = exynos_adc_v2_clear_irq, >> + .start_conv = exynos_adc_v2_start_conv, >> + .adc_isr = exynos_adc_v3_isr, >> +}; >> + >> static const struct of_device_id exynos_adc_match[] = { >> { >> .compatible = "samsung,s3c2410-adc", @@ -518,6 +579,9 @@ >static >> const struct of_device_id exynos_adc_match[] = { >> }, { >> .compatible = "samsung,exynos7-adc", >> .data = &exynos7_adc_data, >> + }, { >> + .compatible = "samsung,exynos-adc-v3", >> + .data = &exynos_adc_v3_adc_data, >> }, >> {}, >> }; >> @@ -719,6 +783,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) @@ >> -885,8 +955,8 @@ static int exynos_adc_probe(struct platform_device >> *pdev) >> >> mutex_init(&info->lock); >> >> - ret = request_irq(info->irq, exynos_adc_isr, >> - 0, dev_name(&pdev->dev), info); >> + ret = request_irq(info->irq, info->data->adc_isr ? info->data->adc_isr >: >> + exynos_adc_isr, 0, dev_name(&pdev->dev), >info); > >I'd rather see the slightly larger change of providing adc_isr for existing parts >and the conditional part here going away. > >Jonathan > > >> if (ret < 0) { >> dev_err(&pdev->dev, "failed requesting irq, irq = %d\n", >> info->irq);
diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c index 3b3868aa2533..61752e798fd6 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_V3 register definitions */ +#define ADC_V3_DAT(x) ((x) + 0x08) +#define ADC_V3_DAT_SUM(x) ((x) + 0x0C) +#define ADC_V3_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_V3_CHANNELS 16 #define MAX_ADC_V2_CHANNELS 10 #define MAX_ADC_V1_CHANNELS 8 #define MAX_EXYNOS3250_ADC_CHANNELS 2 @@ -164,6 +171,7 @@ struct exynos_adc_data { void (*exit_hw)(struct exynos_adc *info); void (*clear_irq)(struct exynos_adc *info); void (*start_conv)(struct exynos_adc *info, unsigned long addr); + irqreturn_t (*adc_isr)(int irq, void *dev_id); }; static void exynos_adc_unprepare_clk(struct exynos_adc *info) @@ -484,6 +492,59 @@ static const struct exynos_adc_data exynos7_adc_data = { .start_conv = exynos_adc_v2_start_conv, }; +static void exynos_adc_v3_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_v3_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 irqreturn_t exynos_adc_v3_isr(int irq, void *dev_id) +{ + struct exynos_adc *info = (struct exynos_adc *)dev_id; + u32 mask = info->data->mask; + + info->value = readl(ADC_V3_DAT(info->regs)) & mask; + + if (info->data->clear_irq) + info->data->clear_irq(info); + + complete(&info->completion); + + return IRQ_HANDLED; +} + +static const struct exynos_adc_data exynos_adc_v3_adc_data = { + .num_channels = MAX_ADC_V3_CHANNELS, + .mask = ADC_DATX_MASK, /* 12 bit ADC resolution */ + + .init_hw = exynos_adc_v3_init_hw, + .exit_hw = exynos_adc_v3_exit_hw, + .clear_irq = exynos_adc_v2_clear_irq, + .start_conv = exynos_adc_v2_start_conv, + .adc_isr = exynos_adc_v3_isr, +}; + static const struct of_device_id exynos_adc_match[] = { { .compatible = "samsung,s3c2410-adc", @@ -518,6 +579,9 @@ static const struct of_device_id exynos_adc_match[] = { }, { .compatible = "samsung,exynos7-adc", .data = &exynos7_adc_data, + }, { + .compatible = "samsung,exynos-adc-v3", + .data = &exynos_adc_v3_adc_data, }, {}, }; @@ -719,6 +783,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) @@ -885,8 +955,8 @@ static int exynos_adc_probe(struct platform_device *pdev) mutex_init(&info->lock); - ret = request_irq(info->irq, exynos_adc_isr, - 0, dev_name(&pdev->dev), info); + ret = request_irq(info->irq, info->data->adc_isr ? info->data->adc_isr : + exynos_adc_isr, 0, dev_name(&pdev->dev), info); if (ret < 0) { dev_err(&pdev->dev, "failed requesting irq, irq = %d\n", info->irq);