Hi Andy,

Thanks for the review.

-----Original Message-----
From: Andy Shevchenko <redacted>
Sent: Monday 25 October 2021 11:11 AM
To: Anand Ashok Dumbre <redacted>
Cc: Linux Kernel Mailing List <redacted>; Jonathan
Cameron [off-list ref]; Lars-Peter Clausen [off-list ref]; linux-
iio [off-list ref]; git [off-list ref]; Michal Simek
[off-list ref]; Peter Meerwald [off-list ref];
devicetree [off-list ref]; Manish Narani
[off-list ref]
Subject: Re: [PATCH v7 2/4] iio: adc: Add Xilinx AMS driver

On Tue, Oct 19, 2021 at 6:22 PM Anand Ashok Dumbre
[off-list ref] wrote:

quoted

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.

quoted

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.

quoted

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 <redacted>

What does this SoB mean here? Have you read Submitting Patches?

Will add the co-developed by tag here.

quoted

Signed-off-by: Anand Ashok Dumbre <redacted>

...

quoted

+config XILINX_AMS
+       tristate "Xilinx AMS driver"
+       depends on ARCH_ZYNQMP || COMPILE_TEST
+       depends on HAS_IOMEM
+       help

quoted

+         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.

quoted

+         The driver supports Voltage and Temperature monitoring on Xilinx

Ultrascale

quoted

+         devices.
+
+         The driver can also be built as a module. If so, the module will be

called

quoted

+         xilinx-ams.

...

quoted

+ *  Manish Narani [off-list ref]

A-ha! You probably forgot the Co-developed-by tag above.

Correct will add.

quoted

+ *  Rajnikant Bhojani [off-list ref]

...

Missed headers, like bits.h.

Will add.

quoted

+#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>

quoted

+#include <linux/of_address.h>

Do you need this? Maybe mod_devicetable.h?

quoted

+#include <linux/platform_device.h>
+#include <linux/slab.h>

...

quoted

+static const unsigned int AMS_UNMASK_TIMEOUT_MS = 500;

Why not define (esp. taking into account another similar define below?

Makes sense

...

quoted

+#define AMS_REGCFG1_ALARM_MASK         0xF0F
+#define AMS_REGCFG3_ALARM_MASK         0x3F

quoted

+#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.

...

quoted

+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.

quoted

+};

...

quoted

+       AMS_SEQ_MAX

This seems correct, no comma is needed :-)

...

quoted

+struct ams {
+       void __iomem *base;
+       void __iomem *ps_base;
+       void __iomem *pl_base;
+       struct clk *clk;
+       struct device *dev;

quoted

+       /* check kernel doc above */

Useless

quoted

+       struct mutex lock;

quoted

+       /* check kernel doc above */

Ditto.
	

Will remove the comments

quoted

+       spinlock_t intr_lock;
+       unsigned int alarm_mask;
+       unsigned int masked_alarm;
+       u64 intr_mask;
+       int irq;
+       struct delayed_work ams_unmask_work; };

...

quoted

+       writel((val & ~mask) | (data & mask), ams->ps_base + offset);

Split to assignment and simple writel() call. Same to the rest.

Will do.

...

quoted

+       ams->intr_mask &= ~mask;
+       ams->intr_mask |= (val & mask);

This may be combined to one line as it's a standard pattern.

Will do.

...

quoted

+       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,

quoted

+                                 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,

quoted

+                                 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.

quoted

+       if (ams->pl_base) {
+               pl_alarm_mask = (alarm_mask >> AMS_PL_ALARM_START);
+               pl_alarm_mask = FIELD_GET(AMS_PL_ALARM_MASK,

alarm_mask);

quoted

+               /* 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)

<<

quoted

+                        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)

&

quoted

+                       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.

...

quoted

+       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.

...

quoted

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

BIT(0) ?

Will fix.

...

quoted

+       ams_update_intrmask(ams, ~0, ~0);

Replace ~0 to proper GENMASK()./BIT() combination which takes into
account real bits used by hardware.

Will fix.

...

quoted

+       case IIO_CHAN_INFO_RAW:
+               mutex_lock(&ams->lock);
+               if (chan->scan_index >= (AMS_PS_SEQ_MAX * 3)) {

Too many parens.

quoted

+                       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;

...

quoted

+       return -EINVAL;

Use corresponding defaul cases in each of the switches.

Will do.

...

quoted

+       int offset = 0;

Make the assignment as an else branch, so all offset assignments will be
grouped together.

quoted

+       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;
+       }

...

quoted

+       return 0;

default case.

Will do.

quoted

+}

...

quoted

+static const struct iio_chan_spec
+*ams_event_to_channel(struct iio_dev *indio_dev, u32 event)

Unusual indentation.

Will fix.

...

quoted

+       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.

...

quoted

+       return (ams->alarm_mask &
+ ams_get_alarm_mask(chan->scan_index)) ? 1 : 0;

!! would work as well.

...

quoted

+               /*
+                * The temperature channel only supports over-temperature
+                * events

Missed period.

quoted

+                */

...

quoted

+       /* only process alarms that are not masked */

Inconsistent style (here capitalization is missed). Make all comments in the
code consistent.

quoted

+       isr0 &= ~((ams->intr_mask & AMS_ISR0_ALARM_MASK) |
+ ams->masked_alarm);

quoted

+

Redundant blank line.

quoted

+       if (!isr0)

How did you test this branch? (Hint: something very important should be
done here)

Missing spin_unlock.

quoted

+               return IRQ_NONE;

...

quoted

+       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.

...

quoted

+       /* 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.

quoted

+       if (!ams_channels)
+               return -ENOMEM;

...

quoted

+       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.

quoted

+               ret = ams_init_module(indio_dev, np, ams_channels);
+               if (ret < 0)
+                       goto err;
+
+               num_channels += ret;
+       }

...

quoted

+       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.

...

quoted

+       if (!pdev->dev.of_node)
+               return -ENODEV;

Drop this, please. It will allow reuse of the driver in ACPI environments.

...

quoted

+       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.

quoted

+               ret = -EPROBE_DEFER;
+               return ret;
+       }

...

quoted

+       ret = devm_iio_device_register(&pdev->dev, indio_dev);
+
+       return ret;

return devm_...

Will do.

...

quoted

+       clk_prepare_enable(ams->clk);

It might fail.

Will add return

--
With Best Regards,
Andy Shevchenko