On 09/01/2016 10:30 PM, Linus Walleij wrote:
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?
done
+/* 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.
ok. let's beautify these
+/* 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.
Agreed. However, in this precise case, the variable is used for error
handling purpose only.
Renamed quite a few of them in the whole driver.
+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?
zpa2326_disable_device() is used to switch hardware to a low power state.
It is used in many locations to implement one-shot/on demand sampling
without switching regulators off.
The reason is that getting out of hardware low power states
(zpa2326_enable_device()) is short enough (1 ms) so that we can afford
switching back and forth all the time. However, we do have no control over
regulators power up time...
Hence :
* regulators related operations are integrated within pm_runtime,
initial power up and
final power off things,
* whereas zpa2326_enable_device() and zpa2326_disable_device() are used
along with
sampling machinery.
Hope this is clear enough.
+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.
The idea here is to prevent reader from getting back to the top of file
to see macro definition (when used in a single location). Makes things much
easier to read provided it doesn't pollute the natural instruction flow
(to me).
+ /*
+ * 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.
That's an interesting one ! I must admit this looks *really* ugly.
I initially used IIO_VAL_FRACTIONAL. But on my Raspberry it returned rubbish
to userspace. I supposed I hit some kind of corner case but had no time to
investigate this : in the advent of a real bug, this would need some
extended regression testing to prevent from breaking multiple drivers.
In addition, these will be constant folded (at least with my
toolchains). I decided
a good comment would propably do the job...
+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.)
ongoing.
Thanks,
Grégor.
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
