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

[Anand Ashok Dumbre]

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@xxxxxxxxxxxxxxx>
> Sent: Wednesday 17 November 2021 8:03 PM
> To: Anand Ashok Dumbre <ANANDASH@xxxxxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx; jic23@xxxxxxxxxx; lars@xxxxxxxxxx; linux-
> iio@xxxxxxxxxxxxxxx; git <git@xxxxxxxxxx>; Michal Simek
> <michals@xxxxxxxxxx>; gregkh@xxxxxxxxxxxxxxxxxxx; rafael@xxxxxxxxxx;
> linux-acpi@xxxxxxxxxxxxxxx; heikki.krogerus@xxxxxxxxxxxxxxx; Manish Narani
> <MNARANI@xxxxxxxxxx>
> 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", &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