Hi Philipp,

Thank you for the review.

On Tue, Jul 27, 2021 at 8:13 AM Philipp Zabel [off-list ref] wrote:

Hi Prabhakar,

On Mon, 2021-07-26 at 19:28 +0100, Lad Prabhakar wrote:

quoted

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

[...]

quoted

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 @@

[...]

quoted

+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.

Ok will move the devm_reset_control_get_exclusive() call for presetn here.

quoted

+     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).

Ouch my understanding was, there won't be any harm in asserting the
reset line. Agree with will move this below
reset_control_deassert(adc->adrstn).

quoted

+
+     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.

Agreed.

quoted

+
+     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.

Agreed will move the devres action here.

quoted

+     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.

Agreed will move the devres action here.

Cheers,
Prabhakar

regards
Philipp