linux-kernel - Re: [PATCH v3 2/3] iio: adc: Add driver for Renesas RZ/G2L A/D converter

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 27 Jul 2021 09:13:44 +0200
From:   Philipp Zabel <p.zabel@...gutronix.de>
To:     Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        Rob Herring <robh+dt@...nel.org>,
        Jonathan Cameron <jic23@...nel.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Magnus Damm <magnus.damm@...il.com>,
        Alexandru Ardelean <aardelean@...iqon.com>,
        linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
        linux-renesas-soc@...r.kernel.org
Cc:     linux-kernel@...r.kernel.org,
        Prabhakar <prabhakar.csengg@...il.com>,
        Biju Das <biju.das.jz@...renesas.com>
Subject: Re: [PATCH v3 2/3] iio: adc: Add driver for Renesas RZ/G2L A/D
 converter

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@...renesas.com>
> Reviewed-by: Biju Das <biju.das.jz@...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