Date:   Mon, 15 Nov 2021 10:58:53 +0000
From:   Anand Ashok Dumbre <ANANDASH@...inx.com>
To:     Jonathan Cameron <jic23@...nel.org>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "lars@...afoo.de" <lars@...afoo.de>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        git <git@...inx.com>, Michal Simek <michals@...inx.com>,
        "pmeerw@...erw.net" <pmeerw@...erw.net>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        Manish Narani <MNARANI@...inx.com>,
        Andy Shevchenko <andy.shevchenko@...il.com>
Subject: RE: [PATCH v8 2/4] iio: adc: Add Xilinx AMS driver

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@...inx.com> 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@...inx.com>
> > Signed-off-by: Manish Narani <manish.narani@...inx.com>
> > Signed-off-by: Anand Ashok Dumbre <anand.ashok.dumbre@...inx.com>
> 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@...inx.com>
> > + *  Rajnikant Bhojani <rajnikant.bhojani@...inx.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/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

Powered by blists - more mailing lists