linux-kernel - Re: [PATCH 02/12] pinctrl: Add core pinctrl support for Aspeed SoCs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CACRpkdZ0p+jNHBV1AqhVWQ2Q8RRrHVfsAz3XU3_+f6My22=Zxg@mail.gmail.com>
Date:	Thu, 11 Aug 2016 10:41:16 +0200
From:	Linus Walleij <linus.walleij@...aro.org>
To:	Andrew Jeffery <andrew@...id.au>
Cc:	Alexandre Courbot <gnurou@...il.com>,
	Joel Stanley <joel@....id.au>,
	Mark Rutland <mark.rutland@....com>,
	Rob Herring <robh+dt@...nel.org>,
	Russell King <linux@...linux.org.uk>,
	Benjamin Herrenschmidt <benh@...nel.crashing.org>,
	Jeremy Kerr <jk@...abs.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 02/12] pinctrl: Add core pinctrl support for Aspeed SoCs

On Wed, Jul 20, 2016 at 7:58 AM, Andrew Jeffery <andrew@...id.au> wrote:

> --- a/arch/arm/mach-aspeed/Kconfig
> +++ b/arch/arm/mach-aspeed/Kconfig
> @@ -5,6 +5,7 @@ menuconfig ARCH_ASPEED
>         select WATCHDOG
>         select ASPEED_WATCHDOG
>         select MOXART_TIMER
> +       select PINCTRL
>         help
>           Say Y here if you want to run your kernel on an ASpeed BMC SoC.

This needs to be a separate patch sent to the ARM SoC tree.
I don't like to merge patches to other subsystems if it can be
avoided.

> +static inline void aspeed_sig_desc_print_val(
> +               const struct aspeed_sig_desc *desc, bool enable, u32 rv)
> +{
> +#if defined(CONFIG_DEBUG_PINCTRL)
> +       pr_debug("SCU%x[0x%08x]=0x%x, got 0x%x from 0x%08x\n", desc->reg,
> +                       desc->mask, enable ? desc->enable : desc->disable,
> +                       (rv & desc->mask) >> __ffs(desc->mask), rv);
> +#endif
> +}

You can just use pr_debug(). CONFIG_DEBUG_PINCTRL enables
DEBUG_KERNEL which activates debug prints so this is a truism.

> +static bool aspeed_sig_desc_eval(const struct aspeed_sig_desc *desc,
> +               bool enabled, struct regmap *map)
> +static bool aspeed_sig_expr_eval(const struct aspeed_sig_expr *expr,
> +               bool enabled, struct regmap *map)

These need kerneldoc too, they are kind of hard to understand.

> +static bool aspeed_gpio_in_exprs(const struct aspeed_sig_expr **exprs)
> +{
> +       if (!exprs)
> +               return false;
> +
> +       while (*exprs) {
> +               if (strncmp((*exprs)->signal, "GPIO", 4) == 0)
> +                       return true;

This looks a bit fragile and hard to debug. Do you have some better
idea of how to do this but not resort to string comparison?

Apart from that it looks pretty alright, complex but such is life
with complex hardware.

Yours,
Linus Walleij