RE: [PATCH v7 2/4] iio: adc: Add Xilinx AMS driver

[Anand Ashok Dumbre]

Hi Andy,

Thanks for the review.

> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> Sent: Monday 25 October 2021 11:11 AM
> To: Anand Ashok Dumbre <ANANDASH@xxxxxxxxxx>
> Cc: Linux Kernel Mailing List <linux-kernel@xxxxxxxxxxxxxxx>; Jonathan
> Cameron <jic23@xxxxxxxxxx>; Lars-Peter Clausen <lars@xxxxxxxxxx>; linux-
> iio <linux-iio@xxxxxxxxxxxxxxx>; git <git@xxxxxxxxxx>; Michal Simek
> <michals@xxxxxxxxxx>; Peter Meerwald <pmeerw@xxxxxxxxxx>;
> devicetree <devicetree@xxxxxxxxxxxxxxx>; Manish Narani
> <MNARANI@xxxxxxxxxx>
> Subject: Re: [PATCH v7 2/4] iio: adc: Add Xilinx AMS driver
> 
> On Tue, Oct 19, 2021 at 6:22 PM Anand Ashok Dumbre
> <anand.ashok.dumbre@xxxxxxxxxx> 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 external master. Out of these interface currently only DRP is
> 
> from an external
> interfaces

Will fix the grammar.

> 
> > 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.
> >
> > Signed-off-by: Manish Narani <manish.narani@xxxxxxxxxx>
> 
> What does this SoB mean here? Have you read Submitting Patches?

Will add the co-developed by tag here.

> 
> > Signed-off-by: Anand Ashok Dumbre <anand.ashok.dumbre@xxxxxxxxxx>
> 
> ...
> 
> > +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.
> 
> It's not important for most of the users. Please, strat help with more useful
> information like below.

Will do.

> 
> > +         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.
> 
> ...
> 
> > + *  Manish Narani <mnarani@xxxxxxxxxx>
> 
> A-ha! You probably forgot the Co-developed-by tag above.

Correct will add.

> 
> > + *  Rajnikant Bhojani <rajnikant.bhojani@xxxxxxxxxx>
> 
> ...
> 
> Missed headers, like bits.h.

Will add.

> 
> > +#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/of_address.h>
> 
> Do you need this? Maybe mod_devicetable.h?
> 
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> 
> ...
> 
> > +static const unsigned int AMS_UNMASK_TIMEOUT_MS = 500;
> 
> Why not define (esp. taking into account another similar define below?
> 

Makes sense

> ...
> 
> > +#define AMS_REGCFG1_ALARM_MASK         0xF0F
> > +#define AMS_REGCFG3_ALARM_MASK         0x3F
> 
> > +#define AMS_PL_ALARM_MASK              0xFFFF0000U
> > +#define AMS_ISR0_ALARM_MASK            0xFFFFFFFFU
> > +#define AMS_ISR1_ALARM_MASK            0xE000001FU
> > +#define AMS_ISR1_EOC_MASK              0x00000008U
> 
> What is so special about these that they are not using combinations of
> GENMASK() / BIT()?
> 
Will add for those.

> ...
> 
> > +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
> 
> Is it terminator line? Doesn't sound like it to me. So, please add a comma.
> Same for the rest.
> 
> > +};
> 
> ...
> 
> > +       AMS_SEQ_MAX
> 
> This seems correct, no comma is needed :-)
> 
> ...
> 
> > +struct ams {
> > +       void __iomem *base;
> > +       void __iomem *ps_base;
> > +       void __iomem *pl_base;
> > +       struct clk *clk;
> > +       struct device *dev;
> 
> > +       /* check kernel doc above */
> 
> Useless
> 
> > +       struct mutex lock;
> 
> > +       /* check kernel doc above */
> 
> Ditto.
> 	

Will remove the comments

> > +       spinlock_t intr_lock;
> > +       unsigned int alarm_mask;
> > +       unsigned int masked_alarm;
> > +       u64 intr_mask;
> > +       int irq;
> > +       struct delayed_work ams_unmask_work; };
> 
> ...
> 
> > +       writel((val & ~mask) | (data & mask), ams->ps_base + offset);
> 
> Split to assignment and simple writel() call. Same to the rest.
> 
Will do.

