Re: [PATCH v2 0/4] Add Mediatek SoC Pinctrl/GPIO driver for MT8135.

Re: [PATCH v2 0/4] Add Mediatek SoC Pinctrl/GPIO driver for MT8135.

[Matthias Brugger]

On 23/09/14 05:39, Hongzhou.Yang wrote:
> This is v2 of add Mediatek SoC Pinctrl/GPIO drvier for MT8135.
> It is based on Joe.C' basic device tree support.
> See http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/288582.html
> 
> This driver include common and MT8135 part, other Mediatek SoCs will share the common part,
> and MT8135 part only support MT8135. MT8135 has GPIO controller, it includes 203 pins.
> 
> Changes in v2:
> - According to Heiko Stubner' suggestion, use generic pinconfig.

Please always add people that made comments on your former versions in
CC. This way it is easier for them to follow the debate about your patches.

Thanks,
Matthias

> - Remove pinmux_ops.disable implement.
> - Due to limit of message body, move mt8135-pinfunc.h to '[PATCH 3/4] add pinctrl node for MT8135'.
> 
> 
> Hongzhou Yang (3):
> ARM: mediatek: Add Pinctrl/GPIO driver for mt8135.
> dt-bindings: Add pinctrl bindings for mt65xx/mt81xx.
> ARM: dts: mt8135: Add pinctrl node for mt8135.
> 
> Joe.C (1):
> arm: mediatek: Add config option for mediatek SoCs.
> 
> .../devicetree/bindings/pinctrl/pinctrl-mt65xx.txt |   98 +
> arch/arm/boot/dts/mt8135-pinfunc.h                 | 1304 +++++++++++
> arch/arm/boot/dts/mt8135.dtsi                      |   11 +
> arch/arm/mach-mediatek/Kconfig                     |   23 +-
> drivers/pinctrl/Kconfig                            |    1 +
> drivers/pinctrl/Makefile                           |    1 +
> drivers/pinctrl/mediatek/Kconfig                   |   12 +
> drivers/pinctrl/mediatek/Makefile                  |    5 +
> drivers/pinctrl/mediatek/pinctrl-mt8135.c          |   82 +
> drivers/pinctrl/mediatek/pinctrl-mtk-common.c      |  792 +++++++
> drivers/pinctrl/mediatek/pinctrl-mtk-common.h      |   95 +
> drivers/pinctrl/mediatek/pinctrl-mtk-mt8135.h      | 2460 ++++++++++++++++++++
> include/dt-bindings/pinctrl/mt65xx.h               |   23 +
> 13 files changed, 4904 insertions(+), 3 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/pinctrl/pinctrl-mt65xx.txt
> create mode 100644 arch/arm/boot/dts/mt8135-pinfunc.h
> create mode 100644 drivers/pinctrl/mediatek/Kconfig
> create mode 100644 drivers/pinctrl/mediatek/Makefile
> create mode 100644 drivers/pinctrl/mediatek/pinctrl-mt8135.c
> create mode 100644 drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> create mode 100644 drivers/pinctrl/mediatek/pinctrl-mtk-common.h
> create mode 100644 drivers/pinctrl/mediatek/pinctrl-mtk-mt8135.h
> create mode 100644 include/dt-bindings/pinctrl/mt65xx.h
> 
> --
> 1.8.1.1.dirty
> 
> 
> 
> 

Re: [PATCH v2 4/4] ARM: dts: mt8135: Add pinctrl node for mt8135.

[Arnd Bergmann]

On Tuesday 23 September 2014 11:39:05 Hongzhou. Yang wrote:
> +#define MT8135_PIN_0_MSDC0_DAT7__FUNC_GPIO0 (MT_PIN_NO(0) | 0)
> +#define MT8135_PIN_0_MSDC0_DAT7__FUNC_MSDC0_DAT7 (MT_PIN_NO(0) | 1)
> +#define MT8135_PIN_0_MSDC0_DAT7__FUNC_EINT49 (MT_PIN_NO(0) | 2)
> +#define MT8135_PIN_0_MSDC0_DAT7__FUNC_I2SOUT_DAT (MT_PIN_NO(0) | 3)
> +#define MT8135_PIN_0_MSDC0_DAT7__FUNC_DAC_DAT_OUT (MT_PIN_NO(0) | 4)
> +#define MT8135_PIN_0_MSDC0_DAT7__FUNC_PCM1_DO (MT_PIN_NO(0) | 5)
> +#define MT8135_PIN_0_MSDC0_DAT7__FUNC_SPI1_MO (MT_PIN_NO(0) | 6)
> +#define MT8135_PIN_0_MSDC0_DAT7__FUNC_NALE (MT_PIN_NO(0) | 7)
> +

This list looks like it just describes the hardware, I think it would
be better to put the values directly into the DT, rather than
using such macros.

	Arnd

Re: [PATCH v2 4/4] ARM: dts: mt8135: Add pinctrl node for mt8135.

[Arnd Bergmann]

On Tuesday 23 September 2014 21:58:14 Joe. C wrote:
> Thanks for review. 
> The intend for these macros is helpin pinctrl user to write DT node.
> With these macro, we could write like this for i2c0:
> 
> mediatek,pinfunc = <MT8135_PIN_100_SDA0__FUNC_SDA0 
>                         MT8135_PIN_101_SCL0__FUNC_SCL0>;
> 
> We feel this is less error prone and easier to write than this:
> 
> mediatek,pinfunc = <MT_PIN_FUNC(100, 1) MT_PIN_FUNC(101, 1)>

But you don't actually use the same macros in the driver, so in effect
you just move the definitions from the file they are needed in to another
file as a macro.

It is no less error prone to define those macros in mt8135-pinfunc.h
than in the pinctrl node, just less readable.

	Arnd

Re: [PATCH v2 4/4] ARM: dts: mt8135: Add pinctrl node for mt8135.

[Sascha Hauer]

On Tue, Sep 23, 2014 at 04:10:09PM +0200, Arnd Bergmann wrote:
> On Tuesday 23 September 2014 21:58:14 Joe. C wrote:
> > Thanks for review. 
> > The intend for these macros is helpin pinctrl user to write DT node.
> > With these macro, we could write like this for i2c0:
> > 
> > mediatek,pinfunc = <MT8135_PIN_100_SDA0__FUNC_SDA0 
> >                         MT8135_PIN_101_SCL0__FUNC_SCL0>;
> > 
> > We feel this is less error prone and easier to write than this:
> > 
> > mediatek,pinfunc = <MT_PIN_FUNC(100, 1) MT_PIN_FUNC(101, 1)>
> 
> But you don't actually use the same macros in the driver, so in effect
> you just move the definitions from the file they are needed in to another
> file as a macro.
> 
> It is no less error prone to define those macros in mt8135-pinfunc.h
> than in the pinctrl node, just less readable.

