On Fri, Sep 3, 2021 at 7:42 PM Jacopo Mondi <jacopo@xxxxxxxxxx> 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@xxxxxxxxxx> 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. ... > > > +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; > > > +} ... > > > +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. > > 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. > > > > +} -- With Best Regards, Andy Shevchenko