> ...
> 
> > +       ams->intr_mask &= ~mask;
> > +       ams->intr_mask |= (val & mask);
> 
> This may be combined to one line as it's a standard pattern.
> 
Will do.

> ...
> 
> > +       if (ams->ps_base) {
> > +               /* Configuring PS alarm enable */
> > +               cfg = ~((alarm_mask & AMS_ISR0_ALARM_2_TO_0_MASK) <<
> > +                              AMS_CONF1_ALARM_2_TO_0_SHIFT);
> > +               cfg &= ~((alarm_mask & AMS_ISR0_ALARM_6_TO_3_MASK) <<
> > +                               AMS_CONF1_ALARM_6_TO_3_SHIFT);
> > +               ams_ps_update_reg(ams, AMS_REG_CONFIG1,
> AMS_REGCFG1_ALARM_MASK,
> > +                                 cfg);
> > +
> > +               cfg = ~((alarm_mask >> AMS_CONF3_ALARM_12_TO_7_SHIFT) &
> > +                               AMS_ISR0_ALARM_12_TO_7_MASK);
> > +               ams_ps_update_reg(ams, AMS_REG_CONFIG3,
> AMS_REGCFG3_ALARM_MASK,
> > +                                 cfg);
> > +       }
> 
> By factoring out the body of the conditional to a helper function you may:
> - decrease indentation
> - make code better to read
> - reduce LOCs

Will do.

> 
> > +       if (ams->pl_base) {
> > +               pl_alarm_mask = (alarm_mask >> AMS_PL_ALARM_START);
> > +               pl_alarm_mask = FIELD_GET(AMS_PL_ALARM_MASK,
> alarm_mask);
> > +               /* Configuring PL alarm enable */
> > +               cfg = ~((pl_alarm_mask & AMS_ISR0_ALARM_2_TO_0_MASK) <<
> > +                       AMS_CONF1_ALARM_2_TO_0_SHIFT);
> > +               cfg &= ~((pl_alarm_mask & AMS_ISR0_ALARM_6_TO_3_MASK)
> <<
> > +                        AMS_CONF1_ALARM_6_TO_3_SHIFT);
> > +               ams_pl_update_reg(ams, AMS_REG_CONFIG1,
> > +                                 AMS_REGCFG1_ALARM_MASK, cfg);
> > +
> > +               cfg = ~((pl_alarm_mask >> AMS_CONF3_ALARM_12_TO_7_SHIFT)
> &
> > +                       AMS_ISR0_ALARM_12_TO_7_MASK);
> > +               ams_pl_update_reg(ams, AMS_REG_CONFIG3,
> > +                                 AMS_REGCFG3_ALARM_MASK, cfg);
> > +       }
> 
> Ditto. And the same applies to all the rest where it gains something from the
> above list of improvements.
> 
Will do.

> ...
> 
> > +       int i;
> > +       unsigned long long scan_mask;
> > +       struct ams *ams = iio_priv(indio_dev);
> 
> Reversed xmas tree order, please.
> Same for the rest.
> 
Will do.

> ...
> 
> > +       /* Run calibration of PS & PL as part of the sequence */
> > +       scan_mask = 0x1 | BIT(AMS_PS_SEQ_MAX);
> 
> BIT(0) ?
> 

Will fix.
> ...
> 
> > +       ams_update_intrmask(ams, ~0, ~0);
> 
> Replace ~0 to proper GENMASK()./BIT() combination which takes into
> account real bits used by hardware.
> 
Will fix.

> ...
> 
> > +       case IIO_CHAN_INFO_RAW:
> > +               mutex_lock(&ams->lock);
> > +               if (chan->scan_index >= (AMS_PS_SEQ_MAX * 3)) {
> 
> Too many parens.
> 
> > +                       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;
> 
> ...
> 
> > +       return -EINVAL;
> 
> Use corresponding defaul cases in each of the switches.
> 

Will do.

> ...
> 
> > +       int offset = 0;
> 
> Make the assignment as an else branch, so all offset assignments will be
> grouped together.
> 
> > +       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;
> > +       }
> 
> ...
> 
> > +       return 0;
> 
> default case.

