Message-ID: <CAOMwXhNcmbk92TsNTGVQdfWrbVycVW++-+46+8onDZ5xPCD-Jw@mail.gmail.com>
Date:	Wed, 8 Jan 2014 14:59:14 +0000
From:	Laszlo Papp <lpapp@....org>
To:	Linus Walleij <linus.walleij@...aro.org>
Cc:	Guenter Roeck <linux@...ck-us.net>,
	Lee Jones <lee.jones@...aro.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/3] gpio: MAX6650/6651 support

On Tue, Jan 7, 2014 at 2:33 PM, Linus Walleij <linus.walleij@...aro.org> wrote:
> On Mon, Dec 23, 2013 at 5:08 PM, Laszlo Papp <lpapp@....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@....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?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists