śr., 6 lis 2019 o 07:48 Kent Gibson <warthog618@xxxxxxxxx> napisał(a): > > On Tue, Nov 05, 2019 at 10:07:58PM +0100, Bartosz Golaszewski wrote: > > wt., 5 lis 2019 o 17:24 Bartosz Golaszewski > > <bgolaszewski@xxxxxxxxxxxx> napisał(a): > > > > > > wt., 5 lis 2019 o 16:26 Kent Gibson <warthog618@xxxxxxxxx> napisał(a): > > > > > > > > On Tue, Nov 05, 2019 at 10:04:22AM +0800, Kent Gibson wrote: > > > > > Patches are against Bart's gpio/for-kent branch[1]. > > > > > > > > > > The patch has been successfully tested against gpio-mockup, and > > > > > on a Raspberry Pi, in both cases using the feature/pud_set_config > > > > > branch of my Go gpiod library[2], as well as with my feature/pud > > > > > development branch of libgpiod[3]. Patch 7 has only been tested using > > > > > my gpiod library as libgpiod has not yet been updated to support the > > > > > SET_CONFIG ioctl. > > > > > > > > > > > > > I've just pushed a first pass at SET_CONFIG support into my libgpiod > > > > feature/pud branch. It is causing me a bit of grief. Due to the way > > > > the libgpiod API is structured, with the direction flags pulled out into > > > > the request type, I thought it would be cleaner to keep changes to direction > > > > orthogonal to changes to the other handle flags. > > > > > > > > > > I'd love to see that branch - is it public? > > > > > > > So I've added these methods to the API: > > > > > > > > int gpiod_line_set_config(struct gpiod_line *line, int flags) > > > > int gpiod_line_set_direction_input(struct gpiod_line *line) > > > > int gpiod_line_set_direction_output(struct gpiod_line *line, > > > > int value) > > > > > > > > along with their bulk equivalents. > > > > > > > > I've coded that and started adding tests when I tripped over changing > > > > bias. The kernel requires a direction to be set, but I'm setting it > > > > as-is in gpiod_line_set_config - so that wont work. > > > > Open drain/source are in the same boat - they require output mode. > > > > > > > > > > Ha! Yes this is a problem - how about this: > > > > > > If the caller of set_config in the kernel doesn't pass any of the > > > direction flags, then we read the current direction, set the right > > > flag in lflags and only then call the function validating the flags? > > > > > > Cc'ing Linus, I'm not sure when he was dropped from this discussion. > I was also thinking along that line - the config would be carried over > from the initial request and any subsequent SET_CONFIGs. > The kernel could overlay the existing config over any field set > "as-is" before performing validation. > The validation requirement would stand, but you don't need to pass the > complete state, possibly including output values, with each SET_CONFIG > request. > > In the bias case above, I create the line as an output and so should > be able to set the bias, even if neither INPUT nor OUTPUT is set in > the SET_CONFIG request. > > > After another thought: this would be a bit inconsistent with the rest > > of the flags. IIRC this was the reason for me to split the > > request_type and other flags into two separate fields in libgpiod in > > the first place. > > > > It is a bit inconsisent, so how about changing the rest of the flags > to make them consistent? They need to have an as-is state, which > corresponds to no flags set, and you then leave that field as is. > We currently have four fields in the handle flags - direction, active > state, drive, and bias. Of those, direction and bias have as-is states. > What we are missing are additional flags so we have an as-is state for > active state and drive. > > Currently: > ACTIVE_LOW == 0 => ACTIVE_HIGH, and > OPEN_DRAIN == 0 && OPEN_SOURCE == 0 => PUSH_PULL. > > If we added an ACTIVE_HIGH flag, to counter ACTIVE_LOW, and PUSH_PULL > to counter OPEN_DRAIN/OPEN_SOURCE, then we can have SET_CONFIG change > the four fields (direction, active state, drive and bias), independently, > or not, as the caller sees fit. > > For backward compatibility, the lines would be created with ACTIVE_HIGH > and PUSH_PULL set, should they be requested "as-is", by the new > definition. > I'm not in favor of having the same behavior triggered in two different ways: one explicit and one implicit. This API got released and we have to live with it, I'm afraid. We could for instance add a comment to the uAPI header though. I also don't think new AS_IS flags are necessary. We can live fine with certain inconveniences in the ioctl() API as long as user-space libraries make up for it by structuring these calls differently. To summarize: I'd prefer to make the SET_CONFIG ioctl() require specifying the direction and then simply caching it in user-space. > This feels like the right solution to me - as I write this anyway. > The biggest downside I can see is that it means pulling v6, or at least > the SET_CONFIG patches, pending an update. > > > When I think about it: the kernel behavior should be as predictable as > > possible - if we keep the behavior as is in v6, I don't see why we > > couldn't make userspace cache (or re-read) the current direction when > > setting flags other than direction? Do you see any trouble in that? > > That way we'd avoid having the kernel treat different flags in > > different way. > > > > If in userspace then it will have to be cached - the kernel still has > issues reading back output values for emulated open drain/source outputs. > Fixing that is somewhere down my todo list. > Right, but we've lived with problems in this area for a long time and nobody complained - maybe it can wait just a bit more. :) > I can't think of any concrete problems with caching. > It gives me "I have a bad feeling about this" vibe, but I can't pin > down why. Maybe wanting to avoid shadowing kernel state in userspace? > The user-space can always read back the current state with the lineinfo ioctl(), right? Any problems with that? > But, as above, I'd rather fix the flags so we have as-is for all, and > caching becomes unnecessary. > This idea in turn gives me a bad feeling. There's something too implicit in this behavior for me. Sounds like it's too easy to get it wrong from user-space. > > Bart > > > > > > I see these options: > > > > 1. set the direction as part of gpiod_line_set_config > > > > 2. relax the kernel restriction. > > > > > > Yes, I don't think we should force users to always pass the direction > > > flag in set_config. Good point. > > > > > > > 3. don't support changing bias or open source/drain. > > > > > > No! We definitely want to support it in libgpiod. > > Agreed. > > > > > > > > 4. rethink the API. > > > > > > As for libgpiod: I think we should have a low-level > > > gpiod_line_set_config() that would set both the direction and other > > > flags (it could for instance only accept two request flags: input and > > > output) and then a higher-level set_flags(), set_direction_input(), > > > set_direction_output() that would call the low-level variant and - for > > > set_flags() without the direction argument - it could simply retrieve > > > the current direction and pass it to gpiod_line_set_config(). > > > > > I agree that there should add be a fully capable low level option. > > The low level function would look have a signature like this: > > int gpiod_line_set_config(struct gpiod_line *line, int direction, int flags, > const int *default_vals) > > The existing gpiod_line_set_config would be renamed gpiod_line_set_flags. > > > > But this is only a vague idea - I'd have to actually start looking at > > > the code to be sure. I'd love to see what you came up with so far > > > though! > > > > > Indeed - what I had in mind changed radically once I had a closer look > at the libgpiod API. And it is still changing. > Thanks for doing it! It's also great you included some test cases! Bart > Cheers, > Kent. >