Re: [PATCH 2/2 v4] pinctrl: introduce generic pin config

[Linus Walleij]

On Wed, Nov 30, 2011 at 8:45 PM, Stephen Warren <swarren@xxxxxxxxxx> wrote:
> Linus Walleij wrote at Thursday, November 24, 2011 11:46 AM:

>> +void pinconf_generic_dump_pin(struct pinctrl_dev *pctldev,
> ...
>> +     for(i = 0; i < ARRAY_SIZE(conf_items); i++) {
> ...
>> +             /* We want to check out this parameter */
>> +             config = (unsigned long) conf_items[i].param;
>
> Don't you need to use to_config_packed() here?

Yep! Fixed it.

>> +             ret = pin_config_get(pctldev, pin, &config);
>> +             /* These are legal errors */
>> +             if (ret == -EINVAL || ret == -ENOTSUPP)
>> +                     continue;
>
> Ah, I guess I see why you consider -ENOTSUPP an error that you don't want
> to print to syslog. Maybe you should call _pin_config_get() here which
> doesn't spew messages on -ENOTSUPP, but have the public pin_config_get()
> function spew errors.

I just deleted the error check for now since it's not necessary.
Better keep the code compact and simple.

>> +             if (ret) {
>> +                     seq_printf(s, "ERROR READING CONFIG SETTING %d ", i);
>> +                     continue;
>> +             }
>> +             /* Space between multiple configs */
>> +             seq_puts(s, " ");
>> +             seq_puts(s, conf_items[i].display);
>> +             /* Print unit if available */
>> +             if (conf_items[i].format && config != 0)
>> +                     seq_printf(s, " (%lu %s)", config,
>> +                                conf_items[i].format);
>
> Don't you need to use to_config_argument() here?

Yes. I'm too sloppy :-(

>> @@ -169,6 +169,8 @@ static void pinconf_dump_pin(struct pinctrl_dev *pctldev,
>>  {
>>       const struct pinconf_ops *ops = pctldev->desc->confops;
>>
>> +     /* no-op when not using generic pin config */
>> +     pinconf_generic_dump_pin(pctldev, s, pin);
>>       if (ops && ops->pin_config_dbg_show)
>>               ops->pin_config_dbg_show(pctldev, s, pin);
>>  }
>
> This is really a comment on the previous patch, but what about config
> params that only apply to groups?

Oh! I need to create a separate debugfs file for that.
Thanks for this nice catch!

>> diff --git a/include/linux/pinctrl/pinconf-generic.h b/include/linux/pinctrl/pinconf-generic.h
>
>> +/*
>> + * You shouldn't even be able to compile with these enums etc unless you're
>> + * using generic pin config. That is why this is defined out.
>> + */
>> +#ifdef CONFIG_GENERIC_PINCONF
>
> Hmm. I'd prefer to have drivers able to use both generic values and
> extend them with custom values. Can't we just use the top bit of the
> param value to indicate 0:standard (from the enum below) 1:custom
> (something meaningful to only the individual pinctrl driver). This
> could then trigger calling pinconf_generic_dump_pin() or not for
> example.

Hm...

That will get messy when if I refactor this stuff, add new enums
and whatever.

>> +#ifdef CONFIG_GENERIC_PINCONF
>> +     bool is_generic;
>> +#endif
>
> ... and get rid of that flag.

This is for the case wher you have both generic and non-generic
config controllers onboard a system.

Like in the generic debugfs dump function:

	if (!ops->is_generic)
		return;

If I take this out, the generic debugfs code will be used for
everything.

And then the generic sematics which you didn't like in the
previous patch:

if (ret == -EINVAL || ret == -ENOTSUPP)

Need to go back in, else the generic debugfs stuff won't
work.

I'm really not sure about that kind of extension, it feels to
me like a hard-to grasp unstable middleground between two
clean-cut solutions "all custom" or "all generic".

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/