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 <anand.ashok.dumbre@xxxxxxxxxx> wrote:
>
> > 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.
> > 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 <manish.narani@xxxxxxxxxx>
> > Signed-off-by: Manish Narani <manish.narani@xxxxxxxxxx>
> > Signed-off-by: Anand Ashok Dumbre <anand.ashok.dumbre@xxxxxxxxxx>
>
> A few minor additions from me to what Andy has noted.
>
> Thanks,
>
> Jonathan
>
> > 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@xxxxxxxxxx>
> > + * Rajnikant Bhojani <rajnikant.bhojani@xxxxxxxxxx> */
> > +
> > +#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?
> > +
>
> ...
>
> > +/**
> > + * 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?
>
> > + u64 intr_mask;
> > + struct delayed_work ams_unmask_work; };
> > +
>
>
> ...
>
>
> > +
> > +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.
>
> > + }
> > +}
> > +
>
> ...
>
> > +
> > +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 :)
>
> > + 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.
>
> > +
> > + num_chan = ARRAY_SIZE(ams_ps_channels) +
> ARRAY_SIZE(ams_pl_channels) +
> > + ARRAY_SIZE(ams_ctrl_channels);
> > +
> > + ams_chan_size = array_size(num_chan, sizeof(struct
> iio_chan_spec));
> > + 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);
> > + 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,
> > + IIO_EV_DIR_FALLING);
> > + rising_off =
> > +
> ams_get_alarm_offset(ams_channels[i].scan_index,
> > + IIO_EV_DIR_RISING);
> > + if (ams_channels[i].scan_index >=
> AMS_PS_SEQ_MAX) {
> > + 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));
> > + 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.
>
> > + 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.
