Re: [alsa-devel] [PATCH v7 12/17] gpio: madera: Support Cirrus Logic Madera class codecs

On Mon, Jan 15, 2018 at 2:42 PM, Richard Fitzgerald rf@opensource.wolfsonmicro.com wrote:

This adds support for the GPIOs on Cirrus Logic Madera class codecs. Any pins not used for special functions (see the pinctrl driver) can be used as general single-bit input or output lines. The number of available GPIOs varies between codecs.

+config GPIO_MADERA

•   bool "Cirrus Logic Madera class codecs"
  

Not module?

+/*

• • GPIO support for Cirrus Logic Madera codecs
• • Copyright 2015-2017 Cirrus Logic

SPDX ?

• • This program is free software; you can redistribute it and/or modify
• • it under the terms of the GNU General Public License version 2 as
• • published by the Free Software Foundation.
• */

+#include <linux/device.h> +#include <linux/gpio.h> +#include <linux/kernel.h>

+#include <linux/module.h>

?!?! Same to the MODULE_*() macros and Co.

+static int madera_gpio_get_direction(struct gpio_chip *chip,

•                                unsigned int offset)
  

+{

•   ret = regmap_read(madera->regmap,
  

•                     MADERA_GPIO1_CTRL_2 + (2 * offset), &val);
  

Redundant parens here and everywhere. * has higher priority and noone I'm pretty sure can think other way.

•   if (ret < 0)
  

•           return ret;
  

•   return (val & MADERA_GP1_DIR_MASK) >> MADERA_GP1_DIR_SHIFT;
  

!! missed ?

+}

+static int madera_gpio_direction_out(struct gpio_chip *chip,

•                                unsigned int offset, int value)
  

+{

•   struct madera_gpio *madera_gpio = gpiochip_get_data(chip);
  

•   struct madera *madera = madera_gpio->madera;
  

•   unsigned int regval;
  

•   int ret;
  

•   if (value)
  

•           regval = MADERA_GP1_LVL;
  

•   else
  

•           regval = 0;
  

Perhaps

regval = value ? ... : 0;

?

•   if (value)
  

•           regval = MADERA_GP1_LVL;
  

•   else
  

•           regval = 0;
  

Ditto.

+static int madera_gpio_probe(struct platform_device *pdev) +{

•   struct madera *madera = dev_get_drvdata(pdev->dev.parent);
  

•   struct madera_pdata *pdata = dev_get_platdata(madera->dev);
  

•   struct madera_gpio *madera_gpio;
  

•   int ret;
  

•   madera_gpio->madera = madera;
  

•   madera_gpio->gpio_chip = template_chip;
  

•   madera_gpio->gpio_chip.parent = &pdev->dev;
  

•   if (IS_ENABLED(CONFIG_OF_GPIO))
  

•           madera_gpio->gpio_chip.of_node = madera->dev->of_node;
  

Isn't it done in GPIO core?

•   if (pdata && pdata->gpio_base)
  

•           madera_gpio->gpio_chip.base = pdata->gpio_base;
  

platform data in modern driver? Hmm...

•   else
  

•           madera_gpio->gpio_chip.base = -1;
  

•   ret = devm_gpiochip_add_data(&pdev->dev, &madera_gpio->gpio_chip,
  

•                                madera_gpio);
  

•   if (ret < 0) {
  

•           dev_err(&pdev->dev, "Could not register gpiochip, %d\n", ret);
  

•           return ret;
  

•   }
  

•   ret = gpiochip_add_pin_range(&madera_gpio->gpio_chip, "madera-pinctrl",
  

•                                0, 0, madera_gpio->gpio_chip.ngpio);
  

•   if (ret) {
  

•           dev_warn(&pdev->dev, "Failed to add pin range (%d)\n", ret);
  

•           return ret;
  

•   }
  

Consider if your messages above indeed make value. Hint: device core prints a warning if ->probe() fails.

+static struct platform_driver madera_gpio_driver = {

•   .driver.name    = "madera-gpio",
  

Please, do in more portable way, i.e. .driver = { .name = "...", },

•   .driver.owner   = THIS_MODULE,
  

Is this still needed? I suppose below macro does it for ya.

•   .probe          = madera_gpio_probe,
  

+};

+module_platform_driver(madera_gpio_driver);

+MODULE_DESCRIPTION("GPIO interface for Madera codecs"); +MODULE_AUTHOR("Nariman Poushin nariman@opensource.wolfsonmicro.com"); +MODULE_AUTHOR("Richard Fitzgerald rf@opensource.wolfsonmicro.com"); +MODULE_LICENSE("GPL v2"); +MODULE_ALIAS("platform:madera-gpio");

See above