Re: [PATCH v2 2/3] iio: chemical: Add Sensteair Sunrise 006-0-007 driver

[Jacopo Mondi <jacopo@jmondi.org>]

Hi Andy,

On Fri, Aug 20, 2021 at 05:56:48PM +0300, Andy Shevchenko wrote:
> On Fri, Aug 20, 2021 at 5:42 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> ...
>
> > > > +#include <linux/i2c.h>
> > >
> > > > +#include <linux/iio/iio.h>
> > > > +#include <linux/iio/sysfs.h>
> > >
> > > Can you move this as a separate group...
> > >
> > > > +#include <linux/kernel.h>
> > > > +#include <linux/mod_devicetable.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/mutex.h>
> > > > +#include <linux/regmap.h>
> > > > +#include <linux/sysfs.h>
> > >
> > > ...here ?
> >
> > Ah, is this customary in the subsystem ?
>
> When it's a group of headers for a certain subsystem it makes it clear
> that the driver belongs to it.

Ack, I'll move

>
> ...
>
> > > > +static void sunrise_wakeup(struct sunrise_dev *sunrise)
> > > > +{
> > > > +       struct i2c_client *client = sunrise->client;
> > > > +
> > > > +       /*
> > > > +        * Wake up sensor by sending sensor address: START, sensor address,
> > > > +        * STOP. Sensor will not ACK this byte.
> > > > +        *
> > > > +        * The chip returns in low power state after 15msec without
> > > > +        * communications or after a complete read/write sequence.
> > > > +        */
> > >
> > > I'm wondering if there is a better way to perform this.
> >
> > Do you mean using a different API instead of i2c_smbus_xfer()?
>
> I meant on hw level.

That doesn't seem to be possible, I'm afraid.

The chip manual clearly reports that the chip needs to be waken up as
it enters low power state automatically after 15ms without
communications or after a completed transaction

Quoting TDE5531 which is now referenced in the commit message:

----
Senseair Sunrise spend most of its time in deep sleep mode to minimize
power consumption, this have the effect that it is necessary to wake
up the sensor before it is possible to communicate with it.  Sensor
will wake up on a falling edge on SDA, it is recommended to send
sensors address to wake it up. When sensors address is used to wake up
the sensor the sensor will not acknowledge this byte.

Communication sequence:
1) Wake up sensor by sending sensor address (START, sensor address,
STOP). Sensor will not ACK this byte.

2) Normal I2C read/write operations. I2C communication must be started
within 15ms after the wake-up byte, each byte sent to or from the
sensor sets the timeout to 15 ms. After a complete read or write
sequence sensor will enter sleep mode immediately.
---

> ...
>
> > > > +               dev_err(sunrise->dev, "Read word failed: reg 0x%2x (%d)\n",
> > > > +                       reg, ret);
> > >
> > > One line?
> >
> > Goes over 80, as all the other identical comments below.
>
> It's still less than 90, so go for it!

Where does the 90 cols limit comes from ? :)

Anyway, I guess that's up to the subsystem, so I'll re-arrange lines
close to 80 cols

>
> ...
>
> > > > +       ret = sunrise_calibrate(sunrise);
> > > > +
> > > > +       return ret ?: len;
> > >
> > > In this case
> > >
> > >   if (ret)
> > >     return ret;
> > >
> > > return len;
> > >
> > > will look more natural.
> >
> > I had this and I switched before sending. Matter of tastes I guess.
> > I actually changed this as I thought it would have pleased you :)
>
> Use your common sense. Not every place where you can put it really needs it.
> Either way you choose is okay because of style, but to be sure ask the
> maintainer :-)

It's a really minor detail, and I also prefer the style you suggested
so I'll go for it.

>
> ...
>
> > > > +       for (i = 0; i < ARRAY_SIZE(error_codes); ++i) {
> > > > +               if (!(value & BIT(error_codes[i])))
> > > > +                       continue;
> > >
> > > for_each_set_bit()
> >
> > only 12 bits are valid, the top 4 are undocumented. They -should- be
> > zeroed but who knows.
>
> I didn't get your answer. The limit for the for-loop in both cases
> will be the same, but for_each_set_bit() is what you open-coded. Why
> do you need the open-coded variant, what is the benefit?
>

I should have looked at the for_each_set_bit() definition to find out
that it supports a size argument. I assumed it ranged on the full size
of the variable to inspect. Sorry for being sloppy ;)

> ...
>
> > > > +       /* Error statuses. */
> > > > +       {
> > > > +               .name = "error_status",
> > > > +               .read = sunrise_error_status_read,
> > > > +               .shared = IIO_SEPARATE,
> > > > +       },
> > > > +       IIO_ENUM_AVAILABLE("error_status", &sunrise_error_statuses_enum),
> > >
> > > > +       {},
> > >
> > > No comma for terminator entries.
>
> I assume non-commented ones are agreed on.
>

Ack

> ...
>
> > > > +
> > > > +       return 0;
> > >
> > > Dead code?
> >
> > It is, but it seems nicer :) Should I drop it ?
>
> We don't need dead code in the kernel.

Nobody does actually, so I'll drop this.

Thanks
   j

>
> --
> With Best Regards,
> Andy Shevchenko