Hey Jonathan,

Thanks for the review. 

On Tue, 19 Oct 2021 16:20:46 +0100
Anand Ashok Dumbre [off-list ref] wrote:

quoted

The AMS includes an ADC as well as on-chip sensors that can be used to
sample external voltages and monitor on-die operating conditions, such
as temperature and supply voltage levels. The AMS has two SYSMON

blocks.

quoted

PL-SYSMON block is capable of monitoring off chip voltage and
temperature.
PL-SYSMON block has DRP, JTAG and I2C interface to enable monitoring
from external master. Out of these interface currently only DRP is
supported.
Other block PS-SYSMON is memory mapped to PS.
The AMS can use internal channels to monitor voltage and temperature
as well as one primary and up to 16 auxiliary channels for measuring
external voltages.
The voltage and temperature monitoring channels also have event
capability which allows to generate an interrupt when their value
falls below or raises above a set threshold.

Signed-off-by: Manish Narani <redacted>
Signed-off-by: Anand Ashok Dumbre <redacted>

It would be good at some point to move away from of specific firmware
property reading, but on a platform like this I guess you can be fairly sure no
one will be using ACPI or other firmware description options so I'm not going
to insist on it for an initial merge.

Other comment I have are fairly minor but we need to leave some time for
other reviews and in particular DT binding review.

Sure. I will wait for the DT binding review before I send my fixes.

quoted

---
 drivers/iio/adc/Kconfig      |   13 +
 drivers/iio/adc/Makefile     |    1 +
 drivers/iio/adc/xilinx-ams.c | 1341
++++++++++++++++++++++++++++++++++
 3 files changed, 1355 insertions(+)
 create mode 100644 drivers/iio/adc/xilinx-ams.c

...

quoted

diff --git a/drivers/iio/adc/xilinx-ams.c

b/drivers/iio/adc/xilinx-ams.c new file mode 100644 index
000000000000..597cdda8a618

--- /dev/null
+++ b/drivers/iio/adc/xilinx-ams.c

@@ -0,0 +1,1341 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Xilinx AMS driver
+ *
+ *  Copyright (C) 2021 Xilinx, Inc.
+ *
+ *  Manish Narani <mnarani@xilinx.com>
+ *  Rajnikant Bhojani <rajnikant.bhojani@xilinx.com>  */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/kernel.h>
+#include <linux/module.h>

#include <linux/mod_devicetable.h> for the of_device_id structure
defintion.

Will add it in the next series.

quoted

+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#include <linux/iio/events.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+

...

quoted

+/**
+ * struct ams - Driver data for xilinx-ams
+ * @base: physical base address of device
+ * @ps_base: physical base address of PS device
+ * @pl_base: physical base address of PL device
+ * @clk: clocks associated with the device
+ * @dev: pointer to device struct
+ * @lock: to handle multiple user interaction
+ * @intr_lock: to protect interrupt mask values
+ * @masked_alarm: currently masked due to alarm
+ * @alarm_mask: alarm configuration
+ * @interrupt_mask: interrupt configuration
+ * @irq: interrupt number of the sysmon
+ * @ams_unmask_work: re-enables event once the event condition
+disappears
+ *
+ * This structure contains necessary state for Sysmon driver to
+operate  */ struct ams {
+	void __iomem *base;
+	void __iomem *ps_base;
+	void __iomem *pl_base;
+	struct clk *clk;
+	struct device *dev;
+	/* check kernel doc above */
+	struct mutex lock;
+	/* check kernel doc above */
+	spinlock_t intr_lock;
+	unsigned int alarm_mask;

Docs should be same order as the fields.

quoted

+	unsigned int masked_alarm;
+	u64 intr_mask;

That's not the name in the docs.  Run kernel-doc script over this and fix all the
errors / warnings.

Will do it before I send next series.
Will fix the error.

quoted

+	int irq;
+	struct delayed_work ams_unmask_work; };
+

...

quoted

+
+static irqreturn_t ams_irq(int irq, void *data) {
+	struct iio_dev *indio_dev = data;
+	struct ams *ams = iio_priv(indio_dev);
+	u32 isr0;
+
+	spin_lock(&ams->intr_lock);
+
+	isr0 = readl(ams->base + AMS_ISR_0);
+
+	/* only process alarms that are not masked */
+	isr0 &= ~((ams->intr_mask & AMS_ISR0_ALARM_MASK) |
+ams->masked_alarm);
+
+	if (!isr0)

lock held.

Will fix.

quoted

+		return IRQ_NONE;
+
+	/* clear interrupt */
+	writel(isr0, ams->base + AMS_ISR_0);
+
+	/* Mask the alarm interrupts until cleared */
+	ams->masked_alarm |= isr0;
+	ams_update_intrmask(ams, 0, 0);
+
+	ams_handle_events(indio_dev, isr0);
+
+	schedule_delayed_work(&ams->ams_unmask_work,
+			      msecs_to_jiffies(AMS_UNMASK_TIMEOUT_MS));
+
+	spin_unlock(&ams->intr_lock);
+
+	return IRQ_HANDLED;
+}
+

