RE: [PATCH 21/23] iio: adc: exynos-adc: Add support for ADC V3 controller

[Alim Akhtar]

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