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

[Johan Hovold <johan@kernel.org>]

On Thu, Sep 25, 2014 at 05:47:11AM +0000, Muthu Mani wrote:
> > -----Original Message-----
> > From: Johan Hovold [mailto:jhovold@gmail.com] On Behalf Of Johan Hovold

> > > +static int cy_gpio_direction_input(struct gpio_chip *chip,
> > > +                                     unsigned offset) {
> > > +     return 0;
> > 
> > Aren't the GPIOs output-only?
> 
> No.

Then you should implement this callback to configure the pin as an
input.

Or can the configuration only be made through your configuration tools
and stored in eeprom?

Then you need to retrieve the actual config and implement these
callbacks (e.g. fail if not matching the eeprom config) along with
get_direction().

> > > +}
> > > +
> > > +static int cy_gpio_direction_output(struct gpio_chip *chip,
> > > +                                     unsigned offset, int value) {
> > > +     return 0;
> > > +}
> > > +
> > > +static int cyusbs23x_gpio_probe(struct platform_device *pdev) {
> > > +     struct cyusbs23x *cyusbs;
> > > +     struct cyusbs_gpio *cy_gpio;
> > > +     int ret = 0;
> > > +
> > > +     dev_dbg(&pdev->dev, "%s\n", __func__);
> > > +
> > > +     cyusbs = dev_get_drvdata(pdev->dev.parent);
> > > +
> > > +     cy_gpio = devm_kzalloc(&pdev->dev, sizeof(*cy_gpio), GFP_KERNEL);
> > > +     if (cy_gpio == NULL)
> > > +             return -ENOMEM;
> > > +
> > > +     cy_gpio->cyusbs = cyusbs;
> > > +     /* registering gpio */
> > > +     cy_gpio->gpio.label = "cyusbs23x gpio";
> > > +     cy_gpio->gpio.dev = &pdev->dev;
> > > +     cy_gpio->gpio.owner = THIS_MODULE;
> > > +     cy_gpio->gpio.base = -1;
> > > +     cy_gpio->gpio.ngpio = 12; /* total GPIOs */
> > 
> > I think you need to read out the gpio config from the device eeprom and
> > implement a request() callback where you verify that a requested gpio is
> > actually available (mode dependent, and there are never more than 10 gpios
> > availble).
> 
> In this initial driver, all GPIOs are made available to user space.
> Additional functionalities will be added in the next version of the driver.

I think this needs to be implemented now. It's needed for direction
handling as well as mentioned above.

> > > +     cy_gpio->gpio.can_sleep = true;
> > > +     cy_gpio->gpio.set = cy_gpio_set;
> > > +     cy_gpio->gpio.get = cy_gpio_get;
> > > +     cy_gpio->gpio.direction_input = cy_gpio_direction_input;
> > > +     cy_gpio->gpio.direction_output = cy_gpio_direction_output;
> > > +     ret = gpiochip_add(&cy_gpio->gpio);
> > > +     if (ret < 0) {
> > > +             dev_err(cy_gpio->gpio.dev, "could not add gpio");
> > > +             goto error;
> > > +     }

Thanks,
Johan