Hi Jonathan >-----Original Message----- >From: Jonathan Cameron [mailto:jic23@xxxxxxxxxx] >Sent: Sunday, January 16, 2022 4:50 PM >To: Alim Akhtar <alim.akhtar@xxxxxxxxxxx> >Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; >soc@xxxxxxxxxx; linux-clk@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; >olof@xxxxxxxxx; linus.walleij@xxxxxxxxxx; catalin.marinas@xxxxxxx; >robh+dt@xxxxxxxxxx; krzysztof.kozlowski@xxxxxxxxxxxxx; >s.nawrocki@xxxxxxxxxxx; linux-samsung-soc@xxxxxxxxxxxxxxx; >pankaj.dubey@xxxxxxxxxxx; linux-fsd@xxxxxxxxx; linux- >iio@xxxxxxxxxxxxxxx; Tamseel Shams <m.shams@xxxxxxxxxxx> >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@xxxxxxxxxxx> 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@xxxxxxxxx >> Cc: jic23@xxxxxxxxxx >> Cc: linux-iio@xxxxxxxxxxxxxxx >> Signed-off-by: Tamseel Shams <m.shams@xxxxxxxxxxx> >> Signed-off-by: Alim Akhtar <alim.akhtar@xxxxxxxxxxx> > >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);
