lkml.org 
[lkml]   [2014]   [Jan]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 3/3] gpio: MAX6650/6651 support
From
On Tue, Jan 7, 2014 at 2:33 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> 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.

Hmm, but then I am not sure why the gpio-max*.c are similar in the
drivers/gpio area. Could you please elaborate? They are somewhat
similar to my understanding, but perhaps there is some fundamental
difference I am not aware of?

>> +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?

I personally think it is a bit excessive, so I agree with you. I
wished to stay somewhat consistent to the following drivers:

* gpio-max730x.c
* gpio-max7300.c
* gpio-max7301.c

Are you OK with that then if I do not follow the consistency?

> Anyway if you absolutely have to do
> this don't use "__" prefixes, those are for the preprocessor.
> Just max665x_probe() is fine.

This is because of the same reason as above: consistency. I can drop
it if you wish?

>> +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.

It is for consistency just as before. :-) Could you please elaborate
why it is better to be avoided, or at least point me to some
documentation?

>> +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.

Hmm.

>> +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.

The MAX6651 chip is multifunctional, but it is ultimate a fan
controller IC as per Kconfig guided text. If you prefer, I can rename
the term here to refer to the GPIO subfunctionality, or you had
something else in mind?

>> +++ 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.

Could you please elaborate what more stuff you have in mind for this?

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

I am actually not sure about the difference... I will have a look.

>> +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.

Ah, getting somewhat clearer (maybe) ...

>
> - 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.

Perhaps, but that is how the chip behaves after all.

>> +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.

OK.

>> + 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.

OK.

>
>> + 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...

OK.

>> +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.

How exactly would you like to have it?

>> +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.

What do you mean here? Could you please elaborate a bit more? The pull
up trait depends on the given hardware. It must be coming from
platform data, e.g. we can supply the right variant from our custom
board file.

>> + gpio->gp.base = pdata->base;
>
> Why can't you always use dynamic assignments of GPIO numbers?

Because this information is board related.

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

See above.

>> + }
>> + 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.

I will have a look...

>
>> + 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.

Well, Guenter wanted to go through the gpio system... Perhaps, it was
not made clear that pinctrl would be better. I just followed the GPIO
generic guideline. :)

>
>> +EXPORT_SYMBOL_GPL(__max665x_probe);
>> +
>> +int __max665x_remove(struct device *dev)
>
> Argh these __underscores, get rid of them.

OK.

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

Why so? How would you make it customizable by board files otherwise?

>
>> + 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.

OK?


\
 
 \ /
  Last update: 2014-01-08 16:21    [W:0.077 / U:0.028 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site