IMO these defines make writing and reviewing dts files much easier. Once
the macros are used widely and thus are likely correct you can be sure
that every board has sane and non conflicting pin setups. The pin names
can often be found in the schematics of a board, so you can verify the
pinmux settings without looking in the SoC datasheet.

If all you have in the dts files is MT_PIN_FUNC(pinx, funcy) then it
can be quite hard to verify pins. First you have to get an idea how
the pin numbers match the pins in the datasheet. In the bindings the
pins are numbered, in the datasheet they usually only have names.
Afterwards you have to find out which functions this pin has and how
these match to the function number in the binding. Repeat this for
dozens of pins.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 4/4] ARM: dts: mt8135: Add pinctrl node for mt8135.

[Chen-Yu Tsai]

Hi,

On Tue, Sep 23, 2014 at 9:58 PM, Joe.C <srv_yingjoe.chen@mediatek.com> wrote:
> On Tue, 2014-09-23 at 15:03 +0200, Arnd Bergmann wrote:
>> On Tuesday 23 September 2014 11:39:05 Hongzhou. Yang wrote:
>> > +#define MT8135_PIN_0_MSDC0_DAT7__FUNC_GPIO0 (MT_PIN_NO(0) | 0)
>> > +#define MT8135_PIN_0_MSDC0_DAT7__FUNC_MSDC0_DAT7 (MT_PIN_NO(0) | 1)
>> > +#define MT8135_PIN_0_MSDC0_DAT7__FUNC_EINT49 (MT_PIN_NO(0) | 2)
>> > +#define MT8135_PIN_0_MSDC0_DAT7__FUNC_I2SOUT_DAT (MT_PIN_NO(0) | 3)
>> > +#define MT8135_PIN_0_MSDC0_DAT7__FUNC_DAC_DAT_OUT (MT_PIN_NO(0) | 4)
>> > +#define MT8135_PIN_0_MSDC0_DAT7__FUNC_PCM1_DO (MT_PIN_NO(0) | 5)
>> > +#define MT8135_PIN_0_MSDC0_DAT7__FUNC_SPI1_MO (MT_PIN_NO(0) | 6)
>> > +#define MT8135_PIN_0_MSDC0_DAT7__FUNC_NALE (MT_PIN_NO(0) | 7)
>> > +
>>
>> This list looks like it just describes the hardware, I think it would
>> be better to put the values directly into the DT, rather than
>> using such macros.
>
> Hi,
>
> Thanks for review.
> The intend for these macros is helpin pinctrl user to write DT node.
> With these macro, we could write like this for i2c0:
>
> mediatek,pinfunc = <MT8135_PIN_100_SDA0__FUNC_SDA0
>                         MT8135_PIN_101_SCL0__FUNC_SCL0>;
>
> We feel this is less error prone and easier to write than this:
>
> mediatek,pinfunc = <MT_PIN_FUNC(100, 1) MT_PIN_FUNC(101, 1)>


Since you've already imposed the following limit in the DT bindings:

  The mediatek,pinfunc can be either a single value or an array.
  If it is an array, that means all pins use same config in this node.

Maybe you could split the bindings into

  mediatek,pins = <1 2 3 4>;
  mediatek,func = <1>;  (or some macro)

Or better yet, since you've already defined all the function names
in the pinctrl driver, you could just match by name for the functions.
That makes it very verbose and explicit about what you specify.

That's how we do it for sunxi anyway. Of course we have all the pin
function documentation open, so users and reviewers alike can double
check. Just a suggestion. :)


Cheers
ChenYu

Re: [PATCH v2 4/4] ARM: dts: mt8135: Add pinctrl node for mt8135.

[Linus Walleij]

On Tue, Sep 23, 2014 at 5:39 AM, Hongzhou.Yang
<srv_hongzhou.yang@mediatek.com> wrote:

> From: Hongzhou Yang <hongzhou.yang@mediatek.com>
>
> Add pinctrl node to mt8135.dtsi.
>
> Signed-off-by: Hongzhou Yang <hongzhou.yang@mediatek.com>
(...)
> +#define MT8135_PIN_0_MSDC0_DAT7__FUNC_GPIO0 (MT_PIN_NO(0) | 0)
> +#define MT8135_PIN_0_MSDC0_DAT7__FUNC_MSDC0_DAT7 (MT_PIN_NO(0) | 1)
> +#define MT8135_PIN_0_MSDC0_DAT7__FUNC_EINT49 (MT_PIN_NO(0) | 2)
> +#define MT8135_PIN_0_MSDC0_DAT7__FUNC_I2SOUT_DAT (MT_PIN_NO(0) | 3)

I haven't got to reviewing the driver, but this looks just wrong.

Have the magic numbers in the driver.

Use strings to describe functions, not integers.

We need to move toward standardized device tree bindings
for this stuff, and that means using strings, not magic
numbers.

Yours,
Linus Walleij

Re: [PATCH v2 4/4] ARM: dts: mt8135: Add pinctrl node for mt8135.

[Sascha Hauer]

Hi Linus,

On Wed, Sep 24, 2014 at 01:23:09PM +0200, Linus Walleij wrote:
> On Tue, Sep 23, 2014 at 5:39 AM, Hongzhou.Yang
> <srv_hongzhou.yang@mediatek.com> wrote:
> 
> > From: Hongzhou Yang <hongzhou.yang@mediatek.com>
> >
> > Add pinctrl node to mt8135.dtsi.
> >
> > Signed-off-by: Hongzhou Yang <hongzhou.yang@mediatek.com>
> (...)
> > +#define MT8135_PIN_0_MSDC0_DAT7__FUNC_GPIO0 (MT_PIN_NO(0) | 0)
> > +#define MT8135_PIN_0_MSDC0_DAT7__FUNC_MSDC0_DAT7 (MT_PIN_NO(0) | 1)
> > +#define MT8135_PIN_0_MSDC0_DAT7__FUNC_EINT49 (MT_PIN_NO(0) | 2)
> > +#define MT8135_PIN_0_MSDC0_DAT7__FUNC_I2SOUT_DAT (MT_PIN_NO(0) | 3)
> 
> I haven't got to reviewing the driver, but this looks just wrong.
> 
> Have the magic numbers in the driver.
> 
> Use strings to describe functions, not integers.

