| Message ID | 20211117161028.11775-4-anand.ashok.dumbre@xilinx.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | Add Xilinx AMS Driver | expand |
Hi Anand,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on linux/master linus/master v5.16-rc1 next-20211117]
[cannot apply to xilinx-xlnx/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Anand-Ashok-Dumbre/Add-Xilinx-AMS-Driver/20211118-001150
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: sparc64-randconfig-s032-20211118 (attached as .config)
compiler: sparc64-linux-gcc (GCC) 11.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.4-dirty
# https://github.com/0day-ci/linux/commit/196a4a5b4232a4a12d0843a0002b664720b5d200
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Anand-Ashok-Dumbre/Add-Xilinx-AMS-Driver/20211118-001150
git checkout 196a4a5b4232a4a12d0843a0002b664720b5d200
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=sparc64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
drivers/iio/adc/xilinx-ams.c: In function 'ams_update_intrmask':
drivers/iio/adc/xilinx-ams.c:323:20: error: implicit declaration of function 'FIELD_GET' [-Werror=implicit-function-declaration]
323 | regval = ~(FIELD_GET(AMS_ISR1_INTR_MASK, ams->intr_mask));
| ^~~~~~~~~
drivers/iio/adc/xilinx-ams.c: In function 'ams_update_ps_alarm':
drivers/iio/adc/xilinx-ams.c:360:17: error: implicit declaration of function 'FIELD_PREP' [-Werror=implicit-function-declaration]
360 | cfg = ~(FIELD_PREP(AMS_CONF1_ALARM_2_TO_0_MASK, val));
| ^~~~~~~~~~
drivers/iio/adc/xilinx-ams.c: In function 'ams_parse_firmware':
>> drivers/iio/adc/xilinx-ams.c:121:41: warning: conversion from 'long unsigned int' to 'u32' {aka 'unsigned int'} changes value from '18446744073709486080' to '4294901760' [-Woverflow]
121 | #define AMS_ALARM_THR_MIN ~(BIT(16) - 1)
| ^~~~~~~~~~~~~~
drivers/iio/adc/xilinx-ams.c:1285:40: note: in expansion of macro 'AMS_ALARM_THR_MIN'
1285 | writel(AMS_ALARM_THR_MIN,
| ^~~~~~~~~~~~~~~~~
>> drivers/iio/adc/xilinx-ams.c:121:41: warning: conversion from 'long unsigned int' to 'u32' {aka 'unsigned int'} changes value from '18446744073709486080' to '4294901760' [-Woverflow]
121 | #define AMS_ALARM_THR_MIN ~(BIT(16) - 1)
| ^~~~~~~~~~~~~~
drivers/iio/adc/xilinx-ams.c:1290:40: note: in expansion of macro 'AMS_ALARM_THR_MIN'
1290 | writel(AMS_ALARM_THR_MIN,
| ^~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +121 drivers/iio/adc/xilinx-ams.c
119
120 #define AMS_ALARM_THR_DIRECT_MASK BIT(1)
> 121 #define AMS_ALARM_THR_MIN ~(BIT(16) - 1)
122 #define AMS_ALARM_THR_MAX (BIT(16) - 1)
123
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Wed, Nov 17, 2021 at 04:10:26PM +0000, Anand Ashok Dumbre 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. Thanks for an update, my comments below. ... Missed bitfields.h as kbuild bot noticed. > +#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> ... > +#define AMS_ALARM_THR_DIRECT_MASK BIT(1) > +#define AMS_ALARM_THR_MIN ~(BIT(16) - 1) This is wrong. I already said it. > +#define AMS_ALARM_THR_MAX (BIT(16) - 1) ... > +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 +Comma, same to the rest where the last item is not a terminator one. > +}; > + > +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 ...like here. > +}; > +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 ...but not here! > +}; ... > +#define AMS_SEQ(x) (AMS_SEQ_MAX + (x)) > +#define AMS_VAUX_SEQ(x) (AMS_SEQ_MAX + (x)) What's the difference? > +#define AMS_PS_SEQ_MAX AMS_SEQ_MAX Perhaps this should be above (for the sake of easier reading). > +#define PS_SEQ(x) (x) > +#define PL_SEQ(x) (AMS_PS_SEQ_MAX + (x)) > +#define AMS_CTRL_SEQ_BASE (AMS_PS_SEQ_MAX * 3) ... > + ret = readl_poll_timeout(ams->base + AMS_PS_CSTS, reg, > + (reg & expect), 0, Parentheses are not needed. WHy 0? Is it okay to load CPU like this? > + AMS_INIT_TIMEOUT_US); > + if (ret) > + return ret; ... > + if (ams->pl_base) { > + reg = readl(ams->base + AMS_PL_CSTS); > + if (reg == 0) > + return (int) reg; I already said that this is wrong. > + writel(AMS_PL_RESET_VALUE, ams->pl_base + AMS_VP_VN); > + > + /* put sysmon in a default state */ > + ams_pl_update_reg(ams, AMS_REG_CONFIG1, AMS_CONF1_SEQ_MASK, > + AMS_CONF1_SEQ_DEFAULT); > + } ... > + ret = readl_poll_timeout(ams->base + AMS_ISR_1, reg, > + (reg & expect), 0, AMS_INIT_TIMEOUT_US); > + if (ret) > + return ret; Same two comments as per above readl_poll_timeout() usage. ... > + ret = ams_read_vcc_reg(ams, chan->address, val); > + if (ret) { > + mutex_unlock(&ams->lock); > + return -EINVAL; Shadowed error code. > + } ... > + case IIO_CHAN_INFO_OFFSET: > + /* Only the temperature channel has an offset */ > + *val = AMS_TEMP_OFFSET; > + return IIO_VAL_INT; > + } > + return -EINVAL; Why not keep it in the default case? … > + switch (event) { > + case AMS_ALARM_BIT_TEMP: > + scan_index += AMS_SEQ_TEMP; > + break; > + case AMS_ALARM_BIT_SUPPLY1: > + scan_index += AMS_SEQ_SUPPLY1; > + break; > + case AMS_ALARM_BIT_SUPPLY2: > + scan_index += AMS_SEQ_SUPPLY2; > + break; > + case AMS_ALARM_BIT_SUPPLY3: > + scan_index += AMS_SEQ_SUPPLY3; > + break; > + case AMS_ALARM_BIT_SUPPLY4: > + scan_index += AMS_SEQ_SUPPLY4; > + break; > + case AMS_ALARM_BIT_SUPPLY5: > + scan_index += AMS_SEQ_SUPPLY5; > + break; > + case AMS_ALARM_BIT_SUPPLY6: > + scan_index += AMS_SEQ_SUPPLY6; > + break; > + case AMS_ALARM_BIT_SUPPLY7: > + scan_index += AMS_SEQ_SUPPLY7; > + break; > + case AMS_ALARM_BIT_SUPPLY8: > + scan_index += AMS_SEQ_SUPPLY8; > + break; > + case AMS_ALARM_BIT_SUPPLY9: > + scan_index += AMS_SEQ_SUPPLY9; > + break; > + case AMS_ALARM_BIT_SUPPLY10: > + scan_index += AMS_SEQ_SUPPLY10; > + break; > + case AMS_ALARM_BIT_VCCAMS: > + scan_index += AMS_SEQ_VCCAMS; > + break; > + case AMS_ALARM_BIT_TEMP_REMOTE: > + scan_index += AMS_SEQ_TEMP_REMOTE; > + break; default: ? > + } ... > + if (chan->type == IIO_TEMP) { > + offset = ams_get_alarm_offset(chan->scan_index, > + IIO_EV_DIR_FALLING); One line? > + } ... > + const struct iio_chan_spec *chan; > + > + chan = ams_event_to_channel(indio_dev, event); Can be done in one line. > + /* Only process alarms that are not masked */ > + isr0 &= ~((ams->intr_mask & AMS_ISR0_ALARM_MASK) | ams->masked_alarm); > + Redundant blank line. > + if (!isr0) { > + spin_unlock(&ams->intr_lock); > + return IRQ_NONE; > + } ... > + .mask_separate = BIT(IIO_EV_INFO_ENABLE) | > + BIT(IIO_EV_INFO_VALUE), One line? ... > + fwnode_for_each_child_node(chan_node, child) { > + ret = fwnode_property_read_u32(child, "reg", ®); > + if (ret || reg > (AMS_PL_MAX_EXT_CHANNEL + 30)) Too many parentheses. > + continue; > + memcpy(&channels[num_channels], &ams_pl_channels[reg + > + AMS_PL_MAX_FIXED_CHANNEL - 30], sizeof(*channels)); > + > + if (fwnode_property_read_bool(child, "xlnx,bipolar")) > + channels[num_channels].scan_type.sign = 's'; Use temporary variable for &channels[num_channels] in both cases. > + num_channels++; > + } ... > + memcpy(channels + num_channels, ams_ps_channels, > + sizeof(ams_ps_channels)); Ditto. ... > + memcpy(channels + num_channels, ams_pl_channels, > + AMS_PL_MAX_FIXED_CHANNEL * sizeof(*channels)); Ditto. ... > + memcpy(channels + num_channels, ams_ctrl_channels, > + sizeof(ams_ctrl_channels)); Ditto. ... > +static int ams_parse_firmware(struct iio_dev *indio_dev, > + struct platform_device *pdev) Why do you need second parameter? Doesn't indio_dev already have it? > +{ > + 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); > + 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 enough. > + 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; Why is this needed now since you are using kcalloc()? > + /* Initialize buffer for channel specification */ > + ams_channels = kcalloc(num_chan, sizeof(struct iio_chan_spec), GFP_KERNEL); sizeof(*ams_channels) > + if (!ams_channels) > + return -ENOMEM; ... > + ret = ams_init_module(indio_dev, child, > + ams_channels + num_channels); One line? ... > + writel(AMS_ALARM_THR_MIN, > + ams->pl_base + falling_off); > + writel(AMS_ALARM_THR_MAX, > + ams->pl_base + rising_off); Ditto. ... > + writel(AMS_ALARM_THR_MIN, > + ams->ps_base + falling_off); > + writel(AMS_ALARM_THR_MAX, > + ams->ps_base + rising_off); Ditto. > + dev_chan_size = array_size((size_t)num_channels, sizeof(struct iio_chan_spec)); > + if (dev_chan_size == SIZE_MAX) > + return -EINVAL; Why is it needed now? > + dev_channels = devm_kcalloc(&pdev->dev, (size_t)num_channels, Why casting? > + sizeof(struct iio_chan_spec), GFP_KERNEL); sizeof(*dev_channels) > + if (!dev_channels) { > + ret = -ENOMEM; > + goto free_mem; > + } > + memcpy(dev_channels, ams_channels, > + sizeof(*ams_channels) * num_channels); Hmm... according to the code the num_channels can be less than or equal to num_chan. Hence, what you should use is the devm_krealloc_array(). static inline void *devm_krealloc_aray(...) { ...see how krealloc_array() is defined... } No need to copy memory again. ... > + ret = 0; > + > +free_mem: > + kfree(ams_channels); > + > + return ret; This will go away after switching to devm_kmalloc_array() + devm_krealloc_array(). ... > + ret = ams_parse_firmware(indio_dev, pdev); > + if (ret) { > + dev_err_probe(&pdev->dev, ret, "failure in parsing DT\n"); > + return ret; return dev_err_probe(...); Ditto for the rest similar cases. > + }
On 11/17/21 21:03, Andy Shevchenko wrote: > On Wed, Nov 17, 2021 at 04:10:26PM +0000, Anand Ashok Dumbre 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. > > Thanks for an update, my comments below. > > ... > > Missed bitfields.h as kbuild bot noticed. > >> +#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> > > ... > >> +#define AMS_ALARM_THR_DIRECT_MASK BIT(1) > >> +#define AMS_ALARM_THR_MIN ~(BIT(16) - 1) > > This is wrong. I already said it. > >> +#define AMS_ALARM_THR_MAX (BIT(16) - 1) > > ... > >> +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 > > +Comma, same to the rest where the last item is not a terminator one. And where you are touching this it will be good if you can initiate all values. That's recommendation we got from Greg some time ago. Thanks, Michal
Hi Andy, Thanks for your review. Apologies for sending patch too soon. I missed your reply to my previous patches. > -----Original Message----- > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Sent: Wednesday 17 November 2021 8:03 PM > To: Anand Ashok Dumbre <ANANDASH@xilinx.com> > Cc: linux-kernel@vger.kernel.org; jic23@kernel.org; lars@metafoo.de; linux- > iio@vger.kernel.org; git <git@xilinx.com>; Michal Simek > <michals@xilinx.com>; gregkh@linuxfoundation.org; rafael@kernel.org; > linux-acpi@vger.kernel.org; heikki.krogerus@linux.intel.com; Manish Narani > <MNARANI@xilinx.com> > Subject: Re: [PATCH v10 3/5] iio: adc: Add Xilinx AMS driver > > On Wed, Nov 17, 2021 at 04:10:26PM +0000, Anand Ashok Dumbre 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. > > Thanks for an update, my comments below. > > ... > > Missed bitfields.h as kbuild bot noticed. > > > +#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> > > ... > > > +#define AMS_ALARM_THR_DIRECT_MASK BIT(1) > > > +#define AMS_ALARM_THR_MIN ~(BIT(16) - 1) > > This is wrong. I already said it. Yes, understood. > > > +#define AMS_ALARM_THR_MAX (BIT(16) - 1) > > ... > > > +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 > > +Comma, same to the rest where the last item is not a terminator one. Will add. > > > +}; > > + > > +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 > > ...like here. Will add. > > > +}; > > > +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 > > ...but not here! > > > +}; > > ... > > > +#define AMS_SEQ(x) (AMS_SEQ_MAX + (x)) > > +#define AMS_VAUX_SEQ(x) (AMS_SEQ_MAX + (x)) > > What's the difference? I will remove the AMS_VAUX_SEQ. They are the same thing. I am not sure what the intent from the original author was. > > > +#define AMS_PS_SEQ_MAX AMS_SEQ_MAX > > Perhaps this should be above (for the sake of easier reading). Agreed. > > > +#define PS_SEQ(x) (x) > > +#define PL_SEQ(x) (AMS_PS_SEQ_MAX + (x)) > > +#define AMS_CTRL_SEQ_BASE (AMS_PS_SEQ_MAX * 3) > > ... > > > + ret = readl_poll_timeout(ams->base + AMS_PS_CSTS, reg, > > + (reg & expect), 0, > > Parentheses are not needed. > WHy 0? Is it okay to load CPU like this? No its not. > > > + AMS_INIT_TIMEOUT_US); > > + if (ret) > > + return ret; > > ... > > > + if (ams->pl_base) { > > + reg = readl(ams->base + AMS_PL_CSTS); > > + if (reg == 0) > > > + return (int) reg; > > I already said that this is wrong. > > > + writel(AMS_PL_RESET_VALUE, ams->pl_base + > AMS_VP_VN); > > + > > + /* put sysmon in a default state */ > > + ams_pl_update_reg(ams, AMS_REG_CONFIG1, > AMS_CONF1_SEQ_MASK, > > + AMS_CONF1_SEQ_DEFAULT); > > + } > > ... > > > + ret = readl_poll_timeout(ams->base + AMS_ISR_1, reg, > > + (reg & expect), 0, AMS_INIT_TIMEOUT_US); > > + if (ret) > > + return ret; > > Same two comments as per above readl_poll_timeout() usage. > > ... > > > + ret = ams_read_vcc_reg(ams, chan->address, val); > > + if (ret) { > > + mutex_unlock(&ams->lock); > > + return -EINVAL; > > Shadowed error code. I don’t understand. > > > + } > > ... > > > + case IIO_CHAN_INFO_OFFSET: > > + /* Only the temperature channel has an offset */ > > + *val = AMS_TEMP_OFFSET; > > + return IIO_VAL_INT; > > + } > > > + return -EINVAL; > > Why not keep it in the default case? Oops missed one there. > > … > > > + switch (event) { > > + case AMS_ALARM_BIT_TEMP: > > + scan_index += AMS_SEQ_TEMP; > > + break; > > + case AMS_ALARM_BIT_SUPPLY1: > > + scan_index += AMS_SEQ_SUPPLY1; > > + break; > > + case AMS_ALARM_BIT_SUPPLY2: > > + scan_index += AMS_SEQ_SUPPLY2; > > + break; > > + case AMS_ALARM_BIT_SUPPLY3: > > + scan_index += AMS_SEQ_SUPPLY3; > > + break; > > + case AMS_ALARM_BIT_SUPPLY4: > > + scan_index += AMS_SEQ_SUPPLY4; > > + break; > > + case AMS_ALARM_BIT_SUPPLY5: > > + scan_index += AMS_SEQ_SUPPLY5; > > + break; > > + case AMS_ALARM_BIT_SUPPLY6: > > + scan_index += AMS_SEQ_SUPPLY6; > > + break; > > + case AMS_ALARM_BIT_SUPPLY7: > > + scan_index += AMS_SEQ_SUPPLY7; > > + break; > > + case AMS_ALARM_BIT_SUPPLY8: > > + scan_index += AMS_SEQ_SUPPLY8; > > + break; > > + case AMS_ALARM_BIT_SUPPLY9: > > + scan_index += AMS_SEQ_SUPPLY9; > > + break; > > + case AMS_ALARM_BIT_SUPPLY10: > > + scan_index += AMS_SEQ_SUPPLY10; > > + break; > > + case AMS_ALARM_BIT_VCCAMS: > > + scan_index += AMS_SEQ_VCCAMS; > > + break; > > + case AMS_ALARM_BIT_TEMP_REMOTE: > > + scan_index += AMS_SEQ_TEMP_REMOTE; > > + break; > > default: ? This is limited by hw bits. For default I will use the default scan_index value. Is that ok? > > > + } > > ... > > > + if (chan->type == IIO_TEMP) { > > + offset = ams_get_alarm_offset(chan->scan_index, > > + IIO_EV_DIR_FALLING); > > One line? Agreed. > > > + } > > ... > > > + const struct iio_chan_spec *chan; > > + > > + chan = ams_event_to_channel(indio_dev, event); > > Can be done in one line. > > > > + /* Only process alarms that are not masked */ > > + isr0 &= ~((ams->intr_mask & AMS_ISR0_ALARM_MASK) | > > +ams->masked_alarm); > > > + > > Redundant blank line. Agreed. > > > + if (!isr0) { > > + spin_unlock(&ams->intr_lock); > > + return IRQ_NONE; > > + } > > ... > > > + .mask_separate = BIT(IIO_EV_INFO_ENABLE) | > > + BIT(IIO_EV_INFO_VALUE), > > One line? Agreed. > > ... > > > + fwnode_for_each_child_node(chan_node, child) { > > + ret = fwnode_property_read_u32(child, "reg", ®); > > > + if (ret || reg > (AMS_PL_MAX_EXT_CHANNEL + 30)) > > Too many parentheses. Is it a good practice to not have parantheses around (AMS_PL_MAX_EXT_CHANNEL + 30) ? > > > + continue; > > > + memcpy(&channels[num_channels], &ams_pl_channels[reg > + > > + AMS_PL_MAX_FIXED_CHANNEL - 30], sizeof(*channels)); > > + > > + if (fwnode_property_read_bool(child, "xlnx,bipolar")) > > + channels[num_channels].scan_type.sign = 's'; > > Use temporary variable for &channels[num_channels] in both cases. Makes sense. > > > + num_channels++; > > + } > > ... > > > + memcpy(channels + num_channels, ams_ps_channels, > > + sizeof(ams_ps_channels)); > > Ditto. > > ... > > > + memcpy(channels + num_channels, ams_pl_channels, > > + AMS_PL_MAX_FIXED_CHANNEL * sizeof(*channels)); > > Ditto. > > ... > > > + memcpy(channels + num_channels, ams_ctrl_channels, > > + sizeof(ams_ctrl_channels)); > > Ditto. > Will do for all cases. > ... > > > +static int ams_parse_firmware(struct iio_dev *indio_dev, > > + struct platform_device *pdev) > > Why do you need second parameter? Doesn't indio_dev already have it? Will do. > > > +{ > > + 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); > > + 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 enough. Will remove it. > > > + 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; > > Why is this needed now since you are using kcalloc()? > > > + /* Initialize buffer for channel specification */ > > + ams_channels = kcalloc(num_chan, sizeof(struct iio_chan_spec), > > +GFP_KERNEL); > > sizeof(*ams_channels) > > > + if (!ams_channels) > > + return -ENOMEM; > > ... > > > + ret = ams_init_module(indio_dev, child, > > + ams_channels + num_channels); > > One line? > > ... > > > + writel(AMS_ALARM_THR_MIN, > > + ams->pl_base + falling_off); > > + writel(AMS_ALARM_THR_MAX, > > + ams->pl_base + rising_off); > > Ditto. > > ... > > > + writel(AMS_ALARM_THR_MIN, > > + ams->ps_base + falling_off); > > + writel(AMS_ALARM_THR_MAX, > > + ams->ps_base + rising_off); > > Ditto. > > > + dev_chan_size = array_size((size_t)num_channels, sizeof(struct > iio_chan_spec)); > > + if (dev_chan_size == SIZE_MAX) > > + return -EINVAL; > > Why is it needed now? > > > + dev_channels = devm_kcalloc(&pdev->dev, (size_t)num_channels, > > Why casting? > > > + sizeof(struct iio_chan_spec), GFP_KERNEL); > > sizeof(*dev_channels) > > > + if (!dev_channels) { > > + ret = -ENOMEM; > > + goto free_mem; > > + } > > > + memcpy(dev_channels, ams_channels, > > + sizeof(*ams_channels) * num_channels); > > Hmm... according to the code the num_channels can be less than or equal to > num_chan. Hence, what you should use is the devm_krealloc_array(). > > static inline void *devm_krealloc_aray(...) { > ...see how krealloc_array() is defined... > } > > No need to copy memory again. Will take a look. > > ... > > > + ret = 0; > > + > > +free_mem: > > + kfree(ams_channels); > > + > > + return ret; > > This will go away after switching to devm_kmalloc_array() + > devm_krealloc_array(). > > ... > > > + ret = ams_parse_firmware(indio_dev, pdev); > > + if (ret) { > > + dev_err_probe(&pdev->dev, ret, "failure in parsing DT\n"); > > + return ret; > > return dev_err_probe(...); > > Ditto for the rest similar cases. Sure. > > > + } > > -- > With Best Regards, > Andy Shevchenko > Thanks, Anand
On Thu, Nov 18, 2021 at 02:14:41PM +0000, Anand Ashok Dumbre wrote: > > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Sent: Wednesday 17 November 2021 8:03 PM > > On Wed, Nov 17, 2021 at 04:10:26PM +0000, Anand Ashok Dumbre wrote: ... > > > + ret = ams_read_vcc_reg(ams, chan->address, val); > > > + if (ret) { > > > + mutex_unlock(&ams->lock); > > > + return -EINVAL; > > > > Shadowed error code. > > I don’t understand. You get an error code in ret variable, but you always return -EINVAL, why? > > > + } ... > > default: ? > > This is limited by hw bits. > For default I will use the default scan_index value. > Is that ok? I think so, you know hardware better, you decide what to put there. ... > > > + if (ret || reg > (AMS_PL_MAX_EXT_CHANNEL + 30)) > > > > Too many parentheses. > > Is it a good practice to not have parantheses around (AMS_PL_MAX_EXT_CHANNEL + 30) ? It's minor thing, up to you. > > > + continue; ... > > > + dev_chan_size = array_size((size_t)num_channels, sizeof(struct > > iio_chan_spec)); > > > + if (dev_chan_size == SIZE_MAX) > > > + return -EINVAL; > > > > Why is it needed now? > > > > > + dev_channels = devm_kcalloc(&pdev->dev, (size_t)num_channels, > > > > Why casting? > > > > > + sizeof(struct iio_chan_spec), GFP_KERNEL); > > > > sizeof(*dev_channels) > > > > > + if (!dev_channels) { > > > + ret = -ENOMEM; > > > + goto free_mem; > > > + } > > > > > + memcpy(dev_channels, ams_channels, > > > + sizeof(*ams_channels) * num_channels); > > > > Hmm... according to the code the num_channels can be less than or equal to > > num_chan. Hence, what you should use is the devm_krealloc_array(). > > > > static inline void *devm_krealloc_aray(...) { > > ...see how krealloc_array() is defined... > > } > > > > No need to copy memory again. > > Will take a look.
Hi Anand,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on jic23-iio/togreg]
[also build test ERROR on linux/master linus/master v5.16-rc1 next-20211118]
[cannot apply to xilinx-xlnx/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Anand-Ashok-Dumbre/Add-Xilinx-AMS-Driver/20211118-001150
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: arc-randconfig-r035-20211118 (attached as .config)
compiler: arceb-elf-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/196a4a5b4232a4a12d0843a0002b664720b5d200
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Anand-Ashok-Dumbre/Add-Xilinx-AMS-Driver/20211118-001150
git checkout 196a4a5b4232a4a12d0843a0002b664720b5d200
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=arc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All error/warnings (new ones prefixed by >>):
drivers/iio/adc/xilinx-ams.c: In function 'ams_update_intrmask':
>> drivers/iio/adc/xilinx-ams.c:323:20: error: implicit declaration of function 'FIELD_GET' [-Werror=implicit-function-declaration]
323 | regval = ~(FIELD_GET(AMS_ISR1_INTR_MASK, ams->intr_mask));
| ^~~~~~~~~
In file included from drivers/iio/adc/xilinx-ams.c:11:
>> include/linux/bits.h:35:29: warning: left shift count >= width of type [-Wshift-count-overflow]
35 | (((~UL(0)) - (UL(1) << (l)) + 1) & \
| ^~
include/linux/bits.h:38:38: note: in expansion of macro '__GENMASK'
38 | (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
| ^~~~~~~~~
drivers/iio/adc/xilinx-ams.c:131:41: note: in expansion of macro 'GENMASK'
131 | #define AMS_ISR1_INTR_MASK GENMASK(63, 32)
| ^~~~~~~
drivers/iio/adc/xilinx-ams.c:323:30: note: in expansion of macro 'AMS_ISR1_INTR_MASK'
323 | regval = ~(FIELD_GET(AMS_ISR1_INTR_MASK, ams->intr_mask));
| ^~~~~~~~~~~~~~~~~~
>> include/linux/bits.h:36:18: warning: right shift count is negative [-Wshift-count-negative]
36 | (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
| ^~
include/linux/bits.h:38:38: note: in expansion of macro '__GENMASK'
38 | (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
| ^~~~~~~~~
drivers/iio/adc/xilinx-ams.c:131:41: note: in expansion of macro 'GENMASK'
131 | #define AMS_ISR1_INTR_MASK GENMASK(63, 32)
| ^~~~~~~
drivers/iio/adc/xilinx-ams.c:323:30: note: in expansion of macro 'AMS_ISR1_INTR_MASK'
323 | regval = ~(FIELD_GET(AMS_ISR1_INTR_MASK, ams->intr_mask));
| ^~~~~~~~~~~~~~~~~~
>> include/linux/bits.h:35:29: warning: left shift count >= width of type [-Wshift-count-overflow]
35 | (((~UL(0)) - (UL(1) << (l)) + 1) & \
| ^~
include/linux/bits.h:38:38: note: in expansion of macro '__GENMASK'
38 | (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
| ^~~~~~~~~
drivers/iio/adc/xilinx-ams.c:131:41: note: in expansion of macro 'GENMASK'
131 | #define AMS_ISR1_INTR_MASK GENMASK(63, 32)
| ^~~~~~~
drivers/iio/adc/xilinx-ams.c:329:28: note: in expansion of macro 'AMS_ISR1_INTR_MASK'
329 | regval = FIELD_GET(AMS_ISR1_INTR_MASK, ams->intr_mask);
| ^~~~~~~~~~~~~~~~~~
>> include/linux/bits.h:36:18: warning: right shift count is negative [-Wshift-count-negative]
36 | (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
| ^~
include/linux/bits.h:38:38: note: in expansion of macro '__GENMASK'
38 | (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
| ^~~~~~~~~
drivers/iio/adc/xilinx-ams.c:131:41: note: in expansion of macro 'GENMASK'
131 | #define AMS_ISR1_INTR_MASK GENMASK(63, 32)
| ^~~~~~~
drivers/iio/adc/xilinx-ams.c:329:28: note: in expansion of macro 'AMS_ISR1_INTR_MASK'
329 | regval = FIELD_GET(AMS_ISR1_INTR_MASK, ams->intr_mask);
| ^~~~~~~~~~~~~~~~~~
drivers/iio/adc/xilinx-ams.c: In function 'ams_update_ps_alarm':
>> drivers/iio/adc/xilinx-ams.c:360:17: error: implicit declaration of function 'FIELD_PREP' [-Werror=implicit-function-declaration]
360 | cfg = ~(FIELD_PREP(AMS_CONF1_ALARM_2_TO_0_MASK, val));
| ^~~~~~~~~~
In file included from drivers/iio/adc/xilinx-ams.c:11:
drivers/iio/adc/xilinx-ams.c: In function 'ams_enable_channel_sequence':
>> include/linux/bits.h:36:18: warning: right shift count is negative [-Wshift-count-negative]
36 | (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
| ^~
include/linux/bits.h:38:38: note: in expansion of macro '__GENMASK'
38 | (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
| ^~~~~~~~~
drivers/iio/adc/xilinx-ams.c:102:41: note: in expansion of macro 'GENMASK'
102 | #define AMS_PL_SEQ_MASK GENMASK(59, 22)
| ^~~~~~~
drivers/iio/adc/xilinx-ams.c:448:39: note: in expansion of macro 'AMS_PL_SEQ_MASK'
448 | scan_mask = FIELD_GET(AMS_PL_SEQ_MASK, scan_mask);
| ^~~~~~~~~~~~~~~
>> include/linux/bits.h:36:18: warning: right shift count is negative [-Wshift-count-negative]
36 | (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
| ^~
include/linux/bits.h:38:38: note: in expansion of macro '__GENMASK'
38 | (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
| ^~~~~~~~~
drivers/iio/adc/xilinx-ams.c:99:41: note: in expansion of macro 'GENMASK'
99 | #define AMS_REG_SEQ1_MASK GENMASK(37, 22)
| ^~~~~~~
drivers/iio/adc/xilinx-ams.c:453:36: note: in expansion of macro 'AMS_REG_SEQ1_MASK'
453 | regval = FIELD_GET(AMS_REG_SEQ1_MASK, scan_mask);
| ^~~~~~~~~~~~~~~~~
drivers/iio/adc/xilinx-ams.c: In function 'ams_init_device':
>> include/linux/bits.h:36:18: warning: right shift count is negative [-Wshift-count-negative]
36 | (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
| ^~
include/linux/bits.h:38:38: note: in expansion of macro '__GENMASK'
38 | (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
| ^~~~~~~~~
drivers/iio/adc/xilinx-ams.c:124:41: note: in expansion of macro 'GENMASK'
124 | #define AMS_ALARM_MASK GENMASK(63, 0)
| ^~~~~~~
drivers/iio/adc/xilinx-ams.c:501:34: note: in expansion of macro 'AMS_ALARM_MASK'
501 | ams_update_intrmask(ams, AMS_ALARM_MASK, AMS_ALARM_MASK);
| ^~~~~~~~~~~~~~
>> include/linux/bits.h:36:18: warning: right shift count is negative [-Wshift-count-negative]
36 | (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
| ^~
include/linux/bits.h:38:38: note: in expansion of macro '__GENMASK'
38 | (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
| ^~~~~~~~~
drivers/iio/adc/xilinx-ams.c:124:41: note: in expansion of macro 'GENMASK'
124 | #define AMS_ALARM_MASK GENMASK(63, 0)
| ^~~~~~~
drivers/iio/adc/xilinx-ams.c:501:50: note: in expansion of macro 'AMS_ALARM_MASK'
501 | ams_update_intrmask(ams, AMS_ALARM_MASK, AMS_ALARM_MASK);
| ^~~~~~~~~~~~~~
drivers/iio/adc/xilinx-ams.c: In function 'ams_unmask_worker':
>> include/linux/bits.h:36:18: warning: right shift count is negative [-Wshift-count-negative]
36 | (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
| ^~
include/linux/bits.h:38:38: note: in expansion of macro '__GENMASK'
38 | (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
| ^~~~~~~~~
drivers/iio/adc/xilinx-ams.c:124:41: note: in expansion of macro 'GENMASK'
124 | #define AMS_ALARM_MASK GENMASK(63, 0)
| ^~~~~~~
drivers/iio/adc/xilinx-ams.c:1017:35: note: in expansion of macro 'AMS_ALARM_MASK'
1017 | ams_update_intrmask(ams, ~AMS_ALARM_MASK, ~AMS_ALARM_MASK);
| ^~~~~~~~~~~~~~
>> include/linux/bits.h:36:18: warning: right shift count is negative [-Wshift-count-negative]
36 | (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
| ^~
include/linux/bits.h:38:38: note: in expansion of macro '__GENMASK'
38 | (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
| ^~~~~~~~~
drivers/iio/adc/xilinx-ams.c:124:41: note: in expansion of macro 'GENMASK'
124 | #define AMS_ALARM_MASK GENMASK(63, 0)
| ^~~~~~~
drivers/iio/adc/xilinx-ams.c:1017:52: note: in expansion of macro 'AMS_ALARM_MASK'
1017 | ams_update_intrmask(ams, ~AMS_ALARM_MASK, ~AMS_ALARM_MASK);
| ^~~~~~~~~~~~~~
drivers/iio/adc/xilinx-ams.c: In function 'ams_irq':
>> include/linux/bits.h:36:18: warning: right shift count is negative [-Wshift-count-negative]
36 | (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
| ^~
include/linux/bits.h:38:38: note: in expansion of macro '__GENMASK'
38 | (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
| ^~~~~~~~~
drivers/iio/adc/xilinx-ams.c:124:41: note: in expansion of macro 'GENMASK'
124 | #define AMS_ALARM_MASK GENMASK(63, 0)
| ^~~~~~~
drivers/iio/adc/xilinx-ams.c:1049:35: note: in expansion of macro 'AMS_ALARM_MASK'
1049 | ams_update_intrmask(ams, ~AMS_ALARM_MASK, ~AMS_ALARM_MASK);
| ^~~~~~~~~~~~~~
>> include/linux/bits.h:36:18: warning: right shift count is negative [-Wshift-count-negative]
36 | (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
| ^~
include/linux/bits.h:38:38: note: in expansion of macro '__GENMASK'
38 | (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
| ^~~~~~~~~
drivers/iio/adc/xilinx-ams.c:124:41: note: in expansion of macro 'GENMASK'
124 | #define AMS_ALARM_MASK GENMASK(63, 0)
| ^~~~~~~
drivers/iio/adc/xilinx-ams.c:1049:52: note: in expansion of macro 'AMS_ALARM_MASK'
1049 | ams_update_intrmask(ams, ~AMS_ALARM_MASK, ~AMS_ALARM_MASK);
| ^~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +/FIELD_GET +323 drivers/iio/adc/xilinx-ams.c
313
314 static void ams_update_intrmask(struct ams *ams, u64 mask, u64 val)
315 {
316 u32 regval;
317
318 ams->intr_mask = (ams->intr_mask & ~mask) | (val & mask);
319
320 regval = ~(ams->intr_mask | ams->masked_alarm);
321 writel(regval, ams->base + AMS_IER_0);
322
> 323 regval = ~(FIELD_GET(AMS_ISR1_INTR_MASK, ams->intr_mask));
324 writel(regval, ams->base + AMS_IER_1);
325
326 regval = ams->intr_mask | ams->masked_alarm;
327 writel(regval, ams->base + AMS_IDR_0);
328
329 regval = FIELD_GET(AMS_ISR1_INTR_MASK, ams->intr_mask);
330 writel(regval, ams->base + AMS_IDR_1);
331 }
332
333 static void ams_disable_all_alarms(struct ams *ams)
334 {
335 /* disable PS module alarm */
336 if (ams->ps_base) {
337 ams_ps_update_reg(ams, AMS_REG_CONFIG1, AMS_REGCFG1_ALARM_MASK,
338 AMS_REGCFG1_ALARM_MASK);
339 ams_ps_update_reg(ams, AMS_REG_CONFIG3, AMS_REGCFG3_ALARM_MASK,
340 AMS_REGCFG3_ALARM_MASK);
341 }
342
343 /* disable PL module alarm */
344 if (ams->pl_base) {
345 ams_pl_update_reg(ams, AMS_REG_CONFIG1,
346 AMS_REGCFG1_ALARM_MASK,
347 AMS_REGCFG1_ALARM_MASK);
348 ams_pl_update_reg(ams, AMS_REG_CONFIG3,
349 AMS_REGCFG3_ALARM_MASK,
350 AMS_REGCFG3_ALARM_MASK);
351 }
352 }
353
354 static void ams_update_ps_alarm(struct ams *ams, unsigned long alarm_mask)
355 {
356 u32 cfg;
357 u32 val;
358
359 val = FIELD_GET(AMS_ISR0_ALARM_2_TO_0_MASK, alarm_mask);
> 360 cfg = ~(FIELD_PREP(AMS_CONF1_ALARM_2_TO_0_MASK, val));
361
362 val = FIELD_GET(AMS_ISR0_ALARM_6_TO_3_MASK, alarm_mask);
363 cfg &= ~(FIELD_PREP(AMS_CONF1_ALARM_6_TO_3_MASK, val));
364
365 ams_ps_update_reg(ams, AMS_REG_CONFIG1, AMS_REGCFG1_ALARM_MASK, cfg);
366
367 val = FIELD_GET(AMS_ISR0_ALARM_12_TO_7_MASK, alarm_mask);
368 cfg = ~(FIELD_PREP(AMS_CONF1_ALARM_12_TO_7_MASK, val));
369 ams_ps_update_reg(ams, AMS_REG_CONFIG3, AMS_REGCFG3_ALARM_MASK, cfg);
370 }
371
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Wed, 17 Nov 2021 16:10:26 +0000 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 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@xilinx.com> > Signed-off-by: Manish Narani <manish.narani@xilinx.com> > Signed-off-by: Anand Ashok Dumbre <anand.ashok.dumbre@xilinx.com> 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@xilinx.com> > + * Rajnikant Bhojani <rajnikant.bhojani@xilinx.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/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. > + 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; > +}
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@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 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@xilinx.com> > > Signed-off-by: Manish Narani <manish.narani@xilinx.com> > > Signed-off-by: Anand Ashok Dumbre <anand.ashok.dumbre@xilinx.com> > > 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@xilinx.com> > > + * Rajnikant Bhojani <rajnikant.bhojani@xilinx.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/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.
Hi Andy, Thanks for the review. > -----Original Message----- > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Sent: Thursday 18 November 2021 3:43 PM > To: Anand Ashok Dumbre <ANANDASH@xilinx.com> > Cc: linux-kernel@vger.kernel.org; jic23@kernel.org; lars@metafoo.de; linux- > iio@vger.kernel.org; git <git@xilinx.com>; Michal Simek > <michals@xilinx.com>; gregkh@linuxfoundation.org; rafael@kernel.org; > linux-acpi@vger.kernel.org; heikki.krogerus@linux.intel.com; Manish Narani > <MNARANI@xilinx.com> > Subject: Re: [PATCH v10 3/5] iio: adc: Add Xilinx AMS driver > > On Thu, Nov 18, 2021 at 02:14:41PM +0000, Anand Ashok Dumbre wrote: > > > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > Sent: Wednesday 17 November 2021 8:03 PM On Wed, Nov 17, 2021 at > > > 04:10:26PM +0000, Anand Ashok Dumbre wrote: > > ... > > > > > + ret = ams_read_vcc_reg(ams, chan->address, val); > > > > + if (ret) { > > > > + mutex_unlock(&ams->lock); > > > > + return -EINVAL; > > > > > > Shadowed error code. > > > > I don’t understand. > > You get an error code in ret variable, but you always return -EINVAL, why? Got it. Will fix it. > > > > > + } > > ... > > > > default: ? > > > > This is limited by hw bits. > > For default I will use the default scan_index value. > > Is that ok? > > I think so, you know hardware better, you decide what to put there. Will do > > ... > > > > > + if (ret || reg > (AMS_PL_MAX_EXT_CHANNEL + 30)) > > > > > > Too many parentheses. > > > > Is it a good practice to not have parantheses around > (AMS_PL_MAX_EXT_CHANNEL + 30) ? > > It's minor thing, up to you. > > > > > + continue; > > ... > > > > > + dev_chan_size = array_size((size_t)num_channels, sizeof(struct > > > iio_chan_spec)); > > > > + if (dev_chan_size == SIZE_MAX) > > > > + return -EINVAL; > > > > > > Why is it needed now? I didn’t see kcalloc did it internally. I will remove it. > > > > > > > + dev_channels = devm_kcalloc(&pdev->dev, (size_t)num_channels, > > > > > > Why casting? Removed. > > > > > > > + sizeof(struct iio_chan_spec), GFP_KERNEL); > > > > > > sizeof(*dev_channels) > > > > > > > + if (!dev_channels) { > > > > + ret = -ENOMEM; > > > > + goto free_mem; > > > > + } > > > > > > > + memcpy(dev_channels, ams_channels, > > > > + sizeof(*ams_channels) * num_channels); > > > > > > Hmm... according to the code the num_channels can be less than or > > > equal to num_chan. Hence, what you should use is the > devm_krealloc_array(). > > > > > > static inline void *devm_krealloc_aray(...) { > > > ...see how krealloc_array() is defined... > > > } > > > > > > No need to copy memory again. > > > > Will take a look. I did see that there was previously a devm_krealloc_array implementation sent for the other Xilinx xadc driver. https://patchwork.kernel.org/project/linux-iio/patch/20201102142228.14949-2-brgl@bgdev.pl/ From the conversations based here, should the use of devm_krealloc just be fine? If not, I will send a patch with devm_krealloc for now, and then I can send a patch later for devm_krealloc array and Its usage for both Xilinx-ams and xadc driver as well. > > -- > With Best Regards, > Andy Shevchenko > Thanks, Anand
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig index 8bf5b62a73f4..f6efe11ea1a8 100644 --- a/drivers/iio/adc/Kconfig +++ b/drivers/iio/adc/Kconfig @@ -1288,4 +1288,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. + + 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 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@xilinx.com> + * Rajnikant Bhojani <rajnikant.bhojani@xilinx.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/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> + +/* 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_CONFIG4 0x110 +#define AMS_REG_SEQ_CH0 0x120 +#define AMS_REG_SEQ_CH1 0x124 +#define AMS_REG_SEQ_CH2 0x118 + +#define AMS_VUSER0_MASK BIT(0) +#define AMS_VUSER1_MASK BIT(1) +#define AMS_VUSER2_MASK BIT(2) +#define AMS_VUSER3_MASK BIT(3) + +#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 FIELD_PREP(AMS_CONF1_SEQ_MASK, 0) +#define AMS_CONF1_SEQ_CONTINUOUS FIELD_PREP(AMS_CONF1_SEQ_MASK, 1) +#define AMS_CONF1_SEQ_SINGLE_CHANNEL FIELD_PREP(AMS_CONF1_SEQ_MASK, 2) + +#define AMS_REG_SEQ0_MASK GENMASK(15, 0) +#define AMS_REG_SEQ2_MASK GENMASK(21, 16) +#define AMS_REG_SEQ1_MASK GENMASK(37, 22) + +#define AMS_PS_SEQ_MASK GENMASK(21, 0) +#define AMS_PL_SEQ_MASK GENMASK(59, 22) + +#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 BIT(1) +#define AMS_ALARM_THR_MIN ~(BIT(16) - 1) +#define AMS_ALARM_THR_MAX (BIT(16) - 1) + +#define AMS_ALARM_MASK GENMASK(63, 0) +#define AMS_NO_OF_ALARMS 32 +#define AMS_PL_ALARM_START 16 +#define AMS_PL_ALARM_MASK GENMASK(31, 16) +#define AMS_ISR0_ALARM_MASK GENMASK(31, 0) +#define AMS_ISR1_ALARM_MASK (GENMASK(31, 29) | GENMASK(4, 0)) +#define AMS_ISR1_EOC_MASK BIT(3) +#define AMS_ISR1_INTR_MASK GENMASK(63, 32) +#define AMS_ISR0_ALARM_2_TO_0_MASK GENMASK(2, 0) +#define AMS_ISR0_ALARM_6_TO_3_MASK GENMASK(6, 3) +#define AMS_ISR0_ALARM_12_TO_7_MASK GENMASK(13, 8) +#define AMS_CONF1_ALARM_2_TO_0_MASK GENMASK(3, 1) +#define AMS_CONF1_ALARM_6_TO_3_MASK GENMASK(11, 8) +#define AMS_CONF1_ALARM_12_TO_7_MASK GENMASK(5, 0) +#define AMS_REGCFG1_ALARM_MASK \ + (AMS_CONF1_ALARM_2_TO_0_MASK | AMS_CONF1_ALARM_6_TO_3_MASK | BIT(0)) +#define AMS_REGCFG3_ALARM_MASK AMS_CONF1_ALARM_12_TO_7_MASK + +#define AMS_PS_CSTS_PS_READY (BIT(27) | BIT(16)) +#define AMS_PL_CSTS_ACCESS_MASK BIT(1) + +#define AMS_PL_MAX_FIXED_CHANNEL 10 +#define AMS_PL_MAX_EXT_CHANNEL 20 + +#define AMS_INIT_TIMEOUT_US 10000 +#define AMS_UNMASK_TIMEOUT_MS 500 + +/* + * 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 -((280230LL << 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_CTRL_SEQ_BASE (AMS_PS_SEQ_MAX * 3) + +#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), \ + .event_spec = ams_temp_events, \ + .scan_index = _scan_index, \ + .num_event_specs = ARRAY_SIZE(ams_temp_events), \ +} + +#define AMS_CHAN_VOLTAGE(_scan_index, _addr, _alarm) { \ + .type = IIO_VOLTAGE, \ + .indexed = 1, \ + .address = (_addr), \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ + BIT(IIO_CHAN_INFO_SCALE), \ + .event_spec = (_alarm) ? ams_voltage_events : NULL, \ + .scan_index = _scan_index, \ + .num_event_specs = (_alarm) ? ARRAY_SIZE(ams_voltage_events) : 0, \ +} + +#define AMS_PS_CHAN_TEMP(_scan_index, _addr) \ + AMS_CHAN_TEMP(PS_SEQ(_scan_index), _addr) +#define AMS_PS_CHAN_VOLTAGE(_scan_index, _addr) \ + AMS_CHAN_VOLTAGE(PS_SEQ(_scan_index), _addr, true) + +#define AMS_PL_CHAN_TEMP(_scan_index, _addr) \ + AMS_CHAN_TEMP(PL_SEQ(_scan_index), _addr) +#define AMS_PL_CHAN_VOLTAGE(_scan_index, _addr, _alarm) \ + AMS_CHAN_VOLTAGE(PL_SEQ(_scan_index), _addr, _alarm) +#define AMS_PL_AUX_CHAN_VOLTAGE(_auxno) \ + AMS_CHAN_VOLTAGE(PL_SEQ(AMS_VAUX_SEQ(_auxno)), \ + AMS_REG_VAUX(_auxno), false) +#define AMS_CTRL_CHAN_VOLTAGE(_scan_index, _addr) \ + AMS_CHAN_VOLTAGE(PL_SEQ(AMS_VAUX_SEQ(AMS_SEQ(_scan_index))), \ + _addr, false) + +/** + * 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; + u64 intr_mask; + struct delayed_work ams_unmask_work; +}; + +static inline void ams_ps_update_reg(struct ams *ams, unsigned int offset, + u32 mask, u32 data) +{ + u32 val, regval; + + val = readl(ams->ps_base + offset); + regval = (val & ~mask) | (data & mask); + writel(regval, ams->ps_base + offset); +} + +static inline void ams_pl_update_reg(struct ams *ams, unsigned int offset, + u32 mask, u32 data) +{ + u32 val, regval; + + val = readl(ams->pl_base + offset); + regval = (val & ~mask) | (data & mask); + writel(regval, ams->pl_base + offset); +} + +static void ams_update_intrmask(struct ams *ams, u64 mask, u64 val) +{ + u32 regval; + + ams->intr_mask = (ams->intr_mask & ~mask) | (val & mask); + + regval = ~(ams->intr_mask | ams->masked_alarm); + writel(regval, ams->base + AMS_IER_0); + + regval = ~(FIELD_GET(AMS_ISR1_INTR_MASK, ams->intr_mask)); + writel(regval, ams->base + AMS_IER_1); + + regval = ams->intr_mask | ams->masked_alarm; + writel(regval, ams->base + AMS_IDR_0); + + regval = FIELD_GET(AMS_ISR1_INTR_MASK, ams->intr_mask); + writel(regval, ams->base + AMS_IDR_1); +} + +static void ams_disable_all_alarms(struct ams *ams) +{ + /* disable PS module alarm */ + if (ams->ps_base) { + ams_ps_update_reg(ams, AMS_REG_CONFIG1, AMS_REGCFG1_ALARM_MASK, + AMS_REGCFG1_ALARM_MASK); + ams_ps_update_reg(ams, AMS_REG_CONFIG3, AMS_REGCFG3_ALARM_MASK, + AMS_REGCFG3_ALARM_MASK); + } + + /* disable PL module alarm */ + if (ams->pl_base) { + ams_pl_update_reg(ams, AMS_REG_CONFIG1, + AMS_REGCFG1_ALARM_MASK, + AMS_REGCFG1_ALARM_MASK); + ams_pl_update_reg(ams, AMS_REG_CONFIG3, + AMS_REGCFG3_ALARM_MASK, + AMS_REGCFG3_ALARM_MASK); + } +} + +static void ams_update_ps_alarm(struct ams *ams, unsigned long alarm_mask) +{ + u32 cfg; + u32 val; + + val = FIELD_GET(AMS_ISR0_ALARM_2_TO_0_MASK, alarm_mask); + cfg = ~(FIELD_PREP(AMS_CONF1_ALARM_2_TO_0_MASK, val)); + + val = FIELD_GET(AMS_ISR0_ALARM_6_TO_3_MASK, alarm_mask); + cfg &= ~(FIELD_PREP(AMS_CONF1_ALARM_6_TO_3_MASK, val)); + + ams_ps_update_reg(ams, AMS_REG_CONFIG1, AMS_REGCFG1_ALARM_MASK, cfg); + + val = FIELD_GET(AMS_ISR0_ALARM_12_TO_7_MASK, alarm_mask); + cfg = ~(FIELD_PREP(AMS_CONF1_ALARM_12_TO_7_MASK, val)); + ams_ps_update_reg(ams, AMS_REG_CONFIG3, AMS_REGCFG3_ALARM_MASK, cfg); +} + +static void ams_update_pl_alarm(struct ams *ams, unsigned long alarm_mask) +{ + unsigned long pl_alarm_mask; + u32 cfg; + u32 val; + + pl_alarm_mask = FIELD_GET(AMS_PL_ALARM_MASK, alarm_mask); + + val = FIELD_GET(AMS_ISR0_ALARM_2_TO_0_MASK, pl_alarm_mask); + cfg = ~(FIELD_PREP(AMS_CONF1_ALARM_2_TO_0_MASK, val)); + + val = FIELD_GET(AMS_ISR0_ALARM_6_TO_3_MASK, pl_alarm_mask); + cfg &= ~(FIELD_PREP(AMS_CONF1_ALARM_6_TO_3_MASK, val)); + + ams_pl_update_reg(ams, AMS_REG_CONFIG1, AMS_REGCFG1_ALARM_MASK, cfg); + + val = FIELD_GET(AMS_ISR0_ALARM_12_TO_7_MASK, pl_alarm_mask); + cfg = ~(FIELD_PREP(AMS_CONF1_ALARM_12_TO_7_MASK, val)); + ams_pl_update_reg(ams, AMS_REG_CONFIG3, AMS_REGCFG3_ALARM_MASK, cfg); +} + +static void ams_update_alarm(struct ams *ams, unsigned long alarm_mask) +{ + unsigned long flags; + + if (ams->ps_base) + ams_update_ps_alarm(ams, alarm_mask); + + if (ams->pl_base) + ams_update_pl_alarm(ams, alarm_mask); + + spin_lock_irqsave(&ams->intr_lock, flags); + ams_update_intrmask(ams, AMS_ISR0_ALARM_MASK, ~alarm_mask); + spin_unlock_irqrestore(&ams->intr_lock, flags); +} + +static void ams_enable_channel_sequence(struct iio_dev *indio_dev) +{ + struct ams *ams = iio_priv(indio_dev); + unsigned long long scan_mask; + int i; + u32 regval; + + /* + * Enable channel sequence. First 22 bits of scan_mask represent + * PS channels, and next remaining bits represent PL channels. + */ + + /* Run calibration of PS & PL as part of the sequence */ + scan_mask = BIT(0) | BIT(AMS_PS_SEQ_MAX); + for (i = 0; i < indio_dev->num_channels; i++) + scan_mask |= BIT_ULL(indio_dev->channels[i].scan_index); + + if (ams->ps_base) { + /* put sysmon in a soft reset to change the sequence */ + ams_ps_update_reg(ams, AMS_REG_CONFIG1, AMS_CONF1_SEQ_MASK, + AMS_CONF1_SEQ_DEFAULT); + + /* configure basic channels */ + regval = FIELD_GET(AMS_REG_SEQ0_MASK, scan_mask); + writel(regval, ams->ps_base + AMS_REG_SEQ_CH0); + + regval = FIELD_GET(AMS_REG_SEQ2_MASK, scan_mask); + writel(regval, ams->ps_base + AMS_REG_SEQ_CH2); + + /* set continuous sequence mode */ + ams_ps_update_reg(ams, AMS_REG_CONFIG1, AMS_CONF1_SEQ_MASK, + AMS_CONF1_SEQ_CONTINUOUS); + } + + if (ams->pl_base) { + /* put sysmon in a soft reset to change the sequence */ + ams_pl_update_reg(ams, AMS_REG_CONFIG1, AMS_CONF1_SEQ_MASK, + AMS_CONF1_SEQ_DEFAULT); + + /* configure basic channels */ + scan_mask = FIELD_GET(AMS_PL_SEQ_MASK, scan_mask); + + regval = FIELD_GET(AMS_REG_SEQ0_MASK, scan_mask); + writel(regval, ams->pl_base + AMS_REG_SEQ_CH0); + + regval = FIELD_GET(AMS_REG_SEQ1_MASK, scan_mask); + writel(regval, ams->pl_base + AMS_REG_SEQ_CH1); + + regval = FIELD_GET(AMS_REG_SEQ2_MASK, scan_mask); + writel(regval, ams->pl_base + AMS_REG_SEQ_CH2); + + /* set continuous sequence mode */ + ams_pl_update_reg(ams, AMS_REG_CONFIG1, AMS_CONF1_SEQ_MASK, + AMS_CONF1_SEQ_CONTINUOUS); + } +} + +static int ams_init_device(struct ams *ams) +{ + u32 reg; + u32 expect = AMS_PS_CSTS_PS_READY; + int ret; + + /* reset AMS */ + if (ams->ps_base) { + writel(AMS_PS_RESET_VALUE, ams->ps_base + AMS_VP_VN); + + ret = readl_poll_timeout(ams->base + AMS_PS_CSTS, reg, + (reg & expect), 0, + AMS_INIT_TIMEOUT_US); + if (ret) + return ret; + + /* put sysmon in a default state */ + ams_ps_update_reg(ams, AMS_REG_CONFIG1, AMS_CONF1_SEQ_MASK, + AMS_CONF1_SEQ_DEFAULT); + } + + if (ams->pl_base) { + reg = readl(ams->base + AMS_PL_CSTS); + if (reg == 0) + return (int) reg; + + writel(AMS_PL_RESET_VALUE, ams->pl_base + AMS_VP_VN); + + /* put sysmon in a default state */ + ams_pl_update_reg(ams, AMS_REG_CONFIG1, AMS_CONF1_SEQ_MASK, + AMS_CONF1_SEQ_DEFAULT); + } + + ams_disable_all_alarms(ams); + + /* Disable interrupt */ + ams_update_intrmask(ams, AMS_ALARM_MASK, AMS_ALARM_MASK); + + /* Clear any pending interrupt */ + writel(AMS_ISR0_ALARM_MASK, ams->base + AMS_ISR_0); + writel(AMS_ISR1_ALARM_MASK, ams->base + AMS_ISR_1); + + return 0; +} + +static int ams_enable_single_channel(struct ams *ams, unsigned int offset) +{ + u8 channel_num; + + 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); + + return 0; +} + +static int ams_read_vcc_reg(struct ams *ams, unsigned int offset, u32 *data) +{ + u32 reg; + u32 expect = AMS_ISR1_EOC_MASK; + int ret; + + ret = ams_enable_single_channel(ams, offset); + if (ret) + return ret; + + ret = readl_poll_timeout(ams->base + AMS_ISR_1, reg, + (reg & expect), 0, AMS_INIT_TIMEOUT_US); + if (ret) + return ret; + + *data = readl(ams->base + offset); + + return 0; +} + +static int ams_read_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + int *val, int *val2, long mask) +{ + struct ams *ams = iio_priv(indio_dev); + int ret; + int regval; + + switch (mask) { + case IIO_CHAN_INFO_RAW: + mutex_lock(&ams->lock); + if (chan->scan_index >= AMS_CTRL_SEQ_BASE) { + ret = ams_read_vcc_reg(ams, chan->address, val); + if (ret) { + mutex_unlock(&ams->lock); + return -EINVAL; + } + ams_enable_channel_sequence(indio_dev); + } else if (chan->scan_index >= AMS_PS_SEQ_MAX) + *val = readl(ams->pl_base + chan->address); + else + *val = readl(ams->ps_base + chan->address); + mutex_unlock(&ams->lock); + + return IIO_VAL_INT; + case IIO_CHAN_INFO_SCALE: + switch (chan->type) { + case IIO_VOLTAGE: + if (chan->scan_index < AMS_PS_SEQ_MAX) { + switch (chan->address) { + case AMS_SUPPLY1: + case AMS_SUPPLY2: + case AMS_SUPPLY3: + case AMS_SUPPLY4: + case AMS_SUPPLY9: + case AMS_SUPPLY10: + case AMS_VCCAMS: + *val = AMS_SUPPLY_SCALE_3VOLT; + break; + case AMS_SUPPLY5: + case AMS_SUPPLY6: + case AMS_SUPPLY7: + case AMS_SUPPLY8: + *val = AMS_SUPPLY_SCALE_6VOLT; + break; + default: + *val = AMS_SUPPLY_SCALE_1VOLT; + break; + } + } else if (chan->scan_index >= AMS_PS_SEQ_MAX && + chan->scan_index < AMS_CTRL_SEQ_BASE) { + switch (chan->address) { + case AMS_SUPPLY1: + case AMS_SUPPLY2: + case AMS_SUPPLY3: + case AMS_SUPPLY4: + case AMS_SUPPLY5: + case AMS_SUPPLY6: + case AMS_VCCAMS: + case AMS_VREFP: + case AMS_VREFN: + *val = AMS_SUPPLY_SCALE_3VOLT; + break; + case AMS_SUPPLY7: + regval = readl(ams->pl_base + AMS_REG_CONFIG4); + if (FIELD_GET(AMS_VUSER0_MASK, regval)) + *val = AMS_SUPPLY_SCALE_6VOLT; + else + *val = AMS_SUPPLY_SCALE_3VOLT; + break; + case AMS_SUPPLY8: + regval = readl(ams->pl_base + AMS_REG_CONFIG4); + if (FIELD_GET(AMS_VUSER1_MASK, regval)) + *val = AMS_SUPPLY_SCALE_6VOLT; + else + *val = AMS_SUPPLY_SCALE_3VOLT; + break; + case AMS_SUPPLY9: + regval = readl(ams->pl_base + AMS_REG_CONFIG4); + if (FIELD_GET(AMS_VUSER2_MASK, regval)) + *val = AMS_SUPPLY_SCALE_6VOLT; + else + *val = AMS_SUPPLY_SCALE_3VOLT; + break; + case AMS_SUPPLY10: + regval = readl(ams->pl_base + AMS_REG_CONFIG4); + if (FIELD_GET(AMS_VUSER3_MASK, regval)) + *val = AMS_SUPPLY_SCALE_6VOLT; + else + *val = AMS_SUPPLY_SCALE_3VOLT; + break; + case AMS_VP_VN: + case AMS_REG_VAUX(0) ... AMS_REG_VAUX(15): + *val = AMS_SUPPLY_SCALE_1VOLT; + break; + default: + *val = AMS_SUPPLY_SCALE_1VOLT; + break; + } + } else { + switch (chan->address) { + case AMS_VCC_PSPLL0: + case AMS_VCC_PSPLL3: + case AMS_VCCINT: + case AMS_VCCBRAM: + case AMS_VCCAUX: + case AMS_PSDDRPLL: + case AMS_PSINTFPDDR: + *val = AMS_SUPPLY_SCALE_3VOLT; + break; + default: + *val = AMS_SUPPLY_SCALE_1VOLT; + break; + } + } + *val2 = AMS_SUPPLY_SCALE_DIV_BIT; + return IIO_VAL_FRACTIONAL_LOG2; + case IIO_TEMP: + *val = AMS_TEMP_SCALE; + *val2 = AMS_TEMP_SCALE_DIV_BIT; + return IIO_VAL_FRACTIONAL_LOG2; + default: + return -EINVAL; + } + case IIO_CHAN_INFO_OFFSET: + /* Only the temperature channel has an offset */ + *val = AMS_TEMP_OFFSET; + return IIO_VAL_INT; + } + + return -EINVAL; +} + +static int ams_get_alarm_offset(int scan_index, enum iio_event_direction dir) +{ + int offset; + + if (scan_index >= AMS_PS_SEQ_MAX) + scan_index -= AMS_PS_SEQ_MAX; + + if (dir == IIO_EV_DIR_FALLING) { + if (scan_index < AMS_SEQ_SUPPLY7) + offset = AMS_ALARM_THRESHOLD_OFF_10; + else + offset = AMS_ALARM_THRESHOLD_OFF_20; + } else { + offset = 0; + } + + switch (scan_index) { + case AMS_SEQ_TEMP: + return AMS_ALARM_TEMP + offset; + case AMS_SEQ_SUPPLY1: + return AMS_ALARM_SUPPLY1 + offset; + case AMS_SEQ_SUPPLY2: + return AMS_ALARM_SUPPLY2 + offset; + case AMS_SEQ_SUPPLY3: + return AMS_ALARM_SUPPLY3 + offset; + case AMS_SEQ_SUPPLY4: + return AMS_ALARM_SUPPLY4 + offset; + case AMS_SEQ_SUPPLY5: + return AMS_ALARM_SUPPLY5 + offset; + case AMS_SEQ_SUPPLY6: + return AMS_ALARM_SUPPLY6 + offset; + case AMS_SEQ_SUPPLY7: + return AMS_ALARM_SUPPLY7 + offset; + case AMS_SEQ_SUPPLY8: + return AMS_ALARM_SUPPLY8 + offset; + case AMS_SEQ_SUPPLY9: + return AMS_ALARM_SUPPLY9 + offset; + case AMS_SEQ_SUPPLY10: + return AMS_ALARM_SUPPLY10 + offset; + case AMS_SEQ_VCCAMS: + return AMS_ALARM_VCCAMS + offset; + case AMS_SEQ_TEMP_REMOTE: + return AMS_ALARM_TEMP_REMOTE + offset; + default: + return 0; + } +} + +static const struct iio_chan_spec *ams_event_to_channel(struct iio_dev *dev, + u32 event) +{ + int scan_index = 0, i; + + if (event >= AMS_PL_ALARM_START) { + event -= AMS_PL_ALARM_START; + scan_index = AMS_PS_SEQ_MAX; + } + + switch (event) { + case AMS_ALARM_BIT_TEMP: + scan_index += AMS_SEQ_TEMP; + break; + case AMS_ALARM_BIT_SUPPLY1: + scan_index += AMS_SEQ_SUPPLY1; + break; + case AMS_ALARM_BIT_SUPPLY2: + scan_index += AMS_SEQ_SUPPLY2; + break; + case AMS_ALARM_BIT_SUPPLY3: + scan_index += AMS_SEQ_SUPPLY3; + break; + case AMS_ALARM_BIT_SUPPLY4: + scan_index += AMS_SEQ_SUPPLY4; + break; + case AMS_ALARM_BIT_SUPPLY5: + scan_index += AMS_SEQ_SUPPLY5; + break; + case AMS_ALARM_BIT_SUPPLY6: + scan_index += AMS_SEQ_SUPPLY6; + break; + case AMS_ALARM_BIT_SUPPLY7: + scan_index += AMS_SEQ_SUPPLY7; + break; + case AMS_ALARM_BIT_SUPPLY8: + scan_index += AMS_SEQ_SUPPLY8; + break; + case AMS_ALARM_BIT_SUPPLY9: + scan_index += AMS_SEQ_SUPPLY9; + break; + case AMS_ALARM_BIT_SUPPLY10: + scan_index += AMS_SEQ_SUPPLY10; + break; + case AMS_ALARM_BIT_VCCAMS: + scan_index += AMS_SEQ_VCCAMS; + break; + case AMS_ALARM_BIT_TEMP_REMOTE: + scan_index += AMS_SEQ_TEMP_REMOTE; + break; + } + + for (i = 0; i < dev->num_channels; i++) + if (dev->channels[i].scan_index == scan_index) + break; + + return &dev->channels[i]; +} + +static int ams_get_alarm_mask(int scan_index) +{ + int bit = 0; + + if (scan_index >= AMS_PS_SEQ_MAX) { + bit = AMS_PL_ALARM_START; + scan_index -= AMS_PS_SEQ_MAX; + } + + switch (scan_index) { + case AMS_SEQ_TEMP: + return BIT(AMS_ALARM_BIT_TEMP + bit); + case AMS_SEQ_SUPPLY1: + return BIT(AMS_ALARM_BIT_SUPPLY1 + bit); + case AMS_SEQ_SUPPLY2: + return BIT(AMS_ALARM_BIT_SUPPLY2 + bit); + case AMS_SEQ_SUPPLY3: + return BIT(AMS_ALARM_BIT_SUPPLY3 + bit); + case AMS_SEQ_SUPPLY4: + return BIT(AMS_ALARM_BIT_SUPPLY4 + bit); + case AMS_SEQ_SUPPLY5: + return BIT(AMS_ALARM_BIT_SUPPLY5 + bit); + case AMS_SEQ_SUPPLY6: + return BIT(AMS_ALARM_BIT_SUPPLY6 + bit); + case AMS_SEQ_SUPPLY7: + return BIT(AMS_ALARM_BIT_SUPPLY7 + bit); + case AMS_SEQ_SUPPLY8: + return BIT(AMS_ALARM_BIT_SUPPLY8 + bit); + case AMS_SEQ_SUPPLY9: + return BIT(AMS_ALARM_BIT_SUPPLY9 + bit); + case AMS_SEQ_SUPPLY10: + return BIT(AMS_ALARM_BIT_SUPPLY10 + bit); + case AMS_SEQ_VCCAMS: + return BIT(AMS_ALARM_BIT_VCCAMS + bit); + case AMS_SEQ_TEMP_REMOTE: + return BIT(AMS_ALARM_BIT_TEMP_REMOTE + bit); + default: + return 0; + } +} + +static int ams_read_event_config(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, + enum iio_event_type type, + enum iio_event_direction dir) +{ + struct ams *ams = iio_priv(indio_dev); + + return !!(ams->alarm_mask & ams_get_alarm_mask(chan->scan_index)); +} + +static int ams_write_event_config(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, + enum iio_event_type type, + enum iio_event_direction dir, + int state) +{ + struct ams *ams = iio_priv(indio_dev); + unsigned int alarm; + + alarm = ams_get_alarm_mask(chan->scan_index); + + mutex_lock(&ams->lock); + + if (state) + ams->alarm_mask |= alarm; + else + ams->alarm_mask &= ~alarm; + + ams_update_alarm(ams, ams->alarm_mask); + + mutex_unlock(&ams->lock); + + return 0; +} + +static int ams_read_event_value(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, + enum iio_event_type type, + enum iio_event_direction dir, + enum iio_event_info info, int *val, int *val2) +{ + struct ams *ams = iio_priv(indio_dev); + unsigned int offset = ams_get_alarm_offset(chan->scan_index, dir); + + mutex_lock(&ams->lock); + + if (chan->scan_index >= AMS_PS_SEQ_MAX) + *val = readl(ams->pl_base + offset); + else + *val = readl(ams->ps_base + offset); + + mutex_unlock(&ams->lock); + + return IIO_VAL_INT; +} + +static int ams_write_event_value(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, + enum iio_event_type type, + enum iio_event_direction dir, + enum iio_event_info info, int val, int val2) +{ + struct ams *ams = iio_priv(indio_dev); + unsigned int offset; + + mutex_lock(&ams->lock); + + /* Set temperature channel threshold to direct threshold */ + if (chan->type == IIO_TEMP) { + offset = ams_get_alarm_offset(chan->scan_index, + IIO_EV_DIR_FALLING); + + if (chan->scan_index >= AMS_PS_SEQ_MAX) + ams_pl_update_reg(ams, offset, + AMS_ALARM_THR_DIRECT_MASK, + AMS_ALARM_THR_DIRECT_MASK); + else + ams_ps_update_reg(ams, offset, + AMS_ALARM_THR_DIRECT_MASK, + AMS_ALARM_THR_DIRECT_MASK); + } + + offset = ams_get_alarm_offset(chan->scan_index, dir); + if (chan->scan_index >= AMS_PS_SEQ_MAX) + writel(val, ams->pl_base + offset); + else + writel(val, ams->ps_base + offset); + + mutex_unlock(&ams->lock); + + return 0; +} + +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)); + } +} + +static void ams_handle_events(struct iio_dev *indio_dev, unsigned long events) +{ + unsigned int bit; + + for_each_set_bit(bit, &events, AMS_NO_OF_ALARMS) + ams_handle_event(indio_dev, bit); +} + +/** + * ams_unmask_worker - ams alarm interrupt unmask worker + * @work : work to be done + * + * The ZynqMP threshold interrupts are level sensitive. Since we can't make the + * threshold condition go way from within the interrupt handler, this means as + * soon as a threshold condition is present we would enter the interrupt handler + * again and again. To work around this we mask all active threshold interrupts + * in the interrupt handler and start a timer. In this timer we poll the + * interrupt status and only if the interrupt is inactive we unmask it again. + */ +static void ams_unmask_worker(struct work_struct *work) +{ + struct ams *ams = container_of(work, struct ams, ams_unmask_work.work); + unsigned int status, unmask; + + spin_lock_irq(&ams->intr_lock); + + status = readl(ams->base + AMS_ISR_0); + + /* Clear those bits which are not active anymore */ + unmask = (ams->masked_alarm ^ status) & ams->masked_alarm; + + /* Clear status of disabled alarm */ + unmask |= ams->intr_mask; + + ams->masked_alarm &= status; + + /* Also clear those which are masked out anyway */ + ams->masked_alarm &= ~ams->intr_mask; + + /* Clear the interrupts before we unmask them */ + writel(unmask, ams->base + AMS_ISR_0); + + ams_update_intrmask(ams, ~AMS_ALARM_MASK, ~AMS_ALARM_MASK); + + spin_unlock_irq(&ams->intr_lock); + + /* If still pending some alarm re-trigger the timer */ + if (ams->masked_alarm) + schedule_delayed_work(&ams->ams_unmask_work, msecs_to_jiffies(AMS_UNMASK_TIMEOUT_MS)); +} + +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) { + spin_unlock(&ams->intr_lock); + 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, ~AMS_ALARM_MASK, ~AMS_ALARM_MASK); + + 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 const struct iio_event_spec ams_temp_events[] = { + { + .type = IIO_EV_TYPE_THRESH, + .dir = IIO_EV_DIR_RISING, + .mask_separate = BIT(IIO_EV_INFO_ENABLE) | + BIT(IIO_EV_INFO_VALUE), + }, +}; + +static const struct iio_event_spec ams_voltage_events[] = { + { + .type = IIO_EV_TYPE_THRESH, + .dir = IIO_EV_DIR_RISING, + .mask_separate = BIT(IIO_EV_INFO_VALUE), + }, + { + .type = IIO_EV_TYPE_THRESH, + .dir = IIO_EV_DIR_FALLING, + .mask_separate = BIT(IIO_EV_INFO_VALUE), + }, + { + .type = IIO_EV_TYPE_THRESH, + .dir = IIO_EV_DIR_EITHER, + .mask_separate = BIT(IIO_EV_INFO_ENABLE), + }, +}; + +static const struct iio_chan_spec ams_ps_channels[] = { + AMS_PS_CHAN_TEMP(AMS_SEQ_TEMP, AMS_TEMP), + AMS_PS_CHAN_TEMP(AMS_SEQ_TEMP_REMOTE, AMS_TEMP_REMOTE), + AMS_PS_CHAN_VOLTAGE(AMS_SEQ_SUPPLY1, AMS_SUPPLY1), + AMS_PS_CHAN_VOLTAGE(AMS_SEQ_SUPPLY2, AMS_SUPPLY2), + AMS_PS_CHAN_VOLTAGE(AMS_SEQ_SUPPLY3, AMS_SUPPLY3), + AMS_PS_CHAN_VOLTAGE(AMS_SEQ_SUPPLY4, AMS_SUPPLY4), + AMS_PS_CHAN_VOLTAGE(AMS_SEQ_SUPPLY5, AMS_SUPPLY5), + AMS_PS_CHAN_VOLTAGE(AMS_SEQ_SUPPLY6, AMS_SUPPLY6), + AMS_PS_CHAN_VOLTAGE(AMS_SEQ_SUPPLY7, AMS_SUPPLY7), + AMS_PS_CHAN_VOLTAGE(AMS_SEQ_SUPPLY8, AMS_SUPPLY8), + AMS_PS_CHAN_VOLTAGE(AMS_SEQ_SUPPLY9, AMS_SUPPLY9), + AMS_PS_CHAN_VOLTAGE(AMS_SEQ_SUPPLY10, AMS_SUPPLY10), + AMS_PS_CHAN_VOLTAGE(AMS_SEQ_VCCAMS, AMS_VCCAMS), +}; + +static const struct iio_chan_spec ams_pl_channels[] = { + AMS_PL_CHAN_TEMP(AMS_SEQ_TEMP, AMS_TEMP), + AMS_PL_CHAN_VOLTAGE(AMS_SEQ_SUPPLY1, AMS_SUPPLY1, true), + AMS_PL_CHAN_VOLTAGE(AMS_SEQ_SUPPLY2, AMS_SUPPLY2, true), + AMS_PL_CHAN_VOLTAGE(AMS_SEQ_VREFP, AMS_VREFP, false), + AMS_PL_CHAN_VOLTAGE(AMS_SEQ_VREFN, AMS_VREFN, false), + AMS_PL_CHAN_VOLTAGE(AMS_SEQ_SUPPLY3, AMS_SUPPLY3, true), + AMS_PL_CHAN_VOLTAGE(AMS_SEQ_SUPPLY4, AMS_SUPPLY4, true), + AMS_PL_CHAN_VOLTAGE(AMS_SEQ_SUPPLY5, AMS_SUPPLY5, true), + AMS_PL_CHAN_VOLTAGE(AMS_SEQ_SUPPLY6, AMS_SUPPLY6, true), + AMS_PL_CHAN_VOLTAGE(AMS_SEQ_VCCAMS, AMS_VCCAMS, true), + AMS_PL_CHAN_VOLTAGE(AMS_SEQ_VP_VN, AMS_VP_VN, false), + AMS_PL_CHAN_VOLTAGE(AMS_SEQ_SUPPLY7, AMS_SUPPLY7, true), + AMS_PL_CHAN_VOLTAGE(AMS_SEQ_SUPPLY8, AMS_SUPPLY8, true), + AMS_PL_CHAN_VOLTAGE(AMS_SEQ_SUPPLY9, AMS_SUPPLY9, true), + AMS_PL_CHAN_VOLTAGE(AMS_SEQ_SUPPLY10, AMS_SUPPLY10, true), + AMS_PL_AUX_CHAN_VOLTAGE(0), + AMS_PL_AUX_CHAN_VOLTAGE(1), + AMS_PL_AUX_CHAN_VOLTAGE(2), + AMS_PL_AUX_CHAN_VOLTAGE(3), + AMS_PL_AUX_CHAN_VOLTAGE(4), + AMS_PL_AUX_CHAN_VOLTAGE(5), + AMS_PL_AUX_CHAN_VOLTAGE(6), + AMS_PL_AUX_CHAN_VOLTAGE(7), + AMS_PL_AUX_CHAN_VOLTAGE(8), + AMS_PL_AUX_CHAN_VOLTAGE(9), + AMS_PL_AUX_CHAN_VOLTAGE(10), + AMS_PL_AUX_CHAN_VOLTAGE(11), + AMS_PL_AUX_CHAN_VOLTAGE(12), + AMS_PL_AUX_CHAN_VOLTAGE(13), + AMS_PL_AUX_CHAN_VOLTAGE(14), + AMS_PL_AUX_CHAN_VOLTAGE(15), +}; + +static const struct iio_chan_spec ams_ctrl_channels[] = { + AMS_CTRL_CHAN_VOLTAGE(AMS_SEQ_VCC_PSPLL, AMS_VCC_PSPLL0), + AMS_CTRL_CHAN_VOLTAGE(AMS_SEQ_VCC_PSBATT, AMS_VCC_PSPLL3), + AMS_CTRL_CHAN_VOLTAGE(AMS_SEQ_VCCINT, AMS_VCCINT), + AMS_CTRL_CHAN_VOLTAGE(AMS_SEQ_VCCBRAM, AMS_VCCBRAM), + AMS_CTRL_CHAN_VOLTAGE(AMS_SEQ_VCCAUX, AMS_VCCAUX), + AMS_CTRL_CHAN_VOLTAGE(AMS_SEQ_PSDDRPLL, AMS_PSDDRPLL), + AMS_CTRL_CHAN_VOLTAGE(AMS_SEQ_INTDDR, AMS_PSINTFPDDR), +}; + +static int ams_get_ext_chan(struct fwnode_handle *chan_node, + struct iio_chan_spec *channels, int num_channels) +{ + struct fwnode_handle *child; + unsigned int reg; + int ret; + + fwnode_for_each_child_node(chan_node, child) { + ret = fwnode_property_read_u32(child, "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 (fwnode_property_read_bool(child, "xlnx,bipolar")) + channels[num_channels].scan_type.sign = 's'; + + num_channels++; + } + + return num_channels; +} + +static void ams_iounmap(void *data) +{ + iounmap(data); +} + +static int ams_init_module(struct iio_dev *indio_dev, + struct fwnode_handle *fwnode, + struct iio_chan_spec *channels) +{ + struct ams *ams = iio_priv(indio_dev); + struct device *dev = indio_dev->dev.parent; + int num_channels = 0; + int ret; + + if (fwnode_property_match_string(fwnode, "compatible", + "xlnx,zynqmp-ams-ps") == 0) { + ams->ps_base = fwnode_iomap(fwnode, 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(fwnode, "compatible", + "xlnx,zynqmp-ams-pl") == 0) { + ams->pl_base = fwnode_iomap(fwnode, 0); + 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(fwnode, channels, + num_channels); + } else if (fwnode_property_match_string(fwnode, "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_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); + 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; + + + 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) + 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; +} + +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 const struct of_device_id ams_of_match_table[] = { + { .compatible = "xlnx,zynqmp-ams" }, + { } +}; +MODULE_DEVICE_TABLE(of, ams_of_match_table); + +static void ams_clk_disable_unprepare(void *data) +{ + clk_disable_unprepare(data); +} + +static void ams_cancel_delayed_work(void *data) +{ + cancel_delayed_work(data); +} + +static int ams_probe(struct platform_device *pdev) +{ + struct iio_dev *indio_dev; + struct ams *ams; + int ret; + int irq; + + 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); + + ret = clk_prepare_enable(ams->clk); + if (ret < 0) + return ret; + + ret = devm_add_action_or_reset(&pdev->dev, ams_clk_disable_unprepare, ams->clk); + if (ret < 0) + return ret; + + INIT_DELAYED_WORK(&ams->ams_unmask_work, ams_unmask_worker); + ret = devm_add_action_or_reset(&pdev->dev, ams_cancel_delayed_work, + &ams->ams_unmask_work); + if (ret < 0) + return ret; + + ret = ams_parse_firmware(indio_dev, pdev); + if (ret) { + dev_err_probe(&pdev->dev, ret, "failure in parsing DT\n"); + return ret; + } + + ret = ams_init_device(ams); + if (ret) { + dev_err_probe(&pdev->dev, ret, "failed to initialize AMS\n"); + return ret; + } + + ams_enable_channel_sequence(indio_dev); + + irq = platform_get_irq(pdev, 0); + if (irq < 0) + return ret; + + ret = devm_request_irq(&pdev->dev, irq, &ams_irq, 0, "ams-irq", + indio_dev); + if (ret < 0) { + dev_err_probe(&pdev->dev, ret, "failed to register interrupt\n"); + return ret; + } + + platform_set_drvdata(pdev, indio_dev); + + return devm_iio_device_register(&pdev->dev, indio_dev); +} + +static int __maybe_unused ams_suspend(struct device *dev) +{ + struct ams *ams = iio_priv(dev_get_drvdata(dev)); + + clk_disable_unprepare(ams->clk); + + return 0; +} + +static int __maybe_unused ams_resume(struct device *dev) +{ + struct ams *ams = iio_priv(dev_get_drvdata(dev)); + + return clk_prepare_enable(ams->clk); +} + +static SIMPLE_DEV_PM_OPS(ams_pm_ops, ams_suspend, ams_resume); + +static struct platform_driver ams_driver = { + .probe = ams_probe, + .driver = { + .name = "xilinx-ams", + .pm = &ams_pm_ops, + .of_match_table = ams_of_match_table, + }, +}; +module_platform_driver(ams_driver); + +MODULE_LICENSE("GPL v2"); +MODULE_AUTHOR("Xilinx, Inc.");