mbox series

[v6,0/4] Add Xilinx AMS Driver

Message ID 20210624182939.12881-1-anand.ashok.dumbre@xilinx.com
Headers show
Series Add Xilinx AMS Driver | expand

Message

Anand Ashok Dumbre June 24, 2021, 6:29 p.m. UTC 
Add Xilinx AMS driver which is used for Xilinx's ZynqMP AMS controller.
This AMS driver is used to report various interface voltages and temperatures
across the system.
This driver will be used by iio-hwmon to repport voltages and temperatures
across the system by using various channel interfaces.
This driver handles AMS module including PS-Sysmon & PL-Sysmon. The binding
documentation is added for understanding of AMS, PS, PL Sysmon Channels.

Changes in v2:
	- Added documentation for sysfs (Patch-2)
	- Addressed code style review comments
	- Patch-2 (Now it is Patch-3)
		- Arranged the includes in alphabetical order
		- Removed the wrapper 'ams_pl_write_reg()' and used writel
		  instead
		- Removed the unnecessary delay of 1ms and used polling of EOC
		  instead
		- Removed spin_lock and used mutex only.
		- Used request_irq() instead of devm_request_irq() and handled
		  respective error conditions
		- Moved contents of xilinx-ams.h to inline with xilinx-ams.c
	- Patch-1
		- Addressed Documentation style comments

Changes in v3:
	- Updated bindings document with the suggested modification in v2 review
	- Removed documentation for sysfs
	- Removed extended names for channels in the Xilinx AMS driver
	- Modified dts to use ranges for child nodes
	- Reduced address and size cells to 32-bit instead of 64-bit

Changes in v4:
	- Updated bindings document with the suggested modification in v3 review
	- Changed the Device Tree property 'ranges' for child nodes
	- Used Channel Numbers as 'reg' value in DT to avoid confusion
	- Removed unused NULL arguments as suggested in v3 patch review
	- Addressed comments on Device Tree property naming

Changes in v5:
	- Updated bindings document to the YAML format
	- Updated bindings document with the suggested modification in v4 review
	- Renamed iio_pl_info struct to iio_ams_info in Xilinx AMS driver
	- Updated the Xilinx AMS driver to not use iio_priv_to_dev function
	- Updated Xilinx AMS node to reflect the changes in bindings document
	- Update MAINTAINERS file

Changes in v6:
	- Removed all tabs from bindings document.
	- Removed the xlnx,ext-channels node from the device tree since
	  it is not neeeded.
	- Fixed unit addresses for ps-ams and pl-ams.
	- Removed the names property from bindings.
	- Fixed warnings from checkpatch.pl in the driver.
	- devm_add_action_or_reset() used for exit/error path.
	- devm_request_irq() for managed irq request instead of
	  request_irq()




Anand Ashok Dumbre (4):
  arm64: zynqmp: DT: Add Xilinx AMS node
  iio: adc: Add Xilinx AMS driver
  dt-bindings: iio: adc: Add Xilinx AMS binding documentation
  MAINTAINERS: Add maintainer for xilinx-ams

 .../bindings/iio/adc/xlnx,zynqmp-ams.yaml     |  228 +++
 MAINTAINERS                                   |    7 +
 arch/arm64/boot/dts/xilinx/zynqmp.dtsi        |   26 +-
 drivers/iio/adc/Kconfig                       |   13 +
 drivers/iio/adc/Makefile                      |    1 +
 drivers/iio/adc/xilinx-ams.c                  | 1342 +++++++++++++++++
 6 files changed, 1616 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/xlnx,zynqmp-ams.yaml
 create mode 100644 drivers/iio/adc/xilinx-ams.c

Comments

Jonathan Cameron July 4, 2021, 6:31 p.m. UTC | #1 
On Thu, 24 Jun 2021 19:29:37 +0100
Anand Ashok Dumbre <anand.ashok.dumbre@xilinx.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@xilinx.com>
> Signed-off-by: Anand Ashok Dumbre <anand.ashok.dumbre@xilinx.com>

Hi Anand,