Interrupts, clocks, gpios, dma channels, nearly everything in the device tree is
arbitrarily numbered. Instead of "irq-i2c0" we have <0 36 IRQ_TYPE_LEVEL_HIGH>
in the device tree. These numbers can be resolved efficiently in the
driver by shifting them to get a bitmask or by adding them as offset to
a register base.
Why do you want to make pinctrl different? Thanks to the recently
introduced defines in the device trees these numbers are not magic at
all anymore.

> 
> We need to move toward standardized device tree bindings
> for this stuff, and that means using strings, not magic
> numbers.

Agreed for standardized device tree bindings, but not for using strings.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 4/4] ARM: dts: mt8135: Add pinctrl node for mt8135.

[Sascha Hauer]

Linus,

On Wed, Sep 24, 2014 at 02:40:44PM +0200, Sascha Hauer wrote:
> > I haven't got to reviewing the driver, but this looks just wrong.
> > 
> > Have the magic numbers in the driver.
> > 
> > Use strings to describe functions, not integers.
> 
> Interrupts, clocks, gpios, dma channels, nearly everything in the device tree is
> arbitrarily numbered. Instead of "irq-i2c0" we have <0 36 IRQ_TYPE_LEVEL_HIGH>
> in the device tree. These numbers can be resolved efficiently in the
> driver by shifting them to get a bitmask or by adding them as offset to
> a register base.
> Why do you want to make pinctrl different? Thanks to the recently
> introduced defines in the device trees these numbers are not magic at
> all anymore.

Any opinions on this?

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 4/4] ARM: dts: mt8135: Add pinctrl node for mt8135.

[Linus Walleij]

On Wed, Sep 24, 2014 at 2:40 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Wed, Sep 24, 2014 at 01:23:09PM +0200, Linus Walleij wrote:

>> I haven't got to reviewing the driver, but this looks just wrong.
>>
>> Have the magic numbers in the driver.
>>
>> Use strings to describe functions, not integers.
>
> Interrupts, clocks, gpios, dma channels, nearly everything in the device tree is
> arbitrarily numbered. Instead of "irq-i2c0" we have <0 36 IRQ_TYPE_LEVEL_HIGH>
> in the device tree. These numbers can be resolved efficiently in the
> driver by shifting them to get a bitmask or by adding them as offset to
> a register base.
> Why do you want to make pinctrl different?

Because pin control is about combining groups of pins with
certain functions.

> Thanks to the recently
> introduced defines in the device trees these numbers are not magic at
> all anymore.

Yeah that is good but not what I'm after here.

>> We need to move toward standardized device tree bindings
>> for this stuff, and that means using strings, not magic
>> numbers.
>
> Agreed for standardized device tree bindings, but not for using strings.

What is the alternative? Device Tree is very much about strings,
as is shown by the pin config bindings.

Yours,
Linus Walleij

Re: [PATCH v2 4/4] ARM: dts: mt8135: Add pinctrl node for mt8135.

[Lucas Stach]

Hi Linus,

Am Donnerstag, den 02.10.2014, 16:02 +0200 schrieb Linus Walleij:
> On Wed, Sep 24, 2014 at 2:40 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > On Wed, Sep 24, 2014 at 01:23:09PM +0200, Linus Walleij wrote:
> 
> >> I haven't got to reviewing the driver, but this looks just wrong.
> >>
> >> Have the magic numbers in the driver.
> >>
> >> Use strings to describe functions, not integers.
> >
> > Interrupts, clocks, gpios, dma channels, nearly everything in the device tree is
> > arbitrarily numbered. Instead of "irq-i2c0" we have <0 36 IRQ_TYPE_LEVEL_HIGH>
> > in the device tree. These numbers can be resolved efficiently in the
> > driver by shifting them to get a bitmask or by adding them as offset to
> > a register base.
> > Why do you want to make pinctrl different?
> 
> Because pin control is about combining groups of pins with
> certain functions.
> 
> > Thanks to the recently
> > introduced defines in the device trees these numbers are not magic at
> > all anymore.
> 
> Yeah that is good but not what I'm after here.
> 
> >> We need to move toward standardized device tree bindings
> >> for this stuff, and that means using strings, not magic
> >> numbers.
> >
> > Agreed for standardized device tree bindings, but not for using strings.
> 
> What is the alternative? Device Tree is very much about strings,
> as is shown by the pin config bindings.
> 
Mhm, maybe we are still talking about different things but I just don't
get your point. Traditionally DT is more about plain numbers than
strings. Look at the early examples of PCI or other bus bindings,
defined back in the IEEE 1275 days. Almost everything back then has
been mapped to plain numbers.

Using strings only bloats the DT, not only in it's source form, but also
as a compiled DTB. Having a verbose string based binding is just painful
to work with (I can tell from experience here, as I personally built the
pinmux setup for two Tegra boards). Working with a condensed number
based binding like the one used on i.MX is much easier IMHO.

Most importantly I don't see any benefit in writing:
pin_state_a {
	pins = "i2c0_scl", "i2c0_sda";
	function = "i2c0";
};

instead of
#include <dt-binding/pinctrl/mediatek_foo.h>
pin_state_a {
	pins = <I2C0_SCL>, <I2C0_SDA>;
	function = <I2C0>;
}

Using plain integers makes it much easier to index into pinctrl driver
specific arrays without doing all this string matching in the kernel. It
seems we are eating CPU cycles here for no good reason.

Could you please explain where you see the benefit of using strings
instead of plain integers with proper binding defines attached to them?

Regards,
Lucas
-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

Re: [PATCH v2 4/4] ARM: dts: mt8135: Add pinctrl node for mt8135.

[Linus Walleij]

On Thu, Oct 2, 2014 at 4:28 PM, Lucas Stach <l.stach@pengutronix.de> wrote:
> Am Donnerstag, den 02.10.2014, 16:02 +0200 schrieb Linus Walleij:

>> > Agreed for standardized device tree bindings, but not for using strings.
>>
>> What is the alternative? Device Tree is very much about strings,
>> as is shown by the pin config bindings.
>>
> Mhm, maybe we are still talking about different things but I just don't
> get your point. Traditionally DT is more about plain numbers than
> strings. Look at the early examples of PCI or other bus bindings,
> defined back in the IEEE 1275 days. Almost everything back then has
> been mapped to plain numbers.
>
> Using strings only bloats the DT, not only in it's source form, but also
> as a compiled DTB. (...)

OK I think we have arrived (in this thread and in others) to the old
discussion of whether to use groups or per-pin function setting in
drivers.

Let us move forward like this:

I have proposed a general binding of functions+groups (as strings)
which will be suitable for some.

What would you propose as a general binding for systems using
per-pin configuration?

My problem as a subsystem maintainer is that there are too many
custom bindings. We need to standardize on something.

For pin config we have attained some consensus, and that is indeed
using strings "bias-pull-up" etc, simply because there is no
sane way to enumerate them all, and it is simple to read by
humans.

Yours,
Linus Walleij

Re: [PATCH v2 4/4] ARM: dts: mt8135: Add pinctrl node for mt8135.

[Sascha Hauer]

On Thu, Oct 02, 2014 at 04:02:58PM +0200, Linus Walleij wrote:
> On Wed, Sep 24, 2014 at 2:40 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > On Wed, Sep 24, 2014 at 01:23:09PM +0200, Linus Walleij wrote:
> 
> >> I haven't got to reviewing the driver, but this looks just wrong.
> >>
> >> Have the magic numbers in the driver.
> >>
> >> Use strings to describe functions, not integers.
> >
> > Interrupts, clocks, gpios, dma channels, nearly everything in the device tree is
> > arbitrarily numbered. Instead of "irq-i2c0" we have <0 36 IRQ_TYPE_LEVEL_HIGH>
> > in the device tree. These numbers can be resolved efficiently in the
> > driver by shifting them to get a bitmask or by adding them as offset to
> > a register base.
> > Why do you want to make pinctrl different?
> 
> Because pin control is about combining groups of pins with
> certain functions.
> 
> > Thanks to the recently
> > introduced defines in the device trees these numbers are not magic at
> > all anymore.
> 
> Yeah that is good but not what I'm after here.
> 
> >> We need to move toward standardized device tree bindings
> >> for this stuff, and that means using strings, not magic
> >> numbers.
> >
> > Agreed for standardized device tree bindings, but not for using strings.
> 
> What is the alternative? Device Tree is very much about strings,
> as is shown by the pin config bindings.

The alternative is to use numbers. The majority of SoCs have a bit field
per pad which is used for muxing the pad to different functions. The
natural way to describe this is a pair of numbers: <pad-number>
<function-number>.  The pad number can normally be directly translated
into a register offset and the function number to a value written to
that register. This is true for most SoCs I know of and makes it very
easy to generate code for and to prove for correctness of both the code
and the device tree.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 4/4] ARM: dts: mt8135: Add pinctrl node for mt8135.

[Linus Walleij]

On Mon, Oct 6, 2014 at 9:18 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Thu, Oct 02, 2014 at 04:02:58PM +0200, Linus Walleij wrote:

>> What is the alternative? Device Tree is very much about strings,
>> as is shown by the pin config bindings.
>
> The alternative is to use numbers. The majority of SoCs have a bit field
> per pad which is used for muxing the pad to different functions. The
> natural way to describe this is a pair of numbers: <pad-number>
> <function-number>.  The pad number can normally be directly translated
> into a register offset and the function number to a value written to
> that register. This is true for most SoCs I know of and makes it very
> easy to generate code for and to prove for correctness of both the code
> and the device tree.

To me this sounds more like a discussion on how to set up a good
firmware (BIOS) that the kernel can call into.

We have had many times the discussion whether DT should contain
"jam tables" and such things, i.e. stuff that basically translate
DT directly to register writes.

The one exception we have for this is pinctrl-single.c which uses a
single word to configure a pin. I'm even not sure that was a good
idea but it was a compromise.

If someone wants the above, they should use pinctrl-single.c
(OMAP and HiSilicon uses it already), if they need another necessarily
different driver I'm very reluctant.

Yours,
Linus Walleij

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

[Linus Walleij]

On Tue, Sep 23, 2014 at 5:39 AM, Hongzhou.Yang
<srv_hongzhou.yang@mediatek.com> wrote:

> From: Hongzhou Yang <hongzhou.yang@mediatek.com>
>
> The mediatek SoCs have GPIO controller that handle both the muxing
> and GPIOs.
>
> The GPIO controller have pinmux, pull enable, pull select, direction
> and output high/low control.
>
> This driver include common and mt8135 part. It implements the pinctrl
> part and gpio part.
>
> Signed-off-by: Hongzhou Yang <hongzhou.yang@mediatek.com>
(...)
> diff --git a/drivers/pinctrl/mediatek/Kconfig b/drivers/pinctrl/mediatek/Kconfig
> new file mode 100644
> index 0000000..bae4be6
> --- /dev/null
> +++ b/drivers/pinctrl/mediatek/Kconfig
> @@ -0,0 +1,12 @@
> +if ARCH_MEDIATEK
> +
> +config PINCTRL_MTK_COMMON
> +       bool
> +       select PINMUX
> +       select GENERIC_PINCONF

This should most certainly select GPIOLIB_IRQCHIP, I'm
pretty sure you can use the common chained irqchip handling.

> +++ b/drivers/pinctrl/mediatek/pinctrl-mt8135.c
(...)
> +postcore_initcall(mtk_pinctrl_init);

Why? We prefer to use module_init() these days, and employ deferred
probe to order module probe order. Is there some problem with this?

> +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
(...)
> +#include <linux/io.h>
> +#include <linux/gpio.h>
> +#include <linux/irqdomain.h>
> +#include <linux/irqchip/chained_irq.h>

These two includes go away with GPIOLIB_IRQCHIP

> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/of_irq.h>
> +#include <linux/pinctrl/consumer.h>
> +#include <linux/pinctrl/machine.h>
> +#include <linux/pinctrl/pinctrl.h>
> +#include <linux/pinctrl/pinconf.h>
> +#include <linux/pinctrl/pinconf-generic.h>
> +#include <linux/pinctrl/pinmux.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <dt-bindings/pinctrl/mt65xx.h>
> +
> +#include "../pinconf.h"
> +#include "pinctrl-mtk-common.h"
> +
> +#define PINMUX_MAX_VAL 8
> +#define MAX_GPIO_MODE_PER_REG 5
> +#define GPIO_MODE_BITS        3
> +
> +static const char * const mt_gpio_functions[] = {
> +       "func0", "func1", "func2", "func3",
> +       "func4", "func5", "func6", "func7",
> +};
> +
> +static void __iomem *mt_get_base_addr(struct mt_pinctrl *pctl,
> +               unsigned long pin)
> +{
> +       if (pin >= pctl->devdata->type1_start && pin < pctl->devdata->type1_end)
> +               return pctl->membase2;
> +       return pctl->membase1;
> +}
> +
> +static void mt_pctrl_write_reg(struct mt_pinctrl *pctl,
> +               unsigned long pin,
> +               u32 reg, u32 d)
> +{
> +       writel(d, mt_get_base_addr(pctl, pin) + reg);
> +}

