Re: gpiobus: setting output value while in input mode

On 24/10/2019 18:38, Ian Lepore wrote:
> On Thu, 2019-10-24 at 17:04 +0300, Andriy Gapon wrote:
>> For a lack of a more specific mailing list (or my not being aware of it), asking
>> here.
>>
>> gpioiic, a very simple driver, has this code:
>> ===========================================================================
>> static void
>> gpioiic_setsda(device_t dev, int val)
>> {
>>         struct gpioiic_softc            *sc = device_get_softc(dev);
>>
>>         if (val == 0) {
>>                 GPIOBUS_PIN_SET(sc->sc_busdev, sc->sc_dev, sc->sda_pin, 0);
>>                 GPIOBUS_PIN_SETFLAGS(sc->sc_busdev, sc->sc_dev, sc->sda_pin,
>>                     GPIO_PIN_OUTPUT);
>>         } else {
>>                 GPIOBUS_PIN_SETFLAGS(sc->sc_busdev, sc->sc_dev, sc->sda_pin,
>>                     GPIO_PIN_INPUT);
>>         }
>> }
>> ===========================================================================
>>
>> The interesting case is val == 0.
>>
>> I know that there are GPIO controllers where input and output values are
>> represented by different bits, so it is possible to set a future output value
>> while the pin is in input mode.
>> At the same time, there are controllers where there is a single bit per pin and
>> while the pin is input mode the bit is read-only.  So, it is impossible to
>> preset the pin.
>>
>> How should controllers of the second type handle GPIOBUS_PIN_SET when the pin is
>> in input mode?
>> I see three options:
>> 1) just silently ignore GPIOBUS_PIN_SET or do it in a more glorious way: go all
>> the way to the hardware without caring if that does anything;
>> 2) return an error that would hint that the operation is not possible at this time;
>> 3) try to emulate the first class of controllers; that is, stash the value to
>> apply it when the pin is switched to output mode.

Another possibility:
4) always reject the operation under that condition

>>
>> I personally prefer 2, it's not hard to do (unlike 3) and there would be at
>> least some visibility into the problem.
>>
> 
> A couple years ago I added new flags GPIO_PIN_PRESET_{LOW,HIGH}.  Maybe
> we should document (where?) that the proper way to achieve the effect
> the code wants in the val==0 case is to use the preset flag along with
> the OUTPUT flag.  The driver will preset the pin before changing it to
> output if it is able to do so, otherwise it will do the best it can,
> which is to set the pin to output, then set its value, perhaps
> generating a brief bogus state followed by a transition to the right
> state.  Then of course we'd have to fix all existing drivers to behave
> that way, but I don't think that will be hard.
> 
> My problem with #2 is that whenever you push the problem out to the
> child drivers, one of two things happens:  drivers ignore the error, or
> all drivers have to have essentially identical code to handle the
> error.  In this case, what could a child driver do except react to the
> error by doing the operations in the reverse order?

Well, I think that it is a choice between affected consumer drivers needing to
have some boiler plate code to handle the error and all GPIO controller drivers
needing to have some code to handle GPIO_PIN_PRESET.  I am not sure which one is
less work.  That is, I am not sure how many consumer drivers really *require*
the preset behavior.
Another issue is that we probably need to make *all* controller drivers support
GPIO_PIN_PRESET before any consumer drivers can start using it without extra
checks, fallback code, etc.

Also, if we universally implement GPIO_PIN_PRESET we still need to answer the
question.  Because some consumer might still try to change an input, either by
mistake or for some reason, and we need a rule on how to handle that.

> The current gpio(4) documentation really only covers gpiobus(4) and a
> mentions a few of its older children and how to configure them via
> hints.  We need manpages for some of the newer drivers, and we
> especially need a manpage that documents sys/gpio.h (which is used both
> from userland and internally with gpio_if.m).  I can probably find some
> manpage-writing time over the new few weeks.

That would be great!
gpio would certainly benefit from more documentation.


-- 
Andriy Gapon