Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
IronPort-SDR: hUQp4x/SJplnpc8Mi6XCZU/c0UXYRueMezf0dHQ42xp1f9d6l1l2c4DtcwRIgXGPILmWLrG5J6
 zJ0mc8gJsxgw==
IronPort-SDR: jWOEcGzq6troy1NP13CLTFfaqvuZUeJBoVkPvBeW4r3vbRUuadDgQPeiLaL7c3iQMg08GSayWH
 UKC1wj2Ts6RA==
Date:   Thu, 24 Sep 2020 16:23:34 +0300
From:   Andy Shevchenko <andriy.shevchenko@intel.com>
To:     Uwe =?iso-8859-1?Q?Kleine-K=F6nig?= 
        <u.kleine-koenig@pengutronix.de>
Cc:     Rahul Tanwar <rahul.tanwar@linux.intel.com>,
        linux-pwm@vger.kernel.org, lee.jones@linaro.org,
        thierry.reding@gmail.com, p.zabel@pengutronix.de,
        robh+dt@kernel.org, linux-kernel@vger.kernel.org,
        devicetree@vger.kernel.org, songjun.Wu@intel.com,
        cheol.yong.kim@intel.com, qi-ming.wu@intel.com,
        rahul.tanwar.linux@gmail.com, rtanwar@maxlinear.com
Subject: Re: [PATCH v13 2/2] Add PWM fan controller driver for LGM SoC
Message-ID: <20200924132334.GT3956970@smile.fi.intel.com>
References: <cover.1600158087.git.rahul.tanwar@linux.intel.com>
 <befa655d8beb326fc8aa405a25a8b3e62b7e6a4a.1600158087.git.rahul.tanwar@linux.intel.com>
 <20200924065534.e2anwghhtysv63e7@pengutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20200924065534.e2anwghhtysv63e7@pengutronix.de>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Precedence: bulk

On Thu, Sep 24, 2020 at 08:55:34AM +0200, Uwe Kleine-K?nig wrote:
> On Tue, Sep 15, 2020 at 04:23:37PM +0800, Rahul Tanwar wrote:

...

> > +	ret = lgm_clk_enable(dev, pc);
> > +	if (ret) {
> > +		dev_err(dev, "failed to enable clock\n");
> 
> You used dev_err_probe four times for six error paths. I wonder why you
> didn't use it here (and below for a failing pwmchip_add()).

dev_err_probe() makes sense when we might experience deferred probe. In neither
of mentioned function this can be the case.

> > +		return ret;
> > +	}

...

> > +	ret = lgm_reset_control_deassert(dev, pc);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "cannot deassert reset control\n");
> 
> After lgm_reset_control_deassert is called pc->rst is unused. So there
> is no need to have this member in struct lgm_pwm_chip. The same applies
> to ->clk. (You have to pass rst (or clk) to devm_add_action_or_reset for
> that to work. Looks like a nice idea anyhow.)

True. And above dev_err_probe() is not needed.

-- 
With Best Regards,
Andy Shevchenko