1) Don't you want to use writel_relaxed() and
2) What does this helper really buy you? I would prefer to
  inline this everywhere it's used. At the least tag this
  function as inline.

> +static unsigned int mt_get_port(unsigned long pin)
> +{
> +       return ((pin >> 4) & 0xf) << 4;

Isn't that equivalent to
return pin & 0xf0;

Add a comment explaining what's going on here.


> +static int mt_pmx_gpio_set_direction(struct pinctrl_dev *pctldev,
> +                       struct pinctrl_gpio_range *range,
> +                       unsigned offset,
> +                       bool input)
> +{
> +       unsigned int reg_addr;
> +       unsigned int bit;
> +       struct mt_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
> +
> +       reg_addr = mt_get_port(offset) + pctl->devdata->dir_offset;
> +       bit = 1 << (offset & 0xf);

#include <linux/bitops.h>

bit = BIT(offset & 0xf);

> +       if (input)
> +               reg_addr += (4 << 1);

What is wrong with reg_add += 8;

> +       else
> +               reg_addr += 4;
> +
> +       writel(bit, pctl->membase1 + reg_addr);

Note: here you're not using mt_pctrl_write_reg().
And I think you should use writel_relaxed().

> +static void mt_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
> +{
> +       unsigned int reg_addr;
> +       unsigned int bit;
> +       struct mt_pinctrl *pctl = dev_get_drvdata(chip->dev);
> +
> +       reg_addr = mt_get_port(offset) + pctl->devdata->dout_offset;
> +       bit = 1 << (offset & 0xf);

BIT(offset & 0xf)

> +       if (value)
> +               writel(bit, pctl->membase1 + reg_addr + 4);
> +       else
> +               writel(bit, pctl->membase1 + reg_addr + (4 << 1));

+8

> +static int mt_gpio_set_pull_conf(struct pinctrl_dev *pctldev,
> +               unsigned long pin, enum pin_config_param param,
> +               enum pin_config_param argument)
> +{
> +       unsigned int reg_pullen, reg_pullsel;
> +       unsigned int bit;
> +       struct mt_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
> +       int pullen = 0;
> +       int pullsel = 0;
> +
> +       bit = 1 << (pin & 0xf);

BIT

> +       switch (param) {
> +       case PIN_CONFIG_BIAS_DISABLE:
> +               pullen = 0;
> +               pullsel = 0;
> +               break;
> +       case PIN_CONFIG_BIAS_PULL_UP:
> +               mt_pmx_gpio_set_direction(pctldev, NULL, pin, 1);
> +               pullen = 1;
> +               pullsel = 1;
> +               break;
> +       case PIN_CONFIG_BIAS_PULL_DOWN:
> +               mt_pmx_gpio_set_direction(pctldev, NULL, pin, 1);
> +               pullen = 1;
> +               pullsel = 0;
> +               break;
> +
> +       case PIN_CONFIG_INPUT_ENABLE:
> +               mt_pmx_gpio_set_direction(pctldev, NULL, pin, 1);
> +               break;
> +
> +       case PIN_CONFIG_OUTPUT:
> +               mt_pmx_gpio_set_direction(pctldev, NULL, pin, 0);
> +               mt_gpio_set(pctl->chip, pin, argument);
> +               return 0;
> +
> +       default:
> +               return -EINVAL;
> +       }

Cut the whitespace newlines above I think.

> +       if (pullen)
> +               reg_pullen = mt_get_port(pin) +
> +                       pctl->devdata->pullen_offset + 4;
> +       else
> +               reg_pullen = mt_get_port(pin) +
> +                       pctl->devdata->pullen_offset + (4 << 1);

+8

> +
> +       if (pullsel)
> +               reg_pullsel = mt_get_port(pin) +
> +                       pctl->devdata->pullsel_offset + 4;
> +       else
> +               reg_pullsel = mt_get_port(pin) +
> +                       pctl->devdata->pullsel_offset + (4 << 1);

+8

> +       mt_pctrl_write_reg(pctl, pin, reg_pullen, bit);
> +       mt_pctrl_write_reg(pctl, pin, reg_pullsel, bit);
> +       return 0;
> +}
(...)
> +static int mt_pctrl_is_function_valid(struct mt_pinctrl *pctl,
> +               u32 pin_num, u32 fnum)

Return type should be bool.

> +{
> +       int i;
> +
> +       for (i = 0; i < pctl->devdata->npins; i++) {
> +               const struct mt_desc_pin *pin = pctl->devdata->pins + i;
> +
> +               if (pin->pin.number == pin_num) {
> +                       struct mt_desc_function *func = pin->functions + fnum;
> +
> +                       if (func->name)
> +                               return 1;
> +                       else
> +                               return 0;
> +               }
> +       }
> +
> +       return 0;
> +}

return true/false;

> +static int mt_pctrl_dt_node_to_map_func(struct mt_pinctrl *pctl, u32 pin,
> +               u32 fnum, struct pinctrl_map **maps)
> +static int mt_pctrl_dt_node_to_map_config(struct mt_pinctrl *pctl, u32 pin,
> +               unsigned long *configs, unsigned num_configs,
> +               struct pinctrl_map **maps)
> +static void mt_pctrl_dt_free_map(struct pinctrl_dev *pctldev,
> +                                   struct pinctrl_map *map,
> +                                   unsigned num_maps)
> +static int mt_pctrl_dt_node_to_map(struct pinctrl_dev *pctldev,
> +                                     struct device_node *node,
> +                                     struct pinctrl_map **map,
> +                                     unsigned *num_maps)

I'm worried about the escalating number of custom function->group
and pin config bindings, so I have submitted patches to fix this up and
allow for all-generic bindings to be used.

See:
http://marc.info/?l=devicetree&m=141223584006648&w=2
http://marc.info/?l=devicetree&m=141223586106655&w=2

> +       pins = of_find_property(node, "mediatek,pinfunc", NULL);

So I want to get rid of "mediatek,pinfunc" and just use "function"
for this.

> +       err = pinconf_generic_parse_dt_config(node, &configs, &num_configs);
> +       if (num_configs)
> +               has_config = 1;

This implicitly uses "pins", which is nice.

> +static int mt_gpio_set_mode(struct pinctrl_dev *pctldev,
> +               unsigned long pin, unsigned long mode)
> +{
> +       unsigned int reg_addr;
> +       unsigned char bit;
> +       unsigned int val;
> +       unsigned long flags;
> +       unsigned int mask = (1L << GPIO_MODE_BITS) - 1;
> +       struct mt_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
> +
> +       reg_addr = ((pin / 5) << 4) + pctl->devdata->pinmux_offset;

That 5 and 4 needs some explanation, or atleast being #defined
for readability.

> +       spin_lock_irqsave(&pctl->lock, flags);
> +       val = readl(pctl->membase1 + reg_addr);

readl_relaxed()?

> +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.

> +{
> +       int i, j;
> +
> +       for (i = 0; i < pctl->devdata->npins; i++) {
> +               const struct mt_desc_pin *pin = pctl->devdata->pins + i;
> +
> +               if (!strcmp(pin->pin.name, pin_name)) {
> +                       struct mt_desc_function *func = pin->functions;
> +
> +                       for (j = 0; j < PINMUX_MAX_VAL; j++) {
> +                               if (func->irqnum != 255)

So why does it end at 255? Seems pretty arbitrary.

> +                                       return func;
> +
> +                               func++;
> +                       }
> +               }
> +       }
> +
> +       return NULL;
> +}


> +static int mt_pmx_enable(struct pinctrl_dev *pctldev,
> +                           unsigned function,
> +                           unsigned group)

This is typically named pmx_set_mux() nowadays, please
use the pin control development tree at this point, the change will
be upstream in v3.18.

> +static const struct pinmux_ops mt_pmx_ops = {
> +       .get_functions_count    = mt_pmx_get_funcs_cnt,
> +       .get_function_name      = mt_pmx_get_func_name,
> +       .get_function_groups    = mt_pmx_get_func_groups,
> +       .enable                 = mt_pmx_enable,

.set_mux =...

> +static int mt_gpio_request(struct gpio_chip *chip, unsigned offset)
> +{
> +       return pinctrl_request_gpio(chip->base + offset);
> +}
> +
> +static void mt_gpio_free(struct gpio_chip *chip, unsigned offset)
> +{
> +       pinctrl_free_gpio(chip->base + offset);
> +}
>
> +static int mt_gpio_direction_input(struct gpio_chip *chip,
> +                                       unsigned offset)
> +{
> +       return pinctrl_gpio_direction_input(chip->base + offset);
> +}
> +
> +static int mt_gpio_direction_output(struct gpio_chip *chip,
> +                                       unsigned offset, int value)
> +{
> +       return pinctrl_gpio_direction_output(chip->base + offset);
> +}

This is nice.

> +static int mt_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
> +{
> +       unsigned int reg_addr;
> +       unsigned int bit;
> +       unsigned int read_val = 0;
> +
> +       struct mt_pinctrl *pctl = dev_get_drvdata(chip->dev);
> +
> +       reg_addr =  mt_get_port(offset) + pctl->devdata->dir_offset;
> +       bit = 1 << (offset & 0xf);

bit = BIT(offset & 0xf);

> +       read_val = readl(pctl->membase1 + reg_addr);
> +       return ((read_val & bit) != 0) ? 1 : 0;

No do it like this:

return !!(read_val & bit);

> +static int mt_gpio_get(struct gpio_chip *chip, unsigned offset)
> +{
> +       unsigned int reg_addr;
> +       unsigned int bit;
> +       unsigned int read_val = 0;
> +       struct mt_pinctrl *pctl = dev_get_drvdata(chip->dev);
> +
> +       if (mt_gpio_get_direction(chip, offset))
> +               reg_addr = mt_get_port(offset) + pctl->devdata->dout_offset;
> +       else
> +               reg_addr = mt_get_port(offset) + pctl->devdata->din_offset;
> +
> +       bit = 1 << (offset & 0xf);
> +       read_val = readl(pctl->membase1 + reg_addr);
> +       return ((read_val & bit) != 0) ? 1 : 0;
> +}

Same comments on this function.

> +static int mt_gpio_of_xlate(struct gpio_chip *gc,
> +                               const struct of_phandle_args *gpiospec,
> +                               u32 *flags)
> +{
> +       int pin;
> +
> +       pin = gpiospec->args[0];
> +
> +       if (pin > (gc->base + gc->ngpio))
> +               return -EINVAL;
> +
> +       if (flags)
> +               *flags = gpiospec->args[1];
> +
> +       return pin;
> +}

Why do you need your own xlate function to do this?

What is wrong with of_gpio_simple_xlate() which seems to do
the same thing?

Incidentally that is what you will get if you just leave this
pointer in the vtable as unassigned.

> +static int mt_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
> +{
> +       struct mt_pinctrl *pctl = dev_get_drvdata(chip->dev);
> +       struct mt_pinctrl_group *g = pctl->groups + offset;
> +       struct mt_desc_function *desc = mt_pctrl_desc_find_irq_by_name(
> +                       pctl, g->name);
> +       if (!desc)
> +               return -EINVAL;
> +
> +       mt_gpio_set_mode(pctl->pctl_dev, g->pin, desc->muxval);

No mode setting in the .to_irq() function, that makes the irqchip
not orthogonal to the gpio_chip.

> +       return desc->irqnum;
> +}

By the way use GPIOLIB_IRQCHIP for this and get rid
of .to_irq altogether.

(...)
> +static int mt_pctrl_build_state(struct platform_device *pdev)
> +{
> +       struct mt_pinctrl *pctl = platform_get_drvdata(pdev);
> +       int i;
> +
> +       pctl->ngroups = pctl->devdata->npins;
> +
> +       pctl->groups = devm_kzalloc(&pdev->dev,
> +                                   pctl->ngroups * sizeof(*pctl->groups),
> +                                   GFP_KERNEL);
> +       if (!pctl->groups)
> +               return -ENOMEM;
> +
> +       pctl->grp_names = devm_kzalloc(&pdev->dev,
> +                                   pctl->ngroups * sizeof(*pctl->grp_names),
> +                                   GFP_KERNEL);
> +       if (!pctl->grp_names)
> +               return -ENOMEM;
> +
> +       for (i = 0; i < pctl->devdata->npins; i++) {
> +               const struct mt_desc_pin *pin = pctl->devdata->pins + i;
> +               struct mt_pinctrl_group *group = pctl->groups + i;
> +               const char **func_grp;
> +
> +               group->name = pin->pin.name;
> +               group->pin = pin->pin.number;
> +
> +               func_grp = pctl->grp_names;
> +               while (*func_grp)
> +                       func_grp++;
> +
> +               *func_grp = pin->pin.name;
> +       }
> +
> +       return 0;
> +}

I don't understand what this function is doing so atleast it need
and explanation in kerneldoc above it.

(...)
> +       ret = gpiochip_add(pctl->chip);
> +       if (ret) {
> +               ret = -EINVAL;
> +               goto pctrl_error;
> +       }

Here  you should be using gpiochip_irqchip_add()
followed by gpiochip_set_chained_irqchip().

> +       for (i = 0; i < pctl->devdata->npins; i++) {
> +               const struct mt_desc_pin *pin = pctl->devdata->pins + i;
> +
> +               ret = gpiochip_add_pin_range(pctl->chip, dev_name(&pdev->dev),
> +                                            pin->pin.number,
> +                                            pin->pin.number, 1);
> +               if (ret) {
> +                       ret = -EINVAL;
> +                       goto chip_error;
> +               }

Seems complicated but I don't know how complicated
your GPIO ranges are indeed.

> +chip_error:
> +       if (gpiochip_remove(pctl->chip))

We have removed the return value from gpiochip_remove() so rebase
to upstream here. No if(..)

> diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common.h b/drivers/pinctrl/mediatek/pinctrl-mtk-common.h
(...)
> +struct mt_desc_pin {
> +       struct pinctrl_pin_desc pin;
> +       const char *chip;
> +       struct mt_desc_function *functions;

Why does a pin need to know about functions...

> +};

Don't invent custom pin container structures. Look:

/**
 * struct pinctrl_pin_desc - boards/machines provide information on their
 * pins, pads or other muxable units in this struct
 * @number: unique pin number from the global pin number space
 * @name: a name for this pin
 * @drv_data: driver-defined per-pin data. pinctrl core does not touch this
 */
struct pinctrl_pin_desc {
        unsigned number;
        const char *name;
        void *drv_data;
};

You add your stuff to drv_data() rather than including this struct
into your own struct.

> +#define MT_PIN(_pin, _pad, _chip, ...)                         \
> +       {                                                       \
> +               .pin = _pin,                                    \
> +               .chip = _chip,                                  \
> +               .functions = (struct mt_desc_function[]){       \
> +                       __VA_ARGS__, { } },                     \
> +       }
> +
> +#define MT_FUNCTION(_val, _name)                               \
> +       {                                                       \
> +               .name = _name,                                  \
> +               .muxval = _val,                                 \
> +               .irqnum = 255,                                  \

255 eh?

> +       }
> +
> +#define MT_FUNCTION_IRQ(_val, _name, _irq)                     \
> +       {                                                       \
> +               .name = _name,                                  \
> +               .muxval = _val,                                 \
> +               .irqnum = _irq,                                 \
> +       }
> +
> +struct mt_pinctrl_group {
> +       const char      *name;
> +       unsigned long   config;
> +       unsigned        pin;
> +};
> +
> +struct mt_gpio_devdata {
> +       const struct mt_desc_pin *pins;
> +       int npins;
> +       unsigned int dir_offset;
> +       unsigned int ies_offset;
> +       unsigned int pullen_offset;
> +       unsigned int pullsel_offset;
> +       unsigned int drv_offset;
> +       unsigned int invser_offset;
> +       unsigned int dout_offset;
> +       unsigned int din_offset;
> +       unsigned int pinmux_offset;
> +       unsigned short type1_start;
> +       unsigned short type1_end;
> +};

Add some kerneldoc for this struct as it't not apparently
self-evident.

> +static const struct mt_desc_pin mt_pins_mt8135[] = {
> +       MT_PIN(
> +               PINCTRL_PIN(0, "MSDC0_DAT7"),
> +               "D21", "mt8135",
> +               MT_FUNCTION(0, "GPIO0"),
> +               MT_FUNCTION(1, "MSDC0_DAT7"),
> +               MT_FUNCTION_IRQ(2, "EINT49", 49),
> +               MT_FUNCTION(3, "I2SOUT_DAT"),
> +               MT_FUNCTION(4, "DAC_DAT_OUT"),
> +               MT_FUNCTION(5, "PCM1_DO"),
> +               MT_FUNCTION(6, "SPI1_MO"),
> +               MT_FUNCTION(7, "NALE")
> +       ),

I don't think this is a good idea and to encode all functions in a pin,
rather the revers is custom: define all functions and collect arrays
of pin numbers in the definitions of pin groups, then map the functions
and groups of pins together.

Look at other drivers for examples..

I don't like the device tree bindings for the very same reason: it moves
all this numeric encoding of pin-functions into the device tree instead
of combining group+function strings like most drivers do.

Is there some special reason to why you're turning this on its head?

Yours,
Linus Walleij

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

[Linus Walleij]

On Mon, Oct 6, 2014 at 12:35 PM, Joe.C <yingjoe.chen@mediatek.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

Re: [PATCH v2 3/4] dt-bindings: Add pinctrl bindings for mt65xx/mt81xx.

[Linus Walleij]

On Tue, Sep 23, 2014 at 5:39 AM, Hongzhou.Yang
<srv_hongzhou.yang@mediatek.com> wrote:

> From: Hongzhou Yang <hongzhou.yang@mediatek.com>
>
> Add devicetree bindings for Mediatek SoC pinctrl driver.
>
> Signed-off-by: Hongzhou Yang <hongzhou.yang@mediatek.com>

I have worked on generic pin control bindings a bit because it
is getting out of hand with all these custom bindings.

See:
http://marc.info/?l=devicetree&m=141223584006648&w=2

Especially.

> +- mediatek,pinfunc: List of gpio number and function to mux.

A "GPIO number" and a "pin number" is not the same thing at all,
this is very confusing. Those are two separate number spaces.
This is likely about the pin numbers.

> +The mediatek,pinfunc can use defines directly,
> +which are already defind in boot/dts/mt8135-pinfunc.h.
> +
> +Optional subnode-properties:
> +- generic pin configuration option to use, bias-disable, bias-pull-down,
> +  bias-pull,up, output-low and output-high are valid.
> +  Example :
> +       i2c0_pins_a {
> +               mediatek,pinfunc = <MT8135_PIN_195_SDA1__FUNC_SDA1>;
> +               bias-disable;
> +       };

I don't like this approach at all.

I prefer that pins are put into groups named by strings, like "i2c0-pos0"
inside the driver and then connected to function with a certain
device-related name, such as "i2c0".

Then put the pin configuration (bias etc) in a separate node in the same
state definition like that:

i2c0_pins_a {
         function = "i2c0";
         groups = "i2c0-pos0";
};
i2c0_pins_b {
         bias-disable;
};

Yours,
Linus Walleij

Re: [PATCH v2 3/4] dt-bindings: Add pinctrl bindings for mt65xx/mt81xx.

[Lucas Stach]

Am Donnerstag, den 02.10.2014, 16:00 +0200 schrieb Linus Walleij:
> On Tue, Sep 23, 2014 at 5:39 AM, Hongzhou.Yang
> <srv_hongzhou.yang@mediatek.com> wrote:
> 
> > From: Hongzhou Yang <hongzhou.yang@mediatek.com>
> >
> > Add devicetree bindings for Mediatek SoC pinctrl driver.
> >
> > Signed-off-by: Hongzhou Yang <hongzhou.yang@mediatek.com>
> 
> I have worked on generic pin control bindings a bit because it
> is getting out of hand with all these custom bindings.
> 
> See:
> http://marc.info/?l=devicetree&m=141223584006648&w=2
> 
> Especially.
> 
> > +- mediatek,pinfunc: List of gpio number and function to mux.
> 
> A "GPIO number" and a "pin number" is not the same thing at all,
> this is very confusing. Those are two separate number spaces.
> This is likely about the pin numbers.
> 
> > +The mediatek,pinfunc can use defines directly,
> > +which are already defind in boot/dts/mt8135-pinfunc.h.
> > +
> > +Optional subnode-properties:
> > +- generic pin configuration option to use, bias-disable, bias-pull-down,
> > +  bias-pull,up, output-low and output-high are valid.
> > +  Example :
> > +       i2c0_pins_a {
> > +               mediatek,pinfunc = <MT8135_PIN_195_SDA1__FUNC_SDA1>;
> > +               bias-disable;
> > +       };
> 
> I don't like this approach at all.
> 
> I prefer that pins are put into groups named by strings, like "i2c0-pos0"
> inside the driver and then connected to function with a certain
> device-related name, such as "i2c0".
> 
So we should create artificial software groups where there are none in
hardware? This sounds really backward to me. Almost every new SoC out
there has the ability to mux every pin on it's own.

By defining artificial software groups in the driver we are pushing
constraints in the binding that don't really exist in hardware. So if
someone comes up with a pin usage that isn't covered by the existing
groups we need to change the binding. Experience from working with lots
of hardware engineers tells us that if something can be done (if there
are no constraints in HW) it will be done sooner or later.
If the hardware allows this much freedom we should also allow it in the
pinctrl binding.

> Then put the pin configuration (bias etc) in a separate node in the same
> state definition like that:
> 
> i2c0_pins_a {
>          function = "i2c0";
>          groups = "i2c0-pos0";
> };
> i2c0_pins_b {
>          bias-disable;
> };

The problem with that is that different pins might need different
configuration for the same muxed function. To properly reflect this we
would need to duplicate the pin definitions. One popular example is the
MMC interface where part of the pins need to have a pull-up, while
others don't. How would you reflect this with the DT description above?

Regards,
Lucas

-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

Re: [PATCH v2 3/4] dt-bindings: Add pinctrl bindings for mt65xx/mt81xx.

[Linus Walleij]

On Thu, Oct 2, 2014 at 4:41 PM, Lucas Stach <l.stach@pengutronix.de> wrote:
> Am Donnerstag, den 02.10.2014, 16:00 +0200 schrieb Linus Walleij:

>> > +The mediatek,pinfunc can use defines directly,
>> > +which are already defind in boot/dts/mt8135-pinfunc.h.
>> > +
>> > +Optional subnode-properties:
>> > +- generic pin configuration option to use, bias-disable, bias-pull-down,
>> > +  bias-pull,up, output-low and output-high are valid.
>> > +  Example :
>> > +       i2c0_pins_a {
>> > +               mediatek,pinfunc = <MT8135_PIN_195_SDA1__FUNC_SDA1>;
>> > +               bias-disable;
>> > +       };
>>
>> I don't like this approach at all.
>>
>> I prefer that pins are put into groups named by strings, like "i2c0-pos0"
>> inside the driver and then connected to function with a certain
>> device-related name, such as "i2c0".
>>
>
> So we should create artificial software groups where there are none in
> hardware? This sounds really backward to me. Almost every new SoC out
> there has the ability to mux every pin on it's own.

This discussion has been had again and again and again in the past,
please consult the mail archives before repeating history.

Usually the number of function to group mappings look like
they are arbitrary but in electronic practice they are not.

In practice there is one or two places where e.g. the MMC
card will be connected, you will not use every second pin
of the MMC connector for half an SPI port, even if it is possible
in theory.

> By defining artificial software groups in the driver we are pushing
> constraints in the binding that don't really exist in hardware.

The binding is supposed to be helpful when configuring the
system, so for a person setting up a new device tree that may
be helpful.

It gives them some info on what is electronically reasonable.

> So if
> someone comes up with a pin usage that isn't covered by the existing
> groups we need to change the binding.

In my experience people don't change bindings very much, because
the life cycle of an SoC is so short. They get it right the first time or
not at all.

> If the hardware allows this much freedom we should also allow it in the
> pinctrl binding.

I have to push back on this, because the pin control bindings are
exploding, and it is irresponsible of me as subsystem maintainer to
just allow anything.

Can you think of something that is both generic and helpful when
working with other systems than this?

What we need to do is get away from all "necessarily different"
bindings.

>> Then put the pin configuration (bias etc) in a separate node in the same
>> state definition like that:
>>
>> i2c0_pins_a {
>>          function = "i2c0";
>>          groups = "i2c0-pos0";
>> };
>> i2c0_pins_b {
>>          bias-disable;
>> };
>
> The problem with that is that different pins might need different
> configuration for the same muxed function. To properly reflect this we
> would need to duplicate the pin definitions. One popular example is the
> MMC interface where part of the pins need to have a pull-up, while
> others don't. How would you reflect this with the DT description above?

That's no problem. Muxing is group+function and configuration is
per-pin.

See this example:

                        uart2 {
                                uart2_default_mode: uart2_default {
                                        default_mux {
                                                function = "u2";
                                                groups = "u2rxtx_c_1";
                                        };
                                        default_cfg1 {
                                                pins = "GPIO29_W2"; /* RXD */
                                                bias-pull-up;
                                                low-power-disable;
                                        };

                                        default_cfg2 {
                                                pins = "GPIO30_W3"; /* TXD */
                                                output-high;
                                                low-power-disable;
                                        };
                                };

                                uart2_sleep_mode: uart2_sleep {
                                        sleep_cfg1 {
                                                pins = "GPIO29_W2"; /* RXD */
                                                low-power-enable;
                                        };

                                        sleep_cfg2 {
                                                pins = "GPIO30_W3"; /* TXD */
                                                low-power-enable;
                                        };
                                };
                        };


Yours,
Linus Walleij