A few comments inline from a fresh read.
Only significant one is that the use of separate masks and shifts
differs from how they are normally done in the kernel these days.
FIELD_PREP() and FIELD_GET() allow a single mask definition to be
cleanly used for both the shift and masking in a nice clean way.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/Kconfig      |   13 +
>  drivers/iio/adc/Makefile     |    1 +
>  drivers/iio/adc/xilinx-ams.c | 1342 ++++++++++++++++++++++++++++++++++
>  3 files changed, 1356 insertions(+)
>  create mode 100644 drivers/iio/adc/xilinx-ams.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index c7946c439612..c011ab30ec9a 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -1256,4 +1256,17 @@ 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.
> +
> +	  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 a226657d19c0..99e031f44479 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -112,4 +112,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..463e3162726c
> --- /dev/null
> +++ b/drivers/iio/adc/xilinx-ams.c
> @@ -0,0 +1,1342 @@
> +// 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/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/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>
> +
> +static const unsigned int AMS_UNMASK_TIMEOUT_MS = 500;
> +
> +/* AMS registers definitions */
> +#define AMS_ISR_0			0x010
> +#define AMS_ISR_1			0x014
> +#define AMS_IER_0			0x020
> +#define AMS_IER_1			0x024
> +#define AMS_IDR_0			0x028
> +#define AMS_IDR_1			0x02c
> +#define AMS_PS_CSTS			0x040
> +#define AMS_PL_CSTS			0x044
> +
> +#define AMS_VCC_PSPLL0			0x060
> +#define AMS_VCC_PSPLL3			0x06C
> +#define AMS_VCCINT			0x078
> +#define AMS_VCCBRAM			0x07C
> +#define AMS_VCCAUX			0x080
> +#define AMS_PSDDRPLL			0x084
> +#define AMS_PSINTFPDDR			0x09C
> +
> +#define AMS_VCC_PSPLL0_CH		48
> +#define AMS_VCC_PSPLL3_CH		51
> +#define AMS_VCCINT_CH			54
> +#define AMS_VCCBRAM_CH			55
> +#define AMS_VCCAUX_CH			56
> +#define AMS_PSDDRPLL_CH			57
> +#define AMS_PSINTFPDDR_CH		63
> +
> +#define AMS_REG_CONFIG0			0x100
> +#define AMS_REG_CONFIG1			0x104
> +#define AMS_REG_CONFIG3			0x10C
> +#define AMS_REG_SEQ_CH0			0x120
> +#define AMS_REG_SEQ_CH1			0x124
> +#define AMS_REG_SEQ_CH2			0x118
> +
> +#define AMS_TEMP			0x000
> +#define AMS_SUPPLY1			0x004
> +#define AMS_SUPPLY2			0x008
> +#define AMS_VP_VN			0x00c
> +#define AMS_VREFP			0x010
> +#define AMS_VREFN			0x014
> +#define AMS_SUPPLY3			0x018
> +#define AMS_SUPPLY4			0x034
> +#define AMS_SUPPLY5			0x038
> +#define AMS_SUPPLY6			0x03c
> +#define AMS_SUPPLY7			0x200
> +#define AMS_SUPPLY8			0x204
> +#define AMS_SUPPLY9			0x208
> +#define AMS_SUPPLY10			0x20c
> +#define AMS_VCCAMS			0x210
> +#define AMS_TEMP_REMOTE			0x214
> +
> +#define AMS_REG_VAUX(x)			(0x40 + 4 * (x))
> +
> +#define AMS_PS_RESET_VALUE		0xFFFF
> +#define AMS_PL_RESET_VALUE		0xFFFF
> +
> +#define AMS_CONF0_CHANNEL_NUM_MASK	GENMASK(6, 0)
> +
> +#define AMS_CONF1_SEQ_MASK		GENMASK(15, 12)
> +#define AMS_CONF1_SEQ_DEFAULT		(0 << 12)
> +#define AMS_CONF1_SEQ_CONTINUOUS	(2 << 12)
> +#define AMS_CONF1_SEQ_SINGLE_CHANNEL	(3 << 12)

FIELD_PREP(AMS_CONFG1_SEQ_MASK, 0) or even better, define
the values as not shifted and have

FIELD_PREP(AMS_CONFIG1_SEQ_MASK, AMS_CONF1_SEQ_DEFAULT)
etc in the relevant places inline.


> +
> +#define AMS_REG_SEQ0_MASK		0xFFFF
> +#define AMS_REG_SEQ2_MASK		0x003F
> +#define AMS_REG_SEQ1_MASK		0xFFFF
> +#define AMS_REG_SEQ2_MASK_SHIFT		16
> +#define AMS_REG_SEQ1_MASK_SHIFT		22

As below, combine mask and shift into a shifted mask
then you can use FIELD_PREP() to do the magic for you.

> +
> +#define AMS_REGCFG1_ALARM_MASK		0xF0F
> +#define AMS_REGCFG3_ALARM_MASK		0x3F
> +
> +#define AMS_ALARM_TEMP			0x140
> +#define AMS_ALARM_SUPPLY1		0x144
> +#define AMS_ALARM_SUPPLY2		0x148
> +#define AMS_ALARM_SUPPLY3		0x160
> +#define AMS_ALARM_SUPPLY4		0x164
> +#define AMS_ALARM_SUPPLY5		0x168
> +#define AMS_ALARM_SUPPLY6		0x16c
> +#define AMS_ALARM_SUPPLY7		0x180
> +#define AMS_ALARM_SUPPLY8		0x184
> +#define AMS_ALARM_SUPPLY9		0x188
> +#define AMS_ALARM_SUPPLY10		0x18c
> +#define AMS_ALARM_VCCAMS		0x190
> +#define AMS_ALARM_TEMP_REMOTE		0x194
> +#define AMS_ALARM_THRESHOLD_OFF_10	0x10
> +#define AMS_ALARM_THRESHOLD_OFF_20	0x20
> +
> +#define AMS_ALARM_THR_DIRECT_MASK	0x01
> +#define AMS_ALARM_THR_MIN		0x0000
> +#define AMS_ALARM_THR_MAX		0xffff
> +
> +#define AMS_NO_OF_ALARMS		32
> +#define AMS_PL_ALARM_START		16
> +#define AMS_ISR0_ALARM_MASK		0xFFFFFFFFU
> +#define AMS_ISR1_ALARM_MASK		0xE000001FU
> +#define AMS_ISR1_EOC_MASK		0x00000008U
> +#define AMS_ISR1_INTR_MASK_SHIFT	32
> +#define AMS_ISR0_ALARM_2_TO_0_MASK	0x07
> +#define AMS_ISR0_ALARM_6_TO_3_MASK	0x78
> +#define AMS_ISR0_ALARM_12_TO_7_MASK	0x3F
> +#define AMS_CONF1_ALARM_2_TO_0_SHIFT	1

Can we handle these via a single mask that includes the shift
and FIELD_PREP() etc?  That tends to make it easier to
review by ensuring we don't have multiple defines to deal with.

