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

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 3 Sep 2021 23:21:05 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Fri, Sep 3, 2021 at 7:42 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
> > 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@jmondi.org> wrote:  
> 
> ...
> 
> > > > +       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.  
> 
> Any when struct regmap_config is defined with devm_regmap_i2c_init() call.
> 
> This one is not I²C, but gives you an idea.
> https://elixir.bootlin.com/linux/latest/source/drivers/mfd/intel_soc_pmic_mrfld.c#L98
> 
> ...
> 
> > 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.  
> 
> Up to maintainers then.

Andy is requesting something different here by asking for a local
struct device *dev = sunrise->client->dev;

but personally I wouldn't bother when there is only one use in a given
function. I wouldn't mind if you did do it either - this one is very much
personal taste and so I tend to not worry about it!

Having the extra pointer in iio_priv() was a case of architecture being
a bit 'fuzzy' for convenience which is a different matter.

> 
> ...
> 
> > > > +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.  
> 
> I didn't get what this comment has with what I have proposed.
> 
> Maybe it wasn't obvious, so I have proposed to change 4 LOCs by 1 LOC, so
> 
> if (ret)
>   dev_err(...);
> return ret;

one of the checking scripts tends to moan about this so it's nice to clean it
out at the start and avoid the inevitable follow up patch :)

> 
> > > > +}  
> 
> ...
> 
> > > > +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 ?  
> 
> I think what you think about duplication is not and will get even LOC
> benefit. Using kstrtobool() directly in the callers is better than
> hiding like this.

Part of the issue here is that the naming perhaps suggests a hardware rad
whereas it's just a bit of string handling.

Whilst it's shared code, there is only a rather tangential connection between
the two call sites and the function itself ended up a bit weird.

I'd go with what Andy is suggesting and just have this handling inline.


> 
> > > 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.
> >  
> > > > +}  
> 
>