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


\
 
 \ /
  Last update: 2014-01-13 13:41    [W:0.106 / U:0.136 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site