Re: [PATCH v5 1/7] pwm: pca9685: Switch to atomic API

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 1750 bytes --]

On Thu, Dec 17, 2020 at 12:10:10PM -0500, Sven Van Asbroeck wrote:
> On Thu, Dec 17, 2020 at 11:48 AM Clemens Gruber
> <clemens.gruber@pqgruber.com> wrote:
> >
> > I can initialize the values to 0 of course and check the file for other
> > places with missing initializations.
> >
> > Or would it be better to check the return codes of regmap_read/write in
> > such cases? I'm not sure.
> 
> I think that checking the regmap_read/write return values is overkill
> in this driver. These functions can't realistically fail, except if the i2c
> bus is bad, i.e. h/w failure or intermittency. And that's an externality
> which I believe we can ignore.

I think there are (rare) occasions where it's fine to not check for
errors, i.e. if you definitively know that calls can't fail. However,
given that this uses regmap and you don't really know what's backing
this, I think it's always better to err on the side of caution and
properly check the return values.

The fact that this can be externally caused is actually a reason why
we shouldn't be ignoring any errors. If there's a chip that's hogging
the I2C bus or if you've even just mistyped the I2C client's address
in DT, it's better if the PWM driver tells you with an error message
than if it is silently ignoring the errors and keeps you guessing at
why the PWM isn't behaving the way it should.

Granted, the error code isn't always going to pinpoint exactly what's
going wrong, but for serious errors often the I2C bus driver will let
you know with an extra error message. However, it's much easier to go
looking for that error message if the PWM driver lets you know that
something went wrong.

Please just add full checking of regmap operations.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]