iio: devm helpers and Murata zpa2326 barometer support

Introduce driver for Murata ZPA2326 pressure and temperature sensor:
http://www.murata.com/en-us/products/productdetail?partno=ZPA2326-0311A-R

Signed-off-by Gregor Boirie <[email protected]>

drivers/iio/pressure/zpa2326.c new file +20 -0

+static int zpa2326_power_on(const struct iio_dev         *indio_dev,
+                           const struct zpa2326_private *private)
+{
+       int err = regulator_enable(private->vref);
+
+       if (err)
+               return err;
+
+       err = regulator_enable(private->vdd);
+       if (err)
+               goto vref;
+
+       zpa2326_dbg(indio_dev, "powered on");
+
+       err = zpa2326_enable_device(indio_dev);
+       if (err)
+               goto vdd;
+
+       err = zpa2326_reset_device(indio_dev);
+       if (err)

drivers/iio/pressure/zpa2326.c new file +16 -0

+static int zpa2326_dequeue_pressure(const struct iio_dev *indio_dev,
+                                   u32                  *pressure)
+{
+       int err = zpa2326_read_byte(indio_dev, ZPA2326_STATUS_REG);
+       int cleared = -1;
+
+       if (err < 0)
+               return err;
+
+       *pressure = 0;
+
+       if (err & ZPA2326_STATUS_P_OR) {
+               /*
+                * Fifo overrun : first sample dequeued from fifo is the
+                * newest.
+                */

Quoted 4 lines · Gregor Boirie

Introduce driver for Murata ZPA2326 pressure and temperature sensor:
http://www.murata.com/en-us/products/productdetail?partno=ZPA2326-0311A-R

Signed-off-by Gregor Boirie <[email protected]>

Looking nice!

Documentation/devicetree/bindings/iio/pressure/zpa2326.txt line 14

Quoted 6 lines

Documentation/devicetree/bindings/iio/pressure/zpa2326.txt new file +6 -0

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

vref-supply?

drivers/iio/pressure/zpa2326.c line 89

Quoted 7 lines

drivers/iio/pressure/zpa2326.c new file +7 -0

+/* Hardware sampling frequency descriptor. */
+struct zpa2326_frequency {
+       /* Stringified frequency in Hertz. */
+       const char *hz;
+       /* Output Data Rate word as expected by ZPA2326_CTRL_REG3_REG. */
+       u16         odr;
+};

I still prefer the kerneldoc style of documenting, even if we're not building the
documentation from this file. But OK it's a matter of taste maybe.

drivers/iio/pressure/zpa2326.c line 110

Quoted 2 lines

drivers/iio/pressure/zpa2326.c new file +2 -0

+/* Per-device internal private state. */
+struct zpa2326_private {

That opinion applies also for this struct.

drivers/iio/pressure/zpa2326.c line 170

Quoted 8 lines

drivers/iio/pressure/zpa2326.c new file +8 -0

+/*
+ * 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 zpa2326_read_byte(const struct iio_dev *indio_dev, u8 address)

I also prefer functions to use kerneldoc style. Even if they are not built into documents. For uniformness.

drivers/iio/pressure/zpa2326.c line 287

Quoted 4 lines

drivers/iio/pressure/zpa2326.c new file +4 -0

+static int zpa2326_enable_device(const struct iio_dev *indio_dev)
+{
+       int err = zpa2326_write_byte(indio_dev, ZPA2326_CTRL_REG0_REG,
+                                    ZPA2326_CTRL_REG0_ENABLE);

Argh declaring a variable and immediately performing an operation assigning it a value scares me. I can't say why, it's just that I want it like:

int ret;

ret = zpa...

I also use "ret" over "err". The rationale is that it is a return code, not an error code. It is only an error code if it is negative or != 0. Again that is a personal preference I guess, at least I have an explanation for it.

Quoted 20 lines

drivers/iio/pressure/zpa2326.c new file +20 -0

+static int zpa2326_power_on(const struct iio_dev         *indio_dev,
+                           const struct zpa2326_private *private)
+{
+       int err = regulator_enable(private->vref);
+
+       if (err)
+               return err;
+
+       err = regulator_enable(private->vdd);
+       if (err)
+               goto vref;
+
+       zpa2326_dbg(indio_dev, "powered on");
+
+       err = zpa2326_enable_device(indio_dev);
+       if (err)
+               goto vdd;
+
+       err = zpa2326_reset_device(indio_dev);
+       if (err)

drivers/iio/pressure/zpa2326.c line 395

Quoted 10 lines

drivers/iio/pressure/zpa2326.c new file +10 -0

+               goto disable;
+
+       return 0;
+
+disable:
+       zpa2326_disable_device(indio_dev);
+vdd:
+       regulator_disable(private->vdd);
+vref:
+       regulator_disable(private->vref);

I would personally name the labels "out_disable_device" "out_disable_vdd", "out_disable_vref" but maybe I have a bit of talkative style.

drivers/iio/pressure/zpa2326.c line 409

Quoted 9 lines

drivers/iio/pressure/zpa2326.c new file +9 -0

+/* Power off device, i.e. disable attached power regulators. */
+static void zpa2326_power_off(const struct iio_dev         *indio_dev,
+                             const struct zpa2326_private *private)
+{
+       regulator_disable(private->vdd);
+       regulator_disable(private->vref);
+
+       zpa2326_dbg(indio_dev, "powered off");
+}

Why is zpa2326_disable_device() called on the error path but not in the power off function?

Quoted 21 lines

drivers/iio/pressure/zpa2326.c new file +21 -0

+static int zpa2326_dequeue_pressure(const struct iio_dev *indio_dev,
+                                   u32                  *pressure)
+{
+       int err = zpa2326_read_byte(indio_dev, ZPA2326_STATUS_REG);
+       int cleared = -1;
+
+       if (err < 0)
+               return err;
+
+       *pressure = 0;
+
+       if (err & ZPA2326_STATUS_P_OR) {
+               /*
+                * Fifo overrun : first sample dequeued from fifo is the
+                * newest.
+                */
+               zpa2326_warn(indio_dev, "fifo overflow");
+
+               err = zpa2326_read_block(indio_dev, ZPA2326_PRESS_OUT_XL_REG, 3,
+                                        (u8 *)pressure);
+               if (err)

drivers/iio/pressure/zpa2326.c line 538

Quoted 5 lines

drivers/iio/pressure/zpa2326.c new file +5 -0

+                       return err;
+
+#define ZPA2326_FIFO_DEPTH (16U)
+               /* Hardware fifo may hold no more than 16 pressure samples. */
+               return zpa2326_clear_fifo(indio_dev, ZPA2326_FIFO_DEPTH - 1);

I would not put a #define in the middle of the code like that, but in the top of the file. But I've seen others do this so it's no strong opinion.

drivers/iio/pressure/zpa2326.c line 1446

Quoted 7 lines

drivers/iio/pressure/zpa2326.c new file +7 -0

+                       /*
+                        * Pressure resolution is 1/64 Pascal. Scale to kPascal
+                        * as required by IIO ABI.
+                        */
+                       *val = 0;
+                       *val2 = 1000000 / 64;
+                       return IIO_VAL_INT_PLUS_NANO;

If what you return is a fraction why not:

*val = 1000000;
*val2 = 64;
return IIO_VAL_FRACTIONAL;

?

If you insist on performing a division in the code, use DIV_ROUND_CLOSEST().

drivers/iio/pressure/zpa2326.c line 1476

Quoted 6 lines

drivers/iio/pressure/zpa2326.c new file +6 -0

+       case IIO_CHAN_INFO_OFFSET:
+               switch (chan->type) {
+               case IIO_TEMP:
+                       *val = -17683000 / 649;
+                       *val2 = ((17683000 % 649) * 1000000000ULL) / 649ULL;
+                       return IIO_VAL_INT_PLUS_NANO;

Same here. Rewrite that equation as a fraction and it becomes much prettier I think.

drivers/iio/pressure/zpa2326.c line 1539

Quoted 12 lines

drivers/iio/pressure/zpa2326.c new file +12 -0

+static IIO_DEV_ATTR_SAMP_FREQ(S_IWUSR | S_IRUGO,
+                             zpa2326_show_frequency,
+                             zpa2326_store_frequency);
+
+/* Expose supported hardware sampling frequencies (Hz) through sysfs. */
+static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("1 5 11 23");
+
+static struct attribute *zpa2326_attributes[] = {
+       &iio_dev_attr_sampling_frequency.dev_attr.attr,
+       &iio_const_attr_sampling_frequency_available.dev_attr.attr,
+       NULL
+};

I was asked to supply these through the raw read/write functions in my driver by using IIO_CHAN_INFO_SAMP_FREQ. It works like a charm! If you want it to apply to the whole device, use the .info_mask_shared_by_all = = BIT(IIO_CHAN_INFO_SAMP_FREQ),

(See my MPU-3050 driver patch for an example.)

Yours,
Linus Walleij