From mboxrd@z Thu Jan 1 00:00:00 1970 From: Philipp Zabel Subject: Re: [PATCH v7] reset: Add driver for gpio-controlled reset pins Date: Thu, 30 May 2013 11:08:48 +0200 Message-ID: <1369904928.24693.6.camel@pizza.hi.pengutronix.de> References: <1369753575-9818-1-git-send-email-p.zabel@pengutronix.de> <51A68CF8.7000303@wwwdotorg.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <51A68CF8.7000303-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: devicetree-discuss-bounces+gldd-devicetree-discuss=m.gmane.org-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org Sender: "devicetree-discuss" To: Stephen Warren Cc: Marek Vasut , Fabio Estevam , Mike Turquette , Len Brown , Sascha Hauer , "Rafael J. Wysocki" , Pavel Machek , devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org List-Id: devicetree@vger.kernel.org Hi Stephen, Am Mittwoch, den 29.05.2013, 17:19 -0600 schrieb Stephen Warren: > On 05/28/2013 09:06 AM, Philipp Zabel wrote: > > This driver implements a reset controller device that toggle a gpio > > connected to a reset pin of a peripheral IC. The delay between assertion > > and de-assertion of the reset signal can be configured via device tree. > > > diff --git a/drivers/reset/gpio-reset.c b/drivers/reset/gpio-reset.c > > > +static void __gpio_reset_set(struct reset_controller_dev *rcdev, int asserted) > > Nit: Technically I think __ is a reserved name-space, so you shouldn't > prefix "user-level" (rather than libc/similar) symbols with it, but it's > not a big deal. I'll s/__//. > > +static int gpio_reset_probe(struct platform_device *pdev) > > > + if (of_gpio_named_count(np, "reset-gpios") != 1) > > + return -EINVAL; > > Should that error-path (and the path for of_property_read_u32() failure) > include a dev_err() like some already do, so it's obvious what the > failure is? For reset-gpios, yes. reset-delay-us is marked as optional in the bindings. > > + reset_controller_register(&drvdata->rcdev); > > + > > + platform_set_drvdata(pdev, drvdata); > > It might be better to set the drvdata right after it's been allocated. > It perhaps doesn't matter much here, but if the reset controller core > ever starts calling into the registered device inside the call to > reset_controller_register(), then there might be a problem. Alright. I'll move ist above the reset_controller_register() call. > Aside from those minor issues, > Reviewed-by: Stephen Warren thanks Philipp