On Mon, 2019-10-07 at 12:52 +0100, Jonathan Cameron wrote: > > On Mon, 7 Oct 2019 09:10:06 +0000 > "Sa, Nuno" <Nuno.Sa@xxxxxxxxxx> wrote: > > > On Sun, 2019-10-06 at 11:37 +0100, Jonathan Cameron wrote: > > > [External] > > > > > > On Fri, 4 Oct 2019 15:55:18 +0200 > > > Nuno Sá <nuno.sa@xxxxxxxxxx> wrote: > > > > > > > The LTC2983 is a Multi-Sensor High Accuracy Digital Temperature > > > > Measurement System. It measures a wide variety of temperature > > > > sensors and > > > > digitally outputs the result, in °C or °F, with 0.1°C accuracy > > > > and > > > > 0.001°C resolution. It can measure the temperature of all > > > > standard > > > > thermocouples (type B,E,J,K,N,S,R,T), standard 2-,3-,4-wire > > > > RTDs, > > > > thermistors and diodes. > > > > > > > > Signed-off-by: Nuno Sá <nuno.sa@xxxxxxxxxx> > > > > > > Two things inline that I'd missed before. Otherwise looks good. > > > > > > Thanks, > > > > > > Jonathan > > > > > > > --- > > > > Changes in v2: > > > > * Added some needed blank lines (for readability); > > > > * Allocate iio_chan in the setup() function; > > > > * Rename reset to sleep; > > > > * Remove unneeded dev_dbg calls; > > > > * Remove unneeded line wrapping; > > > > * Remove unneeded comments; > > > > * Remove extend_names. Use the standard ABI; > > > > * Adapt the scales to report in millivolt and milli degrees; > > > > * Adapt the of_property readings to the renaming of the > > > > properties; > > > > * For custom thermistors, excitation-current cannot be set to > > > > Auto > > > > range. > > > > > > > > Changes in v3: > > > > * Use normal `devm_request_irq`; > > > > * Handle and decode the new devicetree properties for sensor > > > > configuration. > > > > > > > > MAINTAINERS | 7 + > > > > drivers/iio/temperature/Kconfig | 10 + > > > > drivers/iio/temperature/Makefile | 1 + > > > > drivers/iio/temperature/ltc2983.c | 1554 > > > > +++++++++++++++++++++++++++++ > > > > 4 files changed, 1572 insertions(+) > > > > create mode 100644 drivers/iio/temperature/ltc2983.c > > > > > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > > > index f0c03740b9fb..14a256e785ca 100644 > > > > --- a/MAINTAINERS > > > > +++ b/MAINTAINERS > > > > @@ -9491,6 +9491,13 @@ S: Maintained > > > > F: Documentation/devicetree/bindings/iio/dac/ltc1660.txt > > > > F: drivers/iio/dac/ltc1660.c > > > > > > > > +LTC2983 IIO TEMPERATURE DRIVER > > > > +M: Nuno Sá <nuno.sa@xxxxxxxxxx> > > > > +W: http://ez.analog.com/community/linux-device-drivers > > > > +L: linux-iio@xxxxxxxxxxxxxxx > > > > +S: Supported > > > > +F: drivers/iio/temperature/ltc2983.c > > > > + > > > > LTC4261 HARDWARE MONITOR DRIVER > > > > M: Guenter Roeck <linux@xxxxxxxxxxxx> > > > > L: linux-hwmon@xxxxxxxxxxxxxxx > > > > diff --git a/drivers/iio/temperature/Kconfig > > > > b/drivers/iio/temperature/Kconfig > > > > index 737faa0901fe..04b5a67b593c 100644 > > > > --- a/drivers/iio/temperature/Kconfig > > > > +++ b/drivers/iio/temperature/Kconfig > > > > @@ -4,6 +4,16 @@ > > > > # > > > > menu "Temperature sensors" > > > > > > > > +config LTC2983 > > > > + tristate "Analog Devices Multi-Sensor Digital > > > > Temperature > > > > Measurement System" > > > > + depends on SPI > > > > > > Select REGMAP_SPI needed I think. > > > > ack. > > > > > > + help > > > > + Say yes here to build support for the LTC2983 Multi- > > > > Sensor > > > > + high accuracy digital temperature measurement system. > > > > + > > > > + To compile this driver as a module, choose M here: > > > > the module > > > > + will be called ltc2983. > > > > + > > > > config MAXIM_THERMOCOUPLE > > > > tristate "Maxim thermocouple sensors" > > > > depends on SPI > > > > diff --git a/drivers/iio/temperature/Makefile > > > > b/drivers/iio/temperature/Makefile > > > > index baca4776ca0d..d6b850b0cf63 100644 > > > > --- a/drivers/iio/temperature/Makefile > > > > +++ b/drivers/iio/temperature/Makefile > > > > @@ -3,6 +3,7 @@ > > > > # Makefile for industrial I/O temperature drivers > > > > # > > > > > > > > +obj-$(CONFIG_LTC2983) += ltc2983.o > > > > obj-$(CONFIG_HID_SENSOR_TEMP) += hid-sensor-temperature.o > > > > obj-$(CONFIG_MAXIM_THERMOCOUPLE) += maxim_thermocouple.o > > > > obj-$(CONFIG_MAX31856) += max31856.o > > > > diff --git a/drivers/iio/temperature/ltc2983.c > > > > b/drivers/iio/temperature/ltc2983.c > > > > new file mode 100644 > > > > index 000000000000..f899c1d75f8a > > > > --- /dev/null > > > > +++ b/drivers/iio/temperature/ltc2983.c > > > > @@ -0,0 +1,1554 @@ > > > ... > > > > > > > +static int ltc2983_chan_read(struct ltc2983_data *st, > > > > + const struct ltc2983_sensor *sensor, > > > > int *val) > > > > +{ > > > > + u32 start_conversion = 0; > > > > + int ret; > > > > + unsigned long time; > > > > + __be32 temp; > > > > + > > > > + /* > > > > + * Do not allow channel readings if device is in sleep > > > > state. > > > > + * A read/write on the spi bus would bring the device > > > > prematurely > > > > + * out of sleep. > > > > + */ > > > > + if (st->sleep) > > > > + return -EPERM; > > > > + > > > > + start_conversion = LTC2983_STATUS_START(true); > > > > + start_conversion |= LTC2983_STATUS_CHAN_SEL(sensor- > > > > >chan); > > > > + dev_dbg(&st->spi->dev, "Start conversion on chan:%d, > > > > status:%02X\n", > > > > + sensor->chan, start_conversion); > > > > + /* start conversion */ > > > > + ret = regmap_write(st->regmap, LTC2983_STATUS_REG, > > > > start_conversion); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + reinit_completion(&st->completion); > > > > + /* > > > > + * wait for conversion to complete. > > > > + * 300 ms should be more than enough to complete the > > > > conversion. > > > > + * Depending on the sensor configuration, there are 2/3 > > > > conversions > > > > + * cycles of 82ms. > > > > + */ > > > > + time = wait_for_completion_timeout(&st->completion, > > > > + msecs_to_jiffies(300 > > > > )); > > > > + if (!time) { > > > > + dev_warn(&st->spi->dev, "Conversion timed > > > > out\n"); > > > > + return -ETIMEDOUT; > > > > + } > > > > + > > > > + /* read the converted data */ > > > > + ret = regmap_bulk_read(st->regmap, > > > > LTC2983_CHAN_RES_ADDR(sensor->chan), > > > > + &temp, sizeof(temp)); > > > > > > I'd missed this before. regmap_bulk_read can directly use the > > > supplied buffer > > > for dma. Hence it needs to be dma safe. That means you have to > > > have > > > it > > > in it's own cacheline. There is no way of enforcing that on the > > > stack so > > > either allocate it locally from the heap, or put it at the end of > > > the > > > data structure and mark it __cacheline_aligned (we make sure > > > those > > > structures > > > are also cacheline aligned and on the heap specifically to allow > > > us > > > to do that > > > trick). > > > > Just for my understanding. I do see the the need of using > > __cacheline_aligned on the struct parameter we want to align. I was > > now > > trying to understand your comment on parenthesis. Why do we need > > that > > trick? Wouldn't be sufficient to have the struct element marked > > with > > __cacheline_aligned? > > > > Ah. That is down to what __cacheline_aligned actually does and > how we create the region accessed by iio_priv() > > If we have a stand alone structure allocated with kmalloc with an > element marked __cacheline_aligned it will indeed have the correct > alignment. > > iio_priv is done by increasing the size of the memory allocation > done in iio_device_alloc, the iio_priv() pointer then gives us > the address of a region beyond the end of the iio_dev structure. Got it. > That region has to also be cacheline aligned so that when we find > the location of the __cacheline_aligned element of the structure > we are putting there (basically an offset from iio_priv) it will > also have the right alignment. Understood. Then, the only thing I'm still wondering is that we need to have the guarantee that kmalloc also returns a pointer which is at least __cacheline_aligned for our iio_dev struct so that, our private struct has the correct alignment (and thus the element). Is this correct? > The compiler has no way of knowing we are doing this stuff so it > just adds padding to the private structure on the assumption > the structure itself is aligned. > > Jonathan > > Thanks for your help and explanation! Nuno Sá
