Messages in this thread | | | From | Andy Shevchenko <> | Date | Wed, 22 Apr 2020 22:49:44 +0300 | Subject | Re: [PATCH 1/6] iio: chemical: scd30: add core driver |
| |
On Wed, Apr 22, 2020 at 5:22 PM Tomasz Duszynski <tomasz.duszynski@octakon.com> 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@octakon.com>
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
| |