Re: [PATCH v3 3/3] gpio: add support for Cypress CYUSBS234 USB-GPIO adapter

[Alexandre Courbot <gnurou@xxxxxxxxx>]

On Thu, Oct 9, 2014 at 8:46 AM, RR <rajaram.officemails@xxxxxxxxx> wrote:
> On Wed, Oct 8, 2014 at 12:18 AM, Alexandre Courbot <gnurou@xxxxxxxxx> wrote:
>> On Wed, Oct 8, 2014 at 4:09 PM, Muthu Mani <muth@xxxxxxxxxxx> wrote:
>>>> -----Original Message-----
>>>> From: Alexandre Courbot [mailto:gnurou@xxxxxxxxx]
>>>> Sent: Tuesday, October 07, 2014 3:34 PM
>>>> To: Muthu Mani
>>>> Cc: Samuel Ortiz; Lee Jones; Wolfram Sang; Linus Walleij; Greg Kroah-
>>>> Hartman; linux-i2c@xxxxxxxxxxxxxxx; linux-gpio@xxxxxxxxxxxxxxx; linux-
>>>> usb@xxxxxxxxxxxxxxx; Linux Kernel Mailing List; Rajaram Regupathy; Johan
>>>> Hovold
>>>> Subject: Re: [PATCH v3 3/3] gpio: add support for Cypress CYUSBS234 USB-
>>>> GPIO adapter
>>>>
>>>> On Mon, Oct 6, 2014 at 11:47 PM, Muthu Mani <muth@xxxxxxxxxxx> wrote:
>>>> > +
>>>> > +static int cy_gpio_direction_input(struct gpio_chip *chip,
>>>> > +                                       unsigned offset) {
>>>> > +       return 0;
>>>> > +}
>>>> > +
>>>> > +static int cy_gpio_direction_output(struct gpio_chip *chip,
>>>> > +                                       unsigned offset, int value) {
>>>> > +       return 0;
>>>> > +}
>>>>
>>>> If that chip is capable of both output and input, shouldn't these functions be
>>>> implemented? I think this has already been pointed out in a previous version
>>>> but you did not reply.
>>>
>>> Thanks for your inputs.
>>>
>>> Only the GPIOs which are configured to be output GPIO can be set.
>>
>> In that case cy_gpio_set() should return an error for GPIOs which are
>> not configured as outputs. Is that guaranteed by the current
>> implementation?
>>
>>> The set operation would fail trying to set the input or unconfigured GPIOs.
>>> In this version of driver, this support is not added, it can be introduced in future versions.
>>> I will add a TODO note in the code.
>>
>> Argh, no TODO please. Actual code that will turn this code into a
>> solid driver that can be merged.
>
> Does a driver targeted for a custom device has to implement every
> functionality in the 1st version ? My understanding is that Linux
> follows incremental model and allows incremental merge.

Certainly, but we are talking about very basic functionality here.

Right now this driver will incorrectly returns success when trying to
set the direction of a GPIO, even though it did not do anything. There
is also no hint that _set will return an error if called on an
input/unconfigured GPIO.

So while we are not asking for a driver to be perfect on the first
iteration, setting the direction of a GPIO is a very basic
functionality that is essential for a GPIO driver to just work. Custom
device or not, this is mainline. The rest of the driver seems to be
ok, so please take the last few steps that will allow us to merge a
featured driver.
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html