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

Andy Shevchenko
2025-09-09T10:34:25Z

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