On Tue, 31 May 2022 14:12:46 +0530 "m.shams" <m.shams@xxxxxxxxxxx> wrote: > Hi Jonathan, > > On Fri, 20 May 2022 20:28:19 +0530 > Tamseel Shams <m.shams@xxxxxxxxxxx> wrote: > > >> From: Alim Akhtar <alim.akhtar@xxxxxxxxxxx> > >> > >> 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@xxxxxxxxxxx> > >> Signed-off-by: Tamseel Shams <m.shams@xxxxxxxxxxx> > > > > 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 >