> +#define AMS_CONF1_ALARM_6_TO_3_SHIFT	5
> +#define AMS_CONF3_ALARM_12_TO_7_SHIFT	8


> +
> +#define AMS_PS_CSTS_PS_READY		0x08010000U
> +#define AMS_PL_CSTS_ACCESS_MASK		0x00000001U
> +
> +#define AMS_PL_MAX_FIXED_CHANNEL	10
> +#define AMS_PL_MAX_EXT_CHANNEL		20
> +
> +#define AMS_INIT_TIMEOUT_US		10000
> +
> +/*
> + * Following scale and offset value is derived from
> + * UG580 (v1.7) December 20, 2016
> + */
> +#define AMS_SUPPLY_SCALE_1VOLT		1000
> +#define AMS_SUPPLY_SCALE_3VOLT		3000
> +#define AMS_SUPPLY_SCALE_6VOLT		6000
> +#define AMS_SUPPLY_SCALE_DIV_BIT	16
> +
> +#define AMS_TEMP_SCALE			509314
> +#define AMS_TEMP_SCALE_DIV_BIT		16
> +#define AMS_TEMP_OFFSET			-((280230L << 16) / 509314)
> +
> +enum ams_alarm_bit {
> +	AMS_ALARM_BIT_TEMP,
> +	AMS_ALARM_BIT_SUPPLY1,
> +	AMS_ALARM_BIT_SUPPLY2,
> +	AMS_ALARM_BIT_SUPPLY3,
> +	AMS_ALARM_BIT_SUPPLY4,
> +	AMS_ALARM_BIT_SUPPLY5,
> +	AMS_ALARM_BIT_SUPPLY6,
> +	AMS_ALARM_BIT_RESERVED,
> +	AMS_ALARM_BIT_SUPPLY7,
> +	AMS_ALARM_BIT_SUPPLY8,
> +	AMS_ALARM_BIT_SUPPLY9,
> +	AMS_ALARM_BIT_SUPPLY10,
> +	AMS_ALARM_BIT_VCCAMS,
> +	AMS_ALARM_BIT_TEMP_REMOTE
> +};
> +
> +enum ams_seq {
> +	AMS_SEQ_VCC_PSPLL,
> +	AMS_SEQ_VCC_PSBATT,
> +	AMS_SEQ_VCCINT,
> +	AMS_SEQ_VCCBRAM,
> +	AMS_SEQ_VCCAUX,
> +	AMS_SEQ_PSDDRPLL,
> +	AMS_SEQ_INTDDR
> +};
> +
> +enum ams_ps_pl_seq {
> +	AMS_SEQ_CALIB,
> +	AMS_SEQ_RSVD_1,
> +	AMS_SEQ_RSVD_2,
> +	AMS_SEQ_TEST,
> +	AMS_SEQ_RSVD_4,
> +	AMS_SEQ_SUPPLY4,
> +	AMS_SEQ_SUPPLY5,
> +	AMS_SEQ_SUPPLY6,
> +	AMS_SEQ_TEMP,
> +	AMS_SEQ_SUPPLY2,
> +	AMS_SEQ_SUPPLY1,
> +	AMS_SEQ_VP_VN,
> +	AMS_SEQ_VREFP,
> +	AMS_SEQ_VREFN,
> +	AMS_SEQ_SUPPLY3,
> +	AMS_SEQ_CURRENT_MON,
> +	AMS_SEQ_SUPPLY7,
> +	AMS_SEQ_SUPPLY8,
> +	AMS_SEQ_SUPPLY9,
> +	AMS_SEQ_SUPPLY10,
> +	AMS_SEQ_VCCAMS,
> +	AMS_SEQ_TEMP_REMOTE,
> +	AMS_SEQ_MAX
> +};
> +
> +#define AMS_SEQ(x)		(AMS_SEQ_MAX + (x))
> +#define AMS_VAUX_SEQ(x)		(AMS_SEQ_MAX + (x))
> +
> +#define AMS_PS_SEQ_MAX		AMS_SEQ_MAX
> +#define PS_SEQ(x)		(x)
> +#define PL_SEQ(x)		(AMS_PS_SEQ_MAX + (x))
> +
> +#define AMS_CHAN_TEMP(_scan_index, _addr) { \
> +	.type = IIO_TEMP, \
> +	.indexed = 1, \
> +	.address = (_addr), \
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> +		BIT(IIO_CHAN_INFO_SCALE) | \
> +		BIT(IIO_CHAN_INFO_OFFSET), \
> +	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> +	.event_spec = ams_temp_events, \
> +	.num_event_specs = ARRAY_SIZE(ams_temp_events), \
> +	.scan_index = (_scan_index), \
> +	.scan_type = { \
> +		.sign = 'u', \
> +		.realbits = 12, \
> +		.storagebits = 16, \
> +		.shift = 4, \
> +		.endianness = IIO_CPU, \

Currently these are only documentation i think as no support for using them
in this driver (buffered modes).  You could drop them until you need them
in some future patch.

> +	}, \
> +}
> +

...

