On Fri, Aug 20, 2021 at 5:42 PM Jacopo Mondi <jacopo@xxxxxxxxxx> 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.
...
> > > +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.
...
> > > + 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!
...
> > > + 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 :-)
...
> > > + 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?
...
> > > + /* 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.
...
> > > +
> > > + return 0;
> >
> > Dead code?
>
> It is, but it seems nicer :) Should I drop it ?
We don't need dead code in the kernel.
--
With Best Regards,
Andy Shevchenko