...

quoted

+
+static int ams_parse_dt(struct iio_dev *indio_dev, struct
+platform_device *pdev) {
+	struct ams *ams = iio_priv(indio_dev);
+	struct iio_chan_spec *ams_channels, *dev_channels;
+	struct device_node *child_node = NULL, *np = pdev->dev.of_node;
+	int ret, vol_ch_cnt = 0, temp_ch_cnt = 0, i, rising_off, falling_off;
+	unsigned int num_channels = 0;
+
+	/* Initialize buffer for channel specification */
+	ams_channels = kzalloc(sizeof(ams_ps_channels) +
+			       sizeof(ams_pl_channels) +
+			       sizeof(ams_ctrl_channels), GFP_KERNEL);
+	if (!ams_channels)
+		return -ENOMEM;
+
+	if (of_device_is_available(np)) {
+		ret = ams_init_module(indio_dev, np, ams_channels);
+		if (ret < 0)
+			goto err;
+
+		num_channels += ret;
+	}
+
+	for_each_child_of_node(np, child_node) {
+		if (of_device_is_available(child_node)) {
+			ret = ams_init_module(indio_dev, child_node,
+					      ams_channels + num_channels);
+			if (ret < 0)
+				goto err;

for_each_child_of_node() holds a reference if we jump out of the loop
before it terminates.
https://elixir.bootlin.com/linux/latest/source/drivers/of/base.c#L715
is where it ultimately releases that reference when the loop is terminating.
Her you need to do it manually with an of_node_put() call

Correct. Will fix this. 

quoted

+
+			num_channels += ret;
+		}
+	}
+
+	for (i = 0; i < num_channels; i++) {
+		if (ams_channels[i].type == IIO_VOLTAGE)
+			ams_channels[i].channel = vol_ch_cnt++;
+		else
+			ams_channels[i].channel = temp_ch_cnt++;
+
+		if (ams_channels[i].scan_index < (AMS_PS_SEQ_MAX * 3)) {
+			/* set threshold to max and min for each channel */
+			falling_off =
+

	ams_get_alarm_offset(ams_channels[i].scan_index,

quoted

+						     IIO_EV_DIR_FALLING);
+			rising_off =
+

	ams_get_alarm_offset(ams_channels[i].scan_index,

quoted

+						     IIO_EV_DIR_RISING);
+			if (ams_channels[i].scan_index >=

AMS_PS_SEQ_MAX) {

quoted

+				writel(AMS_ALARM_THR_MIN,
+				       ams->pl_base + falling_off);
+				writel(AMS_ALARM_THR_MAX,
+				       ams->pl_base + rising_off);
+			} else {
+				writel(AMS_ALARM_THR_MIN,
+				       ams->ps_base + falling_off);
+				writel(AMS_ALARM_THR_MAX,
+				       ams->ps_base + rising_off);
+			}
+		}
+	}
+
+	dev_channels = devm_kzalloc(&pdev->dev, sizeof(*dev_channels) *
+				    num_channels, GFP_KERNEL);
+	if (!dev_channels) {
+		ret = -ENOMEM;
+		goto err;
+	}

We now have the option of devm_krealloc()   If you used that in conjunction
with devm_kzalloc to replace the kzalloc above, you could avoid this need to
copy.  Not important though if you prefer doing this manual version.

For now I will leave this as is. But will update after initial check-in.

quoted

+
+	memcpy(dev_channels, ams_channels,
+	       sizeof(*ams_channels) * num_channels);
+	indio_dev->channels = dev_channels;
+	indio_dev->num_channels = num_channels;
+
+	ret = 0;
+err:
+	kfree(ams_channels);
+
+	return ret;
+}
+

...

quoted

+static int ams_probe(struct platform_device *pdev) {
+	struct iio_dev *indio_dev;
+	struct ams *ams;
+	int ret;
+
+	if (!pdev->dev.of_node)
+		return -ENODEV;
+
+	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*ams));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	ams = iio_priv(indio_dev);
+	mutex_init(&ams->lock);
+	spin_lock_init(&ams->intr_lock);
+
+	indio_dev->name = "xilinx-ams";
+
+	indio_dev->info = &iio_ams_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	ams->base = devm_platform_ioremap_resource(pdev, 0);
+	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,

ams_clk_disable_unprepare,

quoted

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

ams_unmask_worker);

quoted

+	devm_add_action_or_reset(&pdev->dev,

ams_cancel_delayed_work,

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);
+	if (ams->irq == -EPROBE_DEFER) {
+		ret = -EPROBE_DEFER;
+		return ret;
+	}
+
+	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);
+
+	ret = devm_iio_device_register(&pdev->dev, indio_dev);
+

return devm_...

Will do.

quoted

+	return ret;
+}
+

Thanks,
Anand