Re: [PATCH v2 04/17] uapi: Add ARM SCMI definitions

[Cristian Marussi <cristian.marussi@arm.com>]

On Mon, Jan 19, 2026 at 11:43:43AM +0000, Jonathan Cameron wrote:
> On Wed, 14 Jan 2026 11:46:08 +0000
> Cristian Marussi <cristian.marussi@arm.com> wrote:
> 
> > Add a number of structures and ioctls definitions used by the ARM
> > SCMI Telemetry protocol.
> > 
> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> 
> A few drive by comments.

Hi,

> 
> > diff --git a/include/uapi/linux/scmi.h b/include/uapi/linux/scmi.h
> > new file mode 100644
> > index 000000000000..e4e9939a1bf8
> > --- /dev/null
> > +++ b/include/uapi/linux/scmi.h
> > @@ -0,0 +1,287 @@
> > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > +/*
> > + * Copyright (C) 2026 ARM Ltd.
> > + */
> > +#ifndef _UAPI_LINUX_SCMI_H
> > +#define _UAPI_LINUX_SCMI_H
> > +
> > +/*
> > + * Userspace interface SCMI Telemetry
> > + */
> > +
> > +#include <linux/ioctl.h>
> > +#include <linux/types.h>
> 
> 
> > +/**
> > + * scmi_tlm_intervals  - Update intervals descriptor
> > + *
> > + * @discrete: Flag to indicate the nature of the intervals described in
> > + *	      @update_intervals.
> > + *	      When 'false' @update_intervals is a triplet: min/max/step
> > + * @pad: Padding fields to enforce alignment.
> > + * @num: Number of entries of @update_intervals
> > + * @update_intervals: A variably-sized array containing the update intervals
> > + *
> > + * Used by:
> > + *	RW - SCMI_TLM_GET_INTRVS
> > + *
> > + * Supported by:
> > + *	control/
> > + *	groups/<N>/control
> > + */
> > +struct scmi_tlm_intervals {
> > +	__u8 discrete;
> > +	__u8 pad[3];
> > +	__u32 num;
> 
> Trivial but this seems a little inconsistent. In other
> 'num' entries (e.g. num_des) below a more specific name
> is used.

Yes, agreed. I will fix.

> 
> > +#define SCMI_TLM_UPDATE_INTVL_SEGMENT_LOW	0
> > +#define SCMI_TLM_UPDATE_INTVL_SEGMENT_HIGH	1
> > +#define SCMI_TLM_UPDATE_INTVL_SEGMENT_STEP	2
> > +	__u32 update_intervals[] __counted_by(num);
> > +};
> 
> > +
> > +/**
> > + * scmi_tlm_des_list  - List of all defined DEs
> > + *
> > + * @num_des: Number of entries in @des
> > + * @des: An array containing descriptors for all defined DEs
> > + *
> > + * Used by:
> > + *	RW - SCMI_TLM_GET_DE_LIST
> > + *
> > + * Supported by:
> > + *	control/
> > + */
> > +struct scmi_tlm_des_list {
> > +	__u32 num_des;
> > +	struct scmi_tlm_de_info des[] __counted_by(num_des);
> > +};
> > +
> > +/**
> > + * scmi_tlm_de_sample - A DE reading
> > + *
> > + * @id: DE identifier
> > + * @tstamp: DE reading timestamp (equal 0 is NOT supported)
> > + * @val: Reading of the DE data value
> > + *
> > + * Used by:
> > + *	RW - SCMI_TLM_GET_DE_VALUE
> > + *
> > + * Supported by:
> > + *	control/
> > + */
> > +struct scmi_tlm_de_sample {
> > +	__u32 id;
> 
> Packing issues maybe if this ever ends up on 32 bit machines.
> Even more so once it's in an array below.
>

Oh yes, I missed this...
 
> > +	__u64 tstamp;
> > +	__u64 val;
> > +};
> > +

Thanks,
Cristian