Messages in this thread | | | Date | Tue, 7 Jan 2014 15:33:27 +0100 | Subject | Re: [PATCH 3/3] gpio: MAX6650/6651 support | From | Linus Walleij <> |
| |
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
| |