Message-ID: <BY5PR02MB6916B1D4F3FB8D88C222973CA9849@BY5PR02MB6916.namprd02.prod.outlook.com>
Date:   Tue, 26 Oct 2021 10:18:17 +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>
Subject: RE: [PATCH v7 2/4] iio: adc: Add Xilinx AMS driver

Hey Jonathan,

Thanks for the review. 

> On Tue, 19 Oct 2021 16:20:46 +0100
> 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 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 <manish.narani@...inx.com>
> > Signed-off-by: Anand Ashok Dumbre <anand.ashok.dumbre@...inx.com>
> 
> 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.

> 
> > ---
> >  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
> 
> ...
> 
> > 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@...inx.com>
> > + *  Rajnikant Bhojani <rajnikant.bhojani@...inx.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.

> > +#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>
> > +
> ...
> 
> > +/**
> > + * 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.
> > +	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.

> > +	int irq;
> > +	struct delayed_work ams_unmask_work; };
> > +
> 
> ...
> 
> > +
> > +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.

> 
> > +		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;
> > +}
> > +
> 
> ...
> 
> > +
> > +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. 
> > +
> > +			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,
> > +						     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;
> > +	}
> 
> 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.

> > +
> > +	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 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,
> > +				 ams->clk);
> > +
> > +	INIT_DELAYED_WORK(&ams->ams_unmask_work,
> ams_unmask_worker);
> > +	devm_add_action_or_reset(&pdev->dev,
> ams_cancel_delayed_work,
> > +				 &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",
> > +			       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.

> > +	return ret;
> > +}
> > +
> 

Thanks,
Anand

Powered by blists - more mailing lists