> +static int ams_enable_single_channel(struct ams *ams, unsigned int offset)
> +{
> +	u8 channel_num = 0;
> +
> +	switch (offset) {
> +	case AMS_VCC_PSPLL0:
> +		channel_num = AMS_VCC_PSPLL0_CH;
> +		break;
> +	case AMS_VCC_PSPLL3:
> +		channel_num = AMS_VCC_PSPLL3_CH;
> +		break;
> +	case AMS_VCCINT:
> +		channel_num = AMS_VCCINT_CH;
> +		break;
> +	case AMS_VCCBRAM:
> +		channel_num = AMS_VCCBRAM_CH;
> +		break;
> +	case AMS_VCCAUX:
> +		channel_num = AMS_VCCAUX_CH;
> +		break;
> +	case AMS_PSDDRPLL:
> +		channel_num = AMS_PSDDRPLL_CH;
> +		break;
> +	case AMS_PSINTFPDDR:
> +		channel_num = AMS_PSINTFPDDR_CH;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	/* set single channel, sequencer off mode */
> +	ams_ps_update_reg(ams, AMS_REG_CONFIG1, AMS_CONF1_SEQ_MASK,
> +			  AMS_CONF1_SEQ_SINGLE_CHANNEL);
> +
> +	/* write the channel number */
> +	ams_ps_update_reg(ams, AMS_REG_CONFIG0, AMS_CONF0_CHANNEL_NUM_MASK,
> +			  channel_num);

nitpick: Blank line here.

> +	return 0;
> +}
> +

...

> +static int ams_get_ext_chan(struct device_node *chan_node,
> +			    struct iio_chan_spec *channels, int num_channels)
> +{
> +	struct device_node *child;
> +	unsigned int reg;
> +	int ret;
> +
> +	for_each_child_of_node(chan_node, child) {
> +		ret = of_property_read_u32(child, "reg", &reg);
> +		if (ret || reg > (AMS_PL_MAX_EXT_CHANNEL + 30))
> +			continue;
> +
> +		memcpy(&channels[num_channels], &ams_pl_channels[reg +
> +		       AMS_PL_MAX_FIXED_CHANNEL - 30], sizeof(*channels));
> +
> +		if (of_property_read_bool(child,
> +					  "xlnx,bipolar"))

Above fits on one line easily.

> +			channels[num_channels].scan_type.sign =	's';
> +
> +		num_channels++;
> +	}
> +
> +	return num_channels;
> +}
> +

...

> +
> +static int ams_probe(struct platform_device *pdev)
> +{
> +	struct iio_dev *indio_dev;
> +	struct ams *ams;
> +	struct resource *res;
> +	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);
> +
> +	indio_dev->name = "xilinx-ams";
> +
> +	indio_dev->info = &iio_ams_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	ams->base = devm_ioremap_resource(&pdev->dev, res);

devm_platform_ioremap_resource()
Slight reduction in boilerplate.

> +	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, (void *)clk_disable_unprepare,
> +				 ams->clk);
> +
> +	INIT_DELAYED_WORK(&ams->ams_unmask_work, ams_unmask_worker);
> +	devm_add_action_or_reset(&pdev->dev, (void *)cancel_delayed_work,

I'm not keen on casting away the function pointer type.  Normally we'd
just wrap it in a local function, to make it clear it was deliberate and avoid
potential nasty problems if the signature of the function ever changes.

It's 3 lines of boilerplate, but will give me warm fuzzy feelings!
Same for the other case above.  The fact this isn't done in exising kernel
code make this particularly risky.

> +				 &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);

platform_get_irq () can return errors, in particular -EPROBE_DEFER so I'd check
that and return before you call devm_request_irq()
I'm not sure devm_request_irq() will not eat that error code.


> +	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);
> +
> +	return iio_device_register(indio_dev);
> +}
> +
> +static int ams_remove(struct platform_device *pdev)
> +{
> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +
> +	iio_device_unregister(indio_dev);

If this is all you have in remove, then you can use devm_iio_device_register() 
in probe() and not need an remove() callback at all.

> +
> +	return 0;
> +}
> +

...
Anand Ashok Dumbre July 15, 2021, 7:48 a.m. UTC | #2 
> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Sunday 4 July 2021 7:31 PM
> To: Anand Ashok Dumbre <ANANDASH@xilinx.com>
> Cc: lars@metafoo.de; linux-iio@vger.kernel.org; git-dev <git-
> dev@xilinx.com>; Michal Simek <michals@xilinx.com>;
> pmeerw@pmeerw.net; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; Manish Narani <MNARANI@xilinx.com>
> Subject: Re: [PATCH v6 2/4] iio: adc: Add Xilinx AMS driver
> 
> On Thu, 24 Jun 2021 19:29:37 +0100
> Anand Ashok Dumbre <anand.ashok.dumbre@xilinx.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@xilinx.com>
> > Signed-off-by: Anand Ashok Dumbre <anand.ashok.dumbre@xilinx.com>

Hi Jonathan,

Thank you for the review.

My comments inline.

