Re: [PATCH v3 2/2] iio: pressure: driver for Honeywell HSC/SSC series pressure sensors

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

> > > +	u8 buffer[HSC_REG_MEASUREMENT_RD_SIZE];  
> > 
> > This is used for SPI transfers so should be DMA safe. It's not currently.
> > Look at how IIO_DMA_MINALIGN is used in other drivers to ensure there is
> > no unsafe sharing of cachelines.
> > 
> > On some architectures this is fixed by the stuff that bounces all small transfers
> > but I don't think that is universal yet.  If you want more info find the talk
> > by Wolfram Sang from a few years ago an ELCE on I2C DMA safe buffers.  
> 
> that was a nice rabbit hole, thanks for the pointer.

:) 

> 
> now, based on [2] I will skip explicit i2c dma-related code since my requests
> are 4 bytes long. according to the document, any i2c xfer below 8bytes is not
> worth the overhead.
> 
> [2] https://www.kernel.org/doc/html/latest/i2c/dma-considerations.html
> 
> > > +static int hsc_spi_probe(struct spi_device *spi)
> > > +{
> > > +	struct iio_dev *indio_dev;
> > > +	struct hsc_data *hsc;
> > > +	struct device *dev = &spi->dev;
> > > +
> > > +	indio_dev = devm_iio_device_alloc(dev, sizeof(*hsc));
> > > +	if (!indio_dev)
> > > +		return -ENOMEM;
> > > +
> > > +	hsc = iio_priv(indio_dev);
> > > +	hsc->xfer = hsc_spi_xfer;  
> > 
> > Also, pass the callback and spi->dev into hsc probe. Easy to use
> > a container_of() to get back to the struct spi_device *spi  
> 
> I'd rather simply pass along the client struct.
> 

I don't like the fact it has to be a void *

The core code has no idea what is in there.  At least we constraint it
somewhat with a struct device.

If you want to use a union of the possible types, that would also be fine.

> > > +	hsc->client = spi;
> > > +
> > > +	return hsc_probe(indio_dev, &spi->dev, spi_get_device_id(spi)->name,
> > > +			 spi_get_device_id(spi)->driver_data);  
> > Don't use anything form spi_get_device_id()
> > 
> > Name is a fixed string currently so pass that directly.
> > For driver data, there isn't any yet but if there were use
> > spi_get_device_match_data() and make sure to provide the data in all the
> > id tables.  That function will search the firmware ones first then call
> > back to the spi specific varient.  
> 
> along the way driver_data became redundant, so it was removed from the function
> prototype.
> 
> best regards,
> peter
> 
>