Re: [PATCH v10 3/5] iio: adc: Add Xilinx AMS driver

[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