LOn Thu, Sep 1, 2016 at 6:25 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 > > Signed-off-by: Gregor Boirie <gregor.boirie@xxxxxxxxxx> Looking nice! > +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? > +/* 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. > +/* Per-device internal private state. */ > +struct zpa2326_private { That opinion applies also for this struct. > +/* > + * 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. > +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. > +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) > + 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. > +/* 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? > +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) > + 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. > + /* > + * 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(). > + 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. > +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 -- 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
