From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3B819C433ED for ; Wed, 7 Apr 2021 20:47:49 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 0AC48610F8 for ; Wed, 7 Apr 2021 20:47:49 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231185AbhDGUr6 (ORCPT ); Wed, 7 Apr 2021 16:47:58 -0400 Received: from mail.pqgruber.com ([52.59.78.55]:39958 "EHLO mail.pqgruber.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229586AbhDGUr5 (ORCPT ); Wed, 7 Apr 2021 16:47:57 -0400 Received: from workstation.tuxnet (213-47-165-233.cable.dynamic.surfer.at [213.47.165.233]) by mail.pqgruber.com (Postfix) with ESMTPSA id 98964C6B24A; Wed, 7 Apr 2021 22:47:46 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pqgruber.com; s=mail; t=1617828466; bh=ABMI0EjtBbX5kyaLQx8W5hqb07nbu/QgaIGt0Obbm7A=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=xkrCS7QRsSaS0nLAC91OgjhcPtmPAg7uODIsItOQ19L0/F5Ay0W8JHw58ubc8e77d sCB0uXAniAww2PIfOz8+zZsnaPShGuzIcoF6NwzLbTPMYaPdLUv2+tttq7onOJtj20 c2tPI+/mMkabAQfCCzyok5TFSUcRT6xy9rzO7Imw= Date: Wed, 7 Apr 2021 22:47:45 +0200 From: Clemens Gruber To: Uwe =?iso-8859-1?Q?Kleine-K=F6nig?= Cc: linux-pwm@vger.kernel.org, Thierry Reding , Sven Van Asbroeck , devicetree@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v7 8/8] pwm: pca9685: Add error messages for failed regmap calls Message-ID: References: <20210406164140.81423-1-clemens.gruber@pqgruber.com> <20210406164140.81423-8-clemens.gruber@pqgruber.com> <20210407061619.fl6ffos6csvgtnjh@pengutronix.de> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20210407061619.fl6ffos6csvgtnjh@pengutronix.de> Precedence: bulk List-ID: X-Mailing-List: devicetree@vger.kernel.org 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 > > --- > > 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