RE: [PATCH v10 3/5] iio: adc: Add Xilinx AMS driver

Hi Cameron,

Thanks for the review.
...

Subject: Re: [PATCH v10 3/5] iio: adc: Add Xilinx AMS driver

On Wed, 17 Nov 2021 16:10:26 +0000
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 an external master. Out of these interfaces 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.

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

A few minor additions from me to what Andy has noted.

Thanks,

Jonathan

quoted

diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile index

d3f53549720c..4a8f1833993b 100644

--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile

@@ -115,4 +115,5 @@ obj-$(CONFIG_VF610_ADC) += vf610_adc.o
 obj-$(CONFIG_VIPERBOARD_ADC) += viperboard_adc.o  xilinx-xadc-y :=

xilinx-xadc-core.o xilinx-xadc-events.o
 obj-$(CONFIG_XILINX_XADC) += xilinx-xadc.o
+obj-$(CONFIG_XILINX_AMS) += xilinx-ams.o
 obj-$(CONFIG_SD_ADC_MODULATOR) += sd_adc_modulator.o diff --git
a/drivers/iio/adc/xilinx-ams.c b/drivers/iio/adc/xilinx-ams.c new file
mode 100644 index 000000000000..bb3876b51e3e

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

@@ -0,0 +1,1447 @@
+// 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/bits.h>
+#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>
+#include <linux/overflow.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/slab.h>
+
+#include <linux/iio/events.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>

Used?

quoted

+

...

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
+ * @alarm_mask: alarm configuration
+ * @masked_alarm: currently masked due to alarm
+ * @intr_mask: interrupt configuration
+ * @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;
+	struct mutex lock;
+	spinlock_t intr_lock;
+	unsigned int alarm_mask;
+	unsigned int masked_alarm;

Hmm. maybe a rename to make these two less confusing?
Perhaps
current_masked_alarm?

quoted

+	u64 intr_mask;
+	struct delayed_work ams_unmask_work; };
+

...

quoted

+
+static void ams_handle_event(struct iio_dev *indio_dev, u32 event) {
+	const struct iio_chan_spec *chan;
+
+	chan = ams_event_to_channel(indio_dev, event);
+
+	if (chan->type == IIO_TEMP) {
+		/*
+		 * The temperature channel only supports over-temperature
+		 * events.
+		 */
+		iio_push_event(indio_dev,
+			       IIO_UNMOD_EVENT_CODE(chan->type, chan-
channel,
+						    IIO_EV_TYPE_THRESH,
+						    IIO_EV_DIR_RISING),
+			iio_get_time_ns(indio_dev));
+	} else {
+		/*
+		 * For other channels we don't know whether it is a upper or
+		 * lower threshold event. Userspace will have to check the
+		 * channel value if it wants to know.
+		 */
+		iio_push_event(indio_dev,
+			       IIO_UNMOD_EVENT_CODE(chan->type, chan-
channel,
+						    IIO_EV_TYPE_THRESH,
+						    IIO_EV_DIR_EITHER),
+			iio_get_time_ns(indio_dev));

I think alignment is wrong here. iio_get_time_ns() should align with opening
bracket as well.

quoted

+	}
+}
+

...

quoted

+
+static int ams_parse_firmware(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 fwnode_handle *child = NULL, *fwnode =
+dev_fwnode(&pdev->dev);

Where you have values being set, I'd prefer separate line per variable.
Tends to be a little more readable and we need all the help we can get :)

quoted

+	size_t dev_chan_size, ams_chan_size, num_chan;
+	int ret, ch_cnt = 0, i, rising_off, falling_off;
+	unsigned int num_channels = 0;
+

One blank line is almost always enough. Definitely is here.

quoted

+
+	num_chan = ARRAY_SIZE(ams_ps_channels) +

ARRAY_SIZE(ams_pl_channels) +

quoted

+		ARRAY_SIZE(ams_ctrl_channels);
+
+	ams_chan_size = array_size(num_chan, sizeof(struct

iio_chan_spec));

quoted

+	if (ams_chan_size == SIZE_MAX)
+		return -EINVAL;
+
+	/* Initialize buffer for channel specification */
+	ams_channels = kcalloc(num_chan, sizeof(struct iio_chan_spec),

GFP_KERNEL);

quoted

+	if (!ams_channels)
+		return -ENOMEM;
+
+	if (fwnode_device_is_available(fwnode)) {
+		ret = ams_init_module(indio_dev, fwnode, ams_channels);
+		if (ret < 0)
+			goto free_mem;
+
+		num_channels += ret;
+	}
+
+	fwnode_for_each_child_node(fwnode, child) {
+		if (fwnode_device_is_available(child)) {
+			ret = ams_init_module(indio_dev, child,
+					      ams_channels + num_channels);
+			if (ret < 0) {
+				fwnode_handle_put(child);
+				goto free_mem;
+			}
+
+			num_channels += ret;
+		}
+	}
+
+	for (i = 0; i < num_channels; i++) {
+		ams_channels[i].channel = ch_cnt++;
+
+		if (ams_channels[i].scan_index < AMS_CTRL_SEQ_BASE) {
+			/* 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_chan_size = array_size((size_t)num_channels, sizeof(struct

iio_chan_spec));

quoted

+	if (dev_chan_size == SIZE_MAX)

Why not goto free_mem for this error case?
Obviously should never happen, but should handle the error anyway.

Correct. But I didn't realize that kcalloc has this check done internally.
So I will remove this in the next patch.

quoted

+		return -EINVAL;
+
+	dev_channels = devm_kcalloc(&pdev->dev, (size_t)num_channels,
+				    sizeof(struct iio_chan_spec), GFP_KERNEL);
+	if (!dev_channels) {
+		ret = -ENOMEM;
+		goto free_mem;
+	}
+
+	memcpy(dev_channels, ams_channels,
+	       sizeof(*ams_channels) * num_channels);
+	indio_dev->channels = dev_channels;
+	indio_dev->num_channels = num_channels;
+
+	ret = 0;
+
+free_mem:
+	kfree(ams_channels);
+
+	return ret;
+}

I agree with everything.