linux-kernel - Re: [PATCH v6] USB: serial: ftdi_sio: implement GPIO support for FT-X devices

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180925110021.GE3332@localhost>
Date:   Tue, 25 Sep 2018 13:00:21 +0200
From:   Johan Hovold <johan@...nel.org>
To:     Karoly Pados <pados@...os.hu>
Cc:     Johan Hovold <johan@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        Loic Poulain <loic.poulain@...aro.org>
Subject: Re: [PATCH v6] USB: serial: ftdi_sio: implement GPIO support for
 FT-X devices

On Tue, Sep 25, 2018 at 10:46:30AM +0000, Karoly Pados wrote:
> Hi,
> 
> >> +#if defined(CONFIG_GPIOLIB)
> >> +static const char * const ftdi_ftx_gpio_names[] = {
> >> + "CBUS0", "CBUS1", "CBUS2", "CBUS3"
> >> +};
> >> +#endif
> > 
> > We want to keep the ifdeffery to a minimum, so move this inside the
> > gpiolib ifdef below (and possibly even into the function where it is
> > used).
> > 
> > Also note that these names are shared with FT232R, but not with FT232H.
> > 
> 
> What naming do you suggest then?
> 
> My personal preference would be however to leave this name as is, because
> this patch only adds support for the FT-X. Even if support for others can 
> be added relatively trivially after this, there is explicitly no GPIO 
> support for FT232R *yet*. If somebody else adds GPIO support for the FT232R
> in a later patch, he/she should make corresponding adjustments themselves,
> including naming changes. IMHO.

Yes, that's perfectly fine. I was merely pointing it out as background
info which could possibly affect how you choose to address this (e.g.
moving it into the ftx function or not, but also that can be changed
later of course).

> >> + if (priv->gpio_output & BIT(gpio))
> >> + return 0;
> >> + else
> >> + return 1;
> > 
> > This could just simplified using negation (!), but perhaps this is
> > easier to parse as it stands.
> > 
> 
> Sorry, it is not clear what your preferred action here is. 
> So should I leave it as is then or not?

Just do

	res = !(priv->gpio_output & BIT(gpio));

And I think you should add locking here to.

Johan