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

Jonathan Cameron
2023-12-01T18:24:56Z

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