Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1650215 > unrolled thread

Re: [PATCH 4/4] iio: hi8435: cleanup reset gpio

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 4/4] iio: hi8435: cleanup reset gpio Nikita Yushchenko <nikita.yoush@cogentembedded.com> - 2017-05-25 08:30 +0200
    Re: [PATCH 4/4] iio: hi8435: cleanup reset gpio Jonathan Cameron <jic23@kernel.org> - 2017-05-28 17:50 +0200
      Re: [PATCH 4/4] iio: hi8435: cleanup reset gpio Nikita Yushchenko <nikita.yoush@cogentembedded.com> - 2017-05-29 10:00 +0200
    Re: [PATCH 4/4] iio: hi8435: cleanup reset gpio Linus Walleij <linus.walleij@linaro.org> - 2017-05-29 19:10 +0200

#1650215 — Re: [PATCH 4/4] iio: hi8435: cleanup reset gpio

>>>>> Reset GPIO is active low.
>>>>>
>>>>> Currently driver uses gpiod_set_value(1) to clean reset, which depends
>>>>> on device tree to contain GPIO_ACTIVE_HIGH - that does not match reality.
>>>>>
>>>>> This fixes driver to use _raw version of gpiod_set_value() to enforce
>>>>> active-low semantics despite of what's written in device tree. Allowing
>>>>> device tree to override that only opens possibility for errors and does
>>>>> not add any value.
>>>>>
>>>>> Additionally, use _cansleep version to make things work with i2c-gpio
>>>>> and other sleeping gpio drivers.  
>>>> The reset gpio comes from platform hence it should be handled by DTS.
>>>>
>>>> In driver the gpio should not be raw.
>>>>
>>>> Even the hi8435 is active low but platform may invert signal (f.e. by
>>>> adding trigger on the circuit path).  
>>> I see.  However - isn't this pure theoretic?  Does such case exist?  
>> I assure you that this is frequently used.
>>
>> Simply search google for "simple voltage level shifter"
>> It might be on PNP or NPN transistor, hence logic might be inverted.
>>
>>>
>>> In vast majority of cases, GPIO polarity is chip-specific, not
>>> chip-use-specific.  Thus this knowlege belongs to driver and not to
>>> device tree describing particular chip usage.  Having this always
>>> defined at usage side is IMO major source of errors.  
>> GPIO comes from SoC then "circuit path" and finally chip reset input.
>>
>> What do you propose if h/w circuit path has simple voltage level shifter 
>> on transistor. How to differentiate PNP and NPN cases?
> 
> Hmm. Ah well, I clearly jumped too fast on this set and should have
> left it for a while longer (I rushed a little as I'm away next weekend
> and the cycle is moving towards rc3)
> 
> Sorry about that.
> 
> Anyhow, I am tempted to queue a revert of this patch.  The level
> shifting case hadn't occurred to me (oops).
> 
> Thoughts?

Well here is the full story.

- I found that chip's reset line is active low per datasheet, but device
tree for board I work with states it is active high

- I checked driver code and found that driver depends on this incorrect
setting, it won't work if device tree will state that gpio is active low

- I could revert values in driver code AND in device tree, this way make
device tree be correct (against reality) but make dtb files flashed into
existing systems incompatible with future kernels -  which I disliked

- Thus I thought that I can remove explicit definition of polarity from
device tree (replacing it with neutrally-looking zero), and change
driver to use _raw.  I assumed that there is no real gain to let device
tree override gpio polarity for signal that is per-datasheet always
active low

- Thinking further on this, I realized that for common case signal
polarity is something defined by chip, and thus this knowledge belongs
to chip's driver and not to chip user's device tree. Moreover, device
tree writer could easily be not aware of signal polarity (too many
datasheets are NDA-closed), thus hello copy-pasting, try-and-check and
other counterproductive approaches.

- Then Vladimir pointed real-life case with signal inversion by handmade
level shifter. Although scope of this is likely limited to hw labs,
support for this is wanted and thus some way to override polarity in
chip user's dts be available.  Still, this should be optional, without
requiring dts to always define polarity of each gpio.


It becomes obvious that this topic has global scope, it is not something
to solve within hi8345 driver or within iio.  For patch in question,
possibilities are:
- revert the patch, restore situation with driver depending on wrong
statement in dts, maybe document that in bindings,
- replace patch with code assuming that device tree has correct
definition of reset gpio polarity; break existing device trees (all are
out-of-tree as of today),
- keep the patch, thus not break anything and still stop requiring
device tree to contain wrong statement, but make entire situation
somewhat hacky and loose support for board reverting signal between gpio
provider and hi8435's pin (hopefully no such board exists).

I don't know. Maintainer should decide.

Nikita

[toc] | [next] | [standalone]

#1652167

On Thu, 25 May 2017 09:27:18 +0300
Nikita Yushchenko <nikita.yoush@cogentembedded.com> wrote:

> >>>>> Reset GPIO is active low.
> >>>>>
> >>>>> Currently driver uses gpiod_set_value(1) to clean reset, which depends
> >>>>> on device tree to contain GPIO_ACTIVE_HIGH - that does not match reality.
> >>>>>
> >>>>> This fixes driver to use _raw version of gpiod_set_value() to enforce
> >>>>> active-low semantics despite of what's written in device tree. Allowing
> >>>>> device tree to override that only opens possibility for errors and does
> >>>>> not add any value.
> >>>>>
> >>>>> Additionally, use _cansleep version to make things work with i2c-gpio
> >>>>> and other sleeping gpio drivers.    
> >>>> The reset gpio comes from platform hence it should be handled by DTS.
> >>>>
> >>>> In driver the gpio should not be raw.
> >>>>
> >>>> Even the hi8435 is active low but platform may invert signal (f.e. by
> >>>> adding trigger on the circuit path).    
> >>> I see.  However - isn't this pure theoretic?  Does such case exist?    
> >> I assure you that this is frequently used.
> >>
> >> Simply search google for "simple voltage level shifter"
> >> It might be on PNP or NPN transistor, hence logic might be inverted.
> >>  
> >>>
> >>> In vast majority of cases, GPIO polarity is chip-specific, not
> >>> chip-use-specific.  Thus this knowlege belongs to driver and not to
> >>> device tree describing particular chip usage.  Having this always
> >>> defined at usage side is IMO major source of errors.    
> >> GPIO comes from SoC then "circuit path" and finally chip reset input.
> >>
> >> What do you propose if h/w circuit path has simple voltage level shifter 
> >> on transistor. How to differentiate PNP and NPN cases?  
> > 
> > Hmm. Ah well, I clearly jumped too fast on this set and should have
> > left it for a while longer (I rushed a little as I'm away next weekend
> > and the cycle is moving towards rc3)
> > 
> > Sorry about that.
> > 
> > Anyhow, I am tempted to queue a revert of this patch.  The level
> > shifting case hadn't occurred to me (oops).
> > 
> > Thoughts?  
> 
> Well here is the full story.
> 
> - I found that chip's reset line is active low per datasheet, but device
> tree for board I work with states it is active high
> 
> - I checked driver code and found that driver depends on this incorrect
> setting, it won't work if device tree will state that gpio is active low
> 
> - I could revert values in driver code AND in device tree, this way make
> device tree be correct (against reality) but make dtb files flashed into
> existing systems incompatible with future kernels -  which I disliked
> 
> - Thus I thought that I can remove explicit definition of polarity from
> device tree (replacing it with neutrally-looking zero), and change
> driver to use _raw.  I assumed that there is no real gain to let device
> tree override gpio polarity for signal that is per-datasheet always
> active low
> 
> - Thinking further on this, I realized that for common case signal
> polarity is something defined by chip, and thus this knowledge belongs
> to chip's driver and not to chip user's device tree. Moreover, device
> tree writer could easily be not aware of signal polarity (too many
> datasheets are NDA-closed), thus hello copy-pasting, try-and-check and
> other counterproductive approaches.
> 
> - Then Vladimir pointed real-life case with signal inversion by handmade
> level shifter. Although scope of this is likely limited to hw labs,
> support for this is wanted and thus some way to override polarity in
> chip user's dts be available.  Still, this should be optional, without
> requiring dts to always define polarity of each gpio.
I have real hardware that effectively does the equivalent for interrupt
lines coming into the processor from some sensors.  Note quite the same,
but shows these things often do turn up as a fix on 'real' hardware.
> 
> 
> It becomes obvious that this topic has global scope, it is not something
> to solve within hi8345 driver or within iio.  For patch in question,
> possibilities are:
> - revert the patch, restore situation with driver depending on wrong
> statement in dts, maybe document that in bindings,
> - replace patch with code assuming that device tree has correct
> definition of reset gpio polarity; break existing device trees (all are
> out-of-tree as of today),
> - keep the patch, thus not break anything and still stop requiring
> device tree to contain wrong statement, but make entire situation
> somewhat hacky and loose support for board reverting signal between gpio
> provider and hi8435's pin (hopefully no such board exists).
> 
> I don't know. Maintainer should decide.
For now I went with a revert as the low risk option - we aren't making
the situation potentially worse as it's already broken.  It of
course would be great to rapidly come to a conclusion that has
the best of all possible worlds!

