mail archive of the barebox mailing list
 help / color / mirror / Atom feed
From: Andrey Smirnov <andrew.smirnov@gmail.com>
To: Trent Piepho <tpiepho@kymetacorp.com>,
	Sascha Hauer <s.hauer@pengutronix.de>
Cc: "nikita.yoush@cogentembedded.com"
	<nikita.yoush@cogentembedded.com>,
	"barebox@lists.infradead.org" <barebox@lists.infradead.org>,
	"cphealy@gmail.com" <cphealy@gmail.com>
Subject: Re: [PATCH 2/4] gpiolib: Add code to support "active low" GPIOs
Date: Wed, 24 May 2017 13:36:48 -0700	[thread overview]
Message-ID: <CAHQ1cqEoXNdQA=1kCtxpC3dmJhYGiBY5PBxAOFCwBbZyJ5nj+w@mail.gmail.com> (raw)
In-Reply-To: <1495649778.5146.219.camel@kymetacorp.com>

On Wed, May 24, 2017 at 11:16 AM, Trent Piepho <tpiepho@kymetacorp.com> wrote:
> On Wed, 2017-05-24 at 10:26 +0300, Nikita Yushchenko wrote:
>> First point is that words "active high" (or "active low") have very
>> clear meaning. And situation when chip's signal is active low, but one
>> has to write "active high" in signal definition to make things working,
>> is not something I welcome in systems I deal with.
>>
>> Second point is that by cleaning up the above, you make drivers depend
>> on correct polarity settings in dts. Which means that when writing dts,
>> you get need to dig over datasheets (which may be under NDA) to find out
>> polarity of each signal. This looks like breakage of information
>> locality - knowledge of chip's signals polarity belongs to chip's
>> driver. Common case of signals connected directly to gpio providers
>> should just work.  It's nice to have a way to override driver's default,
>
> I agree with this.  It's pretty much random if a given signal will want
> a high value to mean asserted or not.

Yes, and that the point of having "active low" being configurable in
device tree so it would be possible to use exactly the same driver
code for slightly different setups.

> And plenty of signals switch
> "modes" and it's not even clear which mode should be considered
> "asserted".

First this statement is so vague that it is hard to make any argument
about it. Second, just because a feature doesn't cover every possible
use-case doesn't mean that it doesn't have a place in the code base at
all.

> If drivers expect me to put active low/high in the
> bindings, then it means for every signal one must get the datasheet and
> figure out what a high signal means.  And then likely look though the
> driver code to make sure the driver sets 1 to mean that.
>

I don't see how "active low" changes the way you troubleshoot things
at all. If you are not lucky and you code didn't just work from the
first try, wouldn't you want to verify that you specified correct GPIO
number by looking at the schematic? And if so wouldn't you see what
high/low means by looking at the chip's symbol? If you don't have
access to a schematic, the only way I see to proceed with debugging it
is to probe correct pin on the chip with a scope, for which you'd need
at least an abridged datasheet that would have pinout documented.

Regardless of any of that, I seems to me that this is an argument
about personal preferences (I find the feature in question useful and
don't think it is confusing, you guys have dislike it) so I don't
think we'd resolve this any time soon.

IMHO, whether any of likes it or not, OF_GPIO_ACTIVE_LOW is an
existing feature and the only technical question is if it should be
supported by Barebox on per-driver basis or if there should be a
central API for it.

Sascha, are you still OK with me proceeding with making and using an
API, or should I just scrap that and handle this as a part of my
"usb-no-xcev"?

Thanks,
Andrey Smirnov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

  reply	other threads:[~2017-05-24 20:37 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-05-22 15:24 [PATCH 1/4] gpio-imx: Do not use gpio_set_value() Andrey Smirnov
2017-05-22 15:24 ` [PATCH 2/4] gpiolib: Add code to support "active low" GPIOs Andrey Smirnov
2017-05-23  6:30   ` Nikita Yushchenko
2017-05-23  8:33     ` Sascha Hauer
2017-05-24  0:16       ` Andrey Smirnov
2017-05-24  0:14     ` Andrey Smirnov
2017-05-24  7:26       ` Nikita Yushchenko
2017-05-24 18:16         ` Trent Piepho
2017-05-24 20:36           ` Andrey Smirnov [this message]
2017-05-25  6:36             ` Nikita Yushchenko
2017-05-25 17:10               ` Andrey Smirnov
2017-05-25 17:45             ` Sascha Hauer
2017-05-22 15:24 ` [PATCH 3/4] gpiolib: Add support for GPIO "hog" nodes Andrey Smirnov
2017-05-23  6:52   ` Nikita Yushchenko
2017-05-23 23:25     ` Andrey Smirnov
2017-05-24  6:43       ` Nikita Yushchenko
2017-05-30 14:38         ` Andrey Smirnov
2017-05-24  7:26       ` Sascha Hauer
2017-05-22 15:24 ` [PATCH 4/4] usb-nop-xceiv: Add support for 'reset-gpios' binding Andrey Smirnov
2017-05-23  6:55   ` Nikita Yushchenko
2017-05-24  0:17     ` Andrey Smirnov
2017-05-23  6:08 ` [PATCH 1/4] gpio-imx: Do not use gpio_set_value() Nikita Yushchenko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAHQ1cqEoXNdQA=1kCtxpC3dmJhYGiBY5PBxAOFCwBbZyJ5nj+w@mail.gmail.com' \
    --to=andrew.smirnov@gmail.com \
    --cc=barebox@lists.infradead.org \
    --cc=cphealy@gmail.com \
    --cc=nikita.yoush@cogentembedded.com \
    --cc=s.hauer@pengutronix.de \
    --cc=tpiepho@kymetacorp.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox