linux-kernel - Re: [PATCH v2 2/4] ARM: mediatek: Add Pinctrl/GPIO driver for mt8135.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <CACRpkdZ7XiQHACQ2jhEGeTR6V7dJcX2jR40yPqLgLONnka9zVw@mail.gmail.com>
Date:	Tue, 21 Oct 2014 11:08:15 +0200
From:	Linus Walleij <linus.walleij@...aro.org>
To:	"Joe.C" <yingjoe.chen@...iatek.com>
Cc:	"Hongzhou.Yang" <srv_hongzhou.yang@...iatek.com>,
	Rob Herring <robh+dt@...nel.org>,
	Matthias Brugger <matthias.bgg@...il.com>,
	srv_heupstream@...iatek.com, Sascha Hauer <kernel@...gutronix.de>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	Russell King <linux@....linux.org.uk>,
	Grant Likely <grant.likely@...aro.org>,
	Hongzhou Yang <hongzhou.yang@...iatek.com>,
	Catalin Marinas <catalin.marinas@....com>,
	Vladimir Murzin <vladimir.murzin@....com>,
	Ashwin Chaugule <ashwin.chaugule@...aro.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>, dandan.he@...iatek.com
Subject: Re: [PATCH v2 2/4] ARM: mediatek: Add Pinctrl/GPIO driver for mt8135.

On Mon, Oct 6, 2014 at 12:35 PM, Joe.C <yingjoe.chen@...iatek.com> wrote:
> On Thu, 2014-10-02 at 15:38 +0200, Linus Walleij wrote:
> (...)
>> > +static struct mt_desc_function *
>> > +mt_pctrl_desc_find_irq_by_name(struct mt_pinctrl *pctl,
>> > +                                        const char *pin_name)
>>
>> Why is it called *find_irq_by_name if it returns a
>> function? Seems more like find_function_from_pin_name
>> really.
>>
>> And I don't know if that is such a good idea.
>
> In 8135 & 6589, not every gpio pin support interrupt function. For those
> support interrupt, it will have a EINT function and use a different EINT
> offset number.
> In 8127, EINT support is merged into gpio function, but they still use a
> different EINT offset number.
(...)
> This function is used to find EINT function for the pin. Maybe we should
> name this mt_pctrl_desc_find_irq_function_from_name to make it more
> clear.

OK such translation is usually the work of the irqdomain. Is there
some reason why it is not used in this driver then?

>> > +                       for (j = 0; j < PINMUX_MAX_VAL; j++) {
>> > +                               if (func->irqnum != 255)
>>
>> So why does it end at 255? Seems pretty arbitrary.
>
> If a function support interrupt, we put its interrupt number in irqnum,
> otherwise it will be 255. Does it make it more clear if we use macro
> name MT_NO_EINT_SUPPORT?

Yes.

> We use a different interrupt number than gpio pin number. I think it
> more nature to use EINT interrupt number as the hw_number, so I think we
> can't use gpiochip_irqchip_add and we still need to provide our
> own .to_irq mapping function.

You should be able to use irqdomain to translate I think.

> While it might still be possible to generate group+function array based
> on datasheet, IMHO the structure will be more complicate and harder to
> prove the correctness.
>
> So we choose to use descriptor array + macros in device tree because it
> is quite simple to generate the pin descriptors and easier to notice if
> there's error in device tree pin groups description.

There is a parallel discussion on this, or two maybe.

The number of pin control bindings is exploding and I need to
push back.

Please help out defining generic pin control bindings for this
use case and we can move forward.

Yours,
Linus Walleij
--
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/