Thanks,
Anand
> 
> Hi Anand,
> 
> A few comments inline from a fresh read.
> Only significant one is that the use of separate masks and shifts differs from
> how they are normally done in the kernel these days.
> FIELD_PREP() and FIELD_GET() allow a single mask definition to be cleanly
> used for both the shift and masking in a nice clean way.
> 
> Thanks,
> 
> Jonathan
> 
> > ---
> >  drivers/iio/adc/Kconfig      |   13 +
> >  drivers/iio/adc/Makefile     |    1 +
> >  drivers/iio/adc/xilinx-ams.c | 1342
> > ++++++++++++++++++++++++++++++++++
> >  3 files changed, 1356 insertions(+)
> >  create mode 100644 drivers/iio/adc/xilinx-ams.c
> >
> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig index
> > c7946c439612..c011ab30ec9a 100644
> > --- a/drivers/iio/adc/Kconfig
> > +++ b/drivers/iio/adc/Kconfig
> > @@ -1256,4 +1256,17 @@ 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.
> > +
> > +	  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
> > a226657d19c0..99e031f44479 100644
> > --- a/drivers/iio/adc/Makefile
> > +++ b/drivers/iio/adc/Makefile
> > @@ -112,4 +112,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..463e3162726c
> > --- /dev/null
> > +++ b/drivers/iio/adc/xilinx-ams.c
> > @@ -0,0 +1,1342 @@
> > +// 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/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/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>
> > +
> > +static const unsigned int AMS_UNMASK_TIMEOUT_MS = 500;
> > +
> > +/* AMS registers definitions */
> > +#define AMS_ISR_0			0x010
> > +#define AMS_ISR_1			0x014
> > +#define AMS_IER_0			0x020
> > +#define AMS_IER_1			0x024
> > +#define AMS_IDR_0			0x028
> > +#define AMS_IDR_1			0x02c
> > +#define AMS_PS_CSTS			0x040
> > +#define AMS_PL_CSTS			0x044
> > +
> > +#define AMS_VCC_PSPLL0			0x060
> > +#define AMS_VCC_PSPLL3			0x06C
> > +#define AMS_VCCINT			0x078
> > +#define AMS_VCCBRAM			0x07C
> > +#define AMS_VCCAUX			0x080
> > +#define AMS_PSDDRPLL			0x084
> > +#define AMS_PSINTFPDDR			0x09C
> > +
> > +#define AMS_VCC_PSPLL0_CH		48
> > +#define AMS_VCC_PSPLL3_CH		51
> > +#define AMS_VCCINT_CH			54
> > +#define AMS_VCCBRAM_CH			55
> > +#define AMS_VCCAUX_CH			56
> > +#define AMS_PSDDRPLL_CH			57
> > +#define AMS_PSINTFPDDR_CH		63
> > +
> > +#define AMS_REG_CONFIG0			0x100
> > +#define AMS_REG_CONFIG1			0x104
> > +#define AMS_REG_CONFIG3			0x10C
> > +#define AMS_REG_SEQ_CH0			0x120
> > +#define AMS_REG_SEQ_CH1			0x124
> > +#define AMS_REG_SEQ_CH2			0x118
> > +
> > +#define AMS_TEMP			0x000
> > +#define AMS_SUPPLY1			0x004
> > +#define AMS_SUPPLY2			0x008
> > +#define AMS_VP_VN			0x00c
> > +#define AMS_VREFP			0x010
> > +#define AMS_VREFN			0x014
> > +#define AMS_SUPPLY3			0x018
> > +#define AMS_SUPPLY4			0x034
> > +#define AMS_SUPPLY5			0x038
> > +#define AMS_SUPPLY6			0x03c
> > +#define AMS_SUPPLY7			0x200
> > +#define AMS_SUPPLY8			0x204
> > +#define AMS_SUPPLY9			0x208
> > +#define AMS_SUPPLY10			0x20c
> > +#define AMS_VCCAMS			0x210
> > +#define AMS_TEMP_REMOTE			0x214
> > +
> > +#define AMS_REG_VAUX(x)			(0x40 + 4 * (x))
> > +
> > +#define AMS_PS_RESET_VALUE		0xFFFF
> > +#define AMS_PL_RESET_VALUE		0xFFFF
> > +
> > +#define AMS_CONF0_CHANNEL_NUM_MASK	GENMASK(6, 0)
> > +
> > +#define AMS_CONF1_SEQ_MASK		GENMASK(15, 12)
> > +#define AMS_CONF1_SEQ_DEFAULT		(0 << 12)
> > +#define AMS_CONF1_SEQ_CONTINUOUS	(2 << 12)
> > +#define AMS_CONF1_SEQ_SINGLE_CHANNEL	(3 << 12)
> 
> FIELD_PREP(AMS_CONFG1_SEQ_MASK, 0) or even better, define the values
> as not shifted and have
> 
> FIELD_PREP(AMS_CONFIG1_SEQ_MASK, AMS_CONF1_SEQ_DEFAULT) etc in
> the relevant places inline.
> 
Will fix this in next series.
> 
> > +
> > +#define AMS_REG_SEQ0_MASK		0xFFFF
> > +#define AMS_REG_SEQ2_MASK		0x003F
> > +#define AMS_REG_SEQ1_MASK		0xFFFF
> > +#define AMS_REG_SEQ2_MASK_SHIFT		16
> > +#define AMS_REG_SEQ1_MASK_SHIFT		22
> 
> As below, combine mask and shift into a shifted mask then you can use
> FIELD_PREP() to do the magic for you.