Will do.

> 
> > +}
> 
> ...
> 
> > +static const struct iio_chan_spec
> > +*ams_event_to_channel(struct iio_dev *indio_dev, u32 event)
> 
> Unusual indentation.

Will fix.

> 
> ...
> 
> > +       case AMS_ALARM_BIT_TEMP_REMOTE:
> > +               scan_index += AMS_SEQ_TEMP_REMOTE;
> > +               break;
> 
> default?
> Same for the rest of the cases like this.

Will add.

> 
> ...
> 
> > +       return (ams->alarm_mask &
> > + ams_get_alarm_mask(chan->scan_index)) ? 1 : 0;
> 
> !! would work as well.
> 
> ...
> 
> > +               /*
> > +                * The temperature channel only supports over-temperature
> > +                * events
> 
> Missed period.
> 
> > +                */
> 
> ...
> 
> > +       /* only process alarms that are not masked */
> 
> Inconsistent style (here capitalization is missed). Make all comments in the
> code consistent.
> 
> > +       isr0 &= ~((ams->intr_mask & AMS_ISR0_ALARM_MASK) |
> > + ams->masked_alarm);
> 
> > +
> 
> Redundant blank line.
> 
> > +       if (!isr0)
> 
> How did you test this branch? (Hint: something very important should be
> done here)
Missing spin_unlock.

> 
> > +               return IRQ_NONE;
> 
> ...
> 
> > +       for_each_child_of_node(chan_node, child) {
> > +               ret = of_property_read_u32(child, "reg", &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 (of_property_read_bool(child, "xlnx,bipolar"))
> > +                       channels[num_channels].scan_type.sign = 's';
> > +
> > +               num_channels++;
> > +       }
> 
> Use device property API here instead of *of_*() calls.


> 
> ...
> 
> > +       /* Initialize buffer for channel specification */
> > +       ams_channels = kzalloc(sizeof(ams_ps_channels) +
> > +                              sizeof(ams_pl_channels) +
> > +                              sizeof(ams_ctrl_channels), GFP_KERNEL);
> 
> Use the corresponding macro from overflow.h.
> 
> > +       if (!ams_channels)
> > +               return -ENOMEM;
> 
> ...
> 
> > +       if (of_device_is_available(np)) {
> 
> fwnode_device_is_available()

Currently acpi is not supported with this driver. But I will add support in the next series of patches.
I don’t have a full understanding of ACPI and its interfaces. So would it be okay once the first iteration
gets checked in, I will add ACPI support on top.

> 
> > +               ret = ams_init_module(indio_dev, np, ams_channels);
> > +               if (ret < 0)
> > +                       goto err;
> > +
> > +               num_channels += ret;
> > +       }
> 
> ...
> 
> > +       for_each_child_of_node(np, child_node) {
> > +               if (of_device_is_available(child_node)) {
> > +                       ret = ams_init_module(indio_dev, child_node,
> > +                                             ams_channels + num_channels);
> > +                       if (ret < 0)
> > +                               goto err;
> > +
> > +                       num_channels += ret;
> > +               }
> > +       }
> 
> As per above.
> 
> ...
> 
> > +       if (!pdev->dev.of_node)
> > +               return -ENODEV;
> 
> Drop this, please. It will allow reuse of the driver in ACPI environments.
> 
> ...
> 
> > +       ams->irq = platform_get_irq(pdev, 0);
> > +       if (ams->irq == -EPROBE_DEFER) {
> 
> Is IRQ optional or not?

Its not. I will add a generic handling.

> 
> > +               ret = -EPROBE_DEFER;
> > +               return ret;
> > +       }
> 
> ...
> 
> > +       ret = devm_iio_device_register(&pdev->dev, indio_dev);
> > +
> > +       return ret;
> 
> return devm_...

Will do.

> 
> ...
> 
> > +       clk_prepare_enable(ams->clk);
> 
> It might fail.

Will add return

> 
> 
> --
> With Best Regards,
> Andy Shevchenko