[PATCH] staging: wfx: fix reset GPIO polarity

Michał Mirosław mirq-linux at rere.qmqm.pl
Fri Dec 6 17:57:28 UTC 2019


On Thu, Dec 05, 2019 at 03:43:49PM +0000, Jérôme Pouiller wrote:
> On Thursday 5 December 2019 15:49:55 CET Michał Mirosław wrote:
> > On Thu, Dec 05, 2019 at 02:08:23PM +0000, Jérôme Pouiller wrote:
> > > On Wednesday 4 December 2019 17:59:46 CET Michał Mirosław wrote:
> > > > Driver inverts meaning of GPIO_ACTIVE_LOW/HIGH. Fix it to prevent
> > > > confusion.
> > > >
> > > > Signed-off-by: Michał Mirosław <mirq-linux at rere.qmqm.pl>
> > > > ---
> > > >  drivers/staging/wfx/bus_spi.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/staging/wfx/bus_spi.c b/drivers/staging/wfx/bus_spi.c
> > > > index ab0cda1e124f..73d0157a86ba 100644
> > > > --- a/drivers/staging/wfx/bus_spi.c
> > > > +++ b/drivers/staging/wfx/bus_spi.c
> > > > @@ -199,9 +199,9 @@ static int wfx_spi_probe(struct spi_device *func)
> > > >         if (!bus->gpio_reset) {
> > > >                 dev_warn(&func->dev, "try to load firmware anyway\n");
> > > >         } else {
> > > > -               gpiod_set_value(bus->gpio_reset, 0);
> > > > -               udelay(100);
> > > >                 gpiod_set_value(bus->gpio_reset, 1);
> > > > +               udelay(100);
> > > > +               gpiod_set_value(bus->gpio_reset, 0);
> > > >                 udelay(2000);
> > > >         }
> > > Hello Michał,
> > >
> > > I did not find real consensus in kernel code. My personal taste would
> > > be to keep this gpio "ACTIVE_HIGH" and rename it gpio_nreset. What do
> > > you think about it?
> > >
> > > (in add, this solution would explicitly change the name of the DT
> > > attribute instead of changing the semantic of the existing attribute)
> > 
> > As a user (board developer) I would expect that DT describes the
> > GPIO meaning directly: so when I specify GPIO_ACTIVE_HIGH flag I also
> > wire up the board so that outputing 1 would match the active state of
> > the chip's signal (that might be inverted for some reason). I think we
> > should stick to what is said in Documentation/devicetree/bindings/gpio.txt
> > (section 1.1).
> > 
> > Since this is a new driver in kernel I would prefer to fix it at the start.
> > Changing the name of the GPIO would also be ok, but since there is no DT
> > binding yet, I guess there will come up an issue of 'compatible' string
> > format that does not match 'vendor,chip' now, so we can use the difference
> > for backwards compatibility with out-of-tree driver if needed.
> 
> Current 'compatible' string is "silabs,wfx-spi" (for now, it is the
> same for out-of-tree and in-tree driver). And indeed, "wfx" does not
> names a chip.
> 
> The three chips currently supported are wf200, wf200s and wfm200. Since
> the driver provides DT bindings for SPI and SDIO buses, I think we
> have to keep the "-spi" suffix. So compatible strings should be
> "silabs,wf200-spi", "silabs,wf200s-spi" and "silabs,wfm200-spi", right?
[...]

I wonder if the '-spi' part is necessary? The interface is determined by
putting device node as a child of an SPI or MMC controller node. Kernel
won't probe SPI driver for MMC device anyway (nor the other way around).

Best Regards
Michał Mirosław


More information about the devel mailing list