linux-renesas-soc.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Philipp Zabel <p.zabel@pengutronix.de>
To: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>,
	Geert Uytterhoeven <geert+renesas@glider.be>,
	Rob Herring <robh+dt@kernel.org>,
	Jonathan Cameron <jic23@kernel.org>,
	Lars-Peter Clausen <lars@metafoo.de>,
	Magnus Damm <magnus.damm@gmail.com>,
	Alexandru Ardelean <aardelean@deviqon.com>,
	linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
	linux-renesas-soc@vger.kernel.org
Cc: linux-kernel@vger.kernel.org,
	Prabhakar <prabhakar.csengg@gmail.com>,
	Biju Das <biju.das.jz@bp.renesas.com>
Subject: Re: [PATCH v3 2/3] iio: adc: Add driver for Renesas RZ/G2L A/D converter
Date: Tue, 27 Jul 2021 09:13:44 +0200	[thread overview]
Message-ID: <f23358e3e040cc8522b259669ec61a22c5439394.camel@pengutronix.de> (raw)
In-Reply-To: <20210726182850.14328-3-prabhakar.mahadev-lad.rj@bp.renesas.com>

Hi Prabhakar,

On Mon, 2021-07-26 at 19:28 +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 <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
>  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/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 @@
[...]
> +static void rzg2l_adc_pm_runtime_disable(void *data)
> +{
> +	struct iio_dev *indio_dev = data;
> +
> +	pm_runtime_disable(indio_dev->dev.parent);
> +}
> +
> +static void rzg2l_adc_reset_assert(void *data)
> +{
> +	struct reset_control *reset = data;
> +
> +	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);
> +	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);
> +	}

I'd request the "presetn" control up here, so if that fails we don't
touch the "adrst-n" reset line.

> +	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;
> +	}

This is the wrong way around. Installing devres actions should be done
after the thing they are supposed to revert in case of error. You should
move this down below the reset_control_deassert(adc->adrstn).

> +
> +	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);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to register presetn assert devm action, %d\n",
> +			ret);
> +		return ret;
> +	}

Same as above, this belongs after the presetn deassert below.

> +
> +	ret = reset_control_deassert(adc->adrstn);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to deassert adrstn pin, %d\n", ret);
> +		return ret;
> +	}

Here is the place to install the adrstn assert action.

> +	ret = reset_control_deassert(adc->presetn);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to deassert presetn pin, %d\n", ret);
> +		return ret;
> +	}

And here is the place to install the presetn assert action.

regards
Philipp

  reply	other threads:[~2021-07-27  7:13 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-26 18:28 [PATCH v3 0/3] Renesas RZ/G2L ADC driver support Lad Prabhakar
2021-07-26 18:28 ` [PATCH v3 1/3] dt-bindings: iio: adc: Add binding documentation for Renesas RZ/G2L A/D converter Lad Prabhakar
2021-07-26 21:42   ` Rob Herring
2021-07-27  8:06     ` Lad, Prabhakar
2021-07-27 20:30   ` Rob Herring
2021-07-26 18:28 ` [PATCH v3 2/3] iio: adc: Add driver " Lad Prabhakar
2021-07-27  7:13   ` Philipp Zabel [this message]
2021-07-27  8:02     ` Lad, Prabhakar
2021-07-27  8:13       ` Philipp Zabel
2021-07-27  8:54         ` Lad, Prabhakar
2021-07-31 17:11   ` Jonathan Cameron
2021-07-31 18:31     ` Lad, Prabhakar
2021-08-01 16:59       ` Jonathan Cameron
2021-08-01 19:51         ` Lad, Prabhakar
2021-07-26 18:28 ` [PATCH v3 3/3] arm64: dts: renesas: r9a07g044: Add ADC node Lad Prabhakar

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=f23358e3e040cc8522b259669ec61a22c5439394.camel@pengutronix.de \
    --to=p.zabel@pengutronix.de \
    --cc=aardelean@deviqon.com \
    --cc=biju.das.jz@bp.renesas.com \
    --cc=devicetree@vger.kernel.org \
    --cc=geert+renesas@glider.be \
    --cc=jic23@kernel.org \
    --cc=lars@metafoo.de \
    --cc=linux-iio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-renesas-soc@vger.kernel.org \
    --cc=magnus.damm@gmail.com \
    --cc=prabhakar.csengg@gmail.com \
    --cc=prabhakar.mahadev-lad.rj@bp.renesas.com \
    --cc=robh+dt@kernel.org \
    --subject='Re: [PATCH v3 2/3] iio: adc: Add driver for Renesas RZ/G2L A/D converter' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).