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