Hi Jonathan,
Thanks for reviewing my patch.
> Subject: Re: [PATCH v8 2/4] iio: adc: Add Xilinx AMS driver
>
> On Mon, 8 Nov 2021 21:05:07 +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>
> Hi Anand,
>
> +Cc Andy for a question on of_iomap() and whether we should add
> +something similar
> to the generic firmware handling. Personally I'd be happy with leaving it for
> now on basis of it being something to come back and tidy up later, but would
> like your opinion as you are more familiar with that stuff.
>
> Given you are going around again, I've picked out a few minor things on a
> final read through to clean up but subject to the above and review from
> others I'm happy with this now and hopefully can pick up v9.
>
> Thanks,
>
> Jonathan
>
>
> > ---
> > drivers/iio/adc/Kconfig | 15 +
> > drivers/iio/adc/Makefile | 1 +
> > drivers/iio/adc/xilinx-ams.c | 1452
> > ++++++++++++++++++++++++++++++++++
> > 3 files changed, 1468 insertions(+)
> > create mode 100644 drivers/iio/adc/xilinx-ams.c
> >
> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig index
> > af168e1c9fdb..6d711f401326 100644
> > --- a/drivers/iio/adc/Kconfig
> > +++ b/drivers/iio/adc/Kconfig
> > @@ -1278,4 +1278,19 @@ config XILINX_XADC
> > The driver can also be build as a module. If so, the module will be
> called
> > xilinx-xadc.
> >
> > +config XILINX_AMS
> > + tristate "Xilinx AMS driver"
> > + depends on ARCH_ZYNQMP || COMPILE_TEST
> > + depends on HAS_IOMEM
> > + help
> > + Say yes here to have support for the Xilinx AMS for
> Ultrascale/Ultrascale+
> > + System Monitor. With this you can measure and monitor the
> Voltages and
> > + Temperature values on the SOC.
>
> Already noted in other review thread
>
> > +
> > + The driver supports Voltage and Temperature monitoring on Xilinx
> Ultrascale
> > + devices.
> > +
> > + The driver can also be built as a module. If so, the module will be
> called
> > + xilinx-ams.
> > +
> > endmenu
> > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile index
> > d68550f493e3..8ced2a3a153f 100644
> > --- a/drivers/iio/adc/Makefile
> > +++ b/drivers/iio/adc/Makefile
> > @@ -114,4 +114,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..6e325c4f0a5d
> > --- /dev/null
> > +++ b/drivers/iio/adc/xilinx-ams.c
> > @@ -0,0 +1,1452 @@
> > +// 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/of_address.h>
>
> So this is the last of specific header needed for of_iomap() and similar.
>
> > +#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>
> > +
>
> ...
>
> > +/**
> > + * 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
> > + * @irq: interrupt number of the sysmon
>
> Run kernel-doc script over the file and fix the warnings. Here you dropped
> the field in this version but missed the docs.
I will fix this. Not sure how I missed when I ran kernel doc.
>
> > + * @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;
> > + u64 intr_mask;
> > + struct delayed_work ams_unmask_work; };
> > +
>
> ...
>
>
> > +
> > +static int ams_init_module(struct iio_dev *indio_dev, struct
> fwnode_handle *np,
> > + struct iio_chan_spec *channels)
> > +{
> > + struct ams *ams = iio_priv(indio_dev);
> > + struct device *dev = indio_dev->dev.parent;
> > + struct device_node *node;
> > + int num_channels = 0;
> > + int ret;
> > +
> > + node = to_of_node(np);
> > + if (fwnode_property_match_string(np, "compatible",
> > + "xlnx,zynqmp-ams-ps") == 0) {
> > + ams->ps_base = of_iomap(node, 0);
> > + if (!ams->ps_base)
> > + return -ENXIO;
> > + ret = devm_add_action_or_reset(dev, ams_iounmap, ams-
> >ps_base);
> > + if (ret < 0)
> > + return ret;
> > +
> > + /* add PS channels to iio device channels */
> > + memcpy(channels + num_channels, ams_ps_channels,
> > + sizeof(ams_ps_channels));
> > + num_channels += ARRAY_SIZE(ams_ps_channels);
> > + } else if (fwnode_property_match_string(np, "compatible",
> > + "xlnx,zynqmp-ams-pl") == 0)
> {
> > + ams->pl_base = of_iomap(node, 0);
>
> Hmm. So of_iomap() leaves us dependent on dt specific calls. Whilst it
> doesn't
> exactly look hard to create a generic version covering at least dt and acpi
> I don' think there is an equivalent acpi function currently defined.
Correct. I tried finding it but couldn't find. This was sort of a workaround that I did.
>
> Andy, do you think this is a good thing to add to the generic firmware node
> handling? It's a bit specialist as most of the time this will be wrapped up
> in the platform device handling or similar rather than being in a child node like
> this.
>
>
> > + if (!ams->pl_base)
> > + return -ENXIO;
> > +
> > + ret = devm_add_action_or_reset(dev, ams_iounmap, ams-
> >pl_base);
> > + if (ret < 0)
> > + return ret;
> > +
> > + /* Copy only first 10 fix channels */
> > + memcpy(channels + num_channels, ams_pl_channels,
> > + AMS_PL_MAX_FIXED_CHANNEL * sizeof(*channels));
> > + num_channels += AMS_PL_MAX_FIXED_CHANNEL;
> > +
> > + num_channels = ams_get_ext_chan(np, channels,
> > + num_channels);
> > + } else if (fwnode_property_match_string(np, "compatible",
> > + "xlnx,zynqmp-ams") == 0) {
> > + /* add AMS channels to iio device channels */
> > + memcpy(channels + num_channels, ams_ctrl_channels,
> > + sizeof(ams_ctrl_channels));
> > + num_channels += ARRAY_SIZE(ams_ctrl_channels);
> > + } else {
> > + return -EINVAL;
> > + }
> > +
> > + return num_channels;
> > +}
> > +
> > +static int ams_parse_dt(struct iio_dev *indio_dev, struct platform_device
> *pdev)
>
> Almost nothing in here is dt specific now, so you could rename it
> ams_parse_firmware()
Sure.
>
> > +{
> > + struct ams *ams = iio_priv(indio_dev);
> > + struct iio_chan_spec *ams_channels, *dev_channels;
> > + struct fwnode_handle *child_node = NULL, *fwnode =
> dev_fwnode(&pdev->dev);
> > + int ret, 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 (fwnode_device_is_available(fwnode)) {
> > + ret = ams_init_module(indio_dev, fwnode, ams_channels);
> > + if (ret < 0)
> > + goto err;
> > +
> > + num_channels += ret;
> > + }
> > +
> > + fwnode_for_each_child_node(fwnode, child_node) {
> > + if (fwnode_device_is_available(child_node)) {
> > + ret = ams_init_module(indio_dev, child_node,
> > + ams_channels + num_channels);
> > + if (ret < 0) {
> > + fwnode_handle_put(child_node);
> > + goto err;
> > + }
> > +
> > + 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_channels = devm_kzalloc(&pdev->dev, sizeof(*dev_channels) *
> > + num_channels, GFP_KERNEL);
> > + if (!dev_channels) {
> > + ret = -ENOMEM;
> > + goto err;
> > + }
> > +
> > + 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;
> > +}
> > +
> > +static const struct iio_info iio_ams_info = {
> > + .read_raw = &ams_read_raw,
> > + .read_event_config = &ams_read_event_config,
> > + .write_event_config = &ams_write_event_config,
> > + .read_event_value = &ams_read_event_value,
> > + .write_event_value = &ams_write_event_value,
> > +};
> > +
>
> ...
>
> > +static int __maybe_unused ams_resume(struct device *dev)
> > +{
> > + struct ams *ams = iio_priv(dev_get_drvdata(dev));
> > + int ret;
> > +
> > + ret = clk_prepare_enable(ams->clk);
>
> Given clk_prepare_enable() uses if (ret) to check in all paths we
> it will not ever return a positive value so you can shorten this to
>
> return clk_prepare_enable(ams->clk);
>
> and drop the local variable ret.
Will do.
>
> > + if (ret < 0)
> > + return ret;
> > +
> > + return 0;
> > +}
> ...
>
> Jonathan
>
Thanks,
Anand
