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

[Andy Shevchenko]

On Thu, Oct 10, 2024 at 02:32:49PM +0000, Nechita, Ramona wrote:

...

> >> +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?

Definitely, basically the rule of thumb is to create your patches based on
the respective subsystem tree. In such a case this is IIO tree togreg branch
(in some cases testing should be used).

There is the mentioned type been introduced.

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

Oh, this is unfortunate. It should be fixed there, indeed.

-- 
With Best Regards,
Andy Shevchenko