Re: [PATCH 10/16] gpio: madera: Support Cirrus Logic Madera class codecs

[Richard Fitzgerald]

On Fri, 2017-04-07 at 11:11 +0200, Linus Walleij wrote:
> On Wed, Apr 5, 2017 at 12:07 PM, Richard Fitzgerald
> <rf@xxxxxxxxxxxxxxxxxxxxxxxxxxx> wrote:
> 
> > This adds support for the GPIOs on Cirrus Logic Madera class codecs.
> 
> A bit terse commit message, could you elaborate a bit on their
> specifics?
> 

Sure.

> >  .../devicetree/bindings/gpio/gpio-madera.txt       |  24 +++
> 
> Again should probably be a separate patch. Again, I don't care much
> as long as the DT people are happy.
> 
> > +++ b/Documentation/devicetree/bindings/gpio/gpio-madera.txt
> > @@ -0,0 +1,24 @@
> > +Cirrus Logic Madera class audio codecs gpio driver
> > +
> > +This is a subnode of the parent mfd node.
> > +
> > +See also the core bindings for the parent MFD driver:
> > +See Documentation/devicetree/bindings/mfd/madera.txt
> > +
> > +Required properties:
> > +  - compatible : must be "cirrus,madera-gpio"
> > +  - gpio-controller : Indicates this device is a GPIO controller.
> > +  - #gpio-cells : Must be 2. The first cell is the pin number. The second cell
> > +    is reserved for future use and must be zero
> > +
> > +Example:
> > +
> > +codec: cs47l85@0 {
> > +       compatible = "cirrus,cs47l85";
> > +
> > +       gpio {
> > +               compatible = "cirrus,madera-gpio";
> > +               gpio-controller;
> > +               #gpio-cells = <2>;
> > +       }
> 
> Maybe you want to use the gpio-line-names = ; property in the example
> to show how nice it is to name the lines?
> 

I'll take a look at that.

> > +config GPIO_MADERA
> > +       tristate "Cirrus Logic Madera class codecs"
> > +       depends on MFD_MADERA
> > +       help
> > +         Support for GPIOs on Cirrus Logic Madera class codecs.
> 
> I wonder if you should not depend on the pin controller instead.
> It seems closer and also likely to act as a back-end for the
> GPIOs.
> 
> > +static int madera_gpio_get(struct gpio_chip *chip, unsigned int offset)
> > +{
> > +       struct madera_gpio *madera_gpio = gpiochip_get_data(chip);
> > +       struct madera *madera = madera_gpio->madera;
> > +       unsigned int val;
> > +       int ret;
> > +
> > +       ret = regmap_read(madera->regmap,
> > +                         MADERA_GPIO1_CTRL_1 + (2 * offset), &val);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       if (val & MADERA_GP1_LVL_MASK)
> > +               return 1;
> > +       else
> > +               return 0;
> 
> Just do this:
> 
> return !!(val & MADERA_GP1_LVL_MASK);
> 

Ok. Personally I like the clarity of the more verbose version rather
than the !! but I can change it.

> > +static struct gpio_chip template_chip = {
> > +       .label                  = "madera",
> > +       .owner                  = THIS_MODULE,
> > +       .direction_input        = madera_gpio_direction_in,
> > +       .get                    = madera_gpio_get,
> > +       .direction_output       = madera_gpio_direction_out,
> > +       .set                    = madera_gpio_set,
> > +       .can_sleep              = true,
> > +};
> 
> - Implement .get_direction()
> 

Ok


> Also consider implementing:
> 
> - request/free/set_config looking like this:
> 
> .request = gpiochip_generic_request,
> .free = gpiochip_generic_free,
> .set_config = gpiochip_generic_config,
> 
> If you also implement the corresponding
> .pin_config_set in struct pinconf_ops and
> .gpio_request_enable() and .gpio_disable_free()
> in struct pinmux_ops, you get a pin control back-end
> that will mux in the pins to GPIO mode if they are wrong
> set, and also set up debounce and/or open drain for the
> GPIO line using the standard GPIO callbacks with pin
> control as a back-end.
> 
> If you also specify "strict" in struct pinmux_ops you block
> the collisions between users of GPIO and other functions
> in the pin control driver.
> 
> (Please go back and look at your pin control driver
> for this.)
> 

I'll take a look at these things.

> Example driver using pin control as GPIO back-end:
> drivers/pinctrl/intel/pinctrl-intel.c
> 
> Other than this it looks fine.
> 
> Yours,
> Linus Walleij