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", ®);
>
> > + 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
