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 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id A1347C433F5 for ; Wed, 13 Oct 2021 16:58:34 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 8A24461139 for ; Wed, 13 Oct 2021 16:58:34 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231317AbhJMRAg (ORCPT ); Wed, 13 Oct 2021 13:00:36 -0400 Received: from mga12.intel.com ([192.55.52.136]:20038 "EHLO mga12.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230415AbhJMRAf (ORCPT ); Wed, 13 Oct 2021 13:00:35 -0400 X-IronPort-AV: E=McAfee;i="6200,9189,10136"; a="207594565" X-IronPort-AV: E=Sophos;i="5.85,371,1624345200"; d="scan'208";a="207594565" Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga106.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 13 Oct 2021 09:58:31 -0700 X-IronPort-AV: E=Sophos;i="5.85,371,1624345200"; d="scan'208";a="547986866" Received: from smile.fi.intel.com (HELO smile) ([10.237.72.159]) by fmsmga004-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 13 Oct 2021 09:58:23 -0700 Received: from andy by smile with local (Exim 4.95) (envelope-from ) id 1makM2-0001gg-8h; Wed, 13 Oct 2021 22:55:58 +0300 Date: Wed, 13 Oct 2021 22:55:58 +0300 From: Andy Shevchenko To: Emil Renner Berthing Cc: linux-riscv , devicetree , linux-clk , "open list:GPIO SUBSYSTEM" , "open list:SERIAL DRIVERS" , Palmer Dabbelt , Paul Walmsley , Rob Herring , Michael Turquette , Stephen Boyd , Thomas Gleixner , Marc Zyngier , Philipp Zabel , Linus Walleij , Greg Kroah-Hartman , Daniel Lezcano , Jiri Slaby , Maximilian Luz , Sagar Kadam , Drew Fustini , Geert Uytterhoeven , Anup Patel , Atish Patra , Matteo Croce , Linux Kernel Mailing List , Huan Feng Subject: Re: [PATCH v1 12/16] pinctrl: starfive: Add pinctrl driver for StarFive SoCs Message-ID: References: <20211012134027.684712-1-kernel@esmil.dk> <20211012134027.684712-13-kernel@esmil.dk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo Precedence: bulk List-ID: X-Mailing-List: linux-clk@vger.kernel.org On Wed, Oct 13, 2021 at 06:38:14PM +0200, Emil Renner Berthing wrote: > On Tue, 12 Oct 2021 at 19:03, Andy Shevchenko wrote: > > On Tue, Oct 12, 2021 at 4:43 PM Emil Renner Berthing wrote: ... > > > +free_pinmux: > > > + devm_kfree(dev, pinmux); > > > +free_pins: > > > + devm_kfree(dev, pins); > > > +free_grpname: > > > + devm_kfree(dev, grpname); > > > > What the heck?! > > Just to be clear. You mean we don't need to explicitly free them > because they're tied to the device right? I don't think the device > will go away just because a single device tree entry can't be parsed, > so on such errors this garbage would be left behind. You can still > argue we shouldn't optimize for broken device trees, I just want to > make it at conscious decision. If you are using devm_kfree() it is quite likely shows either of the following issues: * you mustn't use devm_*() in the first place due to object lifetime; * you shouldn't use devm_kfree() since this is the whole point of devm. > > > +free_pgnames: > > > + devm_kfree(dev, pgnames); > > > > Ditto. ... > > > +out: > > > > Useless label. > > Hmm.. my compiler disagrees. The comment implies that you return directly instead of using `goto out;`. > > > + return ret; ... > > > + v = pinmux[i]; > > > + dout = ((v & BIT(7)) << (31 - 7)) | ((v >> 24) & 0xffU); > > > + doen = ((v & BIT(6)) << (31 - 6)) | ((v >> 16) & 0xffU); > > > + din = (v >> 8) & 0xffU; > > > > What is this voodoo for? > > In order to do pinmux we need the following pieces of information from > the device tree for each pin ("GPIO" they call it): > > output signal: 0-133 + 1bit reverse flag > output enable signal: 0-133 + 1bit reverse flag > optional input signal: 0-74 + special "none" value, right now 0xff > gpio number: 0-63 > > As the code is now all that info is packed into a u32 for each pin > using the GPIOMUX macro defined in the dt-binding header added in > patch 10. There is also a diagram for how this packing is done. The > above voodoo is for unpacking that. > > I'd very much like to hear if you have a better solution for how to > convey that information from the device tree to here. At very least this code should have something like above in the comment. ... > > > + if (din != 0xff) > > > + reg_din = sfp->base + GPIO_IN_OFFSET + 4 * din; > > > + else > > > + reg_din = NULL; > > > > This looks like you maybe use gpio-regmap instead? > > This was discussed at length when Drew sent in the GPIO part of this code: > https://lore.kernel.org/linux-riscv/20210701002037.912625-1-drew@beagleboard.org/ > The conclusion was that because pinmux and controlling the pins from > software in GPIO mode uses the same registers it is better to do a > combined driver like this that can share the lock among other things. And what does prevent exactly to base the GPIO part on gpio-regmap? ... > > > +static const unsigned char starfive_drive_strength[] = { > > > + 14, 21, 28, 35, 42, 49, 56, 63, > > > > Why table? Can you simply use the formula?! > > Heh, yeah. So these are rounded values from a table and I never > noticed that after rounding they follow a nice arithmetic progression. > It'll probably still be nice to have an explanation in the comments > about the formula then. Yup! > > > +}; ... > > > + irq_set_handler_locked(d, handle_bad_irq); > > > > Why is this here? Move it to ->probe(). > > My thinking was that if something tries to set a an unsupported irq > type, we should make sure the caller doesn't get spurious interrupts > because we left the handler at its old value. You already assigned to this handler in the ->probe(), what's this then? ... > > > + if (value <= 6) > > > + writel(value, sfp->padctl + IO_PADSHARE_SEL); > > > + else > > > > > + dev_err(dev, "invalid signal group %u\n", value); > > > > Why _err if you not bail out here? > > My thinking was that if the device tree specifies an invalid signal > group we should just leave the setting alone and not break booting, > but still be loud about it. Maybe that's too lenient and it's better > to crash and burn immediately if someone does that. Here is inconsistency between level of the message and following action. There are (rare!) cases when it's justified, but I believe it's not the case here. You have two choices or justify why you have to use error level without stopping process. ... All uncommented stuff you agreed on, correct? -- With Best Regards, Andy Shevchenko