linux-kernel - Re: [PATCH v2 5/6] staging: iio: ad9832: convert to iio channels and ext

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20251231182158.2cc7d4da@jic23-huawei>
Date: Wed, 31 Dec 2025 18:21:58 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Tomas Borquez <tomasborquez13@...il.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Lars-Peter Clausen
 <lars@...afoo.de>, Michael Hennerich <Michael.Hennerich@...log.com>, David
 Lechner <dlechner@...libre.com>, Nuno Sá
 <nuno.sa@...log.com>, Andy Shevchenko <andy@...nel.org>,
 linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org,
 linux-staging@...ts.linux.dev
Subject: Re: [PATCH v2 5/6] staging: iio: ad9832: convert to iio channels
 and ext_info attrs

On Tue, 30 Dec 2025 17:34:58 -0300
Tomas Borquez <tomasborquez13@...il.com> wrote:

> Convert ad9832 from sysfs attributes to standard channel interface
> using a single IIO_ALTCURRENT channel with ext_info attributes, as this
> device is a current source DAC with one output.
> 
> Signed-off-by: Tomas Borquez <tomasborquez13@...il.com>

A couple of things inline.


> +
> @@ -230,42 +321,111 @@ static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr,

> +	case AD9832_PINCTRL_EN:
> +		if (val != 1 && val != 0)
> +			return -EINVAL;
> +
> +		st->ctrl_ss &= ~AD9832_SELSRC;
> +		st->ctrl_ss |= FIELD_PREP(AD9832_SELSRC, !val);
It's not particularly important as this pattern is common, but there is
FIELD_MODIFY() available to make the above a single thing without
needing the mask twice

> +
> +		st->data = cpu_to_be16(FIELD_PREP(AD9832_CMD_MSK, AD9832_CMD_SYNCSELSRC) |
> +						 st->ctrl_ss);
> +		ret = spi_sync(st->spi, &st->msg);
> +		if (ret)
> +			return ret;
> +
> +		st->pinctrl_en = val;
> +		break;
>  	default:
>  		return -ENODEV;
>  	}
> +
> +	return len;
>  }
>
> -static IIO_DEVICE_ATTR(out_altvoltage0_phasesymbol, 0200, NULL, ad9832_write, AD9832_PHASE_SYM);
> -static IIO_CONST_ATTR(out_altvoltage0_phase_scale, "0.0015339808"); /* 2PI/2^12 rad */
> +static const struct iio_chan_spec_ext_info ad9832_ext_info[] = {
> +	AD9832_CHAN_FREQ("frequency0", 0),
> +	AD9832_CHAN_FREQ("frequency1", 1),
> +	AD9832_CHAN_PHASE("phase0", 0),
> +	AD9832_CHAN_PHASE("phase1", 1),
> +	AD9832_CHAN_PHASE("phase2", 2),
> +	AD9832_CHAN_PHASE("phase3", 3),
> +	{ }
> +};
>  
> -static IIO_DEVICE_ATTR(out_altvoltage0_pincontrol_en, 0200, NULL, ad9832_write, AD9832_PINCTRL_EN);
> -static IIO_DEVICE_ATTR(out_altvoltage0_out_enable, 0200, NULL, ad9832_write, AD9832_OUTPUT_EN);
> +static const struct iio_chan_spec ad9832_channels[] = {
> +	{
> +		.type = IIO_ALTCURRENT,
> +		.output = 1,
> +		.indexed = 1,
> +		.channel = 0,
> +		.ext_info = ad9832_ext_info,
> +	},
> +};
> +
> +static IIO_DEVICE_ATTR(out_altcurrent0_frequency_symbol, 0644,
> +		       ad9832_show, ad9832_store, AD9832_FREQ_SYM);
> +static IIO_DEVICE_ATTR(out_altcurrent0_phase_symbol, 0644,
> +		       ad9832_show, ad9832_store, AD9832_PHASE_SYM);
Why not do these two via ext_info? 

> +static IIO_DEVICE_ATTR(out_altcurrent0_enable, 0644,

This one can be done with standard ABI + infomask element at read_raw
so do it that way rather than via IIO_DEVICE_ATTR() which should be
used only when none of the standard stuff is possible because these
direct attr declarations hide things from internal kernel usage and
mean we have to much more carefully check them against ABI documentation.

> +		       ad9832_show, ad9832_store, AD9832_OUTPUT_EN);
> +
> +/*
> + * TODO: Convert to DT property when graduating from staging.
> + * Pin control configuration depends on hardware wiring.
> + */
> +static IIO_DEVICE_ATTR(out_altcurrent0_pincontrol_en, 0644,
> +		       ad9832_show, ad9832_store, AD9832_PINCTRL_EN);
>  
>  static struct attribute *ad9832_attributes[] = {
> -	&iio_dev_attr_out_altvoltage0_frequency0.dev_attr.attr,
> -	&iio_dev_attr_out_altvoltage0_frequency1.dev_attr.attr,
> -	&iio_const_attr_out_altvoltage0_frequency_scale.dev_attr.attr,
> -	&iio_dev_attr_out_altvoltage0_phase0.dev_attr.attr,
> -	&iio_dev_attr_out_altvoltage0_phase1.dev_attr.attr,
> -	&iio_dev_attr_out_altvoltage0_phase2.dev_attr.attr,
> -	&iio_dev_attr_out_altvoltage0_phase3.dev_attr.attr,
> -	&iio_const_attr_out_altvoltage0_phase_scale.dev_attr.attr,
> -	&iio_dev_attr_out_altvoltage0_pincontrol_en.dev_attr.attr,
> -	&iio_dev_attr_out_altvoltage0_frequencysymbol.dev_attr.attr,
> -	&iio_dev_attr_out_altvoltage0_phasesymbol.dev_attr.attr,
> -	&iio_dev_attr_out_altvoltage0_out_enable.dev_attr.attr,
> +	&iio_dev_attr_out_altcurrent0_frequency_symbol.dev_attr.attr,
> +	&iio_dev_attr_out_altcurrent0_phase_symbol.dev_attr.attr,
> +	&iio_dev_attr_out_altcurrent0_enable.dev_attr.attr,
> +	&iio_dev_attr_out_altcurrent0_pincontrol_en.dev_attr.attr,
>  	NULL,
>  };