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  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]
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

Powered by blists - more mailing lists