You are correct, this needs some wider guidance.  I've cc'd Linus
Walleij more to make sure he has a heads up than to suggest
we continue this conversation in this thread.

I don't know what the 'correct' way forward is.  Funnily enough
my immediate thought is to it it with a big hammer and define
an inverting gpiochip to represent the possible inverter.
So a bit like a gpiochip setting on i2c but in this case
it sits on a gpio.  So when you set the front gpio
(representing the inverter) it knows to set the opposite
on the actual gpio on the processor.

Probably overkill but would let you represent absolutely
many crazy topologies ;) Mind you might just as easily already
exist and I don't know about it (I did take a quick look
but didn't find anything like that)

Jonathan
> 
> Nikita

[toc] | [prev] | [next] | [standalone]

#1652264

>> - Thinking further on this, I realized that for common case signal
>> polarity is something defined by chip, and thus this knowledge belongs
>> to chip's driver and not to chip user's device tree. Moreover, device
>> tree writer could easily be not aware of signal polarity (too many
>> datasheets are NDA-closed), thus hello copy-pasting, try-and-check and
>> other counterproductive approaches.
>>
>> - Then Vladimir pointed real-life case with signal inversion by handmade
>> level shifter. Although scope of this is likely limited to hw labs,
>> support for this is wanted and thus some way to override polarity in
>> chip user's dts be available.  Still, this should be optional, without
>> requiring dts to always define polarity of each gpio.
>
> I have real hardware that effectively does the equivalent for interrupt
> lines coming into the processor from some sensors.  Note quite the same,
> but shows these things often do turn up as a fix on 'real' hardware.

Well no doubt that sometimes there is need to invert polarity for
particular use case.

But still forcing explicit definition of polarity of each and every gpio
usage is a bad way to solve this.

> You are correct, this needs some wider guidance.  I've cc'd Linus
> Walleij more to make sure he has a heads up than to suggest
> we continue this conversation in this thread.

As I've already written elsewhere, possible solution is a new pair of
gpio flags, GPIO_DEFAULT_POLARITY / GPIO_INVERTED_POLARITY, which means
that polarity of signal is, corresponding, chip's default or inverted
chip's default.  And matching API for drivers to set default when
requesting gpio.

> 
> I don't know what the 'correct' way forward is.  Funnily enough
> my immediate thought is to it it with a big hammer and define
> an inverting gpiochip to represent the possible inverter.
> So a bit like a gpiochip setting on i2c but in this case
> it sits on a gpio.  So when you set the front gpio
> (representing the inverter) it knows to set the opposite
> on the actual gpio on the processor.

This may be alternative to GPIO_DEFAULT_POLARITY /
GPIO_INVERTED_POLARITY, that perhaps allows for device tree to better
follow schematics.  Still this does not play well with existing
requirement to explicitly set ACTIVE_LOW / ACTIVE_HIGH at gpio usage side.


Nikita

P.S.

Another view is that for such a trivial entity as gpio, we already have
- physical gpio values,
- logical values that come from ACTIVE_LOW / ACTIVE_HIGH,
- for gpios used as interrupts, active low / active high irq's,
and discussing addition more to that... for simplification? Joking?

At least, if adding default-polarity / inverted-polarity, this should be
done mutually-exclusive with active-high / active-low.  And if adding
inverter gpiochip, then also this should be mutual exclusive with
setting active-high / active-low.

[toc] | [prev] | [next] | [standalone]

#1652663

On Thu, May 25, 2017 at 8:27 AM, Nikita Yushchenko
<nikita.yoush@cogentembedded.com> wrote:

> - I found that chip's reset line is active low per datasheet, but device
> tree for board I work with states it is active high

There you have it, there is a bug in the device tree, because it is
not properly describing the hardware.

> - I checked driver code and found that driver depends on this incorrect
> setting, it won't work if device tree will state that gpio is active low

Yeah...

> - I could revert values in driver code AND in device tree, this way make
> device tree be correct (against reality) but make dtb files flashed into
> existing systems incompatible with future kernels -  which I disliked

Is this one of those systems where people actually flash a DTB and
never change it?

If not, then just patch the DTS source and stop worrying already.
We don't do backwards compatibility just because it's fun.

If it is really deployed and really not seeing updates...

In that case I want a way to check the signature (such as a checksum)
of the DTB and apply a workaround for elder (incorrect) device trees
when detected, and let newer (corrected) device trees describe the
hardware properly.

> - Thus I thought that I can remove explicit definition of polarity from
> device tree (replacing it with neutrally-looking zero), and change
> driver to use _raw.  I assumed that there is no real gain to let device
> tree override gpio polarity for signal that is per-datasheet always
> active low
>
> - Thinking further on this, I realized that for common case signal
> polarity is something defined by chip, and thus this knowledge belongs
> to chip's driver and not to chip user's device tree.

That is a fallacy IMO.

Just like we define regulators for all voltage inputs on a chip and
set the voltage constraints accordingly in the device tree, so we
set the polarity of signals in the device tree.

GPIOs, like regulators, are defined in terms of the consumer
properties. Else having a signal iversion flag (*ACTIVE_LOW) would
not make sense.

> Moreover, device
> tree writer could easily be not aware of signal polarity (too many
> datasheets are NDA-closed), thus hello copy-pasting, try-and-check and
> other counterproductive approaches.

Mistakes will be made but seriously, device tree writers know as much as
driver writers do in my experience. If it is unknown they will look
at some hint from the rail, like it being named RESETN on the schematic
indicating negative (active low) polarity.

> - revert the patch, restore situation with driver depending on wrong
> statement in dts, maybe document that in bindings,
> - replace patch with code assuming that device tree has correct
> definition of reset gpio polarity; break existing device trees (all are
> out-of-tree as of today),
> - keep the patch, thus not break anything and still stop requiring
> device tree to contain wrong statement, but make entire situation
> somewhat hacky and loose support for board reverting signal between gpio
> provider and hi8435's pin (hopefully no such board exists).

I would fix the device tree and the driver to handle it correctly.

Signal active low in the device tree.

Assert with high level in the code.

Then, if backward compatibility for older device trees is needed,
find a way to detect elder device trees and then alter the behaviour
with an extra inversion on those.

Preferably by altering the device tree in memory actually, but that may be
hard. (Code for this exists.)

Yours,
Linus Walleij

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web