Re: [PATCH 2/6] iio: versal-sysmon: add driver for Versal Sysmon

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Fri, Sep 5, 2025 at 11:42 AM Michal Simek <michal.simek@amd.com> wrote:
>
> Sysmon Driver uses Linux IIO framework, which was used to abstract the
> supply voltages and temperatures across the chip as Voltage and Temperature
> Channels in the framework. Since there are only 160 supply voltage
> registers and 184 measurement points, there is no constant mapping of
> supply voltage registers and the measurement points. User has to select
> the voltages to monitor in design tool. Depending on the selection, a

in the design

> voltage supply gets mapped to one of the supply registers. So, this mapping
> information is provided to the driver via a device tree. Depending on the
> number of supplies enabled in the design, the device tree will contain the
> information of name of the supply enabled and the supply register it maps
> to.

...

> +config VERSAL_SYSMON
> +       tristate "Xilinx Sysmon driver for Versal"
> +       depends on HAS_IOMEM
> +       select VERSAL_SYSMON_CORE
> +       help
> +         Say yes here to have support for the Xilinx Sysmon.
> +         The driver will enable users to monitor temperature and voltage on the
> +         Xilinx Versal platform.
> +
> +         The driver can also be build as a module. If so, the module will be called

built

> +         versal-sysmon.

...

> +#include <linux/bits.h>

Quite a few headers are absent...

+ export.h
+ types.h

> +#include "versal-sysmon.h"

...

> +static u32 sysmon_temp_offset(int address)
> +{
> +       switch (address) {
> +       case TEMP_MAX:
> +               return SYSMON_TEMP_MAX;
> +       case TEMP_MIN:
> +               return SYSMON_TEMP_MIN;
> +       case TEMP_MAX_MAX:
> +               return SYSMON_TEMP_MAX_MAX;
> +       case TEMP_MIN_MIN:
> +               return SYSMON_TEMP_MIN_MIN;
> +       case TEMP_HBM:
> +               return SYSMON_TEMP_HBM;
> +       default:
> +               return -EINVAL;
> +       }

> +       return -EINVAL;

Here and in many more, eliminate dead code.

> +}

...

> +static u32 sysmon_supply_thresh_offset(int address,
> +                                      enum iio_event_direction dir)
> +{
> +       if (dir == IIO_EV_DIR_RISING)
> +               return (address * 4) + SYSMON_SUPPLY_TH_UP;

> +       else if (dir == IIO_EV_DIR_FALLING)
> +               return (address * 4) + SYSMON_SUPPLY_TH_LOW;

Redundant 'else'.

> +       return -EINVAL;
> +}

...

> +static void sysmon_hbm_to_millicelsius(int raw_data, int *val, int *val2)
> +{
> +       *val = ((raw_data >> SYSMON_HBM_TEMP_SHIFT) & SYSMON_HBM_TEMP_MASK) *
> +               SYSMON_MILLI_SCALE;

Can the whole driver be switched to FIELD_PREP()/FIELD_GET()/FIELD_MODIFY()?

> +       *val2 = 0;
> +}

...

> +static void sysmon_millicelsius_to_q8p7(u32 *raw_data, int val, int val2)
> +{

> +       (void)val2;

Unneeded.

> +       *raw_data = (val << SYSMON_FRACTIONAL_SHIFT) / SYSMON_MILLI_SCALE;
> +}

...

> +static void sysmon_supply_processedtoraw(int val, int val2, u32 reg_val,
> +                                        u32 *raw_data)
> +{
> +       int exponent = (reg_val & SYSMON_MODE_MASK) >> SYSMON_MODE_SHIFT;
> +       int format = (reg_val & SYSMON_FMT_MASK) >> SYSMON_FMT_SHIFT;
> +       int scale = 1 << (16 - exponent);
> +       int tmp;
> +
> +       tmp = (val * scale) / SYSMON_MILLI_SCALE;
> +
> +       /* Set out of bound values to saturation levels */
> +       if (format) {
> +               if (tmp > SYSMON_UPPER_SATURATION_SIGNED)
> +                       tmp = 0x7fff;
> +               else if (tmp < SYSMON_LOWER_SATURATION_SIGNED)
> +                       tmp = 0x8000;

This looks like s16 limits, can you use them?

> +       } else {
> +               if (tmp > SYSMON_UPPER_SATURATION)
> +                       tmp = 0xffff;
> +               else if (tmp < SYSMON_LOWER_SATURATION)
> +                       tmp = 0x0000;

u16 respectively.

> +       }
> +
> +       *raw_data = tmp & 0xffff;
> +}


