Re: [PATCH v5 2/2] iio: light: add AS7343 multi-spectral sensor driver

[Andy Shevchenko]

On Sat, Sep 19, 2026 at 04:51:45PM -0700, Chang Yu wrote:
> Add a driver for the AMS AS7343 14-channel multi-spectral sensor.
> 
> The AS7343 is a 14-channel spectral sensor featuring 11 visible
> channels, 1 near-infrared channel, 1 clear channel (VIS), and 1
> flicker detection channel.
> 
> The driver exposes 12 spectral channels (11 visible light and 1
> near-infrared) via sysfs. Runtime PM is implemented to stop
> measurements when the device is suspended or torn down. Power is
> never cut (PON=1 always) to preserve register values.
> 
> Future patches will add configurable gain and integration time,
> interrupt support, buffered reads, VIS channel, and flicker
> detection.

...

> +#include <linux/build_bug.h>

This is unusual to meet...

...

> +static inline u8 as7343_atime_steps(unsigned int step)
> +{
> +	BUILD_BUG_ON(step < 1 || step > 256);

How is this any useful? The step == 1 gives the result 0, the step 256,
255 respectively. This covers the whole range of the returned variable
(u8).

> +	return step - 1;
> +}
> +
> +static inline u16 as7343_astep_x2780ns(unsigned int n)
> +{
> +	BUILD_BUG_ON(n < 1 || n > 65535);
> +	return n - 1;
> +}

Ditto.

...

TL;DR: I do not see anything meaningful from these BUILD_BUG_ON():s.
It's not a problem if the result gives just wrapped-up value. Or is it?

...

> +static int as7343_read_label(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan, char *label)
> +{
> +	int channel = chan->channel;

Why signed?
Also split assignment, it will make code more maintainable.

> +	if (channel < 0 || channel >= ARRAY_SIZE(as7343_channel_labels))
> +		return -EINVAL;
> +
> +	return sysfs_emit(label, "%s\n", as7343_channel_labels[channel]);
> +}

-- 
With Best Regards,
Andy Shevchenko