On Mon, Jan 15, 2018 at 2:42 PM, Richard Fitzgerald
<rf@xxxxxxxxxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxxxxxxx>");
> +MODULE_AUTHOR("Richard Fitzgerald <rf@xxxxxxxxxxxxxxxxxxxxxxxxxxx>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:madera-gpio");
See above
--
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
