On Tue, 2019-11-05 at 15:11 +0200, andriy.shevchenko@xxxxxxxxxxxxxxx wrote: > On Tue, Nov 05, 2019 at 12:54:55PM +0000, Vaittinen, Matti wrote: > > On Tue, 2019-11-05 at 14:20 +0200, Andy Shevchenko wrote: > > > On Tue, Nov 05, 2019 at 12:09:10PM +0200, Matti Vaittinen wrote: > > > > The patch series adds definitions for GPIO line directions. > > > > > > > > For occasional GPIO contributor like me it is always a pain to > > > > remember > > > > whether 1 or 0 was used for GPIO direction INPUT/OUTPUT. > > > > Judging > > > > the > > > > fact that I removed few comments like: > > > > > > > > /* Return 0 if output, 1 if input */ > > > > /* This means "out" */ > > > > return 1; /* input */ > > > > return 0; /* output */ > > > > > > > > it seems at least some others may find it hard to remember too. > > > > Adding > > > > defines for these values helps us who really have good - but > > > > short > > > > duration - memory :] > > > > > > > > Please also see the last patch. It adds warning prints to be > > > > emitted > > > > from gpiolib if other than defined values are used. This patch > > > > can > > > > be > > > > dropped out if there is a reason for that to still be allowed. > > > > > > > > This idea comes from RFC series for ROHM BD71828 PMIC and was > > > > initially > > > > discussed with Linus Walleij here: > > > > https://lore.kernel.org/lkml/c06725c3dd34118a324907137758d8b85b3d4043.camel@xxxxxxxxxxxxxxxxx/ > > > > but as this has no dependencies to BD71828 work (which probably > > > > takes a > > > > while) I decided to make it independent series. > > > > > > > > Patches are compile-tested only. I have no HW to really test > > > > them. > > > > Thus I'd > > > > appreciate carefull review. This work is mainly about > > > > converting > > > > zeros > > > > and ones to the new defines but it wouldn't be first time I get > > > > it > > > > wrong > > > > in one of the patches :) > > > > > > For all patches I have been Cc'ed to, NAK. > > > > > > I don't see the advantages now since all known hardware uses the > > > single bit to > > > describe direction (even for Intel where we have separate gating > > > for > > > in and out > > > buffers the direction we basically rely on the bit that enables > > > out > > > buffer). > > > > > > So, that said the direction is always 1 bit and basically 0/1 > > > value. > > > > Yes. At least for now. And this patch didn't change that although > > it > > makes it possible to do so if required. > > > > > Which one > > > is in and which one is out just a matter of an agreement we did. > > > > This one is exactly the problem patch series was created for. Which > > one > > is IN and which is OUT is indeed a matter of an agreement - but > > this > > agreement should be clearly visible, well defined and same for all. > > > > It's *annoying* to try finding out whether it was 1 or 0 one should > > return from get_direction callback for OUTPUT. This is probably the > > reason we have comments like > > > > return 1; /* input */ > > > > there. > > > > As this is global agreement for all GPIO framework users - not > > something that can be agreed per driver basis - we should have GPIO > > framework wide definitions for this. We can just add definitions > > for > > new drivers to benefit - but I think adding them also for existing > > drivers improves readability significantly (see below). > > > > > I would also like to see bloat-o-meter statistics before and > > > after > > > your patch. > > > My guts tell me that the result will be not in the favour of > > > yours > > > solution. > > > > Can you please tell me what type of stats you hope to see? > > bloat-o-meter. It's a script that compares two object files to see > which > functions got fatter, and which are slimmer. You may find it in the > kernel tree > sources (scripts/ folder). Right. Uwe explained that to me. > > I can try > > generating what you are after. The cover letter contained typical > > +/- > > change stats from git and summary: > > > > 62 files changed, 228 insertions(+), 104 deletions(-) > > > > It sure shows that amount of lines was increased (roughly 2 lines > > more > > / changed file) > > Which is making unneeded churn. Not unneeded. A few of us see the value of naming the 1 and 0. > > - but I guess that was expected as I don't like > > ternary. > > And I like them, so, what to do? Well, if you maintain those files and like ternary, then they can be ternary. I don't really care as long as I don't need to be the one maintaining them. Not really a battle I want to invest in. I can even go and drop the patches for files you are maintaining if you see that's the way to go. It's easier for me. As I said, I don't plan to change the meaning of 1 and 0 - although I thought that allowing it might help in the future. Main goal was to name the 1 and 0. Naming can be done even if all users were not converted to use the names - as long as values behind names are not changed. Changing the values is a burden that can be carried by others when it is needed - we can now just make it easier or harder. So as to what to do - please just give me the final decision so that I know if I should just drop the intel patches or use the ternary. Unfortunately I don't right now have the time to waste arguing over it ;) Br, Matti _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
