On 05 May 2014 17:14, Alexandre Courbot wrote:
Ok, you are obviously correct here.
But you won't accept my patch, because you don't want to. Not because
I'm correct, right? Good, thought so.
of_get_gpiod_flags() works the same way as of_get_gpio_flags() which
it mirrors and is correct in that respect. Returning a flags parameter
for the caller to apply while having applied these flags already is
what makes no sense to me. Taste and colours...
No, it's definitely _not_ a matter of taste. My version fixes this
mistake at the earliest possible moment (when the fully parsed gpio is
returned from the call to the chip), the root cause of the problem. Your
version applies this fix at on arbitrary point -- too bad for all other
functions which assume a valid gpio_desc is returned from the lower
levels. Because it isn't.
But of course, that's just my taste that I like non-broken functions. If
you like broken functions, you're free to like them.
If you really need a function that will return GPIO descriptor from an
arbitrary DT property (or maybe a version of gpiod_get() working on a
DT node and not a device instance), that's another story and we can
discuss that if it is well motivated.
Good news :) Such a function was already implemented and available to
everybody and his dog!
Bad news :( You just made that function private because you wouldn't
accept my patch to root out this flag application problem.
Also, clearly and calmly explaining your needs will serve you much
better than screaming around and arbitrarily calling things "broken".
I'll call broken code *broken*. If you don't like it, that's fine. But I
won't call broken code *working and in order* just because you get your
panties in a twist otherwise.
Well, that was a mistake that went through the review process. We
wanted to make a perfect GPIO API but unfortunately you were not
around to advise us.
A real shame indeed.
Good news, once of_get_gpiod_flags() becomes private all public gpiod_
functions should return a correct gpio_desc.
Wrong. First of all, every new gpiod_* function that might be added in
the future will probably need two revisions, because the authors might
not be aware that of_find_gpio returns an invalid gpio_desc. gpiod_find
remains broken as well, as it implicitly relies on gpio_desc stored per
chip to have valid gpio_desc.flags.
As for where to set the descriptor's properties, having it done in
gpiod_get() allows us to handle this in a single place, without having
to rely on lower-level functions to remember doing it themselves. Can
this design be challenged? Sure. It can also go on and on for months
if no solution is clearly superior to the other, and that's likely the
path it will take.
The solution to parse the flags when the gpio_desc is fist used is
clearly superior, because all other functions depending on that function
will /just work/.
Fixing things at some arbitrary branch of the caller tree while leaving
all other branches to fend on their own is poor, dare I say *broken*,
design.
The single place where things need to be handled at is the lowest common
denominator, which all other functions can rely on. If higher level
functions cannot rely on a low-level function, because it lies and does
not return a valid gpio_desc, then they will in turn return an invalid
gpio_desc. And so on.
To make things clear, here is my stance on the question:
1) of_get_gpiod_flags() being public was a mistake that is being addressed.
No, it was useful and now you broke my and possibly many other
programmer's code in making it private. Already by name it was supposed
to be the gpiod_ equivalent to be called when using DT nodes. Thanks
again for breaking this and possibly getting it into mainline.
Now I need two patches in my private tree: one to fix the flag issue and
one to re-export that function.
2) There are no user of of_get_gpiod_flags() in mainline, therefore
removing it from the public API is perfectly ok. We cannot keep track
of all the external trees to see who is using what, and the kernel has
a long history of much more drastic API changes.
Removing something from a *public* API is never ok post hoc. Never. When
things get to API level there are certain guarantees that have to be
met. One would be that functions don't just vanish from revision to
revision.
3) If the accepted way of looking up GPIOs does not cover your needs,
please explain clearly how the current gpiod_get() function is
insufficient and we can work something out. But if all it takes to fix
your issue is some small changes in out-of-mainline code, then I will
advocate this rather than adding functions to the gpiod API.
How come your way is now the accepted way? I thought the accepted way
was that gpio_desc were an opaque type whose flags are set in low-level
code and who can be relied upon to /just work/.
Which comes back to this statement, which seems to be the fundamental
problem here:
Returning a flags parameter
for the caller to apply while having applied these flags already is
what makes no sense to me.
The flags parameter is not returned to be applied by the user.
First of all, gpio_desc is an opaque type, so the user cannot (and
indeed must not) apply these flags to gpio_desc.flags.
However, I think with apply you mean this:
The flags are returned so the user can implement his own logic for
inverting/open draining signals etc.
However, the whole point of the exercise is precisely that no external
user-logic is required for open drain, open source, low-active,
high-active etc. However, this will duplicate all the code that's
already in gpiolib in the user code. This cannot be!
As I see it, the flags are returned as a piece of information to the
user code. This is for historic reasons, because integer gpios rely upon
these flags and because gpio_desc is an opaque type, so the flags cannot
be /easily/ acquired from somewhere else for the current gpio.
However, user code _must not_ change its behavior depending on the
flags, and neither must gpiolib code. Users must be able to trust that
the gpio_desc returned to them is valid. They must be able to trust that
the Right Thing™ happens when they call gpiod_set_value(&my_desc, 1) on
their gpio: it is asserted -- irrespective of what asserted means in the DT.
Your approach catapults us back into the real dark ages. With your
approach, we are back to *driver-level* support for active-low,
open-drain, etc. And that's why the flags *have* to be applied by
gpiolib, else the whole design is *broken*. Deal with it.
Regards,
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
