lkml.org 
[lkml]   [2014]   [Jan]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 3/3] gpio: MAX6650/6651 support
From
On Mon, Dec 23, 2013 at 5:08 PM, Laszlo Papp <lpapp@kde.org> wrote:

> These ICs already have hwmon driver support, but they also have some gpio
> functionality which this addition tries to address. Later on, there would be an
> MFD driver added as well.
>
> Signed-off-by: Laszlo Papp <lpapp@kde.org>

As mentioned please augment the MFD device to use I2C regmap access.

> +++ b/drivers/gpio/gpio-max6651.c
(...)
> +#define PIN_NUMBER 5

As I can see this is really a GPIO+pin control driver it shall be
moved to drivers/pinctrl.

> +static int max6651_probe(struct platform_device *pdev)
> +{
> + struct max665x_gpio *gpio;
> + struct da9055_pdata *pdata;
> + int ret;
> +
> + gpio = devm_kzalloc(&pdev->dev, sizeof(struct max665x_gpio), GFP_KERNEL);
> + if (!gpio)
> + return -ENOMEM;
> +
> + gpio->gp.ngpio = PIN_NUMBER;
> +
> + ret = __max665x_probe(gpio);

Do you *really* have to split up this handling into two files with
criss-cross calls like this? Anyway if you absolutely have to do
this don't use "__" prefixes, those are for the preprocessor.
Just max665x_probe() is fine.

> +static struct platform_driver max6651_driver = {
> + .driver = {
> + .name = "gpio-max6651",
> + .owner = THIS_MODULE,
> + },
> + .probe = max6651_probe,
> + .remove = max6651_remove,
> + .id_table = max6651_id,
> +};
> +
> +static int __init max6651_init(void)
> +{
> + return platform_driver_register(&max6651_driver);
> +}
> +subsys_initcall(max6651_init);

Why does it have to be subsys_initcall? Please try to avoid
this.

> +static void __exit max6651_exit(void)
> +{
> + platform_driver_unregister(&max6651_driver);
> +}
> +module_exit(max6651_exit);

Because then this can just be a module_platform_driver() macro.

> +MODULE_AUTHOR("Laszlo Papp");
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("MAX6651 fan controller");

And *why* should I have a fan controller in the GPIO subsystem?
I don't quite get this.

> +++ b/drivers/gpio/gpio-max665x.c
(...)
> +#define MAX665X_REG_GPIO_DEF 0x04
> +#define MAX665X_REG_GPIO_STAT 0x14
> +
> +/*
> + * Gpio Def register bits
> + */
> +
> +#define PIN0_CONFIG_MASK 0x03
> +#define PIN1_CONFIG_MASK 0x0C
> +#define PIN2_CONFIG_MASK 0x30
> +#define PIN3_CONFIG_MASK 0x40
> +#define PIN4_CONFIG_MASK 0x80
> +
> +#define PIN0_CONFIG_OUT_LOW 0x02
> +#define PIN1_CONFIG_OUT_LOW 0x08
> +#define PIN2_CONFIG_OUT_LOW 0x20

Since this is really pin configuration the driver should support more
stuff in the long run, and then it should be in drivers/pinctrl.

If it is "just" GPIO, then rename all PIN* prefixes to LINE*

> +struct max665x_platform_data {
> + /* number assigned to the first GPIO */
> + unsigned base;
> + /*
> + * bitmask controlling the pullup configuration,
> + *
> + * _note_ the 3 highest bits are unused, because there can be maximum up
> + * to five gpio pins on the MAX6651 chip (three on MAX6650).
> + */
> + u8 input_pullup_active;

So obviously this is not just GPIO but also pin control.

Read Documentation/pinctrl.txt, you will only need to
implement a pin config interface since it seems you have
no muxing in this component.

- In drivers/pinctrl/Kconfig for your driver select
GENERIC_PINCONF

- Implement a pinctrl and pinconf interface apart from
the GPIOlib interface you already have.

- Supply pin control tables to set up biasing.

> +static int max665x_direction_input_or_output_high(struct gpio_chip *chip, unsigned offset)

That sounds like an odd combination.

> +static int max665x_get_level(struct max665x_gpio *gpio, unsigned offset, unsigned gpio_value)
> +{
> + int ret;
> +
> + if (offset < 3) {
> + switch (gpio_value) {
> + case 0:
> + case 3:
> + if (gpio->input_pullup_active & (1 << offset)) {

Why does this only work if pullup is active?

Describe with a comment why this is so.

> + max6651_read_reg(gpio->iodev->i2c, MAX665X_REG_GPIO_STAT, &ret);
> + ret &= (offset + 1);
> + } else {
> + ret = 1;
> + }
> + break;
> + case 2:
> + ret = 0;
> + break;

Describe with a comment this special case and why it behaves like that.

> + default:
> + ret = 0;
> + dev_err(gpio->iodev->i2c, "Failed to obtain the gpio %d value\n", offset);
> + break;
> + }
> + } else {
> + if (gpio_value) {
> + if (gpio->input_pullup_active & (1 << offset)) {
> + max6651_read_reg(gpio->iodev->i2c, MAX665X_REG_GPIO_STAT, &ret);

Same thing...

> +static int max665x_get(struct gpio_chip *chip, unsigned offset)
> +{
> + struct max665x_gpio *gpio = to_max665x_gpio(chip);
> + int level = -EINVAL;
> + u8 config;
> +
> + mutex_lock(&gpio->iodev->iolock);
> +
> + max6651_read_reg(gpio->iodev->i2c, MAX665X_REG_GPIO_DEF, &config);
> +
> + switch (offset) {
> + case 0:
> + level = max665x_get_level(gpio, offset, config & PIN0_CONFIG_MASK);
> + break;
> + case 1:
> + level = max665x_get_level(gpio, offset, (config & PIN1_CONFIG_MASK) >> 2);
> + break;
> + case 2:
> + level = max665x_get_level(gpio, offset, (config & PIN2_CONFIG_MASK) >> 4);
> + break;
> + case 3:
> + level = max665x_get_level(gpio, offset, (config & PIN3_CONFIG_MASK) >> 5);
> + break;
> + case 4:
> + level = max665x_get_level(gpio, offset, (config & PIN3_CONFIG_MASK) >> 6);
> + break;

This looks like it could be made a lot simpler using a table.

> +int __max665x_probe(struct max665x_gpio *gpio)
> +{
> + struct max665x_platform_data *pdata = dev_get_drvdata(gpio->iodev->dev);
> + int offset, ret;
> +
> + mutex_init(&gpio->iodev->iolock);
> + dev_set_drvdata(gpio->iodev->dev, gpio);
> +
> + if (pdata) {
> + gpio->input_pullup_active = pdata->input_pullup_active;

No way. No custom interfaces for setting pullups, use generic pin config.

> + gpio->gp.base = pdata->base;

Why can't you always use dynamic assignments of GPIO numbers?

> + } else {
> + gpio->gp.base = -1;

Like this?

> + }
> + gpio->gp.label = gpio->iodev->dev->driver->name;

= dev_name(gpio->iodev->dev); I think.

> + /*
> + * initialize input pullups according to platform data.
> + */

No, this shall be done using a pinctrl table.

> + ret = gpiochip_add(&gpio->gp);
> + if (ret)
> + goto exit_destroy;

When implementing the pinctrl interface, you may want to use
gpiochip_add_pin_range() to cross-reference between GPIOs
and pinctrl.

> +EXPORT_SYMBOL_GPL(__max665x_probe);
> +
> +int __max665x_remove(struct device *dev)

Argh these __underscores, get rid of them.

> +++ b/include/linux/mfd/max6651-private.h
(...)
> +struct max665x_gpio {
> + u8 input_pullup_active;

No way.

> + struct max6651_dev *iodev;
> + struct gpio_chip gp;
> +};
> +
> +extern int __max665x_remove(struct device *dev);
> +extern int __max665x_probe(struct max665x_gpio *ts);

Seems overengineered, try to keep all in one file.

Yours,
Linus Walleij


\
 
 \ /
  Last update: 2014-01-07 17:01    [W:0.236 / U:0.116 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site