Hi Andy,
On Fri, Sep 03, 2021 at 06:36:44PM +0300, Andy Shevchenko wrote:
> On Fri, Sep 3, 2021 at 5:50 PM Jacopo Mondi <jacopo@xxxxxxxxxx> wrote:
> >
> > Add support for the Senseair Sunrise 006-0-0007 driver through the
> > IIO subsystem.
>
> ...
>
> > +static int sunrise_read_byte(struct sunrise_dev *sunrise, u8 reg)
> > +{
> > + unsigned int val;
> > + int ret;
> > +
> > + /*
> > + * Lock the 'wakeup' session.
> > + *
> > + * If another read/write call sneaks in between the wakeup message
> > + * and the i2c transaction, the chip goes back in sleep state.
> > + */
>
> > + mutex_lock(&sunrise->wakeup_lock);
> > + sunrise_wakeup(sunrise);
> > + ret = regmap_read(sunrise->regmap, reg, &val);
> > + mutex_unlock(&sunrise->wakeup_lock);
>
> Seems to me that you may redefine ->read() for regmap (but double
> check this, esp. in regard to bulk transfers) with wakeup implied and
> in that case you probably can use regmap's lock only.
>
Can you point me to an example where regmap's read is redefined ? I
failed to find one at a quick look.
> > + if (ret) {
>
> > + dev_err(&sunrise->client->dev,
> > + "Read byte failed: reg 0x%2x (%d)\n", reg, ret);
>
> With the same LOCs I slightly prefer temporary variable
>
> struct device *dev = ...;
> ...
> dev_err(dev, ...); // on a single line
>
> Ditto everywhere.
Ugh! I initially had a *dev pointer for the sake of line length in
error messages in the driver's struct, then I'm asked to remove it,
then I'm asked to take a pointer to re-shorten the lines.
>
> > + return ret;
> > + }
> > +
> > + return val;
> > +}
>
> ...
>
> > +static int sunrise_read_word(struct sunrise_dev *sunrise, u8 reg, u16 *val)
> > +{
> > + __be16 be_val;
> > + int ret;
> > +
> > + mutex_lock(&sunrise->wakeup_lock);
> > + sunrise_wakeup(sunrise);
>
> > + ret = regmap_bulk_read(sunrise->regmap, reg, &be_val, 2);
>
> sizeof(be_val)
>
ack
> > + mutex_unlock(&sunrise->wakeup_lock);
> > + if (ret) {
> > + dev_err(&sunrise->client->dev,
> > + "Read word failed: reg 0x%2x (%d)\n", reg, ret);
> > + return ret;
> > + }
> > +
> > + *val = be16_to_cpu(be_val);
> > +
> > + return 0;
> > +}
>
> ...
>
> > +static int sunrise_write_word(struct sunrise_dev *sunrise, u8 reg, u16 data)
> > +{
> > + __be16 be_data = cpu_to_be16(data);
> > + int ret;
> > +
> > + mutex_lock(&sunrise->wakeup_lock);
> > + sunrise_wakeup(sunrise);
> > + ret = regmap_bulk_write(sunrise->regmap, reg, &be_data, 2);
> > + mutex_unlock(&sunrise->wakeup_lock);
> > + if (ret) {
> > + dev_err(&sunrise->client->dev,
> > + "Write word failed: reg 0x%2x (%d)\n", reg, ret);
>
> > + return ret;
> > + }
> > +
> > + return 0;
>
> return ret;
>
I can return a positive value for success and change the checks around
return code to if (ret < 0) but that's driver internal stuff after
all, does it really matter ? Is this more consistent with the Linux
i2c API maybe ? I can change it if it's the case.
> > +}
>
> ...
>
> > + /* Write a calibration command and poll the calibration status bit. */
> > + ret = sunrise_write_word(sunrise, SUNRISE_CALIBRATION_COMMAND_REG,
> > + data->cmd);
>
> Dunno how long it is, but to me seems one line is okay.
>
This is getting frustrating, and that's not completely on you.
In other subsystems I get asked to stay in the 80-cols limits no
matter what. Here there's an arbitrary limit according to the
reviewers' tastes. I feel when we had the strict 80-cols limits it was
easier as I didn't have to please each one preferences.
If I read this
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=bdc48fa11e46f867ea4d75fa59ee87a7f48be144
it seems that the only relevant thing is not to break user visible
messages (which was kind of implied before), everything else goes, up
to 100-cols where you should ask yourself "what are you doing",
according to Linus words.
I've been suggested "up to 90 is probably fine", then I read 100 in
the commit message, but 80 is still _preferred_ according to the
commit message again.
I wish I could count how many characters we have typed to discuss
about this just during the review of this small driver. Seems like a
lot of churn and time to me.
> > + if (ret)
> > + return ret;
>
> ...
>
> > +static ssize_t sunrise_cal_read(const char *buf, size_t len)
> > +{
> > + bool enable;
> > + int ret;
> > +
> > + ret = kstrtobool(buf, &enable);
> > + if (ret)
> > + return ret;
> > +
> > + if (!enable)
> > + return len;
> > +
> > + return 0;
>
> Why is this a separate function to begin with?
Because it is called from two places where I should have duplicated
the code otherwise ?
>
> Not sure I have got the logic behind. If enable is true you return 0?!
>
Yes, so I can
if (ret)
return ret;
in the caller.
> > +}
>
> ...
>
> > +static ssize_t sunrise_cal_factory_write(struct iio_dev *iiodev,
> > + uintptr_t private,
> > + const struct iio_chan_spec *chan,
> > + const char *buf, size_t len)
> > +{
> > + struct sunrise_dev *sunrise = iio_priv(iiodev);
> > + int ret;
> > +
> > + mutex_lock(&sunrise->lock);
> > + ret = sunrise_cal_read(buf, len);
>
> > + if (ret) {
> > + mutex_unlock(&sunrise->lock);
> > + return ret;
> > + }
>
> Possibly
>
> if (ret)
> goto out_unlock;
>
ack
> > + ret = sunrise_calibrate(sunrise,
> > + &calib_data[SUNRISE_CALIBRATION_FACTORY]);
>
> One line?
>
> out_unlock:
>
> > + mutex_unlock(&sunrise->lock);
> > + if (ret)
> > + return ret;
> > +
> > + return len;
> > +}
>
> ...
>
> > + errors = value;
> > + for_each_set_bit(i, &errors, ARRAY_SIZE(sunrise_error_statuses))
> > + len += sysfs_emit_at(buf, len, "%s ", sunrise_error_statuses[i]);
>
> > +
>
> Can drop this (up to you).
>
> > + if (len)
> > + buf[len - 1] = '\n';
>
> ...
>
> > + ret = sunrise_read_word(sunrise, SUNRISE_CO2_FILTERED_COMP_REG,
> > + &value);
>
> One line?
>
Ufff :)
> ...
>
> > + ret = sunrise_read_word(sunrise, SUNRISE_CHIP_TEMPERATURE_REG,
> > + &value);
>
> Ditto.
>
> ...
>
> > + /* x10 to comply with IIO scale (1/1000 degress celsius). */
>
> 1/1000 --> millidegrees
> (Also note spelling)
Thanks for the spelling warning. I'll fix.
Thanks
j
>
> --
> With Best Regards,
> Andy Shevchenko
