Re: [PATCH v1 5/8] iio: dac: ds4424: add Rfs-based scale and per-variant limits

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Mon, Jan 19, 2026 at 07:24:21PM +0100, Oleksij Rempel wrote:
> Parse optional maxim,rfs-ohms values to derive the per-channel output
> current scale (mA per step) for the IIO current ABI.
> 
> Select per-variant parameters to match the shared register map while
> handling different data widths and full-scale current calculations.
> 
> Behavior changes:
> - If maxim,rfs-ohms is present, IIO_CHAN_INFO_SCALE becomes available
>   and reports mA/step derived from Rfs.
> - If maxim,rfs-ohms is missing, SCALE is not exposed to keep older DTs
>   working without requiring updates.
> - RAW writes are now limited to the representable sign-magnitude range
>   of the detected variant to avoid silent truncation (e.g. +/-31 on
>   DS440x).

...

> +struct ds4424_chip_info {
> +	u8 result_mask;
> +	int vref_mv;
> +	int scale_denom;

Wondering if `pahole` is fine with the proposed layout.
Otherwise I would move u8 to the end.

> +};

...

> +static const struct ds4424_chip_info ds4424_info = {
> +	.result_mask = 0x7F,

GENMASK() ?
(Note, we also have GENMASK_U8() IIRC)

> +	.vref_mv = 976,
> +	.scale_denom = 16,
> +};

...

> +/* DS4402 is handled like DS4404 (same resolution and scale formula). */
> +static const struct ds4424_chip_info ds4404_info = {
> +	.result_mask = 0x1F,

Ditto.

> +	.vref_mv = 1230,
> +	.scale_denom = 4,
> +};

...

> -		if (val <= S8_MIN || val > S8_MAX)
> +		/*
> +		 * The hardware uses sign-magnitude representation (not
> +		 * two's complement). Therefore, the range is symmetric:
> +		 * [-max_val, +max_val].
> +		 */
> +		if (val < -max_val || val > max_val)
>  			return -EINVAL;

Right, cool, but I still think even in the fix it would be good to fix that and
here it will be quite logical and clear change. Maybe even introduce a comment
in the fix-patch and just update limits here.

...

> +static int ds4424_setup_channels(struct i2c_client *client,
> +				 struct ds4424_data *data,
> +				 struct iio_dev *indio_dev)
> +{
> +	struct iio_chan_spec channels[DS4424_MAX_DAC_CHANNELS];
> +	size_t channels_size;
> +	int i;
> +
> +	channels_size = indio_dev->num_channels * sizeof(*channels);

Sounds like devm_kmemdup_array() to me...

> +	memcpy(channels, ds4424_channels, channels_size);

> +	/* Enable scale only when rfs is available. */
> +	if (data->has_rfs) {
> +		for (i = 0; i < indio_dev->num_channels; i++)
> +			channels[i].info_mask_separate |=
> +				BIT(IIO_CHAN_INFO_SCALE);
> +	}

Can be done after kmemdup(), right?

> +	indio_dev->channels = devm_kmemdup(&client->dev, channels,
> +					   channels_size, GFP_KERNEL);
> +	if (!indio_dev->channels)
> +		return -ENOMEM;
> +
> +	return 0;
> +}

...

> +static int ds4424_parse_rfs(struct i2c_client *client,
> +			    struct ds4424_data *data,
> +			    struct iio_dev *indio_dev)
> +{

	struct device *dev = &client->dev;

> +	int count, i, ret;
> +
> +	if (!device_property_present(&client->dev, "maxim,rfs-ohms")) {
> +		dev_info_once(&client->dev, "maxim,rfs-ohms missing, scale not supported\n");
> +		return 0;
> +	}
> +
> +	count = device_property_count_u32(&client->dev, "maxim,rfs-ohms");
> +	if (count != indio_dev->num_channels) {
> +		dev_err(&client->dev,
> +			"maxim,rfs-ohms must have %u entries\n",
> +			indio_dev->num_channels);
> +		return -EINVAL;
> +	}
> +
> +	ret = device_property_read_u32_array(&client->dev,
> +					     "maxim,rfs-ohms",
> +					     data->rfs_ohms,
> +					     indio_dev->num_channels);
> +	if (ret) {
> +		dev_err(&client->dev,
> +			"Failed to read maxim,rfs-ohms property\n");
> +		return ret;
> +	}
> +
> +	for (i = 0; i < indio_dev->num_channels; i++) {
> +		if (!data->rfs_ohms[i]) {

> +			dev_err(&client->dev,
> +				"maxim,rfs-ohms entry %d is zero\n",
> +				i);

> +			return -EINVAL;

This is only for probe stage, right?

Then

			return dev_err_probe();

?

> +		}
> +	}
> +
> +	data->has_rfs = true;
> +	return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko