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

[Linus Walleij]

On Thu, May 25, 2017 at 8:27 AM, Nikita Yushchenko
<nikita.yoush@xxxxxxxxxxxxxxxxxx> 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