> +       u32 ret = -EINVAL;

> +       mutex_lock(&sysmon->mutex);
> +       switch (mask) {
> +       case IIO_CHAN_INFO_RAW:
> +               switch (chan->type) {
> +               case IIO_TEMP:
> +                       offset = sysmon_temp_offset(chan->address);
> +                       *val = sysmon->temp_read(sysmon, offset);
> +                       *val2 = 0;
> +                       ret = IIO_VAL_INT;
> +                       break;
> +
> +               case IIO_VOLTAGE:
> +                       offset = sysmon_supply_offset(chan->address);
> +                       sysmon_read_reg(sysmon, offset, &regval);
> +                       *val = (int)regval;
> +                       *val2 = 0;
> +                       ret = IIO_VAL_INT;
> +                       break;
> +
> +               default:
> +                       break;
> +               }> +
> +       spin_unlock_irqrestore(&sysmon->lock, flags);
> +       mutex_unlock(&sysmon->mutex);
> +
> +       return 0;
> +}

...

> +#include <linux/bits.h>

+ io.h

> +#include <linux/moduleparam.h>
> +#include <linux/firmware/xlnx-zynqmp.h>
> +#include "versal-sysmon.h"
> +
> +static LIST_HEAD(sysmon_list_head);

list.h?

> +static struct iio_map sysmon_therm_static_maps[] = {
> +       IIO_MAP("temp", "versal-thermal", "sysmon-temp-channel"),

Where are the IIO_MAP() and struct iio_map defined?

> +       {}
> +};
> +
> +static inline int sysmon_direct_read_reg(struct sysmon *sysmon, u32 offset, u32 *data)

+ types.h for uXX.

> +{
> +       *data = readl(sysmon->base + offset);
> +
> +       return 0;
> +}

...

> +       mutex_init(&sysmon->mutex);

devm?

...

> +       indio_dev->dev.parent = &pdev->dev;
> +       indio_dev->dev.of_node = pdev->dev.of_node;

Drop these, IIO core does the same and even better.

...

> +       mutex_lock(&sysmon->mutex);

scoped_guard() from cleanup.h

> +       if (list_empty(&sysmon_list_head)) {
> +               sysmon->master_slr = true;

No need, just make a sane default and !exist will work in the other
branch the same way. I believe it's

  bool exist = false;

> +       } else {
> +               list_for_each_entry(temp_sysmon, &sysmon_list_head, list) {
> +                       if (temp_sysmon->master_slr)
> +                               exist = true;

Why to continue?

> +               }
> +               sysmon->master_slr = !exist;
> +       }
> +
> +       mutex_unlock(&sysmon->mutex);

...

> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/driver.h>
> +#include <linux/iio/events.h>
> +#include <linux/iio/machine.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/adc/versal-sysmon-events.h>
> +#include <linux/iopoll.h>

> +#include <linux/kernel.h>

Mustn't be used in new drivers.

> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/of_address.h>

Huh?! Please, make sure you are following IWYU principle, please
*drop* all of the headers that are *not* being used (by this header),
there are a LOT of them to be dropped.

...

> +/* Channel IDs for Temp Channels */
> +/* TEMP_MAX gives the current temperature for Production
> + * silicon.
> + * TEMP_MAX gives the current maximum temperature for ES1
> + * silicon.
> + */
> +#define TEMP_MAX       160

Here and everywhere else, bad namings.

...

This is an unfinished review due to the following reasons (from more
important to less):
- AMD is not the first day contributor, the code is awful, it missed a
lot of reviews and updates regarding the last several years of the
kernel development (IIO subsystem in particular). Please, avoid
letting reviewers do *your* job.
- The patches are too long to review, it would be nicer to make them
feature-by-feature, where each one is < 1000 LoCs.
- I started this a few days ago, but I'm busy at the moment, that's
another reason.

--
With Best Regards,
Andy Shevchenko