From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 6B2FDC433F5 for ; Sun, 16 Jan 2022 11:13:41 +0000 (UTC) Received: by smtp.kernel.org (Postfix) id 17ABBC36AE7; Sun, 16 Jan 2022 11:13:41 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id C98FCC36AE3; Sun, 16 Jan 2022 11:13:36 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1642331621; bh=Fg121GflpmYLjIKrlP/YRUChTu7q0Vsm7v3+PjUcPjI=; h=Date:From:To:List-Id:Cc:Subject:In-Reply-To:References:From; b=WZnagPQvLw3cJ+yNzQSJxk2OWbF9EZni/x+LssKH/zCYzy+Zh3oLmjO784ELKrn/F 89XPCH3tTQ3Gy4Kctp/WXCHgrrffRGfMaCQp+44py0Mp7FChw/npW6H4nIjacUcj3S Tu1At+0FbXE/LD5Uf7Z9Ce8kkwdVoWh/xEZPl/JmZuHJgOfLITXUQnkZzTTt43fuVN 0udocqfWwJF854VMQgpmGS+NbYrh6r96q4oG28+zKSwTL33z5WJ+D1PwlfxauHSwSa ZOxy/LIVJPtgWX6AZILTxGAyN14dY6jQMS4GBuxvgqibW4djeXr70S7loqvDpi+rMB oRPawMwx/PK/g== Date: Sun, 16 Jan 2022 11:19:39 +0000 From: Jonathan Cameron To: Alim Akhtar List-Id: 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 Subject: Re: [PATCH 21/23] iio: adc: exynos-adc: Add support for ADC V3 controller Message-ID: <20220116111939.413ece7e@jic23-huawei> In-Reply-To: <20220113121143.22280-22-alim.akhtar@samsung.com> References: <20220113121143.22280-1-alim.akhtar@samsung.com> <20220113121143.22280-22-alim.akhtar@samsung.com> X-Mailer: Claws Mail 4.0.0 (GTK+ 3.24.31; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit On Thu, 13 Jan 2022 17:41:41 +0530 Alim Akhtar 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 > Signed-off-by: Alim Akhtar 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. 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); From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 7F550C433F5 for ; Sun, 16 Jan 2022 11:15:43 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=x1pwvhkpGqkRHxO/5cdDhymKrhQBer81q6uXt/2e3i8=; b=HY6OVJblE/e1zb hp8mRvFGqAlLD2jQio/La8OLFn46BAXTyVdJjpa5+OTwdrB3s6YOIaAybyS8inI2+IrBC1Qbu3F/d YGpsUyrqjHYAtOsMOK4Qld9AYEt/NARuMR2SdPJIHzEY/Uv/F5A/J9mF4ygoz3PiblWICn+OrjQ6I ws6mmlTTOI+Rr83YTJdxaVkynWt+A30diyfXzFpC9wGGxDGdKH8us1aoCaRbkBL6wrumzjKT4gGyT 3YaaVzJxS/BUayVGK3NjuFjp3UjrMh9YNZuF2kqyRBq6zRHFqQ2Pbh6lfKVfSNxF+hAeiCVnkQrpZ 1uBuFhw4VCI0HldrpWoA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1n93Tm-00CKNH-6r; Sun, 16 Jan 2022 11:13:46 +0000 Received: from dfw.source.kernel.org ([2604:1380:4641:c500::1]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1n93Ti-00CKMy-MZ for linux-arm-kernel@lists.infradead.org; Sun, 16 Jan 2022 11:13:44 +0000 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 8B90260E06; Sun, 16 Jan 2022 11:13:41 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id C98FCC36AE3; Sun, 16 Jan 2022 11:13:36 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1642331621; bh=Fg121GflpmYLjIKrlP/YRUChTu7q0Vsm7v3+PjUcPjI=; h=Date:From:To:List-Id:Cc:Subject:In-Reply-To:References:From; b=WZnagPQvLw3cJ+yNzQSJxk2OWbF9EZni/x+LssKH/zCYzy+Zh3oLmjO784ELKrn/F 89XPCH3tTQ3Gy4Kctp/WXCHgrrffRGfMaCQp+44py0Mp7FChw/npW6H4nIjacUcj3S Tu1At+0FbXE/LD5Uf7Z9Ce8kkwdVoWh/xEZPl/JmZuHJgOfLITXUQnkZzTTt43fuVN 0udocqfWwJF854VMQgpmGS+NbYrh6r96q4oG28+zKSwTL33z5WJ+D1PwlfxauHSwSa ZOxy/LIVJPtgWX6AZILTxGAyN14dY6jQMS4GBuxvgqibW4djeXr70S7loqvDpi+rMB oRPawMwx/PK/g== Date: Sun, 16 Jan 2022 11:19:39 +0000 From: Jonathan Cameron To: Alim Akhtar 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 Subject: Re: [PATCH 21/23] iio: adc: exynos-adc: Add support for ADC V3 controller Message-ID: <20220116111939.413ece7e@jic23-huawei> In-Reply-To: <20220113121143.22280-22-alim.akhtar@samsung.com> References: <20220113121143.22280-1-alim.akhtar@samsung.com> <20220113121143.22280-22-alim.akhtar@samsung.com> X-Mailer: Claws Mail 4.0.0 (GTK+ 3.24.31; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220116_031342_857972_764445EE X-CRM114-Status: GOOD ( 31.41 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Thu, 13 Jan 2022 17:41:41 +0530 Alim Akhtar 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 > Signed-off-by: Alim Akhtar 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. 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); _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel