* RE: [PATCH 21/23] iio: adc: exynos-adc: Add support for ADC V3 controller
[not found] ` <20220116111939.413ece7e@jic23-huawei>
@ 2022-01-17 12:20 ` Alim Akhtar
0 siblings, 0 replies; 2+ messages in thread
From: Alim Akhtar @ 2022-01-17 12:20 UTC (permalink / raw)
To: 'Jonathan Cameron'
Cc: linux-arm-kernel, linux-kernel, soc, linux-clk, devicetree, olof,
linus.walleij, catalin.marinas, robh+dt, krzysztof.kozlowski,
s.nawrocki, linux-samsung-soc, pankaj.dubey, linux-fsd, linux-iio,
'Tamseel Shams'
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);
^ permalink raw reply [flat|nested] 2+ messages in thread