Message-ID: <CAOMwXhMtc44SpA5yoGBP38muMqPMi3EkxeAKh3V5c_H=3qoXhw@mail.gmail.com>
Date:	Mon, 13 Jan 2014 12:12:29 +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 Mon, Jan 13, 2014 at 9:43 AM, Linus Walleij <linus.walleij@...aro.org> wrote:
> On Wed, Jan 8, 2014 at 3:59 PM, Laszlo Papp <lpapp@....org> wrote:
>> On Tue, Jan 7, 2014 at 2:33 PM, Linus Walleij <linus.walleij@...aro.org> 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.
--
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