Re: [PATCH v6 3/3] drivers: iio: adc: add support for ad777x family

[Jonathan Cameron]

On Thu, 10 Oct 2024 14:32:49 +0000
"Nechita, Ramona" <Ramona.Nechita@xxxxxxxxxx> wrote:

> Hello,
> 
> I have some questions inline before sending out a new patch.
> 
> 
> ....
> >> +struct ad7779_state {
> >> +	struct spi_device *spi;
> >> +	const struct ad7779_chip_info *chip_info;
> >> +	struct clk *mclk;
> >> +	struct iio_trigger *trig;
> >> +	struct completion completion;
> >> +	unsigned int sampling_freq;
> >> +	enum ad7779_filter filter_enabled;
> >> +	/*
> >> +	 * DMA (thus cache coherency maintenance) requires the
> >> +	 * transfer buffers to live in their own cache lines.
> >> +	 */
> >> +	struct {
> >> +		u32 chans[8];
> >> +		s64 timestamp;  
> >
> >	aligned_s64 timestamp;
> >
> >while it makes no difference in this case, this makes code aligned inside the IIO subsystem.  
> 
> I might be missing something but I can't find the aligned_s64 data type, should I define it myself
> in the driver?

Recent addition to the iio tree so it is in linux-next but not in mainline yet.
https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/commit/?h=togreg&id=e4ca0e59c39442546866f3dd514a3a5956577daf
It just missed last cycle.

> 
> >  
> >> +	} data __aligned(IIO_DMA_MINALIGN);  
> >
> >Note, this is different alignment to the above. And isn't the buffer below should have it instead?

While I'm here:  No to this one.  The s64 alignment is about
performance of CPU access + consistency across CPU architectures.
This one (which happens to always be 8 or more) is about DMA safety.



> >  
> >> +	u32			spidata_tx[8];
> >> +	u8			reg_rx_buf[3];
> >> +	u8			reg_tx_buf[3];
> >> +	u8			reset_buf[8];
> >> +};  
> >
> >....
> >  
> >> +static int ad7779_write_raw(struct iio_dev *indio_dev,
> >> +			    struct iio_chan_spec const *chan, int val, int val2,
> >> +			    long mask)  
> >
> >long? Not unsigned long?  
> 
> I copied the function header directly from iio.h, shouldn't it be left as such?

Hmm. That's an ancient legacy that we should really cleanup at somepoint.
Leave this as long.


> 
> >  
> >> +{
> >> +	struct ad7779_state *st = iio_priv(indio_dev);
> >> +
> >> +	iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> >> +		switch (mask) {
> >> +		case IIO_CHAN_INFO_CALIBSCALE:
> >> +			return ad7779_set_calibscale(st, chan->channel, val2);
> >> +		case IIO_CHAN_INFO_CALIBBIAS:
> >> +			return ad7779_set_calibbias(st, chan->channel, val);
> >> +		case IIO_CHAN_INFO_SAMP_FREQ:
> >> +			return ad7779_set_sampling_frequency(st, val);
> >> +		default:
> >> +			return -EINVAL;
> >> +		}
> >> +	}
> >> +	unreachable();
> >> +}  
> >
> >...
> >  
> >> +static irqreturn_t ad7779_trigger_handler(int irq, void *p) {
> >> +	struct iio_poll_func *pf = p;
> >> +	struct iio_dev *indio_dev = pf->indio_dev;
> >> +	struct ad7779_state *st = iio_priv(indio_dev);
> >> +	int ret;  
> >
> >...
> >  
> 
> Thank you!
> 
> Best Regards,
> Ramona
>