Messages in this thread | | | Date | Thu, 18 Nov 2021 09:07:30 +0100 | Subject | Re: [PATCH v10 3/5] iio: adc: Add Xilinx AMS driver | From | Michal Simek <> |
| |
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
| |