Re: [PATCH v6 1/1] USB: serial: cp210x: Adding GPIO support for CP2105

[Martyn Welch <martyn.welch@collabora.co.uk>]

On Tue, Oct 04, 2016 at 02:13:26PM +0200, Linus Walleij wrote:
> On Sat, Sep 24, 2016 at 12:50 AM, Martyn Welch
> <martyn.welch@collabora.co.uk> wrote:
> 
> > This patch adds support for the GPIO found on the CP2105.
> 
> 
> > This device supports either push-pull or open-drain modes, it doesn't
> > provide an explicit input mode, though the state of the GPIO can be read
> > when used in open-drain mode. Like with pin use, the mode is configured in
> > the one-time programable PROM and can't be changed at runtime.
> 
> I see.
> 
> So implement .direction_input() and .direction_output()
> anyways.
> 
> Return failure on .direction_input() if it is in push-pull mode.
> 
> Return success on all .direction_output() calls.
> 
> Then implement .set_single_ended() and return success for open drain
> if the is in open drain, success for push-pull if the line is in push-pull
> mode, and failure in all other cases. Simple, but correct.
> 

This is proving to be problematic, because we are trying to be clever in
gpiolib and the driver.

I have the driver setup as above, if I have a pin that is configured as
open-drain (and can't be changed) and I try to drive it as an open-source
output with a low output value, it succeeds.

This is because, in gpiolib, if the device can't actually be set as o-s,
we configure it as input to emulate o-s (_gpiod_direction_output_raw):


        else if (test_bit(FLAG_OPEN_SOURCE, &desc->flags)) {
                if (gc->set_single_ended) {
                        ret = gc->set_single_ended(gc, gpio_chip_hwgpio(desc),
                                                   LINE_MODE_OPEN_SOURCE);
                        if (!ret)
                                goto set_output_value;
                }
                /* Emulate open source by not actively driving the line low */
                if (!value)
                        return gpiod_direction_input(desc);


In the driver we can't change it to input, but we are trying to be clever,
so we allow pins which are o-d to be treated as though they are inputs. However, for the o-d to behave like an input at all, the pin must be driven with a "1" (i.e. pulled up, rather than driven low), so:


        /*
         * Return failure if pin is configured in push-pull mode, can only
         * emulate input when pin is configured as open-drain
         */
        if (priv->gpio_mode & BIT(gpio))
                return -ENOTSUPP;

        /*
         * Ensure we are outputting a high state so that we can use the
         * open-drain output as an input
         */
        cp210x_gpio_set(gc, gpio, 1);


So now we have a pin that is supposed to be pulling to ground that is
actually pulling to VCC.

Also, if an output can only be open-drain, attempting to set the pin as
push-pull succeeds because gpiolib (currently) assumes that a pin can
always be p-p and doesn't even check the return value of it's call to
.set_single_ended:


               /* Make sure to disable open drain/source hardware, if any */
                if (gc->set_single_ended)
                        gc->set_single_ended(gc,
                                             gpio_chip_hwgpio(desc),
                                             LINE_MODE_PUSH_PULL);


This is clearly a separate issue.

WRT this driver, I think I need to keep set_single_ended, but change .direction_input to always return a failure and have .direction_output always return success to avoid pins being driven in unexpected ways. Does that sould acceptable?

Martyn