On Mon, Dec 23, 2013 at 5:08 PM, Laszlo Papp <[email protected]> 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 <[email protected]>
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
On Tue, Jan 7, 2014 at 2:33 PM, Linus Walleij <[email protected]> wrote:
> On Mon, Dec 23, 2013 at 5:08 PM, Laszlo Papp <[email protected]> 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 <[email protected]>
>
> 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?
>>> +#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?
I was giving a second thought to this. Would it be acceptable to add
the gpio driver now, and once the need arises, add the pinctrl thin
layer on top of it? My concern is that I would not use anything else
than the gpio functionality of these pins. It would be a needless
additional work (i.e. investment) for my project and employer.
Perhaps, the layer on top of that can be added later without any
drawback if anyone ever finds the need to have more functionality
supported by these pins?
Shall look forward to hearing your opinion.
On Wed, Jan 8, 2014 at 3:59 PM, Laszlo Papp <[email protected]> wrote:
> On Tue, Jan 7, 2014 at 2:33 PM, Linus Walleij <[email protected]> wrote:
>> 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?
The other drivers were merged before the pin control subsystem
existed. If they had been submitted today, the comments would
be the same as for your driver.
>> 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?
Yes. It should be moved to pinctrl anyway.
What about rewriting and fixing up all drivers.
>> 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?
Yes please.
>> 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?
We want to move all drivers to module_init() (device_initcall level)
as shoveling initcalls around is creating an uncontrolled and
unmaintained mess.
To fix this, we're using deferred probe.
See commit d1c3414c2a9d10ef7f0f7665f5d2947cd088c093
"drivercore: Add driver probe deferral mechanism"
>> 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?
Aha OK I can live with this, sorry for missing the Kconfig
fragment.
>> 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?
Implement pin config portions of pin control. See
Documentation/pinctrl.txt
(You answer this yourself later.)
>>> + 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?
struct max_config {
u32 mask;
u8 shift;
};
struct max_config max_configs = {
{
.mask = PIN0_CONFIG_MASK,
.shift = 0,
},
{
.mask = PIN1_CONFIG_MASK,
.shift = 2,
},
....
struct max_config *cfg = max_configs[offset];
level = max665x_get_level(gpio, offset, (config & cfg->mask) >> cfg->shift);
You get the idea.
>> 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.
I mean use pin config from pin control for setting this.
Documentation/pinctrl.txt
Regarding your comment on platform data, see the section named
"Board/machine configuration".
>> Why can't you always use dynamic assignments of GPIO numbers?
>
> Because this information is board related.
OK so this board is not using any dynamic number assignment
system such as ACPI or device tree? Which board is it?
>> 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. :)
GPIO is not sufficient since it needs both GPIO and pin
control interfaces. You need both/and not either/or.
Look at other driver in drivers/pinctrl and you will get the hang
of this.
>>> +++ 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?
I mean use pin config from pin control for setting this.
Documentation/pinctrl.txt
Regarding your comment on platform data, see the section named
"Board/machine configuration".
Yours,
Linus Walleij
On Mon, Jan 13, 2014 at 10:22 AM, Laszlo Papp <[email protected]> wrote:
> I was giving a second thought to this. Would it be acceptable to add
> the gpio driver now, and once the need arises, add the pinctrl thin
> layer on top of it?
I will not accept the platform data setting the pull-ups.
> My concern is that I would not use anything else
> than the gpio functionality of these pins. It would be a needless
> additional work (i.e. investment) for my project and employer.
But you are still expecting me as a subsystem maintainer to
take responsibility of this driver for as long as I have this role?
Rewriting code is a natural part of the community process,
noone claimed it would be easy ;-)
> Perhaps, the layer on top of that can be added later without any
> drawback if anyone ever finds the need to have more functionality
> supported by these pins?
Your driver already supports setting the pulls using a
*custom* platform data field. This is not OK, and shall be
implemented using the pin control subsystem. I will not
merge drivers using custom platform data fields like this.
The reason that the pin control subsystem even existed was
that at the time my drivers were NACKed because I tried to
shoehorn pin control into the GPIO subsystem, and as a
result now we have an apropriate subsystem for it, so please
use it.
Yours,
Linus Walleij
On Mon, Jan 13, 2014 at 9:43 AM, Linus Walleij <[email protected]> wrote:
> On Wed, Jan 8, 2014 at 3:59 PM, Laszlo Papp <[email protected]> wrote:
>> On Tue, Jan 7, 2014 at 2:33 PM, Linus Walleij <[email protected]> wrote:
>
>>> 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?
>
> The other drivers were merged before the pin control subsystem
> existed. If they had been submitted today, the comments would
> be the same as for your driver.
Fair enough, but I have one question in here: shall I keep the gpio
driver in place and the layer on top of that would be established
under drivers/pinctrl or the whole should be moved to there?
>>> 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?
>
> Yes. It should be moved to pinctrl anyway.
Even the gpio functionality itself? So, to clarify my question above,
I thought it would be something like this:
- gpio
\
---
\
- alarm ----- pinctrol
/
----
- etc /
So, pinctrl on top of them, and only this high layer would be moved to
pinctrl. Perhaps, I should check existing drivers and the pinctrl
before asking this... Will have a look very soon.
> What about rewriting and fixing up all drivers.
>
>>> 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?
>
> Yes please.
Good, thanks.
>>> 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?
>
> We want to move all drivers to module_init() (device_initcall level)
> as shoveling initcalls around is creating an uncontrolled and
> unmaintained mess.
>
> To fix this, we're using deferred probe.
>
> See commit d1c3414c2a9d10ef7f0f7665f5d2947cd088c093
> "drivercore: Add driver probe deferral mechanism"
Thanks.
>>> 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?
>
> Aha OK I can live with this, sorry for missing the Kconfig
> fragment.
OK, thanks.
>>>> + 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?
>
> struct max_config {
> u32 mask;
> u8 shift;
> };
>
> struct max_config max_configs = {
> {
> .mask = PIN0_CONFIG_MASK,
> .shift = 0,
> },
> {
> .mask = PIN1_CONFIG_MASK,
> .shift = 2,
> },
> ....
>
> struct max_config *cfg = max_configs[offset];
>
> level = max665x_get_level(gpio, offset, (config & cfg->mask) >> cfg->shift);
>
> You get the idea.
Yes, thanks.
>>> 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.
>
> I mean use pin config from pin control for setting this.
> Documentation/pinctrl.txt
>
> Regarding your comment on platform data, see the section named
> "Board/machine configuration".
OK, I will have a look; thanks.
>>> Why can't you always use dynamic assignments of GPIO numbers?
>>
>> Because this information is board related.
>
> OK so this board is not using any dynamic number assignment
> system such as ACPI or device tree?
I cannot be sure if I understand the question correctly, but what I
can say now is that we use a custom board file not upstreamed.
> Which board is it?
It is our custom board. We have also been developing hardware, not
just software.
>>> 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. :)
>
> GPIO is not sufficient since it needs both GPIO and pin
> control interfaces. You need both/and not either/or.
> Look at other driver in drivers/pinctrl and you will get the hang
> of this.
OK, I believe the Linux kernel is too big to adequate answers from
anyone for any area, so it is fine. At least, I have been pointed to
here, but then it is good that this gpio functionality was not put
into the hwmon directly as suggested over there if I am not mistaken.
On Mon, Jan 13, 2014 at 9:48 AM, Linus Walleij <[email protected]> wrote:
> On Mon, Jan 13, 2014 at 10:22 AM, Laszlo Papp <[email protected]> wrote:
>
>> I was giving a second thought to this. Would it be acceptable to add
>> the gpio driver now, and once the need arises, add the pinctrl thin
>> layer on top of it?
>
> I will not accept the platform data setting the pull-ups.
OK.
>> My concern is that I would not use anything else
>> than the gpio functionality of these pins. It would be a needless
>> additional work (i.e. investment) for my project and employer.
>
> But you are still expecting me as a subsystem maintainer to
> take responsibility of this driver for as long as I have this role?
Well, since we need to make sure that our product rocks and rolls, me
and my employer would need be committed for a quite while, but I can
understand where you are coming from.
> Rewriting code is a natural part of the community process,
> noone claimed it would be easy ;-)
Yes, it is difficult, especially for a C++/OOP person like me... I am
trying to do my best.
>> Perhaps, the layer on top of that can be added later without any
>> drawback if anyone ever finds the need to have more functionality
>> supported by these pins?
>
> Your driver already supports setting the pulls using a
> *custom* platform data field. This is not OK, and shall be
> implemented using the pin control subsystem. I will not
> merge drivers using custom platform data fields like this.
>
> The reason that the pin control subsystem even existed was
> that at the time my drivers were NACKed because I tried to
> shoehorn pin control into the GPIO subsystem, and as a
> result now we have an apropriate subsystem for it, so please
> use it.
OK, thanks.
> Even the gpio functionality itself? So, to clarify my question above,
> I thought it would be something like this:
>
> - gpio
> \
> ---
> \
> - alarm ----- pinctrol
> /
> ----
> - etc /
>
> So, pinctrl on top of them, and only this high layer would be moved to
> pinctrl. Perhaps, I should check existing drivers and the pinctrl
> before asking this... Will have a look very soon.
I have just had a quick look and I am now worried.... It seems that
the pinctrl system was not as mature in 3.2 that I need to support as
these days... The short term task of mine is to get it working for
that release, and trying to upstream all this is an additional
bonus....
Hence, my question is: would it be acceptable to leave the gpio
functionality clearly separate and keep the pinctrl functionality on
top of it?
Failing that, is it somehow possible to get this functionality nicely
working with both versions? I am trying to do my best, but even I have
the limits. The definite target is 3.2.1 for us as of now. If it
cannot be nicely done without maintaining two relatively different
drivers until our update, I am afraid I would need to skip the
upstream now due to the lack of appropriate position.
I would appreciate any suggestion about it. The pinctrl system was
introduced in 3.1 if I am not mistaken, and 3.2 has lotta less drivers
around, etc... Not quite sure about the core foundation though.
On Tue, Jan 14, 2014 at 6:17 PM, Laszlo Papp <[email protected]> wrote:
[CC:ing Jon Corbet on this as he's better at the consensus process
and may correct me here.]
> I have just had a quick look and I am now worried.... It seems that
> the pinctrl system was not as mature in 3.2 that I need to support as
> these days... The short term task of mine is to get it working for
> that release, and trying to upstream all this is an additional
> bonus....
I am just a community maintainer, I do not worry very much about
your company's infatuation with ancient kernel releases, so please
do not bring such issues to us. We only develop and worry about
the mainline kernel, Torvald's HEAD, with stability fixes going into
the -stable kernels (not new stuff like this).
> Hence, my question is: would it be acceptable to leave the gpio
> functionality clearly separate and keep the pinctrl functionality on
> top of it?
What do you mean?
These are two orthogonal subsystems with some GPIO-to-pin
criss-cross APIs, I'm just asking that you implement both APIs:
GPIO and pin control.
Please look at other combined drivers in drivers/pinctrl
and you will see what I mean.
> Failing that, is it somehow possible to get this functionality nicely
> working with both versions?
The kernel development process is not suited to such things.
This is not how we work, we don't want people to develop
*new features* on old kernels, that is just insane.
> The definite target is 3.2.1 for us as of now.
I'm sorry to tell you that your organization does not understand
how the mainline community develop new features. If you want to
work with the community you develop new features on the HEAD,
last kernel release (v3.12, soon v3.13) or directly on top of the
pin control development tree in this case.
The fact that some person or function in your company think it
is a good idea to base development on kernel v3.2.x does not
concern me, nor anyone else in the community.
I mean, of course it is interesting to know odd stuff about the
remote corners of kernel development, but it doesn't influence our
behavior.
> I would appreciate any suggestion about it. The pinctrl system was
> introduced in 3.1 if I am not mistaken, and 3.2 has lotta less drivers
> around, etc... Not quite sure about the core foundation though.
If a company decides to work on a fork from the community
that is their problem, basically. Then they assume all forward-porting
and maintenance costs, sometimes also a huge cost of
backward-porting entire subsystems and an increasing threshold
to migrate to new kernel releases as time goes by.
I'm not saying this is necessarily a bad decision for the company,
but it means forking and disconnecting from the community,
we will not come around and work like you do.
I have seen this behaviour in many companies, and what they
typically do not realize is that working this way comes with a
cost because they are still dependent on new features only
developed upstream and they often do not realize the issue of
escalting maintenance cost as the fork widens over time.
Then we have the ontology problem:
The suggestion from any system manager (or similar
function/role) in an organization that they want to lock down
development to a certain older kernel version that "they know
is stable" or "have agreed upon" or something like that is a
complete fallacy, they are not the architects and developers of
the Linux kernel and they are not competent to make such
statements, to put one thing very clear.
The only people that can make such statements you will find
in the MAINTAINERS file of the kernel and that's it. Very simple.
Usually they will tell you to do your development on the very
latest kernel.
It might be that someone pays your wages to work and think
counter to this very simple rule, but that is another thing, that
is not the truth or the way the world works, it is just a power
relation in the working place and while I do realize that this is a
fact of life, it doesn't change the way the community works.
Yours,
Linus Walleij
On Wed, Jan 15, 2014 at 10:05 AM, Linus Walleij
<[email protected]> wrote:
> On Tue, Jan 14, 2014 at 6:17 PM, Laszlo Papp <[email protected]> wrote:
>
> [CC:ing Jon Corbet on this as he's better at the consensus process
> and may correct me here.]
>
>> I have just had a quick look and I am now worried.... It seems that
>> the pinctrl system was not as mature in 3.2 that I need to support as
>> these days... The short term task of mine is to get it working for
>> that release, and trying to upstream all this is an additional
>> bonus....
>
> I am just a community maintainer, I do not worry very much about
> your company's infatuation with ancient kernel releases, so please
> do not bring such issues to us. We only develop and worry about
> the mainline kernel, Torvald's HEAD, with stability fixes going into
> the -stable kernels (not new stuff like this).
>
>> Hence, my question is: would it be acceptable to leave the gpio
>> functionality clearly separate and keep the pinctrl functionality on
>> top of it?
>
> What do you mean?
>
> These are two orthogonal subsystems with some GPIO-to-pin
> criss-cross APIs, I'm just asking that you implement both APIs:
> GPIO and pin control.
>
> Please look at other combined drivers in drivers/pinctrl
> and you will see what I mean.
>
>> Failing that, is it somehow possible to get this functionality nicely
>> working with both versions?
>
> The kernel development process is not suited to such things.
> This is not how we work, we don't want people to develop
> *new features* on old kernels, that is just insane.
>
>> The definite target is 3.2.1 for us as of now.
>
> I'm sorry to tell you that your organization does not understand
> how the mainline community develop new features. If you want to
> work with the community you develop new features on the HEAD,
> last kernel release (v3.12, soon v3.13) or directly on top of the
> pin control development tree in this case.
>
> The fact that some person or function in your company think it
> is a good idea to base development on kernel v3.2.x does not
> concern me, nor anyone else in the community.
>
> I mean, of course it is interesting to know odd stuff about the
> remote corners of kernel development, but it doesn't influence our
> behavior.
>
>> I would appreciate any suggestion about it. The pinctrl system was
>> introduced in 3.1 if I am not mistaken, and 3.2 has lotta less drivers
>> around, etc... Not quite sure about the core foundation though.
>
> If a company decides to work on a fork from the community
> that is their problem, basically. Then they assume all forward-porting
> and maintenance costs, sometimes also a huge cost of
> backward-porting entire subsystems and an increasing threshold
> to migrate to new kernel releases as time goes by.
>
> I'm not saying this is necessarily a bad decision for the company,
> but it means forking and disconnecting from the community,
> we will not come around and work like you do.
>
> I have seen this behaviour in many companies, and what they
> typically do not realize is that working this way comes with a
> cost because they are still dependent on new features only
> developed upstream and they often do not realize the issue of
> escalting maintenance cost as the fork widens over time.
>
> Then we have the ontology problem:
> The suggestion from any system manager (or similar
> function/role) in an organization that they want to lock down
> development to a certain older kernel version that "they know
> is stable" or "have agreed upon" or something like that is a
> complete fallacy, they are not the architects and developers of
> the Linux kernel and they are not competent to make such
> statements, to put one thing very clear.
>
> The only people that can make such statements you will find
> in the MAINTAINERS file of the kernel and that's it. Very simple.
> Usually they will tell you to do your development on the very
> latest kernel.
>
> It might be that someone pays your wages to work and think
> counter to this very simple rule, but that is another thing, that
> is not the truth or the way the world works, it is just a power
> relation in the working place and while I do realize that this is a
> fact of life, it doesn't change the way the community works.
>
> Yours,
> Linus Walleij
Hmm, this email seems about business stuff and how the community
works, but I did not mean to question that...
I believe I am aware some (most?) of this, but thank you for your
explanation either way. It is useful for the readers. Fwiw, I have
been a remote part of the Nokia Maemo/MeeGo Linux kernel fork and
work. I also developed USB drivers 5-6 years ago where
(out-of-my-desire), we could not upstream drivers for our devices.
Either way, I would like to clarify my previous question as I could
not manage to get the point through: Is it possible /technically/ to
have a similar version with both kernel versions? I am only interested
in this on the technical ground to be able to evaluate if it is worth
dealing with upstreaming at this point at all. I was not expecting you
to accept my situation which is again out-of-my-desire why we have to
stick to an older version.
If the answer is that, "the community has no care to give technical
insight for old versions", that is also fine. No harm done there. I
just thought it would not hurt asking because it could be a potential
benefit for the community if I could manage both without maintain two
separate drivers. The world of course continues further on [1] if we
cannot. Please do not get me wrong, this is not unwillingness, but
currently it would require a huge investment compared to just get
things done and draw the income for the wages. It is out-of-my-desire
or my colleagues' of course, but the life is full of compromise as you
also already know.
Thank you for your answer either way. Cheers.
[1] https://plus.google.com/103895730806848715870/posts/1q34T1iYU4j
On Wed, Jan 15, 2014 at 11:51 AM, Laszlo Papp <[email protected]> wrote:
> Hmm, this email seems about business stuff and how the community
> works, but I did not mean to question that...
Well you did ask to merge something I didn't consider a proper
implementation due to some time constraints so that is why I
brought this up.
> Either way, I would like to clarify my previous question as I could
> not manage to get the point through: Is it possible /technically/ to
> have a similar version with both kernel versions?
When it comes to pin control, no. Because it is developing at
a rapid pace. But the same goes soon for GPIO as well, as we're
refactoring the GPIO pace.
So the answer to the technical question is "it depends"
and it depends on the evolutional change rate in the core of
each subsystem. They can be stable for years and then suddenly
there are changes all over the place, so the back-portability is
sometimes there, sometimes not, and the outcome is
unpredictable.
> I am only interested
> in this on the technical ground to be able to evaluate if it is worth
> dealing with upstreaming at this point at all.
OK sorry if I was a bit explosive in this last reply. I defininately
think you should invest in upstreaming, even if the code you
upstream is not usable in your present company-internal codebase
and current deadlines. Because in the long term, if the thing
you're using it with lives long enough, you will get that
code back one day. And then you can move faster at that point.
Basically I think a person working at the inside of a company
should strive to have *something* booting and ticking using just
the mainline kernel, even if not all drivers are in place. If for
nothing else so for keeping track of what is happening upstream.
> If the answer is that, "the community has no care to give technical
> insight for old versions", that is also fine.
Gave some above I think even if it's not what you wanted. Here
is another thing, <linux/version.h> contains
#define KERNEL_VERSION(a,b,c) (((a) << 16) + ((b) << 8) + (c))
Conjuring a macro from a kernelversion matched to
LINUX_VERSION_CODE so that a driver can #if statements
to conditionally compile stuff for different kernels, like
#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,2,0)
/* foo */
#else
/* bar */
#endif
I *think* this is not supposed to be used to make the same code
compile on different kernel versions (rather for things like
glibc etc that need to behave differently with different kernel
baselines), but it *can* be used for e.g. creating kernel modules
that compile to different code on different kernels.
> Please do not get me wrong, this is not unwillingness, but
> currently it would require a huge investment compared to just get
> things done and draw the income for the wages. It is out-of-my-desire
> or my colleagues' of course, but the life is full of compromise as you
> also already know.
Yeah, I realize that not everyone can live to their desires,
which we could call an insight of weltschmerz or Dukkha. As I
am existentialist I try very hard, every day, to make the right choice
for each situation. But that is for each and every one of us to do.
Good luck!
Yours,
Linus Walleij