Will fix this in next series.
> 
> > +
> > +#define AMS_REGCFG1_ALARM_MASK		0xF0F
> > +#define AMS_REGCFG3_ALARM_MASK		0x3F
> > +
> > +#define AMS_ALARM_TEMP			0x140
> > +#define AMS_ALARM_SUPPLY1		0x144
> > +#define AMS_ALARM_SUPPLY2		0x148
> > +#define AMS_ALARM_SUPPLY3		0x160
> > +#define AMS_ALARM_SUPPLY4		0x164
> > +#define AMS_ALARM_SUPPLY5		0x168
> > +#define AMS_ALARM_SUPPLY6		0x16c
> > +#define AMS_ALARM_SUPPLY7		0x180
> > +#define AMS_ALARM_SUPPLY8		0x184
> > +#define AMS_ALARM_SUPPLY9		0x188
> > +#define AMS_ALARM_SUPPLY10		0x18c
> > +#define AMS_ALARM_VCCAMS		0x190
> > +#define AMS_ALARM_TEMP_REMOTE		0x194
> > +#define AMS_ALARM_THRESHOLD_OFF_10	0x10
> > +#define AMS_ALARM_THRESHOLD_OFF_20	0x20
> > +
> > +#define AMS_ALARM_THR_DIRECT_MASK	0x01
> > +#define AMS_ALARM_THR_MIN		0x0000
> > +#define AMS_ALARM_THR_MAX		0xffff
> > +
> > +#define AMS_NO_OF_ALARMS		32
> > +#define AMS_PL_ALARM_START		16
> > +#define AMS_ISR0_ALARM_MASK		0xFFFFFFFFU
> > +#define AMS_ISR1_ALARM_MASK		0xE000001FU
> > +#define AMS_ISR1_EOC_MASK		0x00000008U
> > +#define AMS_ISR1_INTR_MASK_SHIFT	32
> > +#define AMS_ISR0_ALARM_2_TO_0_MASK	0x07
> > +#define AMS_ISR0_ALARM_6_TO_3_MASK	0x78
> > +#define AMS_ISR0_ALARM_12_TO_7_MASK	0x3F
> > +#define AMS_CONF1_ALARM_2_TO_0_SHIFT	1
> 
> Can we handle these via a single mask that includes the shift and
> FIELD_PREP() etc?  That tends to make it easier to review by ensuring we
> don't have multiple defines to deal with.
> 

Will fix this in next series.

> > +#define AMS_CONF1_ALARM_6_TO_3_SHIFT	5
> > +#define AMS_CONF3_ALARM_12_TO_7_SHIFT	8
> 
> 
> > +
> > +#define AMS_PS_CSTS_PS_READY		0x08010000U
> > +#define AMS_PL_CSTS_ACCESS_MASK		0x00000001U
> > +
> > +#define AMS_PL_MAX_FIXED_CHANNEL	10
> > +#define AMS_PL_MAX_EXT_CHANNEL		20
> > +
> > +#define AMS_INIT_TIMEOUT_US		10000
> > +
> > +/*
> > + * Following scale and offset value is derived from
> > + * UG580 (v1.7) December 20, 2016
> > + */
> > +#define AMS_SUPPLY_SCALE_1VOLT		1000
> > +#define AMS_SUPPLY_SCALE_3VOLT		3000
> > +#define AMS_SUPPLY_SCALE_6VOLT		6000
> > +#define AMS_SUPPLY_SCALE_DIV_BIT	16
> > +
> > +#define AMS_TEMP_SCALE			509314
> > +#define AMS_TEMP_SCALE_DIV_BIT		16
> > +#define AMS_TEMP_OFFSET			-((280230L << 16) /
> 509314)
> > +
> > +enum ams_alarm_bit {
> > +	AMS_ALARM_BIT_TEMP,
> > +	AMS_ALARM_BIT_SUPPLY1,
> > +	AMS_ALARM_BIT_SUPPLY2,
> > +	AMS_ALARM_BIT_SUPPLY3,
> > +	AMS_ALARM_BIT_SUPPLY4,
> > +	AMS_ALARM_BIT_SUPPLY5,
> > +	AMS_ALARM_BIT_SUPPLY6,
> > +	AMS_ALARM_BIT_RESERVED,
> > +	AMS_ALARM_BIT_SUPPLY7,
> > +	AMS_ALARM_BIT_SUPPLY8,
> > +	AMS_ALARM_BIT_SUPPLY9,
> > +	AMS_ALARM_BIT_SUPPLY10,
> > +	AMS_ALARM_BIT_VCCAMS,
> > +	AMS_ALARM_BIT_TEMP_REMOTE
> > +};
> > +
> > +enum ams_seq {
> > +	AMS_SEQ_VCC_PSPLL,
> > +	AMS_SEQ_VCC_PSBATT,
> > +	AMS_SEQ_VCCINT,
> > +	AMS_SEQ_VCCBRAM,
> > +	AMS_SEQ_VCCAUX,
> > +	AMS_SEQ_PSDDRPLL,
> > +	AMS_SEQ_INTDDR
> > +};
> > +
> > +enum ams_ps_pl_seq {
> > +	AMS_SEQ_CALIB,
> > +	AMS_SEQ_RSVD_1,
> > +	AMS_SEQ_RSVD_2,
> > +	AMS_SEQ_TEST,
> > +	AMS_SEQ_RSVD_4,
> > +	AMS_SEQ_SUPPLY4,
> > +	AMS_SEQ_SUPPLY5,
> > +	AMS_SEQ_SUPPLY6,
> > +	AMS_SEQ_TEMP,
> > +	AMS_SEQ_SUPPLY2,
> > +	AMS_SEQ_SUPPLY1,
> > +	AMS_SEQ_VP_VN,
> > +	AMS_SEQ_VREFP,
> > +	AMS_SEQ_VREFN,
> > +	AMS_SEQ_SUPPLY3,
> > +	AMS_SEQ_CURRENT_MON,
> > +	AMS_SEQ_SUPPLY7,
> > +	AMS_SEQ_SUPPLY8,
> > +	AMS_SEQ_SUPPLY9,
> > +	AMS_SEQ_SUPPLY10,
> > +	AMS_SEQ_VCCAMS,
> > +	AMS_SEQ_TEMP_REMOTE,
> > +	AMS_SEQ_MAX
> > +};
> > +
> > +#define AMS_SEQ(x)		(AMS_SEQ_MAX + (x))
> > +#define AMS_VAUX_SEQ(x)		(AMS_SEQ_MAX + (x))
> > +
> > +#define AMS_PS_SEQ_MAX		AMS_SEQ_MAX
> > +#define PS_SEQ(x)		(x)
> > +#define PL_SEQ(x)		(AMS_PS_SEQ_MAX + (x))
> > +
> > +#define AMS_CHAN_TEMP(_scan_index, _addr) { \
> > +	.type = IIO_TEMP, \
> > +	.indexed = 1, \
> > +	.address = (_addr), \
> > +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> > +		BIT(IIO_CHAN_INFO_SCALE) | \
> > +		BIT(IIO_CHAN_INFO_OFFSET), \
> > +	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> > +	.event_spec = ams_temp_events, \
> > +	.num_event_specs = ARRAY_SIZE(ams_temp_events), \
> > +	.scan_index = (_scan_index), \
> > +	.scan_type = { \
> > +		.sign = 'u', \
> > +		.realbits = 12, \
> > +		.storagebits = 16, \
> > +		.shift = 4, \
> > +		.endianness = IIO_CPU, \
> 
> Currently these are only documentation i think as no support for using them
> in this driver (buffered modes).  You could drop them until you need them in
> some future patch.

Makes sense, will remove the scan_type part, but will retain the scan index, since it 
is used in some logic inside the rest of the code.

> 
> > +	}, \
> > +}
> > +
> 
> ...
> 
> > +static int ams_enable_single_channel(struct ams *ams, unsigned int
> > +offset) {
> > +	u8 channel_num = 0;
> > +
> > +	switch (offset) {
> > +	case AMS_VCC_PSPLL0:
> > +		channel_num = AMS_VCC_PSPLL0_CH;
> > +		break;
> > +	case AMS_VCC_PSPLL3:
> > +		channel_num = AMS_VCC_PSPLL3_CH;
> > +		break;
> > +	case AMS_VCCINT:
> > +		channel_num = AMS_VCCINT_CH;
> > +		break;
> > +	case AMS_VCCBRAM:
> > +		channel_num = AMS_VCCBRAM_CH;
> > +		break;
> > +	case AMS_VCCAUX:
> > +		channel_num = AMS_VCCAUX_CH;
> > +		break;
> > +	case AMS_PSDDRPLL:
> > +		channel_num = AMS_PSDDRPLL_CH;
> > +		break;
> > +	case AMS_PSINTFPDDR:
> > +		channel_num = AMS_PSINTFPDDR_CH;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* set single channel, sequencer off mode */
> > +	ams_ps_update_reg(ams, AMS_REG_CONFIG1,
> AMS_CONF1_SEQ_MASK,
> > +			  AMS_CONF1_SEQ_SINGLE_CHANNEL);
> > +
> > +	/* write the channel number */
> > +	ams_ps_update_reg(ams, AMS_REG_CONFIG0,
> AMS_CONF0_CHANNEL_NUM_MASK,
> > +			  channel_num);
> 
> nitpick: Blank line here.

Will fix this in next series.

> 
> > +	return 0;
> > +}
> > +
> 
> ...
> 
> > +static int ams_get_ext_chan(struct device_node *chan_node,
> > +			    struct iio_chan_spec *channels, int num_channels)
> {
> > +	struct device_node *child;
> > +	unsigned int reg;
> > +	int ret;
> > +
> > +	for_each_child_of_node(chan_node, child) {
> > +		ret = of_property_read_u32(child, "reg", &reg);
> > +		if (ret || reg > (AMS_PL_MAX_EXT_CHANNEL + 30))
> > +			continue;
> > +
> > +		memcpy(&channels[num_channels], &ams_pl_channels[reg
> +
> > +		       AMS_PL_MAX_FIXED_CHANNEL - 30], sizeof(*channels));
> > +
> > +		if (of_property_read_bool(child,
> > +					  "xlnx,bipolar"))
> 
> Above fits on one line easily.

Will fix this in next series.

> 
> > +			channels[num_channels].scan_type.sign =	's';
> > +
> > +		num_channels++;
> > +	}
> > +
> > +	return num_channels;
> > +}
> > +
> 
> ...
> 
> > +
> > +static int ams_probe(struct platform_device *pdev) {
> > +	struct iio_dev *indio_dev;
> > +	struct ams *ams;
> > +	struct resource *res;
> > +	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);
> > +
> > +	indio_dev->name = "xilinx-ams";
> > +
> > +	indio_dev->info = &iio_ams_info;
> > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	ams->base = devm_ioremap_resource(&pdev->dev, res);
> 
> devm_platform_ioremap_resource()
> Slight reduction in boilerplate.

Will fix this in next series.

> 
> > +	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, (void
> *)clk_disable_unprepare,
> > +				 ams->clk);
> > +
> > +	INIT_DELAYED_WORK(&ams->ams_unmask_work,
> ams_unmask_worker);
> > +	devm_add_action_or_reset(&pdev->dev, (void
> *)cancel_delayed_work,
> 
> I'm not keen on casting away the function pointer type.  Normally we'd just
> wrap it in a local function, to make it clear it was deliberate and avoid
> potential nasty problems if the signature of the function ever changes.
> 
> It's 3 lines of boilerplate, but will give me warm fuzzy feelings!
> Same for the other case above.  The fact this isn't done in exising kernel code
> make this particularly risky.

Makes sense. I will revert the code back to its original and handle the cases using goto
and inside remove()

> 
> > +				 &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);
> 
> platform_get_irq () can return errors, in particular -EPROBE_DEFER so I'd
> check that and return before you call devm_request_irq() I'm not sure
> devm_request_irq() will not eat that error code.
> 

