Re: [PATCH v2] USB: serial: cp210x: Adding GPIO support for CP2105

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

On 14/01/16 14:29, Konstantin Shkolnyy wrote:
>> From: Martyn Welch [mailto:martyn.welch@collabora.co.uk]
>> Sent: Thursday, January 14, 2016 04:23
>> To: Konstantin Shkolnyy; Johan Hovold; Linus Walleij; Alexandre Courbot
>> Cc: Greg Kroah-Hartman; linux-usb@vger.kernel.org; linux-
>> gpio@vger.kernel.org
>> Subject: Re: [PATCH v2] USB: serial: cp210x: Adding GPIO support for CP2105
>>
>> On 14/01/16 00:27, Konstantin Shkolnyy wrote:
>>> Comments inline, not comprehensive by any measure...
>>>
>>>> -----Original Message-----
>>>> From: linux-usb-owner@vger.kernel.org [mailto:linux-usb-
>>>> owner@vger.kernel.org] On Behalf Of Martyn Welch
>>>> Sent: Wednesday, January 13, 2016 06:31
>>>> To: Johan Hovold; Linus Walleij; Alexandre Courbot
>>>> Cc: Greg Kroah-Hartman; linux-usb@vger.kernel.org; linux-
>>>> gpio@vger.kernel.org; Martyn Welch
>>>> Subject: [PATCH v2] USB: serial: cp210x: Adding GPIO support for CP2105
>>>>
>>> ...
>>>
>>>> +#include <linux/gpio/driver.h>
>>>> +#include <linux/bitops.h>
>>>
>>> Enclose this in CONFIG_GPIOLIB?
>>> ...
>>
>> Is there any point? I took a look at a few other drivers which
>> optionally support GPIO and they don't ifdef the headers.
>
> OK, according to other comment, I need to learn about local ifdef policies. I'm sorry.
> To me, knowing *why * a header is included seems beneficial, but obviously there are other considerations.
>
>> I think the contents of the headers will effectively be ignored if not
>> used and this won't affect module size.
>
> I think a good linker will throw away anything that is not referenced anyway.
> ...
>
>>>> +static int cp210x_gpio_get(struct gpio_chip *gc, unsigned gpio)
>>>> +{
>>>> +	struct cp210x_port_private *port_priv =
>>>> +			 container_of(gc, struct cp210x_port_private, gc);
>>>> +	struct usb_serial *serial = port_priv->serial;
>>>> +	__le32 *buf;
>>>> +	int result, i, length;
>>>> +	int size = 1;
>>>> +	unsigned int data[size];
>>>> +
>>>> +	/* Number of integers required to contain the array */
>>>> +	length = (((size - 1) | 3) + 1) / 4;
>>>
>>> usb_control_msg can deal with any size of the buffer, so use __le16 and
>> remove these length calculations.
>>>
>>
>> OK. (This is the process used in the other calls. Was wondering why they
>> are in the first place, any ideas?)
>
> Johan Hovold previously said he didn't like these functions either. I actually submitted a patch to replace them with simpler ones.
> ...
>

Ah! OK, found them in mail archive. Will take a look.

Martyn