Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1460272 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 02/12] pinctrl: Add core pinctrl support for Aspeed SoCs Linus Walleij <linus.walleij@linaro.org> - 2016-08-11 10:50 +0200
    Re: [PATCH 02/12] pinctrl: Add core pinctrl support for Aspeed SoCs Andrew Jeffery <andrew@aj.id.au> - 2016-08-12 02:40 +0200
      Re: [PATCH 02/12] pinctrl: Add core pinctrl support for Aspeed SoCs Linus Walleij <linus.walleij@linaro.org> - 2016-08-12 15:20 +0200
        Re: [PATCH 02/12] pinctrl: Add core pinctrl support for Aspeed SoCs Benjamin Herrenschmidt <benh@kernel.crashing.org> - 2016-08-13 03:40 +0200
          Re: [PATCH 02/12] pinctrl: Add core pinctrl support for Aspeed SoCs Andrew Jeffery <andrew@aj.id.au> - 2016-08-15 02:40 +0200

#1460272 — Re: [PATCH 02/12] pinctrl: Add core pinctrl support for Aspeed SoCs

On Wed, Jul 20, 2016 at 7:58 AM, Andrew Jeffery <andrew@aj.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

[toc] | [next] | [standalone]

#1460878

[Multipart message — attachments visible in raw view] — view raw

On Thu, 2016-08-11 at 10:41 +0200, Linus Walleij wrote:
> On Wed, Jul 20, 2016 at 7:58 AM, Andrew Jeffery <andrew@aj.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.

Okay, I'll split it out.

> 
> > 
> > +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.

Right, I will clean that up.

> 
> > 
> > +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.

Will do.

> 
> > 
> > +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?

Yes, this is a little unfortunate. GPIO is not always a pin's lowest
priority function (e.g. the RGMII/RMII pins), so this makes the GPIO
case like any other mux function: We need to know when to stop
iterating the arrays when disabling mux functions of higher priority.
The alternative is probably to introduce another field to struct
aspeed_sig_expr and set that as necessary, but that feels redundant if
we keep to a consistent naming for the GPIOs.

Would it be acceptable to document that requirement? Maybe that's just
punting on the problem because it doesn't make it any less difficult to
debug. However, the failure case is already tested in
aspeed_gpio_request_enable() (where all aspeed_gpio_in_exprs() calls
fail for a pin) and to make it easier to debug I can dev_warn() at that
point.

I will do both of the above as part of a v2 unless you are really keen
for an alternative.

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

Mmm, yes. I keep hoping for a day when someone else points out that it
actually has a simple solution so I stop dreading the explanation of
the implementation's mechanics to others.

Anyway, thanks for the review!

Andrew

> 
> Yours,
> Linus Walleij

[toc] | [prev] | [next] | [standalone]

#1461172

On Fri, Aug 12, 2016 at 2:33 AM, Andrew Jeffery <andrew@aj.id.au> wrote:

>> > +
>> > +       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?
>
> Yes, this is a little unfortunate. GPIO is not always a pin's lowest
> priority function (e.g. the RGMII/RMII pins), so this makes the GPIO
> case like any other mux function: We need to know when to stop
> iterating the arrays when disabling mux functions of higher priority.
> The alternative is probably to introduce another field to struct
> aspeed_sig_expr and set that as necessary, but that feels redundant if
> we keep to a consistent naming for the GPIOs.

I would probably prefer that option (introduce another field)
but you should make the overall decision, it's no strong opinion
from my side.

> Would it be acceptable to document that requirement?

Sure.

Yours,
Linus Walleij

[toc] | [prev] | [next] | [standalone]

#1461565

On Fri, 2016-08-12 at 15:18 +0200, Linus Walleij wrote:
> I would probably prefer that option (introduce another field)
> but you should make the overall decision, it's no strong opinion
> from my side.
> 
> > Would it be acceptable to document that requirement?

It might make it a bit less nasty (and easier to change later on
if necessary) to use some kind of:

	bool ast_signal_is_gpio(...)

And stick the strcmp in there.

Cheers,
Ben.

[toc] | [prev] | [next] | [standalone]

#1462510

[Multipart message — attachments visible in raw view] — view raw

On Sat, 2016-08-13 at 10:58 +1000, Benjamin Herrenschmidt wrote:
> On Fri, 2016-08-12 at 15:18 +0200, Linus Walleij wrote:
> > 
> > I would probably prefer that option (introduce another field)
> > but you should make the overall decision, it's no strong opinion
> > from my side.
> > 
> > > 
> > > Would it be acceptable to document that requirement?
> It might make it a bit less nasty (and easier to change later on
> if necessary) to use some kind of:
> 
> 	bool ast_signal_is_gpio(...)
> 
> And stick the strcmp in there.

Yep, I'll do so for v2.

Cheers,

Andrew

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web