Re: [PATCH 1/6] iio: chemical: scd30: add core driver
From: Andy Shevchenko
Date: Wed Apr 22 2020 - 15:50:00 EST
On Wed, Apr 22, 2020 at 5:22 PM Tomasz Duszynski
<tomasz.duszynski@xxxxxxxxxxx> wrote:
>
> Add Sensirion SCD30 carbon dioxide core driver.
And DocLink tar of Datasheet: with a link?
...
> +static SIMPLE_DEV_PM_OPS(scd30_pm_ops, scd30_suspend, scd30_resume);
Would it be used in every module? You will get a compiler warning per
each module that is not using it.
...
> +int scd30_probe(struct device *dev, int irq, const char *name, void *priv,
> + int (*command)(struct scd30_state *state, enum scd30_cmd cmd,
> + u16 arg, char *rsp, int size));
My gosh.
Please, supply proper structure member in priv or alike.
...
> + * Copyright (c) Tomasz Duszynski <tomasz.duszynski@xxxxxxxxxxx>
Year?
...
> +#include <asm/byteorder.h>
asm goes after linux.
> +#include <linux/bits.h>
> +#include <linux/compiler.h>
> +#include <linux/completion.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/errno.h>
> +#include <linux/export.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/types.h>
> +#include <linux/interrupt.h>
> +#include <linux/irqreturn.h>
> +#include <linux/jiffies.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/string.h>
> +#include <linux/sysfs.h>
> +#include <linux/types.h>
Are you sure you need all of them?!
...
> +/* pressure compensation in millibars */
Put the unit as a suffix to each definition and drop useless comment.
> +/* measurement interval in seconds */
Ditto.
> +/* reference CO2 concentration in ppm */
Ditto.
> +enum {
> + CONC,
> + TEMP,
> + HR,
> +};
Way too generic names for anonymous enum.
...
> +static int scd30_command(struct scd30_state *state, enum scd30_cmd cmd, u16 arg,
> + char *rsp, int size)
> +{
> + /*
> + * assumption holds that response buffer pointer has been already
> + * properly aligned so casts are safe
> + */
> + while (size >= sizeof(u32)) {
> + *(u32 *)rsp = be32_to_cpup((__be32 *)rsp);
Seems like rsp should be void * rather than char *.
> + rsp += sizeof(u32);
> + size -= sizeof(u32);
> + }
NIH of https://elixir.bootlin.com/linux/v5.7-rc2/ident/be32_to_cpu_array ?
> + if (size)
It can be done before even while loop with an immediate bail out.
> + *(u16 *)rsp = be16_to_cpup((__be16 *)rsp);
> +
> + return 0;
> +}
...
> +/* simplified float to fixed point conversion with a scaling factor of 0.01 */
> +static int scd30_float_to_fp(int float32)
> +{
> + int fraction, shift,
> + mantissa = float32 & GENMASK(22, 0),
> + sign = float32 & BIT(31) ? -1 : 1,
> + exp = (float32 & ~BIT(31)) >> 23;
> +
> + /* special case 0 */
> + if (!exp && !mantissa)
> + return 0;
> +
> + exp -= 127;
> + if (exp < 0) {
> + exp = -exp;
> + /* return values ranging from 1 to 99 */
> + return sign * ((((BIT(23) + mantissa) * 100) >> 23) >> exp);
shift = 23 + exp;
... >> shift);
> + }
> +
> + /* return values starting at 100 */
> + shift = 23 - exp;
> + float32 = BIT(exp) + (mantissa >> shift);
> + fraction = mantissa & GENMASK(shift - 1, 0);
> +
> + return sign * (float32 * 100 + ((fraction * 100) >> shift));
> +}
Sounds like a candidate to IIO library or even lib/math/*.c.
...
> +static int scd30_wait_meas_irq(struct scd30_state *state)
> +{
> + int ret, timeout = msecs_to_jiffies(state->meas_interval * 1250);
Magic number.
> + reinit_completion(&state->meas_ready);
> + enable_irq(state->irq);
> + ret = wait_for_completion_interruptible_timeout(&state->meas_ready,
> + timeout);
> + if (ret > 0)
> + ret = 0;
> + else if (!ret)
> + ret = -ETIMEDOUT;
> +
> + disable_irq(state->irq);
> +
> + return ret;
> +}
...
> +static int scd30_wait_meas_poll(struct scd30_state *state)
> +{
> + int tries = 5;
> +
> + while (tries--) {
> + int ret;
> + u16 val;
> +
> + ret = scd30_command(state, CMD_MEAS_READY, 0, (char *)&val,
> + sizeof(val));
> + if (ret)
> + return -EIO;
> +
> + /* new measurement available */
> + if (val)
> + break;
> +
> + msleep_interruptible(state->meas_interval * 250);
> + }
> +
> + if (tries == -1)
> + return -ETIMEDOUT;
unsigned int tries = ...;
do {
...
} while (--tries);
if (!tries)
return ...;
looks better and I guess less code in asm.
> + return 0;
> +}
...
> + if (kstrtou16(buf, 0, &val))
> + return -EINVAL;
Shadowed error code. Don't do like this.
> + if (kstrtou16(buf, 0, &val))
> + return -EINVAL;
Ditto.
> + if (kstrtou16(buf, 0, &val))
> + return -EINVAL;
Ditto.
> + val = !!val;
kstrtobool()?
...
> + if (kstrtou16(buf, 0, &val))
> + return -EINVAL;
No shadowed error code, please. Check entire code.
> +static IIO_DEVICE_ATTR_RW(pressure_comp, 0);
> +static IIO_DEVICE_ATTR_RO(pressure_comp_available, 0);
> +static IIO_DEVICE_ATTR_RW(meas_interval, 0);
> +static IIO_DEVICE_ATTR_RO(meas_interval_available, 0);
> +static IIO_DEVICE_ATTR_RW(asc, 0);
> +static IIO_DEVICE_ATTR_RW(frc, 0);
> +static IIO_DEVICE_ATTR_RO(frc_available, 0);
> +static IIO_DEVICE_ATTR_RW(temp_offset, 0);
> +static IIO_CONST_ATTR(temp_offset_available, "[0 1 65535]");
> +static IIO_DEVICE_ATTR_WO(reset, 0);
Do you need all of them? Doesn't IIO core provides a tons of helpers for these?
Btw, where is ABI documentation? It's a show stopper.
--
With Best Regards,
Andy Shevchenko