Re: [PATCH v2 5/6] staging: iio: ad9832: convert to iio channels and ext_info attrs

[Jonathan Cameron]

On Wed, 31 Dec 2025 14:08:52 -0300
Tomas Borquez <tomasborquez13@xxxxxxxxx> wrote:

> On Wed, Dec 31, 2025 at 12:55:50AM +0200, Andy Shevchenko wrote:
> > On Tue, Dec 30, 2025 at 10:35 PM Tomas Borquez <tomasborquez13@xxxxxxxxx> 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.  
> 
> ...
> 
> > > -static int ad9832_write_frequency(struct ad9832_state *st,
> > > -                                 unsigned int addr, unsigned long fout)
> > > +static ssize_t ad9832_write_frequency(struct iio_dev *indio_dev,
> > > +                                     uintptr_t private,  
> > 
> > Torvalds said that uintptr_t shouldn't be used in the Linux kernel,
> > the unsigned long suffice and enough. Why do we need it here?  
> 
> Copied it from the definition of iio_chan_spec_ext_info:
> 
> struct iio_chan_spec_ext_info {
> 	const char *name;
> 	enum iio_shared_by shared;
> 	ssize_t (*read)(struct iio_dev *, uintptr_t private,
> 			struct iio_chan_spec const *, char *buf);
> 	ssize_t (*write)(struct iio_dev *, uintptr_t private,
> 			 struct iio_chan_spec const *, const char *buf,
> 			 size_t len);
> 	uintptr_t private;
> };
> 
> But can change it
As Linus has been clear on his preferences on this, we should probably
clean that iio_chan_spec_ext_info up as well at some
point.  Not urgent though and don't gate this patch on it.  Better
to clean up all existing users in one go.

This will do as a reference for why:

https://lore.kernel.org/all/CAHk-=wgSvPVGZp56uFCjOZoKcgQp7xpsj3P-Hhg+NXvhPnzszg@xxxxxxxxxxxxxx/

Jonathan

> 
> >   
> > > +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);
> > > +static IIO_DEVICE_ATTR(out_altcurrent0_enable, 0644,
> > > +                      ad9832_show, ad9832_store, AD9832_OUTPUT_EN);  
> > 
> > Why not IIO_DEVICE_ATTR_RW()?  
> 
> Not any good reason just didn't know it existed.
> 
> > ...
> >   
> > > +       &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,  
> > 
> > At some point we may also drop the comma in the terminator entry.  
> 
> Will remove it as part of V3
> 
> > -- 
> > With Best Regards,
> > Andy Shevchenko