Will fix this in next series.

> 
> > +	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);
> > +
> > +	return iio_device_register(indio_dev); }
> > +
> > +static int ams_remove(struct platform_device *pdev) {
> > +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> > +
> > +	iio_device_unregister(indio_dev);
> 
> If this is all you have in remove, then you can use devm_iio_device_register()
> in probe() and not need an remove() callback at all.

I think remove will have more functions since I am getting rid of devm_add_action_or_reset()

> 
> > +
> > +	return 0;
> > +}
> > +
> 
> ...
Jonathan Cameron July 15, 2021, 12:52 p.m. UTC | #3 
...

> >   
> > > +	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, (void  
> > *)clk_disable_unprepare,  
> > > +				 ams->clk);
> > > +
> > > +	INIT_DELAYED_WORK(&ams->ams_unmask_work,  
> > ams_unmask_worker);  
> > > +	devm_add_action_or_reset(&pdev->dev, (void  
> > *)cancel_delayed_work,
> > 
> > I'm not keen on casting away the function pointer type.  Normally we'd just
> > wrap it in a local function, to make it clear it was deliberate and avoid
> > potential nasty problems if the signature of the function ever changes.
> > 
> > It's 3 lines of boilerplate, but will give me warm fuzzy feelings!
> > Same for the other case above.  The fact this isn't done in exising kernel code
> > make this particularly risky.  
> 
> Makes sense. I will revert the code back to its original and handle the cases using goto
> and inside remove()
Ah.  Not what I meant.  I was suggesting you add a little function locally
that has the right type and in turn calls cancel_delayed_work().

As that directly exposes the actual function calls, any signature change in future
will cause compile breakage (or be picked up any automated tools doing that refactor).

> 
> >   
> > > +				 &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);  
> > 
> > platform_get_irq () can return errors, in particular -EPROBE_DEFER so I'd
> > check that and return before you call devm_request_irq() I'm not sure
> > devm_request_irq() will not eat that error code.
> >   
> 
> Will fix this in next series.
> 
> >   
> > > +	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);
> > > +
> > > +	return iio_device_register(indio_dev); }
> > > +
> > > +static int ams_remove(struct platform_device *pdev) {
> > > +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> > > +
> > > +	iio_device_unregister(indio_dev);  
> > 
> > If this is all you have in remove, then you can use devm_iio_device_register()
> > in probe() and not need an remove() callback at all.  
> 
> I think remove will have more functions since I am getting rid of devm_add_action_or_reset()

See above.

J

> 
> >   
> > > +
> > > +	return 0;
> > > +}
> > > +  
> > 
> > ...
Anand Ashok Dumbre July 19, 2021, 7:49 a.m. UTC | #4 
> -----Original Message-----
> From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> Sent: Thursday 15 July 2021 1:52 PM
> To: Anand Ashok Dumbre <ANANDASH@xilinx.com>
> Cc: Jonathan Cameron <jic23@kernel.org>; lars@metafoo.de; linux-
> iio@vger.kernel.org; git-dev <git-dev@xilinx.com>; Michal Simek
> <michals@xilinx.com>; pmeerw@pmeerw.net; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org; Manish Narani <MNARANI@xilinx.com>
> Subject: Re: [PATCH v6 2/4] iio: adc: Add Xilinx AMS driver
> 
> ...
> 
> > >
> > > > +	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, (void
> > > *)clk_disable_unprepare,
> > > > +				 ams->clk);
> > > > +
> > > > +	INIT_DELAYED_WORK(&ams->ams_unmask_work,
> > > ams_unmask_worker);
> > > > +	devm_add_action_or_reset(&pdev->dev, (void
> > > *)cancel_delayed_work,
> > >
> > > I'm not keen on casting away the function pointer type.  Normally
> > > we'd just wrap it in a local function, to make it clear it was
> > > deliberate and avoid potential nasty problems if the signature of the
> function ever changes.
> > >
> > > It's 3 lines of boilerplate, but will give me warm fuzzy feelings!
> > > Same for the other case above.  The fact this isn't done in exising
> > > kernel code make this particularly risky.
> >
> > Makes sense. I will revert the code back to its original and handle
> > the cases using goto and inside remove()
> Ah.  Not what I meant.  I was suggesting you add a little function locally that
> has the right type and in turn calls cancel_delayed_work().
> 
> As that directly exposes the actual function calls, any signature change in
> future will cause compile breakage (or be picked up any automated tools
> doing that refactor).

Now I understand.
Will fix it in the next series.

> 
> >
> > >
> > > > +				 &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);
> > >
> > > platform_get_irq () can return errors, in particular -EPROBE_DEFER
> > > so I'd check that and return before you call devm_request_irq() I'm
> > > not sure
> > > devm_request_irq() will not eat that error code.
> > >
> >
> > Will fix this in next series.
> >
> > >
> > > > +	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);
> > > > +
> > > > +	return iio_device_register(indio_dev); }
> > > > +
> > > > +static int ams_remove(struct platform_device *pdev) {
> > > > +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> > > > +
> > > > +	iio_device_unregister(indio_dev);
> > >
> > > If this is all you have in remove, then you can use
> > > devm_iio_device_register() in probe() and not need an remove() callback
> at all.
> >
> > I think remove will have more functions since I am getting rid of
> > devm_add_action_or_reset()
> 
> See above.
> 
> J
> 
> >
> > >
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > >
> > > ...

Thanks,
Anand