Re: [PATCH 1/2 v4] pinctrl: add a pin config interface

[Linus Walleij]

On Thu, Dec 1, 2011 at 6:12 PM, Stephen Warren <swarren@xxxxxxxxxx> wrote:
> Linus Walleij wrote at Thursday, December 01, 2011 3:00 AM:

>> since we move to the concept of the core not knowing what
>> it is passing around, it is better to just have an unsigned long and let
>> the driver and its board file/device tree decide what to do with it.
>>
>> Since I don't have any enum for the param anymore, it would
>> just be another unsigned long, so what's the point?
>
> Well, there is an enum, it's just in the second patch set...

That's optional as it is now. (Oh maybe I should be commenting
on v5, well... sorry for revising too quickly.)

> I think directly supporting param/value is a pretty common case, and
> seems like it deserves first-class support.

Not for the in-kernel U300 and SIRF drivers, and I have only
vague ideas of the non-written drivers yet.

> Supporting it wouldn't complicated the "custom struct pointer" case at
> all; you'd just hard-code one of the parameters to 0 when calling, and
> ignore it when implementing the driver.

Yes true, I did it like that before, but it doesn't look elegant.
I feel more at home with the either/or approach. So either it's this
simple unsigned long being passed around and that is opaque,
if we start passing around two things ... it's somewhat like the
same reason we pass around some struct * and use
countainer_of() to derive containing structs, isntead of passing
a lot of things around. Some of the refactorings around e.g.
IRQ handlers have been to remove extraneous parameters.

So it feels like something of an antipattern, it doesn't look
good to pass around two unknown things when you can
do with one.

> Conversely, explicitly supporting param/value would significantly simplify
> drivers that do want to use this mechanism, since you wouldn't need any
> custom struct types and casting, or all the pack and unpack macros, which
> I think will complicated the common case right now.

I have patches for the U300 in-kernel driver using this
mechanism and it doesn't look complicated I think.

>> Say you pin_config_group_get() and pins have wildly different
>> settings, what do you return?
>
> -ENOTSUPP.

Yeah OK. It's there in V5 so enjoy!

>> Having this return errors will make such binaries with pinconf
>> compiled-out just not boot if the error codes are handled
>> correctly, which is not very helpful.
>
> It seems better to plug in a dummy/stub pinctrl driver for testing
> purposes that allow the pinctrl APIs to succeed by default in cases
> where there's no implementation, which might end up hiding problems.

Hm. That doesn't quite match e.g. the way regulators supply
regulator stubs and dummies. The options for regulators are:

- Compile it out - everything returns success (no NULL etc)

- Compile it in - non-existing regulators will result in
  error codes.

- enable dummy regulators, then you can compile it in,
  and still get dummies for non-existing regulators e.g.
  and (typically) proceed to boot.

I would really not like to deviate from how regulator works
here, but keep the workings the same. So dummies can be
done, but they are for the case when pin control is enabled
but not fully implemented.

Would be good to have Marks input on this... he has more
experience with that kind of stuff.

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/