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 X-Spam-Level: X-Spam-Status: No, score=-12.7 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 8C44CC43214 for ; Sat, 31 Jul 2021 18:32:23 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 6564B60F3A for ; Sat, 31 Jul 2021 18:32:23 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230110AbhGaSc2 (ORCPT ); Sat, 31 Jul 2021 14:32:28 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:38334 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229475AbhGaSc0 (ORCPT ); Sat, 31 Jul 2021 14:32:26 -0400 Received: from mail-yb1-xb2d.google.com (mail-yb1-xb2d.google.com [IPv6:2607:f8b0:4864:20::b2d]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 73714C06175F; Sat, 31 Jul 2021 11:32:19 -0700 (PDT) Received: by mail-yb1-xb2d.google.com with SMTP id j77so19737339ybj.3; Sat, 31 Jul 2021 11:32:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=+TS93CVSfIimTDH64QdCBi4eN/nuRbRnV4BUMOxgST4=; b=an0l1D2ymi9pYRnSKxa4OeSBAMmbR2ri+d9a/KXFfqxGANITZWBq1v83FAro18ndsB /M/n0CRF2Yhtu38FQlL8h2wre0WYFloo25mtAt56Rrq9kY4Zk6CpWslFMZHF2YXYchM9 djaSmqpElV2zk9XBRZ5iB2kWzlMkvKI2ddzVI7nBomvqrX4sYBpFa5osGDuliyOhIAUL J4mK/j+g40X2xjK4hR2GSAqgLi6wUFMvqnWTItVydr1hmDJtk6vFDYDccP49afr6zD/m dPvuPA2BhyDMTEydSfYaRi3VwyXuyh0tkq7PW4zF4VFPeQA+lrGGLjjZVHlr/MjB+xrW zDfQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=+TS93CVSfIimTDH64QdCBi4eN/nuRbRnV4BUMOxgST4=; b=JKg/XEveDLvwL9hK5cE5Hab79tEd+uJCetlFffPwZZwYxZUnyzgqyzrZSU3OyrYEOS wcFjkVyj6/C4nx7Rht//X/cKuVOv3yIXLkmrm8SQfKC8XMKUvVC7PbMjiEVopsfhjwkf YXUC+/RuiUEOKdsigKS7gaE5UV6Or0z66i4Ia3XxVJKJzWnlFTvuX5JBsiOkZX7+jyMa GeggWjuvnDBK6YoMSLPbbYbDlTxLv2KR8LsAtn9xcD22e11LlrTI3meFzBmjfk9NGGzD vvGP0Kbfs+MAV30qvCALlzIWsgt+kX2jeVH2p1+ehpmtluucuO4sXhT5Zd0P6cFMDIbg MFfA== X-Gm-Message-State: AOAM530ZlaOkWDwEZnQWyEj5RaiNwmOuTYBvNraXTpT3UQBY5F2KsvQH OGqIlxXSm/l+jqENsk+BticwNe47J5UZl8qOMcA= X-Google-Smtp-Source: ABdhPJxe9ROzRmrY777gVCWJ7COMMt8cgZWblQecKchl6taZdopz1vYEGV7ugdqERiWFVPFH6YyyvycAr8H+XZ24jkI= X-Received: by 2002:a25:2449:: with SMTP id k70mr10179087ybk.156.1627756338627; Sat, 31 Jul 2021 11:32:18 -0700 (PDT) MIME-Version: 1.0 References: <20210726182850.14328-1-prabhakar.mahadev-lad.rj@bp.renesas.com> <20210726182850.14328-3-prabhakar.mahadev-lad.rj@bp.renesas.com> <20210731181142.430c50f8@jic23-huawei> In-Reply-To: <20210731181142.430c50f8@jic23-huawei> From: "Lad, Prabhakar" Date: Sat, 31 Jul 2021 19:31:52 +0100 Message-ID: Subject: Re: [PATCH v3 2/3] iio: adc: Add driver for Renesas RZ/G2L A/D converter To: Jonathan Cameron Cc: Lad Prabhakar , Geert Uytterhoeven , Rob Herring , Lars-Peter Clausen , Magnus Damm , Philipp Zabel , Alexandru Ardelean , linux-iio , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , Linux-Renesas , LKML , Biju Das Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Jonathan, Thank you for the review. On Sat, Jul 31, 2021 at 6:09 PM Jonathan Cameron wrote: > > On Mon, 26 Jul 2021 19:28:49 +0100 > Lad Prabhakar wrote: > > > Add ADC driver support for Renesas RZ/G2L A/D converter in SW > > trigger mode. > > > > A/D Converter block is a successive approximation analog-to-digital > > converter with a 12-bit accuracy and supports a maximum of 8 input > > channels. > > > > Signed-off-by: Lad Prabhakar > > Reviewed-by: Biju Das > > Hi Lad, > > A few additional comments inline, > > Thanks, > > Jonathan > > > --- > > MAINTAINERS | 8 + > > drivers/iio/adc/Kconfig | 10 + > > drivers/iio/adc/Makefile | 1 + > > drivers/iio/adc/rzg2l_adc.c | 595 ++++++++++++++++++++++++++++++++++++ > > 4 files changed, 614 insertions(+) > > create mode 100644 drivers/iio/adc/rzg2l_adc.c > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index 6c8be735cc91..6a52f9f4604c 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -15839,6 +15839,14 @@ L: linux-renesas-soc@vger.kernel.org > > S: Maintained > > F: drivers/phy/renesas/phy-rcar-gen3-usb*.c > > > > +RENESAS RZ/G2L A/D DRIVER > > +M: Lad Prabhakar > > +L: linux-iio@vger.kernel.org > > +L: linux-renesas-soc@vger.kernel.org > > +S: Supported > > +F: Documentation/devicetree/bindings/iio/adc/renesas,rzg2l-adc.yaml > > +F: drivers/iio/adc/rzg2l_adc.c > > + > > RESET CONTROLLER FRAMEWORK > > M: Philipp Zabel > > S: Maintained > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig > > index db0c8fb60515..af168e1c9fdb 100644 > > --- a/drivers/iio/adc/Kconfig > > +++ b/drivers/iio/adc/Kconfig > > @@ -887,6 +887,16 @@ config ROCKCHIP_SARADC > > To compile this driver as a module, choose M here: the > > module will be called rockchip_saradc. > > > > +config RZG2L_ADC > > + tristate "Renesas RZ/G2L ADC driver" > > + depends on ARCH_R9A07G044 || COMPILE_TEST > > + help > > + Say yes here to build support for the ADC found in Renesas > > + RZ/G2L family. > > + > > + To compile this driver as a module, choose M here: the > > + module will be called rzg2l_adc. > > + > > config SC27XX_ADC > > tristate "Spreadtrum SC27xx series PMICs ADC" > > depends on MFD_SC27XX_PMIC || COMPILE_TEST > > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile > > index f70d877c555a..d68550f493e3 100644 > > --- a/drivers/iio/adc/Makefile > > +++ b/drivers/iio/adc/Makefile > > @@ -82,6 +82,7 @@ obj-$(CONFIG_QCOM_PM8XXX_XOADC) += qcom-pm8xxx-xoadc.o > > obj-$(CONFIG_RCAR_GYRO_ADC) += rcar-gyroadc.o > > obj-$(CONFIG_RN5T618_ADC) += rn5t618-adc.o > > obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o > > +obj-$(CONFIG_RZG2L_ADC) += rzg2l_adc.o > > obj-$(CONFIG_SC27XX_ADC) += sc27xx_adc.o > > obj-$(CONFIG_SPEAR_ADC) += spear_adc.o > > obj-$(CONFIG_STX104) += stx104.o > > diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c > > new file mode 100644 > > index 000000000000..d05a3208ff9d > > --- /dev/null > > +++ b/drivers/iio/adc/rzg2l_adc.c > > @@ -0,0 +1,595 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * RZ/G2L A/D Converter driver > > + * > > + * Copyright (c) 2021 Renesas Electronics Europe GmbH > > + * > > + * Author: Lad Prabhakar > > + */ > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#define DRIVER_NAME "rzg2l-adc" > > As only used in one place, just put it inline there so we don't need > to go find if we want to know the value - I'm lazy. > Its being used in two places 1: indio_dev->name = DRIVER_NAME #In probe call 2: .name = DRIVER_NAME # In platform_driver struct Let me know if you want me to replace them inline and drop the above macro. > > + > > +#define RZG2L_ADM(n) ((n) * 0x4) > > +#define RZG2L_ADM0_ADCE BIT(0) > > +#define RZG2L_ADM0_ADBSY BIT(1) > > +#define RZG2L_ADM0_PWDWNB BIT(2) > > +#define RZG2L_ADM0_SRESB BIT(15) > > +#define RZG2L_ADM1_TRG BIT(0) > > +#define RZG2L_ADM1_MS BIT(2) > > +#define RZG2L_ADM1_BS BIT(4) > > +#define RZG2L_ADM1_EGA_MASK GENMASK(13, 12) > > +#define RZG2L_ADM2_CHSEL_MASK GENMASK(7, 0) > > +#define RZG2L_ADM3_ADIL_MASK GENMASK(31, 24) > > +#define RZG2L_ADM3_ADCMP_MASK GENMASK(23, 16) > > +#define RZG2L_ADM3_ADCMP_E FIELD_PREP(RZG2L_ADM3_ADCMP_MASK, 0xe) > > +#define RZG2L_ADM3_ADSMP_MASK GENMASK(15, 0) > > + > > +#define RZG2L_ADINT 0x20 > > +#define RZG2L_ADINT_INTEN_MASK GENMASK(7, 0) > > +#define RZG2L_ADINT_CSEEN BIT(16) > > +#define RZG2L_ADINT_INTS BIT(31) > > + > > +#define RZG2L_ADSTS 0x24 > > +#define RZG2L_ADSTS_CSEST BIT(16) > > +#define RZG2L_ADSTS_INTST_MASK GENMASK(7, 0) > > + > > +#define RZG2L_ADIVC 0x28 > > +#define RZG2L_ADIVC_DIVADC_MASK GENMASK(8, 0) > > +#define RZG2L_ADIVC_DIVADC_4 FIELD_PREP(RZG2L_ADIVC_DIVADC_MASK, 0x4) > > + > > +#define RZG2L_ADFIL 0x2c > > + > > +#define RZG2L_ADCR(n) (0x30 + ((n) * 0x4)) > > +#define RZG2L_ADCR_AD_MASK GENMASK(11, 0) > > + > > +#define RZG2L_ADSMP_DEFUALT_SAMPLING 0x578 > > + > > +#define RZG2L_ADC_MAX_CHANNELS 8 > > +#define RZG2L_ADC_CHN_MASK 0x7 > > +#define RZG2L_ADC_TIMEOUT usecs_to_jiffies(1 * 4) > > + > > +struct rzg2l_adc_data { > > + const struct iio_chan_spec *channels; > > + u8 num_channels; > > +}; > > + > > +struct rzg2l_adc { > > + void __iomem *base; > > + struct clk *pclk; > > + struct clk *adclk; > > + struct reset_control *presetn; > > + struct reset_control *adrstn; > > + struct completion completion; > > + const struct rzg2l_adc_data *data; > > + u16 last_val[RZG2L_ADC_MAX_CHANNELS]; > > +}; > > + > > +static const char * const rzg2l_adc_channel_name[] = { > > + "adc0", > > + "adc1", > > + "adc2", > > + "adc3", > > + "adc4", > > + "adc5", > > + "adc6", > > + "adc7", > > +}; > > + > > +static unsigned int rzg2l_adc_readl(struct rzg2l_adc *adc, u32 reg) > > +{ > > + return readl(adc->base + reg); > > +} > > + > > +static void rzg2l_adc_writel(struct rzg2l_adc *adc, unsigned int reg, u32 val) > > +{ > > + writel(val, adc->base + reg); > > +} > > + > > +static void rzg2l_adc_pwr(struct rzg2l_adc *adc, bool on) > > +{ > > + u32 reg; > > + > > + reg = rzg2l_adc_readl(adc, RZG2L_ADM(0)); > > + if (on) > > + reg |= RZG2L_ADM0_PWDWNB; > > + else > > + reg &= ~RZG2L_ADM0_PWDWNB; > > + rzg2l_adc_writel(adc, RZG2L_ADM(0), reg); > > + udelay(2); > > +} > > + > > +static void rzg2l_adc_start_stop(struct rzg2l_adc *adc, bool start) > > +{ > > + int timeout = 5; > > + u32 reg; > > + > > + /* stop A/D conversion */ > > This comment seems odd. If it is stopping conversion, how is this function > used to start conversion? Perhaps some more details. > My bad its should be start/stop, said that already the function is named rzg2l_adc_start_stop() so will drop this comment. > > + reg = rzg2l_adc_readl(adc, RZG2L_ADM(0)); > > + if (start) > > + reg |= RZG2L_ADM0_ADCE; > > + else > > + reg &= ~RZG2L_ADM0_ADCE; > > + rzg2l_adc_writel(adc, RZG2L_ADM(0), reg); > > + > > + if (start) > > + return; > > + > > + do { > > + usleep_range(100, 200); > > + reg = rzg2l_adc_readl(adc, RZG2L_ADM(0)); > > + timeout--; > > + if (!timeout) { > > + pr_err("%s stopping ADC timed out\n", __func__); > > + break; > > + } > > + } while (((reg & RZG2L_ADM0_ADBSY) || (reg & RZG2L_ADM0_ADCE))); > > +} > > + > > +static void rzg2l_set_trigger(struct rzg2l_adc *adc) > > +{ > > + u32 reg; > > + > > + /* > > + * Setup ADM1 for SW trigger > > + * EGA[13:12] - Set 00 to indicate hardware trigger is invalid > > + * BS[4] - Enable 1-buffer mode > > + * MS[1] - Enable Select mode > > + * TRG[0] - Enable software trigger mode > > + */ > > + reg = rzg2l_adc_readl(adc, RZG2L_ADM(1)); > > + reg &= ~RZG2L_ADM1_EGA_MASK; > > + reg &= ~RZG2L_ADM1_BS; > > + reg &= ~RZG2L_ADM1_TRG; > > + reg |= RZG2L_ADM1_MS; > > + rzg2l_adc_writel(adc, RZG2L_ADM(1), reg); > > +} > > + > > +static int rzg2l_adc_conversion_setup(struct rzg2l_adc *adc, u8 ch) > > +{ > > + u32 reg; > > + > > + if (rzg2l_adc_readl(adc, RZG2L_ADM(0)) & RZG2L_ADM0_ADBSY) > > + return -EBUSY; > > + > > + rzg2l_set_trigger(adc); > > + > > + /* Select analog input channel subjected to conversion. */ > > + reg = rzg2l_adc_readl(adc, RZG2L_ADM(2)); > > + reg &= ~RZG2L_ADM2_CHSEL_MASK; > > + reg |= BIT(ch); > > + rzg2l_adc_writel(adc, RZG2L_ADM(2), reg); > > + > > + /* > > + * Setup ADINT > > + * INTS[31] - Select pulse signal > > + * CSEEN[16] - Enable channel select error interrupt > > + * INTEN[7:0] - Select channel interrupt > > + */ > > + reg = rzg2l_adc_readl(adc, RZG2L_ADINT); > > + reg &= ~RZG2L_ADINT_INTS; > > + reg &= ~RZG2L_ADINT_INTEN_MASK; > > + reg |= (RZG2L_ADINT_CSEEN | BIT(ch)); > > + rzg2l_adc_writel(adc, RZG2L_ADINT, reg); > > + > > + return 0; > > +} > > + > > +static int rzg2l_adc_set_power(struct iio_dev *indio_dev, bool on) > > +{ > > + struct device *dev = indio_dev->dev.parent; > > + > > + if (on) > > + return pm_runtime_resume_and_get(dev); > > + > > + return pm_runtime_put_sync(dev); > > +} > > + > > +static int rzg2l_adc_conversion(struct iio_dev *indio_dev, struct rzg2l_adc *adc, u8 ch) > > +{ > > + int ret; > > + > > + ret = rzg2l_adc_set_power(indio_dev, true); > > + if (ret) > > + return ret; > > + > > + ret = rzg2l_adc_conversion_setup(adc, ch); > > + if (ret) { > > + rzg2l_adc_set_power(indio_dev, false); > > + return ret; > > + } > > + > > + reinit_completion(&adc->completion); > > + > > + rzg2l_adc_start_stop(adc, true); > > + > > + if (!wait_for_completion_timeout(&adc->completion, RZG2L_ADC_TIMEOUT)) { > > + rzg2l_adc_writel(adc, RZG2L_ADINT, > > + rzg2l_adc_readl(adc, RZG2L_ADINT) & ~RZG2L_ADINT_INTEN_MASK); > > + rzg2l_adc_start_stop(adc, false); > > + rzg2l_adc_set_power(indio_dev, false); > > + return -ETIMEDOUT; > > + } > > + > > + return rzg2l_adc_set_power(indio_dev, false); > > +} > > + > > +static int rzg2l_adc_read_raw(struct iio_dev *indio_dev, > > + struct iio_chan_spec const *chan, > > + int *val, int *val2, long mask) > > +{ > > + struct rzg2l_adc *adc = iio_priv(indio_dev); > > + int ret; > > + u8 ch; > > + > > + switch (mask) { > > + case IIO_CHAN_INFO_RAW: > > + if (chan->type != IIO_VOLTAGE) > > + return -EINVAL; > > + > > + ret = iio_device_claim_direct_mode(indio_dev); > > + if (ret) > > + return ret; > > + > > + ch = chan->channel & RZG2L_ADC_CHN_MASK; > > + ret = rzg2l_adc_conversion(indio_dev, adc, ch); > > + if (ret) { > > + iio_device_release_direct_mode(indio_dev); > > + return ret; > > + } > > + *val = adc->last_val[ch]; > > You should not rely on implementation details of iio_device_claim_direct_mode() > to protect your driver specific data structures. Use your own lock. > Those calls are intended for safe operation when buffered mode in IIO is supported > and currently your driver doesn't support that at all so please remove them. > sure will add drivers own lock. > > + iio_device_release_direct_mode(indio_dev); > > + > > + return IIO_VAL_INT; > > + > > + default: > > + return -EINVAL; > > + } > > +} > > + > > +static int rzg2l_adc_read_label(struct iio_dev *iio_dev, > > + const struct iio_chan_spec *chan, > > + char *label) > > +{ > > + if (chan->channel >= RZG2L_ADC_MAX_CHANNELS) > > + return -EINVAL; > > + > > + return sysfs_emit(label, "%s\n", rzg2l_adc_channel_name[chan->channel]); > > +} > > + > > +static const struct iio_info rzg2l_adc_iio_info = { > > + .read_raw = rzg2l_adc_read_raw, > > + .read_label = rzg2l_adc_read_label, > > +}; > > + > > +static irqreturn_t rzg2l_adc_isr(int irq, void *dev_id) > > +{ > > + struct rzg2l_adc *adc = (struct rzg2l_adc *)dev_id; > > No need for explicit cast from void * to another pointer type. > This is always valid without in C. > Agreed. > > + unsigned long intst; > > + u32 reg; > > + int ch; > > + > > + reg = rzg2l_adc_readl(adc, RZG2L_ADSTS); > > + > > + /* A/D conversion channel select error interrupt */ > > + if (reg & RZG2L_ADSTS_CSEST) { > > + rzg2l_adc_writel(adc, RZG2L_ADSTS, reg); > > + return IRQ_HANDLED; > > + } > > + > > + intst = reg & RZG2L_ADSTS_INTST_MASK; > > + if (!intst) > > + return IRQ_NONE; > > + > > + for_each_set_bit(ch, &intst, RZG2L_ADC_MAX_CHANNELS) { > > + if (intst & BIT(ch)) > > I'm missing how this if can fail given we only end up in here when the bit is > set. > ADC has 8 channels RZG2L_ADSTS register bits [0:7] will be set to 1 when the given channel ADC conversion has been completed. So the above if condition checks if the bit is set to 1 and then reads the corresponding value of that channel. > > + adc->last_val[ch] = rzg2l_adc_readl(adc, RZG2L_ADCR(ch)) & > > + RZG2L_ADCR_AD_MASK; > > + } > > + > > + /* clear the channel interrupt */ > > + rzg2l_adc_writel(adc, RZG2L_ADSTS, reg); > > + > > + complete(&adc->completion); > > + > > + return IRQ_HANDLED; > > +} > > + > > +static int rzg2l_adc_parse_properties(struct platform_device *pdev, struct rzg2l_adc *adc) > > +{ > > + struct iio_chan_spec *chan_array; > > + struct fwnode_handle *fwnode; > > + struct rzg2l_adc_data *data; > > + unsigned int channel; > > + int num_channels; > > + int ret; > > + u8 i; > > + > > + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL); > > + if (!data) > > + return -ENOMEM; > > + > > + num_channels = device_get_child_node_count(&pdev->dev); > > + if (!num_channels) { > > + dev_err(&pdev->dev, "no channel children\n"); > > + return -ENODEV; > > + } > > + > > + if (num_channels > RZG2L_ADC_MAX_CHANNELS) { > > + dev_err(&pdev->dev, "num of channel children out of range\n"); > > + return -EINVAL; > > + } > > + > > + chan_array = devm_kcalloc(&pdev->dev, num_channels, sizeof(*chan_array), > > + GFP_KERNEL); > > + if (!chan_array) > > + return -ENOMEM; > > + > > + i = 0; > > + device_for_each_child_node(&pdev->dev, fwnode) { > > + ret = fwnode_property_read_u32(fwnode, "reg", &channel); > > + if (ret) > > + return ret; > > + > > + if (channel >= RZG2L_ADC_MAX_CHANNELS) > > + return -EINVAL; > > + > > + chan_array[i].type = IIO_VOLTAGE; > > + chan_array[i].indexed = 1; > > + chan_array[i].channel = channel; > > + chan_array[i].info_mask_separate = BIT(IIO_CHAN_INFO_RAW); > > + chan_array[i].datasheet_name = rzg2l_adc_channel_name[channel]; > > + i++; > > + } > > + > > + data->num_channels = num_channels; > > + data->channels = chan_array; > > + adc->data = data; > > + > > + return 0; > > +} > > + > > +static int rzg2l_adc_hw_init(struct rzg2l_adc *adc) > > +{ > > + int timeout = 5; > > + u32 reg; > > + int ret; > > + > > + ret = clk_prepare_enable(adc->pclk); > > + if (ret) > > + return ret; > > + > > + /* SW reset */ > > + reg = rzg2l_adc_readl(adc, RZG2L_ADM(0)); > > + reg |= RZG2L_ADM0_SRESB; > > + rzg2l_adc_writel(adc, RZG2L_ADM(0), reg); > > + > > + while (!(rzg2l_adc_readl(adc, RZG2L_ADM(0)) & RZG2L_ADM0_SRESB)) { > > + if (!timeout) { > > + ret = -EBUSY; > > + goto exit_hw_init; > > + } > > + timeout--; > > + usleep_range(100, 200); > > + } > > + > > + /* Only division by 4 can be set */ > > + reg = rzg2l_adc_readl(adc, RZG2L_ADIVC); > > + reg &= ~RZG2L_ADIVC_DIVADC_MASK; > > + reg |= RZG2L_ADIVC_DIVADC_4; > > + rzg2l_adc_writel(adc, RZG2L_ADIVC, reg); > > + > > + /* > > + * Setup AMD3 > > + * ADIL[31:24] - Should be always set to 0 > > + * ADCMP[23:16] - Should be always set to 0xe > > + * ADSMP[15:0] - Set default (0x578) sampling period > > + */ > > + reg = rzg2l_adc_readl(adc, RZG2L_ADM(3)); > > + reg &= ~RZG2L_ADM3_ADIL_MASK; > > + reg &= ~RZG2L_ADM3_ADCMP_MASK; > > + reg &= ~RZG2L_ADM3_ADSMP_MASK; > > + reg |= (RZG2L_ADM3_ADCMP_E | RZG2L_ADSMP_DEFUALT_SAMPLING); > > + rzg2l_adc_writel(adc, RZG2L_ADM(3), reg); > > + > > +exit_hw_init: > > + clk_disable_unprepare(adc->pclk); > > + > > + return 0; > > +} > > + > > +static void rzg2l_adc_pm_runtime_disable(void *data) > > +{ > > + struct iio_dev *indio_dev = data; > > Register the devm call with the dev instead of the iio_dev. > As this should be right next to the enable, it will be obvious > what it should be. > Agreed will do. > > + > > + pm_runtime_disable(indio_dev->dev.parent); > > +} > > + > > +static void rzg2l_adc_reset_assert(void *data) > > +{ > > + struct reset_control *reset = data; > Why do we need the local variable? > > reset_control_assert(data); > > is fine. > Agreed. > > + > > + reset_control_assert(reset); > > +} > > + > > +static int rzg2l_adc_probe(struct platform_device *pdev) > > +{ > > + struct device *dev = &pdev->dev; > > + struct iio_dev *indio_dev; > > + struct rzg2l_adc *adc; > > + int ret; > > + int irq; > > + > > + indio_dev = devm_iio_device_alloc(dev, sizeof(*adc)); > > + if (!indio_dev) > > + return -ENOMEM; > > + > > + adc = iio_priv(indio_dev); > > + > > + ret = rzg2l_adc_parse_properties(pdev, adc); > > + if (ret) > > + return ret; > > + > > + adc->base = devm_platform_ioremap_resource(pdev, 0); > > + if (IS_ERR(adc->base)) > > + return PTR_ERR(adc->base); > > + > > + irq = platform_get_irq(pdev, 0); > > Please group things together, so we 'get' whatever it is next to where > we use it. It might make the error handling a tiny bit more complex, but > it makes the code easier to review as we don't have to jump backwards and forwards > all the time. With devm_ in heavy use as you have here, that's all magic > in the error paths anyway :) > sure will move this just above the devm_request_irq(). > > + if (irq < 0) { > > + dev_err(dev, "no irq resource\n"); > > + return irq; > > + } > > + > > + adc->pclk = devm_clk_get(dev, "pclk"); > > + if (IS_ERR(adc->pclk)) { > > + dev_err(dev, "Failed to get pclk"); > > + return PTR_ERR(adc->pclk); > > + } > > + > > + adc->adclk = devm_clk_get(dev, "adclk"); > > + if (IS_ERR(adc->adclk)) { > > + dev_err(dev, "Failed to get adclk"); > > + return PTR_ERR(adc->adclk); > > + } > > + > > + adc->adrstn = devm_reset_control_get_exclusive(dev, "adrst-n"); > > + if (IS_ERR(adc->adrstn)) { > > + dev_err(dev, "failed to get adrstn\n"); > > + return PTR_ERR(adc->adrstn); > > + } > > + > > + ret = devm_add_action_or_reset(&pdev->dev, > > + rzg2l_adc_reset_assert, adc->adrstn); > > + if (ret) { > > + dev_err(&pdev->dev, "failed to register adrstn assert devm action, %d\n", > > + ret); > > + return ret; > > + } > > + > > + adc->presetn = devm_reset_control_get_exclusive(dev, "presetn"); > > + if (IS_ERR(adc->presetn)) { > > + dev_err(dev, "failed to get presetn\n"); > > + return PTR_ERR(adc->presetn); > > + } > > + > > + ret = devm_add_action_or_reset(&pdev->dev, > > + rzg2l_adc_reset_assert, adc->presetn); > > As already raised, wrong place. > Agreed. > > + if (ret) { > > + dev_err(&pdev->dev, "failed to register presetn assert devm action, %d\n", > > + ret); > > + return ret; > > + } > > + > > + ret = reset_control_deassert(adc->adrstn); > > + if (ret) { > > + dev_err(&pdev->dev, "failed to deassert adrstn pin, %d\n", ret); > > + return ret; > > + } > > + > > + ret = reset_control_deassert(adc->presetn); > > + if (ret) { > > + dev_err(&pdev->dev, "failed to deassert presetn pin, %d\n", ret); > > + return ret; > > + } > > + > > + ret = rzg2l_adc_hw_init(adc); > > + if (ret) { > > + dev_err(&pdev->dev, "failed to initialize ADC HW, %d\n", ret); > > + return ret; > > + } > > + > > + ret = devm_request_irq(dev, irq, rzg2l_adc_isr, > > + 0, dev_name(dev), adc); > > + if (ret < 0) > > + return ret; > > + > > + init_completion(&adc->completion); > > + > > + platform_set_drvdata(pdev, indio_dev); > > + > > + indio_dev->name = DRIVER_NAME; > > + indio_dev->info = &rzg2l_adc_iio_info; > > + indio_dev->modes = INDIO_DIRECT_MODE; > > + indio_dev->channels = adc->data->channels; > > + indio_dev->num_channels = adc->data->num_channels; > > + > > + pm_runtime_enable(dev); > > I think you also want to set the state to suspended to ensure the resume is > called on get. > pm_runtime_set_suspended() should only be called when runtime is disabled or is it that I am missing something ? > > + ret = devm_add_action_or_reset(&pdev->dev, > > + rzg2l_adc_pm_runtime_disable, indio_dev); > > + if (ret) { > > + dev_err(&pdev->dev, "failed to register pm_runtime_disable devm action, %d\n", > > + ret); > > This is vanishingly unlikely as it's a tiny memory allocation. So I would dropt he error > print in the interests of simplicity. Some of the other items above we don't print > on are more likely to fail. > Sure will drop this. Cheers, Prabhakar > > + return ret; > > + } > > + > > + return devm_iio_device_register(dev, indio_dev); > > +}