-----Original Message-----
From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
Sent: Thursday 15 July 2021 1:52 PM
To: Anand Ashok Dumbre <redacted>
Cc: Jonathan Cameron <jic23@kernel.org>; lars@metafoo.de; linux-
iio@vger.kernel.org; git-dev [off-list ref]; Michal Simek
[off-list ref]; pmeerw@pmeerw.net; devicetree@vger.kernel.org;
linux-kernel@vger.kernel.org; Manish Narani [off-list ref]
Subject: Re: [PATCH v6 2/4] iio: adc: Add Xilinx AMS driver

...

quoted

quoted

quoted

+	if (IS_ERR(ams->base))
+		return PTR_ERR(ams->base);
+
+	ams->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(ams->clk))
+		return PTR_ERR(ams->clk);
+	clk_prepare_enable(ams->clk);
+	devm_add_action_or_reset(&pdev->dev, (void

*)clk_disable_unprepare,

quoted

+				 ams->clk);
+
+	INIT_DELAYED_WORK(&ams->ams_unmask_work,

ams_unmask_worker);

quoted

+	devm_add_action_or_reset(&pdev->dev, (void

*)cancel_delayed_work,

I'm not keen on casting away the function pointer type.  Normally
we'd just wrap it in a local function, to make it clear it was
deliberate and avoid potential nasty problems if the signature of the

function ever changes.

quoted

quoted

It's 3 lines of boilerplate, but will give me warm fuzzy feelings!
Same for the other case above.  The fact this isn't done in exising
kernel code make this particularly risky.

Makes sense. I will revert the code back to its original and handle
the cases using goto and inside remove()

Ah.  Not what I meant.  I was suggesting you add a little function locally that
has the right type and in turn calls cancel_delayed_work().

As that directly exposes the actual function calls, any signature change in
future will cause compile breakage (or be picked up any automated tools
doing that refactor).

Now I understand.
Will fix it in the next series.

quoted

quoted

quoted

+				 &ams->ams_unmask_work);
+
+	ret = ams_init_device(ams);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to initialize AMS\n");
+		return ret;
+	}
+
+	ret = ams_parse_dt(indio_dev, pdev);
+	if (ret) {
+		dev_err(&pdev->dev, "failure in parsing DT\n");
+		return ret;
+	}
+
+	ams_enable_channel_sequence(indio_dev);
+
+	ams->irq = platform_get_irq(pdev, 0);

platform_get_irq () can return errors, in particular -EPROBE_DEFER
so I'd check that and return before you call devm_request_irq() I'm
not sure
devm_request_irq() will not eat that error code.

Will fix this in next series.

quoted

quoted

+	ret = devm_request_irq(&pdev->dev, ams->irq, &ams_irq, 0, "ams-

irq",

quoted

+			       indio_dev);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to register interrupt\n");
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, indio_dev);
+
+	return iio_device_register(indio_dev); }
+
+static int ams_remove(struct platform_device *pdev) {
+	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
+
+	iio_device_unregister(indio_dev);

If this is all you have in remove, then you can use
devm_iio_device_register() in probe() and not need an remove() callback

at all.

quoted

I think remove will have more functions since I am getting rid of
devm_add_action_or_reset()

See above.

J

quoted

quoted

quoted

+
+	return 0;
+}
+

...

Thanks,
Anand