lkml.org 
[lkml]   [2021]   [Apr]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v7 8/8] pwm: pca9685: Add error messages for failed regmap calls
    On Wed, Apr 07, 2021 at 08:16:19AM +0200, Uwe Kleine-König wrote:
    > On Tue, Apr 06, 2021 at 06:41:40PM +0200, Clemens Gruber wrote:
    > > Regmap operations can fail if the underlying subsystem is not working
    > > properly (e.g. hogged I2C bus, etc.)
    > > As this is useful information for the user, print an error message if it
    > > happens.
    > > Let probe fail if the first regmap_read or the first regmap_write fails.
    > >
    > > Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
    > > ---
    > > Changes since v6:
    > > - Rebased
    > >
    > > drivers/pwm/pwm-pca9685.c | 83 ++++++++++++++++++++++++++++-----------
    > > 1 file changed, 59 insertions(+), 24 deletions(-)
    > >
    > > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
    > > index cf0c98e4ef44..8a4993882b40 100644
    > > --- a/drivers/pwm/pwm-pca9685.c
    > > +++ b/drivers/pwm/pwm-pca9685.c
    > > @@ -107,6 +107,30 @@ static bool pca9685_prescaler_can_change(struct pca9685 *pca, int channel)
    > > return test_bit(channel, pca->pwms_enabled);
    > > }
    > >
    > > +static int pca9685_read_reg(struct pca9685 *pca, unsigned int reg, unsigned int *val)
    > > +{
    > > + struct device *dev = pca->chip.dev;
    > > + int err;
    > > +
    > > + err = regmap_read(pca->regmap, reg, val);
    > > + if (err != 0)
    > > + dev_err(dev, "regmap_read of register 0x%x failed: %d\n", reg, err);
    >
    > Please use %pe to emit the error code instead of %d.

    Will do.

    >
    > > +
    > > + return err;
    > > +}
    > > +
    > > +static int pca9685_write_reg(struct pca9685 *pca, unsigned int reg, unsigned int val)
    > > +{
    > > + struct device *dev = pca->chip.dev;
    > > + int err;
    > > +
    > > + err = regmap_write(pca->regmap, reg, val);
    > > + if (err != 0)
    > > + dev_err(dev, "regmap_write to register 0x%x failed: %d\n", reg, err);
    > > +
    > > + return err;
    > > +}
    > > +
    > > /* Helper function to set the duty cycle ratio to duty/4096 (e.g. duty=2048 -> 50%) */
    > > static void pca9685_pwm_set_duty(struct pca9685 *pca, int channel, unsigned int duty)
    > > {
    > > @@ -115,12 +139,12 @@ static void pca9685_pwm_set_duty(struct pca9685 *pca, int channel, unsigned int
    > >
    > > if (duty == 0) {
    > > /* Set the full OFF bit, which has the highest precedence */
    > > - regmap_write(pca->regmap, REG_OFF_H(channel), LED_FULL);
    > > + pca9685_write_reg(pca, REG_OFF_H(channel), LED_FULL);
    >
    > You didn't check all return codes? How did you select the calls to
    > check?

    No, because it would become a big mess and really obstruct readability
    in my opinion.

    So I chose some kind of middleground:
    I decided to check the first regmap_read and regmap_write in probe and
    return the error code if something goes wrong there.
    If something goes wrong after probe, I only print an error message.

    Is that acceptable?

    Thanks,
    Clemens

    \
     
     \ /
      Last update: 2021-04-07 22:48    [W:2.752 / U:0.164 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site