On Wed, Aug 24, 2016 at 4:58 PM, Gregor Boirie <gregor.boirie@xxxxxxxxxx> wrote: > Introduce driver for Murata ZPA2326 pressure and temperature sensor: > http://www.murata.com/en-us/products/productdetail?partno=ZPA2326-0311A-R Grrr no serious spec from the manufacturer. Luckily I found this: http://www.is-line.de/fileadmin/user_upload/is-line/sensorik/hersteller/datasheets/mu_ZPA2326_air_pressure_sensor.pdf > Signed-off-by: Gregor Boirie <gregor.boirie@xxxxxxxxxx> > +++ b/Documentation/devicetree/bindings/iio/pressure/zpa2326.txt > @@ -0,0 +1,23 @@ > +Murata ZPA2326 pressure sensor > + > +Pressure sensor from Murata with SPI and I2C bus interfaces. > + > +Required properties: > +- compatible: "murata,zpa2326" > +- reg: the I2C address or SPI chip select the device will respond to > + > +Optional properties: > +- interrupt-parent: should be the phandle for the interrupt controller > +- interrupts: interrupt mapping for IRQ > +- vdd-supply: an optional regulator that needs to be on to provide VDD > + power to the sensor According to the datasheet there is also VREF so I guess you should include vref-supply and handle that regulator in the code. They don't really say what voltage this is but I suspect something like vddio or vario, i.e. signal level voltage. > +config ZPA2326 > + tristate "Murata ZPA2326 pressure sensor driver" > + depends on I2C || SPI > + select IIO_BUFFER > + select IIO_TRIGGERED_BUFFER > + help > + Say Y here to build support for the Murata ZPA2326 pressure and > + temperature sensor. > + > + To compile this driver as a module, choose M here: the module will > + be called zpa2326. > + > +config ZPA2326_I2C > + tristate "support I2C bus connection" > + depends on I2C && ZPA2326 > + help > + Say Y here to build I2C bus support for the Murata ZPA2326 pressure > + and temperature sensor. > + > + To compile this driver as a module, choose M here: the module will > + be called zpa2326_i2c. > + > +config ZPA2326_SPI > + tristate "support SPI bus connection" > + depends on SPI && ZPA2326 > + help > + Say Y here to build SPI bus support for the Murata ZPA2326 pressure > + and temperature sensor. > + > + To compile this driver as a module, choose M here: the module will > + be called zpa2326_spi. Why are these two not using regmap, i.e. ZPA2326_I2C could select REGMAP_I2C and ZPA2326_SPI could select REGMAP_SPI. This can usually be done very nicely even if the I2C/SPI protocol deviates slightly from the default (and most common) created by the default devm_regmap_init_i2c(), e.g. the SPI specialness we added to drivers/iio/pressure/bmp280-spi.c Regmap cuts down so much code that it's usually worth it even if it can require a bit of overhead. > +/* Comment out to enable really verbose logging. */ > +/*#define DEBUG*/ If we want this then add a Kconfig option to turn it on instead, for example in pincontrol I have this (drivers/pinctrl/Kconfig) config DEBUG_PINCTRL bool "Debug PINCTRL calls" depends on DEBUG_KERNEL help Say Y here to add some extra checks and diagnostics to PINCTRL calls. And in the top-level Makefile: subdir-ccflags-$(CONFIG_DEBUG_PINCTRL) += -DDEBUG This can be made as a totally separate patch as a debug switch for the entire IIO subsystem. > +#include <linux/sched.h> Why? > +#include <linux/pm_runtime.h> Nice! > +/* 200 ms should be enought for the longest conversion time in one-shot mode. */ > +#define ZPA_CONVERSION_TIMEOUT (HZ / 5) Don't define that relative to HZ because HZ is not fixed. Define it as 200 in ms. > +/* Registers map. */ > +#define ZPA_REF_P_XL_REG ((u8)0x8) > +#define ZPA_REF_P_L_REG ((u8)0x9) > +#define ZPA_REF_P_H_REG ((u8)0xa) > +#define ZPA_RES_CONF_REG ((u8)0x10) > +#define ZPA_CTRL_REG0_REG ((u8)0x20) > +# define ZPA_CTRL_REG0_ONE_SHOT BIT(0) > +# define ZPA_CTRL_REG0_ENABLE BIT(1) > +#define ZPA_CTRL_REG1_REG ((u8)0x21) > +# define ZPA_CTRL_REG1_MASK_DATA_READY ((u8)BIT(2)) > +#define ZPA_CTRL_REG2_REG ((u8)0x22) > +# define ZPA_CTRL_REG2_SWRESET BIT(2) > +#define ZPA_CTRL_REG3_REG ((u8)0x23) > +# define ZPA_CTRL_REG3_ODR_SHIFT (4) > +# define ZPA_CTRL_REG3_ENABLE_MEAS BIT(7) > +#define ZPA_INT_SOURCE_REG ((u8)0x24) > +# define ZPA_INT_SOURCE_DATA_READY BIT(2) > +#define ZPA_THS_P_LOW_REG ((u8)0x25) > +#define ZPA_THS_P_HIGH_REG ((u8)0x26) > +#define ZPA_STATUS_REG ((u8)0x27) > +# define ZPA_STATUS_P_DA BIT(1) > +# define ZPA_STATUS_FIFO_E BIT(2) > +# define ZPA_STATUS_P_OR BIT(5) I never saw this creative use of whitespace before, I recommend that you get rid of it. > +#define ZPA_PRESS_OUT_XL_REG ((u8)0x28) > +#define ZPA_PRESS_OUT_L_REG ((u8)0x29) > +#define ZPA_PRESS_OUT_H_REG ((u8)0x2a) > +#define ZPA_TEMP_OUT_L_REG ((u8)0x2b) > +#define ZPA_TEMP_OUT_H_REG ((u8)0x2c) Why this parathesizing and (u8) casting? It looks superweird, try to get rid of the casts. > + /* > + * Underlying I2C / SPI bus adapter used to abstract slave register > + * accesses. > + */ > + const struct zpa_bus *bus; This would be replaced with #include <linux/regmap.h> struct regmap *map; If you use regmap instead. > + /* > + * Interrupt handler stores sampling operation status here for user > + * context usage. > + */ > + int result; I don't understand that comment. Also: use kerneldoc for documenting the fields, see: Documentation/kernel-doc-nano-HOWTO.txt > + /* > + * Optional interrupt line: negative if not declared into DT, in > + * which case user context keeps polling status register to detect > + * sampling completion. > + */ > + int irq; If you use devm_* to request the irq you usually do not need to keep this around. > +/****************************************************************************** > + * Various helpers > + ******************************************************************************/ Usually just skip the excess stars: /* * Various helpers */ But it's just like, my opinion. > +#define zpa_err(_idev, _format, _arg...) \ > + dev_err(zpa_iio2slave(_idev), _format, ##_arg) > + > +#define zpa_warn(_idev, _format, _arg...) \ > + dev_warn(zpa_iio2slave(_idev), _format, ##_arg) > + > +#define zpa_dbg(_idev, _format, _arg...) \ > + dev_dbg(zpa_iio2slave(_idev), _format, ##_arg) > + > +static struct zpa_private *zpa_iio2priv(const struct iio_dev *indio_dev) > +{ > + return (struct zpa_private *)iio_priv(indio_dev); > +} Uhhhh.... Is this necessary? > +/* > + * Fetch single byte from slave register. > + * indio_dev: IIO device the slave is attached to. > + * address: address of register to read from. > + * > + * Returns the fetched byte when successful, a negative error code otherwise. > + */ > +static int zpa_read_byte(const struct iio_dev *indio_dev, u8 address) > +{ > + return zpa_iio2priv(indio_dev)->bus->zpa_read_byte(indio_dev, address); > +} So with regmap you can just inline some calls to read from the map. regmap_read() and regmap_write() > +/* > + * Fetch multiple bytes from a block of contiguous slave registers. > + * indio_dev: IIO device the slave is attached to. > + * address: start address of contiguous registers block to read from. > + * length: number of bytes to fetch. > + * value: location to store fetched data into. > + * > + * Returns: zero when successful, a negative error code otherwise. > + */ > +static int zpa_read_block(const struct iio_dev *indio_dev, > + u8 address, > + u8 length, > + u8 *value) > +{ > + return zpa_iio2priv(indio_dev)->bus->zpa_read_block(indio_dev, address, > + length, value); > +} And it has regmap_bulk_read() too. > +/* > + * Retrieve the most recent pressure sample from hardware fifo. > + * > + * Note that ZPA2326 hardware fifo stores pressure samples only. > + */ > +static int zpa_dequeue_pressure(const struct iio_dev *indio_dev, u32 *pressure) > +{ > + int err = zpa_read_byte(indio_dev, ZPA_STATUS_REG); > + > + if (err < 0) > + return err; > + > + if (!(err & ZPA_STATUS_P_OR)) { > + /* > + * Fifo has not overflown : retrieve newest sample. We need to > + * pop values out until fifo is empty : last fetched pressure is > + * the newest. > + * In nominal cases, we should find a single queued sample only. > + */ > + int cleared = -1; > + > + do { > + err = zpa_read_block(indio_dev, ZPA_PRESS_OUT_XL_REG, 3, > + (u8 *)pressure); > + if (err) > + return err; > + > + err = zpa_read_byte(indio_dev, ZPA_STATUS_REG); > + if (err < 0) > + return err; > + > + cleared++; > + } while (!(err & ZPA_STATUS_FIFO_E)); > + > + if (cleared) > + /* > + * Samples were pushed by hardware during previous > + * rounds but we didn't consume them fast enought: > + * inform user. > + */ > + zpa_warn(indio_dev, "cleared %d fifo entries", cleared); > + > + return 0; > + } Make the error case the exception instead. if (err & ZPA_STATUS_P_OR) { handle error } ...normal flow here ... > +/* Enqueue new channel samples to IIO buffer. */ > +static int zpa_fill_sample_buffer(struct iio_dev *indio_dev, > + const struct zpa_private *private) > +{ > + struct { > + u32 pressure; > + u16 temperature; > + u64 timestamp; > + } sample; (...) > + /* > + * Now push samples using timestamp stored either : > + * - by hardware interrupt handler if interrupt is available: see > + * zpa_handle_irq(), > + * - or oneshot completion polling machinery : see > + * zpa_trigger_oneshot(). > + */ > + iio_push_to_buffers_with_timestamp(indio_dev, &sample, > + private->timestamp); > + return 0; That is an interesting construction, maybe more drivers should pick this up, because it makes things much more clear. > +#ifdef CONFIG_PM > +static int zpa_runtime_suspend(struct device *slave) Usually I just use struct device *dev to have simple variable names. I don't see the "slave" quality of this struct device *. > +{ > + const struct iio_dev *indio_dev = dev_get_drvdata(slave); > + > + if (pm_runtime_autosuspend_expiration(slave)) > + /* Userspace changed autosuspend delay. */ > + return -EAGAIN; Is this something that is needed? > +const struct dev_pm_ops zpa_pm_ops = { > + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, > + pm_runtime_force_resume) > + SET_RUNTIME_PM_OPS(zpa_runtime_suspend, zpa_runtime_resume, NULL) > +}; > +EXPORT_SYMBOL_GPL(zpa_pm_ops); Nice. > +/* Request the PM layer to power supply the device. */ > +static int zpa_resume(const struct iio_dev *indio_dev) > +{ > + struct device *slave = zpa_iio2slave(indio_dev); > + int err = pm_runtime_get_sync(slave); > + > + if (err < 0) > + goto put; > + > + if (err > 0) { > + /* > + * Device was already power supplied: get it out of low power > + * mode. > + */ > + err = zpa_enable_device(indio_dev); > + if (err) > + goto put; > + > + /* Inform caller device was already power supplied. */ > + return 1; > + } > + > + /* Inform caller device has just been brought back to life. */ > + return 0; > + > +put: > + pm_runtime_put_noidle(slave); > + return err; > +} I don't see other drivers bothering with this excess error handling and I think you can just pm_runtime_get_sync() around the areas you need. > +static int zpa_suspend(const struct iio_dev *indio_dev) > +{ > + struct device *slave = zpa_iio2slave(indio_dev); > + int err = zpa_disable_device(indio_dev); > + > + pm_runtime_mark_last_busy(slave); > + > + if (err) > + goto err; > + > + err = pm_runtime_put_autosuspend(slave); > + if (err) { > + dev_warn(slave, "failed to autosuspend (%d)", err); > + goto err; > + } > + > + return 0; > + > +err: > + pm_runtime_put_noidle(slave); > + return err; > +} Likewise just pm_runtime_mark_last_busy() pm_runtime_put_autosuspend() Checking all these errors is overengineered IMO, something going wrong would be so odd that this is cluttering more than helping. > + > +/* Setup runtime power management with autosuspend support. */ > +static void zpa_init_runtime(struct device *slave) > +{ > + pm_runtime_get_noresume(slave); > + pm_runtime_set_active(slave); > + pm_runtime_enable(slave); > + pm_runtime_set_autosuspend_delay(slave, 1000); > + pm_runtime_use_autosuspend(slave); > + pm_runtime_mark_last_busy(slave); > + pm_runtime_put_autosuspend(slave); > +} > + > +static void zpa_fini_runtime(struct device *slave) > +{ > + pm_runtime_disable(slave); > + pm_runtime_set_suspended(slave); > +} > +#else /* !CONFIG_PM */ > +static int zpa_resume(const struct iio_dev *indio_dev) > +{ > + return zpa_enable_device(indio_dev); > +} > + > +static int zpa_suspend(const struct iio_dev *indio_dev) > +{ > + return zpa_disable_device(indio_dev); > +} > + > +#define zpa_init_runtime(_slave) > +#define zpa_fini_runtime(_slave) > +#endif /* !CONFIG_PM */ I would just put runtime PM activation verbatim into the common probe() and disablement verbatim into the common remove(). For an example see: drivers/iio/light/bh1780.c which was removed by runtime PM people (Ulf Hansson). > +static irqreturn_t zpa_handle_irq(int irq, void *data) > +{ > + struct iio_dev *indio_dev = (struct iio_dev *)data; > + > + if (iio_buffer_enabled(indio_dev)) { > + struct zpa_private *priv = zpa_iio2priv(indio_dev); > + > + /* Timestamping needed for buffered sampling only. */ > + priv->timestamp = iio_get_time_ns(indio_dev); > + } I don't understand this. Isn't it so that this interrupt only occurs if the hardware trigger is enabled? And that means that triggered buffer is active and it implies that we're using a buffer so iio_buffer_enabled() is always true here. When I wrote my drivers I rather faced the following: - If I'm using my own hardware trigger, take the timestamp in the interrupt handler top half. - If not, take the sample in the trigger handler. That is, I'm rather looking for a function that tells me if my buffer is using my own hardware trigger, or something else. And that I am tentatively solving with a local variable in the state container, like bool hw_irq_trigger, which feels a bit shaky, so we really should be looking at a way to ask the framework if we're using our own trigger, right? > +static int zpa_acknowledge_irq(const struct iio_dev *indio_dev, > + const struct zpa_private *private) > +{ > + /* > + * Device works according to a level interrupt scheme: reading interrupt > + * status de-asserts interrupt line. > + */ > + int err = zpa_read_byte(indio_dev, ZPA_INT_SOURCE_REG); > + > + if (err < 0) > + return err; > + > + /* Data ready is the only interrupt source we requested. */ > + if (!(err & ZPA_INT_SOURCE_DATA_READY)) { > + zpa_warn(indio_dev, "unexpected interrupt status %02x", err); > + return -ENODATA; > + } This looks strange. I think this should rather lead to IRQ_NONE being returned from the interrupt handler than a negative return value from this function, do you agree? > + /* New sample available: notify our internal trigger consumers. */ > + iio_trigger_poll_chained(private->trigger); > + > + return 0; > +} I think this whole function should be skipped and inlined into zpa_handle_threaded_irq() for clarity. > +static irqreturn_t zpa_handle_threaded_irq(int irq, void *data) > +{ > + struct iio_dev *indio_dev = (struct iio_dev *)data; > + struct zpa_private *priv = zpa_iio2priv(indio_dev); > + irqreturn_t ret = IRQ_NONE; > + > + priv->result = zpa_acknowledge_irq(indio_dev, priv); > + > + if (indio_dev->currentmode == INDIO_BUFFER_SOFTWARE) { > + /* Continuous sampling enabled. */ > + > + if (!priv->result) > + /* Populate IIO buffer */ > + zpa_fill_sample_buffer(indio_dev, priv); > + /* > + * Tell power management layer we've just used the device to > + * prevent from autosuspending to early. > + */ > + pm_runtime_mark_last_busy(zpa_iio2slave(indio_dev)); Runtime PM will not autosuspend while you're holding a reference, i.e. after a get_sync(). This is rather something you should do right before runtime_pm_put_autosuspend() to mark that this was the last time we were busy. I think you can just skip this. > + return (!priv->result) ? IRQ_HANDLED : IRQ_NONE; Here IRQ_NONE is returned correctly. > + } > + > + if (priv->result) > + /* > + * Interrupt happened but no new sample available: likely caused > + * by spurious interrupts, in which case, returning IRQ_NONE > + * allows to benefit from the generic spurious interrupts > + * handling. > + */ > + goto complete; Aha so here the negative error code from the helper function means the default assignment to ret , IRQ_NONE gets returned. That is hard to follow, just inline the other function as I suggest and I think it becomes simpler to see. > + switch (type) { > + case IIO_PRESSURE: > + zpa_dbg(indio_dev, "fetching raw pressure sample"); > + > + err = zpa_read_block(indio_dev, ZPA_PRESS_OUT_XL_REG, 3, > + (u8 *)value); Use a local variable instead of modifying the buffer in-place, else you leave junk in the buffer on the error path. u8 raw_val[3]; zpa_read_block(.... &raw_val); is the pattern I would use. > + if (err) { > + zpa_warn(indio_dev, "failed to fetch pressure (%d)", > + err); > + return err; > + } > + > + /* Pressure is a 24 bits wide little-endian unsigned int. */ > + *value = (((u8 *)value)[2] << 16) | (((u8 *)value)[1] << 8) | > + ((u8 *)value)[0]; That works, some would use some sign extension and le32_to_cpu() I guess but who cares. > + case IIO_TEMP: > + zpa_dbg(indio_dev, "fetching raw temperature sample"); > + > + err = zpa_read_block(indio_dev, ZPA_TEMP_OUT_L_REG, 2, > + (u8 *)value); Do not modify buffer in-place. > + if (err) { > + zpa_warn(indio_dev, "failed to fetch temperature (%d)", > + err); > + return err; > + } > + > + /* Temperature is a 16 bits wide little-endian signed int. */ > + *value = (int)le16_to_cpup((__le16 *)value); > + > + return IIO_VAL_INT; > + > + default: > + BUG(); That's nasty. Just dev_err() and an explanation is better I think. > +/* > + * Reject external trigger attachment if setting up continuous hardware sampling > + * mode. > + */ > +static int zpa_validate_trigger(struct iio_dev *indio_dev, > + struct iio_trigger *trigger) > +{ > + int ret = 0; > + > + mutex_lock(&indio_dev->mlock); > + > + if (indio_dev->currentmode == INDIO_BUFFER_SOFTWARE) > + ret = -EBUSY; > + > + mutex_unlock(&indio_dev->mlock); > + > + return ret; > +} This looks more like something the IIO core should be doing. Why do we need to look for such things in drivers? > +/* Finalize one shot cycle processing in triggered sampling mode. */ > +static void zpa_complete_oneshot_trigger(const struct iio_dev *indio_dev) > +{ > + /* > + * Tell power management layer we've just used the device to prevent > + * from autosuspending to early. > + */ > + pm_runtime_mark_last_busy(zpa_iio2slave(indio_dev)); Again as long as the reference is held this should not be necessary. > +static const struct iio_buffer_setup_ops zpa_ring_setup_ops = { > + .preenable = zpa_preenable_ring, > + .postenable = zpa_postenable_ring, > + .predisable = zpa_predisable_ring, > + .postdisable = zpa_postdisable_ring, > +}; So in my drivers I tend to pick a runtime PM reference with runtime_pm_get_sync() in .preenable and runtime_pm_mark_last_busy() and runtime_pm_put_autosuspend() in .postdisable() so the power is on around the whole buffered measurement cycle. Then for the raw reads some special quirks. > +static int zpa_debugfs_read(struct iio_dev *indio_dev, > + u8 reg, > + unsigned int *value) > +{ > + int err; > + > + switch (reg) { > + case ZPA_REF_P_XL_REG: > + case ZPA_REF_P_L_REG: > + case ZPA_REF_P_H_REG: > + case ZPA_DEVICE_ID_REG: > + case ZPA_RES_CONF_REG: > + case ZPA_CTRL_REG0_REG: > + case ZPA_CTRL_REG1_REG: > + case ZPA_CTRL_REG2_REG: > + case ZPA_CTRL_REG3_REG: > + case ZPA_INT_SOURCE_REG: /* Will acknowledge interrupts. */ > + case ZPA_THS_P_LOW_REG: > + case ZPA_THS_P_HIGH_REG: > + case ZPA_STATUS_REG: > + case ZPA_PRESS_OUT_XL_REG: > + case ZPA_PRESS_OUT_L_REG: > + case ZPA_PRESS_OUT_H_REG: /* Will pop samples out of hardware fifo. */ > + case ZPA_TEMP_OUT_L_REG: > + case ZPA_TEMP_OUT_H_REG: > + break; > + > + default: > + return -EINVAL; > + } > + > + err = zpa_claim_direct_mode(indio_dev); > + if (err < 0) > + return err; > + > + err = zpa_read_byte(indio_dev, reg); > + if (err < 0) > + goto release; > + > + *value = err; > + err = 0; > + > +release: > + zpa_release_direct_mode(indio_dev); > + return err; > +} Regmap provides all of this for free. It has a min_reg and max_reg property. But overall it is not so necessary to be overly protective in debugfs (what register can be read etc): it is debugfs after all, you should know what you're doing. > +static int zpa_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int *val, > + int *val2, > + long mask) > +{ > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + return zpa_sample_oneshot(indio_dev, chan->type, val); So when using rumtime PM I would just take the pm reference around this call actually, apart from around the buffer preenable/postdisable. > + default: > + BUG(); Again that is nasty. > +static const struct iio_chan_spec zpa_channels[] = { > + [0] = { > + .type = IIO_PRESSURE, > + .channel = 0, > + .scan_index = 0, > + .scan_type = { > + .sign = 'u', > + .realbits = 24, > + .storagebits = 32, > + .endianness = IIO_LE, > + }, > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > + BIT(IIO_CHAN_INFO_SCALE), > + }, > + [1] = { > + .type = IIO_TEMP, > + .channel = 1, > + .scan_index = 1, > + .scan_type = { > + .sign = 's', > + .realbits = 16, > + .storagebits = 16, > + .endianness = IIO_LE, > + }, > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > + BIT(IIO_CHAN_INFO_SCALE) | > + BIT(IIO_CHAN_INFO_OFFSET), > + }, > + [2] = IIO_CHAN_SOFT_TIMESTAMP(2), > +}; This looks nice. > +static int _zpa_read_i2c_byte(const struct i2c_client *client, u8 address) > +{ > + int err; > + u8 byte; > + struct i2c_msg msg[] = { > + [0] = { > + .addr = client->addr, > + .flags = client->flags, > + .len = 1, > + .buf = &address > + }, > + [1] = { > + .addr = client->addr, > + .flags = client->flags | I2C_M_RD, > + .len = 1, > + .buf = &byte > + } > + }; > + > + err = i2c_transfer(client->adapter, msg, ARRAY_SIZE(msg)); > + if (err < 0) > + return err; > + > + if (err != ARRAY_SIZE(msg)) > + return -ENOMSG; > + > + return (int)byte; > +} This just looks like a reimplementation of I2C regmap. So use I2C regmap as abstraction. > +static int _zpa_read_spi_byte(struct spi_device *slave, u8 address) > +{ > + struct spi_transfer xfer[2]; > + u8 buf; > + int err; > + > + /* Request read cycle. */ > + address |= ZPA_SPI_ADDR_RD; > + > + /* Zero-initialize to ensure forward compatibility. */ > + memset(xfer, 0, sizeof(xfer)); > + > + /* Register address write cycle. */ > + xfer[0].tx_buf = &address; > + xfer[0].len = sizeof(address); > + > + /* Register content read cycle. */ > + xfer[1].rx_buf = &buf; > + xfer[1].len = sizeof(buf); > + > + err = spi_sync_transfer(slave, xfer, ARRAY_SIZE(xfer)); > + if (err) > + return err; > + > + return buf; > +} Sane here but for SPI, use regmap correct me if I'm wrong. Look at drivers/iio/pressure/bmp280* for regmap inspiration. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
