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

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

[Sascha Hauer]

Hi Linus,

Is this driver generally ok? It basically implements the binding
suggested as RFC (except that mediatek,pins should only be pins).
Are you Ok with this approach? Anything else missing?

Sascha

On Tue, Nov 11, 2014 at 08:38:52PM +0800, Hongzhou Yang wrote:
> This is v3 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-October/296093.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 v3:
>   - Rebase on 3.18-rc1.
>   - Follow '[RFC] pinctrl: Provide a generic device tree binding for per-pin pin controllers', modify mediatek,pins format,
>     add config setting for each pin, the reason is that each pin may have different config.
>     The RFC link: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-October/296491.html
>   - Switch to use syscon regmap.
>   - Add CLR_ADDR() and SET_ADDR() macros.
>   - Add mtk_pconf_set_ies() and mtk_pconf_set_smt() functions to deal with input-enable and input-schmitt-enable.
>   - Rename mt_gpio_devdata to mtk_pinctrl_devdata.
>   - Rename all mt_prefix to mtk_prefix.
>   - Rename mt_pconf_pull_conf() to mtk_pconf_parse_conf, add mtk_pconf_set_pull_select() to deal with pull enable and pull up/down.
>     And we will add PIN_CONFIG_DRIVE_STRENGTH to mtk_pconf_parse_conf().
> 
> According to Linus' suggestion, make some following changes:
>   - Add port_shf, port_mask and port_align members to mtk_pinctrl_devdata struct.
>   - Use BIT(x) instead of 1 << (x).
>   - Remove mt_pctrl_write_reg() and mt_pctrl_read_reg().
>   - Remove mt_gpio_of_xlate().
>   - Use .set_mux() instead of .enable().
>   - Add some comments.
> 
> Hongzhou Yang (3):
>   ARM: mediatek: Add Pinctrl/GPIO driver for mt8135.
>   dt-bindings: Add pinctrl bindings for mt65xx/mt81xx.
>   ARM: dts: mt8135: Add pinctrl/GPIO node for mt8135.
> 
>   .../devicetree/bindings/pinctrl/pinctrl-mt65xx.txt |  123 +
>   arch/arm/boot/dts/mt8135-pinfunc.h                 | 1304 +++++++++++
>   arch/arm/boot/dts/mt8135.dtsi                      |   19 +
>   arch/arm/mach-mediatek/Kconfig                     |    1 +
>   drivers/pinctrl/Kconfig                            |    1 +
>   drivers/pinctrl/Makefile                           |    1 +
>   drivers/pinctrl/mediatek/Kconfig                   |   12 +
>   drivers/pinctrl/mediatek/Makefile                  |    5 +
>   drivers/pinctrl/mediatek/pinctrl-mt8135.c          |   87 +
>   drivers/pinctrl/mediatek/pinctrl-mtk-common.c      |  861 +++++++
>   drivers/pinctrl/mediatek/pinctrl-mtk-common.h      |  123 +
>   drivers/pinctrl/mediatek/pinctrl-mtk-mt8135.h      | 2460 ++++++++++++++++++++
>   include/dt-bindings/pinctrl/mt65xx.h               |   23 +
>   13 files changed, 5020 insertions(+)
>   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
> 
> 
> 

-- 
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 v3 2/3] dt-bindings: Add pinctrl bindings for mt65xx/mt81xx.

[Linus Walleij]

On Tue, Nov 11, 2014 at 1:38 PM, Hongzhou Yang
<hongzhou.yang@mediatek.com> wrote:

> +* Mediatek MT65XX Pin Controller
> +
> +The Mediatek's Pin controller is used to control GPIO pins.

It's not GPIO pins, since they are not always general purpose. It's just
pins. Say "control SoC pins".

> +Required properties:
> +- compatible: value should be either of the following.
> +    (a) "mediatek,mt8135-pinctrl", compatible with mt8135 pinctrl.
> +- mediatek,pctl-regmap: Should be a phandle of the syscfg node.
> +- gpio-controller : Marks the device node as a gpio controller.
> +- #gpio-cells: number of cells in GPIO specifier. Since the generic GPIO
> +  binding is used, the amount of cells must be specified as 2. See the below
> +  mentioned gpio binding representation for description of particular cells.
> +
> +       Eg: <&pio 6 0>
> +       <[phandle of the gpio controller node]
> +       [pin number within the gpio controller]

It's not a pin number really, it is a GPIO offset. But incidentally it's
the same as the pin number. (This is OK...)

> +- mediatek,pins: 2 integers array, represents gpio pinmux number and config
> +  setting. The format as following
> +
> +    node {
> +     mediatek,pins = <PIN_NUMBER_PINMUX>;
> +                     GENERIC_PINCONFIG;
> +    };

As suggested by Sacha, use just "pins" and define the binding as a patch
to Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
that is generic for multiplexing, so we get some order here.

I want you however to put pin multiplexing and pin configuration into
different nodes if possible. I don't like combines muxing and config
nodes. If necessary tag the node with something.

In the end we can also move the parsing functions to the pinctrl core, as
long as the bindings are correct (possibly as a refactoring later).

> +               i2c0_pins_a: i2c0@0 {
> +                       pins1 {
> +                               mediatek,pins = <MT8135_PIN_100_SDA0__FUNC_SDA0>,
> +                                               <MT8135_PIN_101_SCL0__FUNC_SCL0>;
> +                               bias-disable;
> +                       };
> +               };

I would split it up.

i2c0_pins_a: i2c0@0 {
        pins1 {
                pins = <MT8135_PIN_100_SDA0>;
                function = <MT8135_PIN_100_FUNC_SDA0>;
        };
        pins2 {
               pins = <MT8135_PIN_100_SDA0>;
               bias-disable;
        };
};

One node for the multiplexing, one node for the config. This is the
pattern used by most drivers, so I want to have this structure.

It is also easy to tell one node from the other: if it contains "function"
we know it's a multiplexing node, if it doesn't, it's a config node.

Yours,
Linus Walleij

Re: [PATCH v3 1/3] ARM: mediatek: Add Pinctrl/GPIO driver for mt8135.

[Linus Walleij]

()On Tue, Nov 11, 2014 at 1:38 PM, Hongzhou Yang
<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 driver and mt8135 part.
> The common driver include the pinctrl driver and GPIO driver.
> The mt8135 part contain its special device data.
>
> Signed-off-by: Hongzhou Yang <hongzhou.yang@mediatek.com>
(...)
>  menuconfig ARCH_MEDIATEK
>         bool "Mediatek MT65xx & MT81xx SoC" if ARCH_MULTI_V7
>         select ARM_GIC
> +       select PINCTRL

Should it not also select PINCTRL_MTK8135 for the right SoC?

(...)
> +++ b/drivers/pinctrl/mediatek/Kconfig
> @@ -0,0 +1,12 @@
> +if ARCH_MEDIATEK
> +
> +config PINCTRL_MTK_COMMON
> +       bool
> +       select PINMUX
> +       select GENERIC_PINCONF

This is also a GPIO driver but it fails to select GPIOLIB,
OF_GPIO and also possibly GPIOLIB_IRQCHIP.

(...)
> +static int mtk_pctrl_dt_node_to_map_config(struct mtk_pinctrl *pctl, u32 pin,
> +               unsigned long *configs, unsigned num_configs,
> +               struct pinctrl_map **map, unsigned *cnt_maps,
> +               unsigned *num_maps)
> +{
> +       struct mtk_pinctrl_group *grp;
> +       unsigned long *cfgs;
> +
> +       if (*num_maps == *cnt_maps)
> +               return -ENOSPC;
> +
> +       cfgs = kmemdup(configs, num_configs * sizeof(*cfgs),
> +                      GFP_KERNEL);
> +       if (cfgs == NULL)
> +               return -ENOMEM;
> +
> +       grp = mtk_pctrl_find_group_by_pin(pctl, pin);
> +       if (!grp) {
> +               dev_err(pctl->dev, "unable to match pin %d to group\n", pin);
> +               return -EINVAL;
> +       }
> +
> +       (*map)[*num_maps].type = PIN_MAP_TYPE_CONFIGS_GROUP;
> +       (*map)[*num_maps].data.configs.group_or_pin = grp->name;
> +       (*map)[*num_maps].data.configs.configs = cfgs;
> +       (*map)[*num_maps].data.configs.num_configs = num_configs;
> +       (*num_maps)++;
> +
> +       return 0;
> +}

Can't this use pinctrl_utils_add_map_configs()?

> +static void mtk_pctrl_dt_free_map(struct pinctrl_dev *pctldev,
> +                                   struct pinctrl_map *map,
> +                                   unsigned num_maps)
> +{
> +       int i;
> +
> +       for (i = 0; i < num_maps; i++) {
> +               if (map[i].type == PIN_MAP_TYPE_CONFIGS_GROUP)
> +                       kfree(map[i].data.configs.configs);
> +       }
> +
> +       kfree(map);
> +}

Use pinctrl_utils_dt_free_map() instead.

> +static int mtk_dt_cnt_map(struct pinctrl_map **map, unsigned *cnt_maps,
> +               unsigned *num_maps, unsigned cnt)
> +{
> +       unsigned old_num = *cnt_maps;
> +       unsigned new_num = *num_maps + cnt;
> +       struct pinctrl_map *new_map;
> +
> +       if (old_num >= new_num)
> +               return 0;
> +
> +       new_map = krealloc(*map, sizeof(*new_map) * new_num, GFP_KERNEL);
> +       if (!new_map)
> +               return -ENOMEM;
> +
> +       memset(new_map + old_num, 0, (new_num - old_num) * sizeof(*new_map));
> +
> +       *map = new_map;
> +       *cnt_maps = new_num;
> +
> +       return 0;
> +}

Use pinctrl_utils_reserve_map() instead.

> +static int mtk_pctrl_dt_subnode_to_map(struct pinctrl_dev *pctldev,
> +                                     struct device_node *node,
> +                                     struct pinctrl_map **map,
> +                                     unsigned *num_maps, unsigned *cnt_maps)
> +{
> +       struct property *pins;
> +       u32 pinfunc, pin, func;
> +       int num_pins, num_funcs, maps_per_pin;
> +       unsigned long *configs;
> +       unsigned int num_configs;
> +       bool has_config = 0;
> +       int i, err;
> +       unsigned cnt = 0;
> +       struct mtk_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
> +
> +       pins = of_find_property(node, "mediatek,pins", NULL);
> +       if (!pins) {
> +               dev_err(pctl->dev, "missing mediatek,pins property in node %s .\n",
> +                               node->name);
> +               return -EINVAL;
> +       }
> +
> +       err = pinconf_generic_parse_dt_config(node, &configs, &num_configs);
> +       if (num_configs)
> +               has_config = 1;

I'd prefer we first identify if it's a config or mux subnode, then go on to
parse it as mux or config. See comments on patch 2/3.

> +
> +       num_pins = pins->length / sizeof(u32);
> +       num_funcs = num_pins;
> +       maps_per_pin = 0;
> +       if (num_funcs)
> +               maps_per_pin++;
> +       if (has_config && num_pins >= 1)
> +               maps_per_pin++;
> +
> +       if (!num_pins || !maps_per_pin)
> +               return -EINVAL;
> +
> +       cnt = num_pins * maps_per_pin;
> +
> +       err = mtk_dt_cnt_map(map, cnt_maps, num_maps, cnt);
> +       if (err < 0)
> +               goto fail;
> +
> +       for (i = 0; i < num_pins; i++) {
> +               err = of_property_read_u32_index(node, "mediatek,pins",
> +                               i, &pinfunc);

As mentioned use just "pins" and let's figure out how to handle
this in a generic way.

> +static int mtk_gpio_request(struct gpio_chip *chip, unsigned offset)
> +{
> +       return pinctrl_request_gpio(chip->base + offset);
> +}
> +
> +static void mtk_gpio_free(struct gpio_chip *chip, unsigned offset)
> +{
> +       pinctrl_free_gpio(chip->base + offset);
> +}
> +
> +static int mtk_gpio_direction_input(struct gpio_chip *chip,
> +                                       unsigned offset)
> +{
> +       return pinctrl_gpio_direction_input(chip->base + offset);
> +}
> +
> +static int mtk_gpio_direction_output(struct gpio_chip *chip,
> +                                       unsigned offset, int value)
> +{
> +       mtk_gpio_set(chip, offset, value);
> +       return pinctrl_gpio_direction_output(chip->base + offset);
> +}

This is kinda nice!

> +static int mtk_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
> +{
> +       struct mtk_pinctrl *pctl = dev_get_drvdata(chip->dev);
> +       struct mtk_pinctrl_group *g = pctl->groups + offset;
> +       struct mtk_desc_function *desc =
> +                       mtk_pctrl_desc_find_irq_function_from_name(
> +                                       pctl, g->name);
> +       if (!desc)
> +               return -EINVAL;
> +
> +       return desc->irqnum;
> +}

I don't quite get this still. Does this mean every single GPIO line
potentially has it's own unique IRQ line?

Yours,
Linus Walleij

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

[Sascha Hauer]

On Thu, Nov 27, 2014 at 09:44:42AM +0100, Linus Walleij wrote:
> On Tue, Nov 11, 2014 at 1:38 PM, Hongzhou Yang
> <hongzhou.yang@mediatek.com> wrote:
> 
> > +* Mediatek MT65XX Pin Controller
> > +
> > +The Mediatek's Pin controller is used to control GPIO pins.
> 
> It's not GPIO pins, since they are not always general purpose. It's just
> pins. Say "control SoC pins".
> 
> > +Required properties:
> > +- compatible: value should be either of the following.
> > +    (a) "mediatek,mt8135-pinctrl", compatible with mt8135 pinctrl.
> > +- mediatek,pctl-regmap: Should be a phandle of the syscfg node.
> > +- gpio-controller : Marks the device node as a gpio controller.
> > +- #gpio-cells: number of cells in GPIO specifier. Since the generic GPIO
> > +  binding is used, the amount of cells must be specified as 2. See the below
> > +  mentioned gpio binding representation for description of particular cells.
> > +
> > +       Eg: <&pio 6 0>
> > +       <[phandle of the gpio controller node]
> > +       [pin number within the gpio controller]
> 
> It's not a pin number really, it is a GPIO offset. But incidentally it's
> the same as the pin number. (This is OK...)
> 
> > +- mediatek,pins: 2 integers array, represents gpio pinmux number and config
> > +  setting. The format as following
> > +
> > +    node {
> > +     mediatek,pins = <PIN_NUMBER_PINMUX>;
> > +                     GENERIC_PINCONFIG;
> > +    };
> 
> As suggested by Sacha, use just "pins" and define the binding as a patch
> to Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> that is generic for multiplexing, so we get some order here.
> 
> I want you however to put pin multiplexing and pin configuration into
> different nodes if possible. I don't like combines muxing and config
> nodes. If necessary tag the node with something.

Why? I think the properties can live happily together, even when the
parsing functions go to the pinctrl core.

> In the end we can also move the parsing functions to the pinctrl core, as
> long as the bindings are correct (possibly as a refactoring later).



> 
> > +               i2c0_pins_a: i2c0@0 {
> > +                       pins1 {
> > +                               mediatek,pins = <MT8135_PIN_100_SDA0__FUNC_SDA0>,
> > +                                               <MT8135_PIN_101_SCL0__FUNC_SCL0>;
> > +                               bias-disable;
> > +                       };
> > +               };
> 
> I would split it up.
> 
> i2c0_pins_a: i2c0@0 {
>         pins1 {
>                 pins = <MT8135_PIN_100_SDA0>;
>                 function = <MT8135_PIN_100_FUNC_SDA0>;
>         };

The reason to put this in a single define was to make writing the device
trees less error prone. When you have two arrays you must correlate the
entries.

>         pins2 {
>                pins = <MT8135_PIN_100_SDA0>;
>                bias-disable;
>         };

Here we repeat what we've already written above just to add the pinctrl.

Let's have a look at a real world example, here a eMMC controller node
from the i.MX6 riotboard converted to generic pinconf, first your
suggestion, then mine:

emmc_pins: emmc@0 {
	pins1 {
		pins = <MX6QDL_PAD_SD3_CMD
			MX6QDL_PAD_SD3_CLK
			MX6QDL_PAD_SD3_DAT0
			MX6QDL_PAD_SD3_DAT1
			MX6QDL_PAD_SD3_DAT2
			MX6QDL_PAD_SD3_DAT3
			MX6QDL_PAD_SD3_DAT4
			MX6QDL_PAD_SD3_DAT5
			MX6QDL_PAD_SD3_DAT6
			MX6QDL_PAD_SD3_DAT7>;
		function = <MX6QDL_PAD_SD3_CMD__SD3_CMD
			MX6QDL_PAD_SD3_CLK__SD3_CLK
			MX6QDL_PAD_SD3_DAT0__SD3_DATA0
			MX6QDL_PAD_SD3_DAT1__SD3_DATA1
			MX6QDL_PAD_SD3_DAT2__SD3_DATA2
			MX6QDL_PAD_SD3_DAT3__SD3_DATA3
			MX6QDL_PAD_SD3_DAT4__SD3_DATA4
			MX6QDL_PAD_SD3_DAT5__SD3_DATA5
			MX6QDL_PAD_SD3_DAT6__SD3_DATA6
			MX6QDL_PAD_SD3_DAT7__SD3_DATA7>;
	};

	pins2 {
		pins = <MX6QDL_PAD_SD3_CLK>;
		drive-strength = <87>; /* in OHM */
	};

	pins3 {
		pins = <MX6QDL_PAD_SD3_CMD
			MX6QDL_PAD_SD3_DAT0
			MX6QDL_PAD_SD3_DAT1
			MX6QDL_PAD_SD3_DAT2
			MX6QDL_PAD_SD3_DAT3
			MX6QDL_PAD_SD3_DAT4
			MX6QDL_PAD_SD3_DAT5
			MX6QDL_PAD_SD3_DAT6
			MX6QDL_PAD_SD3_DAT7
		>;
		drive-strength = <87>; /* in OHM */
		bias-pull-up = <47000>;
	};
};

emmc_pins: emmc@0 {
	pins1 {
		pins = <MX6QDL_PAD_SD3_CMD__SD3_CMD
			MX6QDL_PAD_SD3_DAT0__SD3_DATA0
			MX6QDL_PAD_SD3_DAT1__SD3_DATA1
			MX6QDL_PAD_SD3_DAT2__SD3_DATA2
			MX6QDL_PAD_SD3_DAT3__SD3_DATA3
			MX6QDL_PAD_SD3_DAT4__SD3_DATA4
			MX6QDL_PAD_SD3_DAT5__SD3_DATA5
			MX6QDL_PAD_SD3_DAT6__SD3_DATA6
			MX6QDL_PAD_SD3_DAT7__SD3_DATA7>;
		drive-strength = <87>; /* in OHM */
		bias-pull-up = <47000>;
	};

	pins2 {
		pins = <MX6QDL_PAD_SD3_CLK__SD3_CLK>;
		drive-strength = <87>; /* in OHM */
	};
};

So this is not even half as big. Also it provides less opportunities for
introducing bugs like: Do all pins have a valid configuration setting?
Do the pins/function arrays have both the same number of entries and do
the entries in both arrays match?

> 
> One node for the multiplexing, one node for the config. This is the
> pattern used by most drivers, so I want to have this structure.
> 
> It is also easy to tell one node from the other: if it contains "function"
> we know it's a multiplexing node, if it doesn't, it's a config node.

So when merging these together a node is always multiplexing and
configuration. But do we really win anything if both are separated? When
both are separated you still need an array of pins in the nodes to
tell which pins this node is for. If this array also contains the
mux information then this won't hurt. You can still ignore it.

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 v3 2/3] dt-bindings: Add pinctrl bindings for mt65xx/mt81xx.

[Linus Walleij]

On Thu, Nov 27, 2014 at 11:18 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Thu, Nov 27, 2014 at 09:44:42AM +0100, Linus Walleij wrote:
>> On Tue, Nov 11, 2014 at 1:38 PM, Hongzhou Yang
>> <hongzhou.yang@mediatek.com> wrote:

>> > +- mediatek,pins: 2 integers array, represents gpio pinmux number and config
>> > +  setting. The format as following
>> > +
>> > +    node {
>> > +     mediatek,pins = <PIN_NUMBER_PINMUX>;
>> > +                     GENERIC_PINCONFIG;
>> > +    };
>>
>> As suggested by Sacha, use just "pins" and define the binding as a patch
>> to Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
>> that is generic for multiplexing, so we get some order here.
>>
>> I want you however to put pin multiplexing and pin configuration into
>> different nodes if possible. I don't like combines muxing and config
>> nodes. If necessary tag the node with something.
>
> Why? I think the properties can live happily together, even when the
> parsing functions go to the pinctrl core.

I'm worried about the semantic ambiguity between the pins = <...>;
property on different pin controllers, whether they are based on
function+group or per-pin. It's not even up to me to decide,
this is for the DT binding people.

In this case:

pins = <MT8135_PIN_100_SDA0__FUNC_SDA0>,
          <MT8135_PIN_101_SCL0__FUNC_SCL0>;

Each element is a 32bit unsigned where the lower and higher
16 bits have different meanings.

In some other pin controller (using generic bindings and
already merged! arch/arm/boot/dts/ste-href-ab8500.dtsi):

gpio39 {
      gpio39_default_mode: gpio39_default {
             default_mux {
                      function = "gpio";
                      groups = "gpio39_a_1";
             };
             default_cfg {
                      pins = "GPIO39_E16";
                      input-enable;
                      bias-pull-down;
             };
     };
};

Can we get away with using the same core parser with this
as well, here the nodes are split and using strings to identify
pins, not 32bit numbers.

I am worried about semantic coexistance...

>> > +               i2c0_pins_a: i2c0@0 {
>> > +                       pins1 {
>> > +                               mediatek,pins = <MT8135_PIN_100_SDA0__FUNC_SDA0>,
>> > +                                               <MT8135_PIN_101_SCL0__FUNC_SCL0>;
>> > +                               bias-disable;
>> > +                       };
>> > +               };
>>
>> I would split it up.
>>
>> i2c0_pins_a: i2c0@0 {
>>         pins1 {
>>                 pins = <MT8135_PIN_100_SDA0>;
>>                 function = <MT8135_PIN_100_FUNC_SDA0>;
>>         };
>
> The reason to put this in a single define was to make writing the device
> trees less error prone. When you have two arrays you must correlate the
> entries.

I see the upside. I'm just worried about ambiguity when comparing
different device trees to each other, because they should be about
readability and understanding, not confusing...

>> One node for the multiplexing, one node for the config. This is the
>> pattern used by most drivers, so I want to have this structure.
>>
>> It is also easy to tell one node from the other: if it contains "function"
>> we know it's a multiplexing node, if it doesn't, it's a config node.
>
> So when merging these together a node is always multiplexing and
> configuration. But do we really win anything if both are separated? When
> both are separated you still need an array of pins in the nodes to
> tell which pins this node is for. If this array also contains the
> mux information then this won't hurt. You can still ignore it.

Well we definately have to support the case with split config and
muxing nodes at least. I am very worried that we would get into
ambguities where that is not possible.

Yours,
Linus Walleij

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

[Sascha Hauer]

Hi Linus,

On Fri, Nov 28, 2014 at 05:12:44PM +0100, Linus Walleij wrote:
> On Thu, Nov 27, 2014 at 11:18 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > On Thu, Nov 27, 2014 at 09:44:42AM +0100, Linus Walleij wrote:
> >> On Tue, Nov 11, 2014 at 1:38 PM, Hongzhou Yang
> >> <hongzhou.yang@mediatek.com> wrote:
> 
> >> > +- mediatek,pins: 2 integers array, represents gpio pinmux number and config
> >> > +  setting. The format as following
> >> > +
> >> > +    node {
> >> > +     mediatek,pins = <PIN_NUMBER_PINMUX>;
> >> > +                     GENERIC_PINCONFIG;
> >> > +    };
> >>
> >> As suggested by Sacha, use just "pins" and define the binding as a patch
> >> to Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> >> that is generic for multiplexing, so we get some order here.
> >>
> >> I want you however to put pin multiplexing and pin configuration into
> >> different nodes if possible. I don't like combines muxing and config
> >> nodes. If necessary tag the node with something.
> >
> > Why? I think the properties can live happily together, even when the
> > parsing functions go to the pinctrl core.
> 
> I'm worried about the semantic ambiguity between the pins = <...>;
> property on different pin controllers, whether they are based on
> function+group or per-pin. It's not even up to me to decide,
> this is for the DT binding people.
> 
> In this case:
> 
> pins = <MT8135_PIN_100_SDA0__FUNC_SDA0>,
>           <MT8135_PIN_101_SCL0__FUNC_SCL0>;
> 
> Each element is a 32bit unsigned where the lower and higher
> 16 bits have different meanings.
> 
> In some other pin controller (using generic bindings and
> already merged! arch/arm/boot/dts/ste-href-ab8500.dtsi):
> 
> gpio39 {
>       gpio39_default_mode: gpio39_default {
>              default_mux {
>                       function = "gpio";
>                       groups = "gpio39_a_1";
>              };
>              default_cfg {
>                       pins = "GPIO39_E16";
>                       input-enable;
>                       bias-pull-down;
>              };
>      };
> };
> 
> Can we get away with using the same core parser with this
> as well, here the nodes are split and using strings to identify
> pins, not 32bit numbers.
> 
> I am worried about semantic coexistance...

We could rename the property from 'pins' to 'pinmux' for this variant of
the binding. Then a parser would know that this property is about pins
and muxing.

> 
> >> > +               i2c0_pins_a: i2c0@0 {
> >> > +                       pins1 {
> >> > +                               mediatek,pins = <MT8135_PIN_100_SDA0__FUNC_SDA0>,
> >> > +                                               <MT8135_PIN_101_SCL0__FUNC_SCL0>;
> >> > +                               bias-disable;
> >> > +                       };
> >> > +               };
> >>
> >> I would split it up.
> >>
> >> i2c0_pins_a: i2c0@0 {
> >>         pins1 {
> >>                 pins = <MT8135_PIN_100_SDA0>;
> >>                 function = <MT8135_PIN_100_FUNC_SDA0>;
> >>         };
> >
> > The reason to put this in a single define was to make writing the device
> > trees less error prone. When you have two arrays you must correlate the
> > entries.
> 
> I see the upside. I'm just worried about ambiguity when comparing
> different device trees to each other, because they should be about
> readability and understanding, not confusing...

Sorry, given the currently existing devicetrees I don't buy that
readability argument. Let's look into the snowball example, here ssp0:

	ssp0_snowball_mode: ssp0_snowball_default {
		snowball_mux {
			ste,function = "ssp0";
			ste,pins = "ssp0_a_1";
		};
		snowball_cfg1 {
			ste,pins = "GPIO144_B13"; /* FRM */
			ste,config = <&gpio_out_hi>;
		};
		snowball_cfg2 {
			ste,pins = "GPIO145_C13"; /* RXD */
			ste,config = <&in_pd>;
		};
		snowball_cfg3 {
			ste,pins =
			"GPIO146_D13", /* TXD */
			"GPIO143_D12"; /* CLK */
			ste,config = <&out_lo>;
		};
	};


For the SSP0 it needs the string "ssp0_a_1" which is documented exactly
nowhere. Only the sourcecode shows that this (totally made up) string
means that the pins DB8500_PIN_D12, DB8500_PIN_B13, DB8500_PIN_C13 and
DB8500_PIN_D13 shall be muxed. The corresponding ste,function property
has the value "ssp0" which again is not documented. The following config
nodes reference the same pins under a different name: "GPIO144_B13",
"GPIO145_C13", "GPIO146_D13" and "GPIO143_D12". Again, these strings are
completely undocumented and only the sourcecode shows which strings can
be used for the ste,pins property. Not only that no documentation shows
which strings are allowed, there's also no documentation which describes
which combination of strings for the different properties make sense.
The use of ## for concatenating defines in the driver makes the whole
stuff even harder to understand. It even took me quite a while to
realize that the binding requires me to configure the muxes in groups,
but the config as individual pins. So no, the current devicetrees are
not about readability.

Rewrite this to:

#define GPIO143_D12_SSP0_CLK	PINMUX_PIN(143, 1)
#define GPIO144_B13_SSP0_FRM	PINMUX_PIN(144, 1)
#define GPIO145_C13_SSP0_RXD	PINMUX_PIN(145, 1)
#define GPIO146_D13_SSP0_TXD	PINMUX_PIN(146, 1)

and we get:

	ssp0_snowball_mode: ssp0_snowball_default {
		snowball_cfg1 {
			pinmux = <GPIO144_B13_SSP0_FRM>;
			ste,config = <&gpio_out_hi>;
		};
		snowball_cfg2 {
			pinmux = <GPIO145_C13_SSP0_RXD>;
			ste,config = <&in_pd>;
		};
		snowball_cfg3 {
			pinmux = <GPIO143_D12_SSP0_CLK GPIO146_D13_SSP0_TXD>;
			ste,config = <&out_lo>;
		};
	};

And the documentation we need is: "For the pinmux property pick macros
from dt-bindings/.../xy.h"

> 
> >> One node for the multiplexing, one node for the config. This is the
> >> pattern used by most drivers, so I want to have this structure.
> >>
> >> It is also easy to tell one node from the other: if it contains "function"
> >> we know it's a multiplexing node, if it doesn't, it's a config node.
> >
> > So when merging these together a node is always multiplexing and
> > configuration. But do we really win anything if both are separated? When
> > both are separated you still need an array of pins in the nodes to
> > tell which pins this node is for. If this array also contains the
> > mux information then this won't hurt. You can still ignore it.
> 
> Well we definately have to support the case with split config and
> muxing nodes at least. I am very worried that we would get into
> ambguities where that is not possible.

Sure we have as we cannot change existing bindings, but I cannot see
any ambiguities. In the end it's the SoC specific driver which decides
over the binding. Of course we do ourselves a favor when all use a
similar binding and use common code to parse it, but when everything
else fails we can still make a SoC specific parser. Nobody wants that
of course, we are all lazy ;)

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 v3 2/3] dt-bindings: Add pinctrl bindings for mt65xx/mt81xx.

[Linus Walleij]

On Tue, Dec 2, 2014 at 2:55 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:

Sorry for taking eternities to get back on this, I ran into a merge window
and some christmas. I do hope we can resolve this in the current
development cycle so we can get this support in.

> On Fri, Nov 28, 2014 at 05:12:44PM +0100, Linus Walleij wrote:
>> On Thu, Nov 27, 2014 at 11:18 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
>> > On Thu, Nov 27, 2014 at 09:44:42AM +0100, Linus Walleij wrote:
>> >> On Tue, Nov 11, 2014 at 1:38 PM, Hongzhou Yang
>> >> <hongzhou.yang@mediatek.com> wrote:

>> >> > +    node {
>> >> > +     mediatek,pins = <PIN_NUMBER_PINMUX>;
>> >> > +                     GENERIC_PINCONFIG;
>> >> > +    };
>> >>
>> >> As suggested by Sacha, use just "pins" and define the binding as a patch
>> >> to Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
>> >> that is generic for multiplexing, so we get some order here.
>> >>
>> >> I want you however to put pin multiplexing and pin configuration into
>> >> different nodes if possible. I don't like combines muxing and config
>> >> nodes. If necessary tag the node with something.
>> >
>> > Why? I think the properties can live happily together, even when the
>> > parsing functions go to the pinctrl core.
>>
>> I'm worried about the semantic ambiguity between the pins = <...>;
>> property on different pin controllers, whether they are based on
>> function+group or per-pin. It's not even up to me to decide,
>> this is for the DT binding people.
>>
>> In this case:
>>
>> pins = <MT8135_PIN_100_SDA0__FUNC_SDA0>,
>>           <MT8135_PIN_101_SCL0__FUNC_SCL0>;
>>
>> Each element is a 32bit unsigned where the lower and higher
>> 16 bits have different meanings.
>>
>> In some other pin controller (using generic bindings and
>> already merged! arch/arm/boot/dts/ste-href-ab8500.dtsi):
>>
>> gpio39 {
>>       gpio39_default_mode: gpio39_default {
>>              default_mux {
>>                       function = "gpio";
>>                       groups = "gpio39_a_1";
>>              };
>>              default_cfg {
>>                       pins = "GPIO39_E16";
>>                       input-enable;
>>                       bias-pull-down;
>>              };
>>      };
>> };
>>
>> Can we get away with using the same core parser with this
>> as well, here the nodes are split and using strings to identify
>> pins, not 32bit numbers.
>>
>> I am worried about semantic coexistance...
>
> We could rename the property from 'pins' to 'pinmux' for this variant of
> the binding. Then a parser would know that this property is about pins
> and muxing.

OK sounds like a viable compromise.

I am mostly worried that none of the fine device tree people say
anything about this stuff we're discussing here, maybe absolutely
nobody else understands...

>> >> i2c0_pins_a: i2c0@0 {
>> >>         pins1 {
>> >>                 pins = <MT8135_PIN_100_SDA0>;
>> >>                 function = <MT8135_PIN_100_FUNC_SDA0>;
>> >>         };
>> >
>> > The reason to put this in a single define was to make writing the device
>> > trees less error prone. When you have two arrays you must correlate the
>> > entries.
>>
>> I see the upside. I'm just worried about ambiguity when comparing
>> different device trees to each other, because they should be about
>> readability and understanding, not confusing...
>
> Sorry, given the currently existing devicetrees I don't buy that
> readability argument. Let's look into the snowball example, here ssp0:
>
>         ssp0_snowball_mode: ssp0_snowball_default {
>                 snowball_mux {
>                         ste,function = "ssp0";
>                         ste,pins = "ssp0_a_1";
>                 };
>                 snowball_cfg1 {
>                         ste,pins = "GPIO144_B13"; /* FRM */
>                         ste,config = <&gpio_out_hi>;
>                 };
>                 snowball_cfg2 {
>                         ste,pins = "GPIO145_C13"; /* RXD */
>                         ste,config = <&in_pd>;
>                 };
>                 snowball_cfg3 {
>                         ste,pins =
>                         "GPIO146_D13", /* TXD */
>                         "GPIO143_D12"; /* CLK */
>                         ste,config = <&out_lo>;
>                 };
>         };

I agree the ste,pins is not a good example, it is insane to have
something group and pins mixed, and that is why
I migrated it in the last merge window, notably the pin multiplex thing
so it now looks like this:

                        ssp0 {
                                ssp0_snowball_mode: ssp0_snowball_default {
                                        snowball_mux {
                                                function = "ssp0";
                                                groups = "ssp0_a_1";
                                        };
                                        snowball_cfg1 {
                                                pins = "GPIO144_B13"; /* FRM */
                                                ste,config = <&gpio_out_hi>;
                                        };
(...)

I'm sorry about not migrating the ste,config part to
generic bindings yet :( that is next.

> For the SSP0 it needs the string "ssp0_a_1" which is documented exactly
> nowhere.

Is this a bug report about the documentation? I don't see how
that is relevant to the overall design.

> Only the sourcecode shows that this (totally made up) string
> means that the pins DB8500_PIN_D12, DB8500_PIN_B13, DB8500_PIN_C13 and
> DB8500_PIN_D13 shall be muxed.

So this pins and pins ambiguity (which has nothing to do with the
generic bindings BTW) is now fixed up somewhat. The first thing is
a group, the pins are pin names.

> The corresponding ste,function property
> has the value "ssp0" which again is not documented. The following config
> nodes reference the same pins under a different name: "GPIO144_B13",
> "GPIO145_C13", "GPIO146_D13" and "GPIO143_D12".

Yes, because it references individual pins, not groups. Config
is per-pin, multiplexing is per-group in the Nomadik case.
(Some hardware and drivers are different.)

> Again, these strings are
> completely undocumented and only the sourcecode shows which strings can
> be used for the ste,pins property. Not only that no documentation shows
> which strings are allowed, there's also no documentation which describes
> which combination of strings for the different properties make sense.

OK again a documentation bug report I guess, if you want to I can
add this to the documentation (there are indeed some pin control
drivers that list these groups and functions). This documentation was
not written by me, but I can sure fix it up if that makes you happier.

> The use of ## for concatenating defines in the driver makes the whole
> stuff even harder to understand. It even took me quite a while to
> realize that the binding requires me to configure the muxes in groups,
> but the config as individual pins.

The hardware is such that muxes are in groups and pin config per-pin.
We cannot augment reality, just describe it in an as structured way
as possible.

To add to the complexity, some pin controllers mux things in groups,
some per-pin (like freescale I think?) some controllers even do config
of things like pull-up across groups of pins rather than individually.

> So no, the current devicetrees are
> not about readability.

Is this an argument that goes away if I fix the documentation?

> #define GPIO143_D12_SSP0_CLK    PINMUX_PIN(143, 1)
> #define GPIO144_B13_SSP0_FRM    PINMUX_PIN(144, 1)
> #define GPIO145_C13_SSP0_RXD    PINMUX_PIN(145, 1)
> #define GPIO146_D13_SSP0_TXD    PINMUX_PIN(146, 1)
>
> and we get:
>
>         ssp0_snowball_mode: ssp0_snowball_default {
>                 snowball_cfg1 {
>                         pinmux = <GPIO144_B13_SSP0_FRM>;
>                         ste,config = <&gpio_out_hi>;
>                 };
>                 snowball_cfg2 {
>                         pinmux = <GPIO145_C13_SSP0_RXD>;
>                         ste,config = <&in_pd>;
>                 };
>                 snowball_cfg3 {
>                         pinmux = <GPIO143_D12_SSP0_CLK GPIO146_D13_SSP0_TXD>;
>                         ste,config = <&out_lo>;
>                 };
>         };

But this gives the false impression that pins can be muxed
individually, and it makes it possible to write device trees that
attempt to do so, while in practice it will not perform on the
hardware.

>> >> One node for the multiplexing, one node for the config. This is the
>> >> pattern used by most drivers, so I want to have this structure.
>> >>
>> >> It is also easy to tell one node from the other: if it contains "function"
>> >> we know it's a multiplexing node, if it doesn't, it's a config node.
>> >
>> > So when merging these together a node is always multiplexing and
>> > configuration. But do we really win anything if both are separated? When
>> > both are separated you still need an array of pins in the nodes to
>> > tell which pins this node is for. If this array also contains the
>> > mux information then this won't hurt. You can still ignore it.
>>
>> Well we definately have to support the case with split config and
>> muxing nodes at least. I am very worried that we would get into
>> ambguities where that is not possible.
>
> Sure we have as we cannot change existing bindings,

For Nomadik I did, because there are no deployed systems suffering
from it. I just had to use some board I had to make some kind of
example. I would encourage any other system not deployed in the
masses with flashed-in device trees to do the same as this is
still somewhat in a flux.

I am worried that there is something in your reasoning that sort of
assumes all pin controllers mux pins one-by-one and not in groups.
How do we make it impossible to write a device tree that also
make hardware that do groupwise config viable without ambiguities?

Yours,
Linus Walleij

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

[Sascha Hauer]

Hi Linus,

On Sat, Jan 10, 2015 at 10:33:46PM +0100, Linus Walleij wrote:
> On Tue, Dec 2, 2014 at 2:55 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> 
> Sorry for taking eternities to get back on this, I ran into a merge window
> and some christmas. I do hope we can resolve this in the current
> development cycle so we can get this support in.

No problem, I'm sure there are nicer things to do than discussing about
this topic ;)
I also hope we get this from the table soon.

> >>
> >> I am worried about semantic coexistance...
> >
> > We could rename the property from 'pins' to 'pinmux' for this variant of
> > the binding. Then a parser would know that this property is about pins
> > and muxing.
> 
> OK sounds like a viable compromise.

Ok, will change this.

> I'm sorry about not migrating the ste,config part to
> generic bindings yet :( that is next.
> 
> > For the SSP0 it needs the string "ssp0_a_1" which is documented exactly
> > nowhere.
> 
> Is this a bug report about the documentation? I don't see how
> that is relevant to the overall design.

The best documentation is one that is not needed. I mandate to use
defines with combinations of pin with mux setting to reduce the
necessary documentation to: "Pick one (many) of these and you're done".
So my criticism here is not mainly that there is no documentation but
that the necessary documention would be very voluminous. Normally it
must cover all possible combinations of pin/mux settings. If you add
this you can equally well add it to defines instead, which makes it
impossible to write inconsistent device trees and makes it easier to
understand what's happening.

> 
> > Only the sourcecode shows that this (totally made up) string
> > means that the pins DB8500_PIN_D12, DB8500_PIN_B13, DB8500_PIN_C13 and
> > DB8500_PIN_D13 shall be muxed.
> 
> So this pins and pins ambiguity (which has nothing to do with the
> generic bindings BTW) is now fixed up somewhat. The first thing is
> a group, the pins are pin names.
> 
> > The corresponding ste,function property
> > has the value "ssp0" which again is not documented. The following config
> > nodes reference the same pins under a different name: "GPIO144_B13",
> > "GPIO145_C13", "GPIO146_D13" and "GPIO143_D12".
> 
> Yes, because it references individual pins, not groups. Config
> is per-pin, multiplexing is per-group in the Nomadik case.
> (Some hardware and drivers are different.)
> 
> > Again, these strings are
> > completely undocumented and only the sourcecode shows which strings can
> > be used for the ste,pins property. Not only that no documentation shows
> > which strings are allowed, there's also no documentation which describes
> > which combination of strings for the different properties make sense.
> 
> OK again a documentation bug report I guess, if you want to I can
> add this to the documentation (there are indeed some pin control
> drivers that list these groups and functions). This documentation was
> not written by me, but I can sure fix it up if that makes you happier.
> 
> > The use of ## for concatenating defines in the driver makes the whole
> > stuff even harder to understand. It even took me quite a while to
> > realize that the binding requires me to configure the muxes in groups,
> > but the config as individual pins.
> 
> The hardware is such that muxes are in groups and pin config per-pin.
> We cannot augment reality, just describe it in an as structured way
> as possible.
> 
> To add to the complexity, some pin controllers mux things in groups,
> some per-pin (like freescale I think?) some controllers even do config
> of things like pull-up across groups of pins rather than individually.
> 
> > So no, the current devicetrees are
> > not about readability.
> 
> Is this an argument that goes away if I fix the documentation?
> 
> > #define GPIO143_D12_SSP0_CLK    PINMUX_PIN(143, 1)
> > #define GPIO144_B13_SSP0_FRM    PINMUX_PIN(144, 1)
> > #define GPIO145_C13_SSP0_RXD    PINMUX_PIN(145, 1)
> > #define GPIO146_D13_SSP0_TXD    PINMUX_PIN(146, 1)
> >
> > and we get:
> >
> >         ssp0_snowball_mode: ssp0_snowball_default {
> >                 snowball_cfg1 {
> >                         pinmux = <GPIO144_B13_SSP0_FRM>;
> >                         ste,config = <&gpio_out_hi>;
> >                 };
> >                 snowball_cfg2 {
> >                         pinmux = <GPIO145_C13_SSP0_RXD>;
> >                         ste,config = <&in_pd>;
> >                 };
> >                 snowball_cfg3 {
> >                         pinmux = <GPIO143_D12_SSP0_CLK GPIO146_D13_SSP0_TXD>;
> >                         ste,config = <&out_lo>;
> >                 };
> >         };
> 
> But this gives the false impression that pins can be muxed
> individually, and it makes it possible to write device trees that
> attempt to do so, while in practice it will not perform on the
> hardware.

If I understand the driver correctly on snowball (ab8500, right?) the
pins can be muxed individually. If you say that does not perform on the
hardware, that's something different. If a board designer decides to use
a pin northeast on the BGA and a pin southwest together for a single
I2C bus, then I as a device tree writer have no other choice but to
support this case, no matter if it's ideal or not. What's written in the
devicetree is dictated by the board designers, not the devicetree
writers.
I don't think board designers will create such a hardware just because
the Linux driver supports this. More likely they will create such a
hardware even though Linux does not support it ;)

> 
> >> >> One node for the multiplexing, one node for the config. This is the
> >> >> pattern used by most drivers, so I want to have this structure.
> >> >>
> >> >> It is also easy to tell one node from the other: if it contains "function"
> >> >> we know it's a multiplexing node, if it doesn't, it's a config node.
> >> >
> >> > So when merging these together a node is always multiplexing and
> >> > configuration. But do we really win anything if both are separated? When
> >> > both are separated you still need an array of pins in the nodes to
> >> > tell which pins this node is for. If this array also contains the
> >> > mux information then this won't hurt. You can still ignore it.
> >>
> >> Well we definately have to support the case with split config and
> >> muxing nodes at least. I am very worried that we would get into
> >> ambguities where that is not possible.
> >
> > Sure we have as we cannot change existing bindings,
> 
> For Nomadik I did, because there are no deployed systems suffering
> from it. I just had to use some board I had to make some kind of
> example. I would encourage any other system not deployed in the
> masses with flashed-in device trees to do the same as this is
> still somewhat in a flux.

In certain cases it may make sense to break existing device trees, I
just wanted to express that with any kind of generic binding I don't
want to enforce breaking existing bindings.

> 
> I am worried that there is something in your reasoning that sort of
> assumes all pin controllers mux pins one-by-one and not in groups.
> How do we make it impossible to write a device tree that also
> make hardware that do groupwise config viable without ambiguities?

Sorry, I don't understand this sentence. What do you mean here?

The bindings I suggested are for individual pin based controllers
only. I know there are group based controllers, but I don't want to
change their bindings. I believe there is no single binding that is
good for both types of controllers.

I think we must face it that individual pin based controllers are
different from group based controllers. That's the main difference
between different pin controllers and I think there are good reasons
to reflect that in the device tree.

You often talk about ambiguities. Could you give an example what
ambiguities you mean? I can't think of a situation where the device tree
is ambiguous. I can only think of a common device tree parser that
misinterpretes the device tree, but that would be a problem in the
implementation, not with the binding.

Note that the way we combine pin/mux in a single define is not new,
the i.MX pin controller uses this already and so far I'm not aware of
any problems this makes. I wouldn't integrate the pinconf settings
in the same define again though, but for this part we have the generic
pinconf bindings.

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 v3 2/3] dt-bindings: Add pinctrl bindings for mt65xx/mt81xx.

[Linus Walleij]

On Mon, Jan 12, 2015 at 1:22 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Sat, Jan 10, 2015 at 10:33:46PM +0100, Linus Walleij wrote:
>> On Tue, Dec 2, 2014 at 2:55 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:

>> > For the SSP0 it needs the string "ssp0_a_1" which is documented exactly
>> > nowhere.
>>
>> Is this a bug report about the documentation? I don't see how
>> that is relevant to the overall design.
>
> The best documentation is one that is not needed. I mandate to use
> defines with combinations of pin with mux setting to reduce the
> necessary documentation to: "Pick one (many) of these and you're done".
> So my criticism here is not mainly that there is no documentation but
> that the necessary documention would be very voluminous.

I don't know. I have since we discussed merged the long
overdue zynq driver that use this generic function+group mechanism.
The docs look like so:

Documentation/devicetree/bindings/pinctrl/xlnx,zynq-pinctrl.txt
(...)
Required properties for pinmux nodes are:
 - groups: A list of pinmux groups.
 - function: The name of a pinmux function to activate for the specified set
   of groups.

Required properties for configuration nodes:
One of:
 - pins: a list of pin names
 - groups: A list of pinmux groups.

The following generic properties as defined in pinctrl-bindings.txt are valid
to specify in a pinmux subnode:
 groups, function

The following generic properties as defined in pinctrl-bindings.txt are valid
to specify in a pinconf subnode:
 groups, pins, bias-disable, bias-high-impedance, bias-pull-up, slew-rate,
 low-power-disable, low-power-enable

 Valid arguments for 'slew-rate' are '0' and '1' to select between slow and fast
 respectively.

 Valid values for groups are:
   ethernet0_0_grp, ethernet1_0_grp, mdio0_0_grp, mdio1_0_grp,
   qspi0_0_grp, qspi1_0_grp, qspi_fbclk, qspi_cs1_grp, spi0_0_grp,
   spi0_1_grp - spi0_2_grp, spi1_0_grp - spi1_3_grp, sdio0_0_grp - sdio0_2_grp,
   sdio1_0_grp - sdio1_3_grp, sdio0_emio_wp, sdio0_emio_cd, sdio1_emio_wp,
   sdio1_emio_cd, smc0_nor, smc0_nor_cs1_grp, smc0_nor_addr25_grp, smc0_nand,
   can0_0_grp - can0_10_grp, can1_0_grp - can1_11_grp, uart0_0_grp -
uart0_10_grp,
   uart1_0_grp - uart1_11_grp, i2c0_0_grp - i2c0_10_grp, i2c1_0_grp -
i2c1_10_grp,
   ttc0_0_grp - ttc0_2_grp, ttc1_0_grp - ttc1_2_grp, swdt0_0_grp - swdt0_4_grp,
   gpio0_0_grp - gpio0_53_grp, usb0_0_grp, usb1_0_grp

 Valid values for pins are:
   MIO0 - MIO53

 Valid values for function are:
   ethernet0, ethernet1, mdio0, mdio1, qspi0, qspi1, qspi_fbclk, qspi_cs1,
   spi0, spi1, sdio0, sdio0_pc, sdio0_cd, sdio0_wp,
   sdio1, sdio1_pc, sdio1_cd, sdio1_wp,
   smc0_nor, smc0_nor_cs1, smc0_nor_addr25, smc0_nand, can0, can1, uart0, uart1,
   i2c0, i2c1, ttc0, ttc1, swdt0, gpio0, usb0, usb1

(...)

Example:
        pinctrl0: pinctrl@700 {
                compatible = "xlnx,pinctrl-zynq";
                reg = <0x700 0x200>;
                syscon = <&slcr>;

                pinctrl_uart1_default: uart1-default {
                        mux {
                                groups = "uart1_10_grp";
                                function = "uart1";
                        };

                        conf {
                                groups = "uart1_10_grp";
                                slew-rate = <0>;
                                io-standard = <1>;
                        };

                        conf-rx {
                                pins = "MIO49";
                                bias-high-impedance;
                        };

                        conf-tx {
                                pins = "MIO48";
                                bias-disable;
                        };
                };
        };


> Normally it
> must cover all possible combinations of pin/mux settings.

I think it's fairly intuitive to combine function uart1 with
group uart1_10_grp without documenting that this is a
valid combination. For complex stuff it may be complex,
but that is the nature of the complex hardware I think.

>> >         ssp0_snowball_mode: ssp0_snowball_default {
>> >                 snowball_cfg1 {
>> >                         pinmux = <GPIO144_B13_SSP0_FRM>;
>> >                         ste,config = <&gpio_out_hi>;
>> >                 };
>> >                 snowball_cfg2 {
>> >                         pinmux = <GPIO145_C13_SSP0_RXD>;
>> >                         ste,config = <&in_pd>;
>> >                 };
>> >                 snowball_cfg3 {
>> >                         pinmux = <GPIO143_D12_SSP0_CLK GPIO146_D13_SSP0_TXD>;
>> >                         ste,config = <&out_lo>;
>> >                 };
>> >         };
>>
>> But this gives the false impression that pins can be muxed
>> individually, and it makes it possible to write device trees that
>> attempt to do so, while in practice it will not perform on the
>> hardware.
>
> If I understand the driver correctly on snowball (ab8500, right?)

No that is drivers/pinctrl/nomadik/pinctrl-nomadik.c
and the db8500 subdriver pinctrl-nomadik-db8500.c

> the
> pins can be muxed individually.

Nope. They have individual registers per-pin, but if you try to
mux them in certain ways you will screw up the hardware or
even cause damage.

They also have to be reconfigured in batch in order to avoid
glitches on the lines, causing spurious IRQs & stuff.

So the driver has to restrict this by enforcing a groups concept
which is there in the hardware, but which is not visible in
the register map.

We have another driver under review, the Broadcom Cygnus.
This one configures a whole patch of pins with a single
register write and thus even reflects the non-one-register-per-pin
layout of the hardware in the register map.
http://marc.info/?l=linux-kernel&m=142113721817137&w=2

>> I am worried that there is something in your reasoning that sort of
>> assumes all pin controllers mux pins one-by-one and not in groups.
>> How do we make it impossible to write a device tree that also
>> make hardware that do groupwise config viable without ambiguities?
>
> Sorry, I don't understand this sentence. What do you mean here?
>
> The bindings I suggested are for individual pin based controllers
> only. I know there are group based controllers, but I don't want to
> change their bindings. I believe there is no single binding that is
> good for both types of controllers.
>
> I think we must face it that individual pin based controllers are
> different from group based controllers. That's the main difference
> between different pin controllers and I think there are good reasons
> to reflect that in the device tree.

OK let's work on a binding for this usecase.

> You often talk about ambiguities. Could you give an example what
> ambiguities you mean?

What happened was this pins = ; arguments were sometimes
strings and sometimes integers, that becomes strange to handle
in code, ambiguous.

I'm fuzzily referring to the concept of things being named the
same way in different device trees, yet lacking commonality,
confusing a human reader that they may be the same thing,
even if it is possible to write schemas and parsers handling
it unambigously, so not ambiguity in the formal logic sense.

If i later want to refactor the code around this to a central
parser I cannot do so because it would lead to formal ambiguities
and is non-doable.

> Note that the way we combine pin/mux in a single define is not new,
> the i.MX pin controller uses this already and so far I'm not aware of
> any problems this makes.

Yeah we never had time to sit down and come up with proper
generic pin control bindings, we went with custom bindings
partly because of general disagreements, partly because I
was new to device tree and honestly had no idea of how
to skin this cat.

Now that we get to formalize generic bindings for DT and
ACPI and whatever alike, I prefer if we make both groupwise
and per-pin pin controllers as strict and well defined as
possible.

One minor problem I have with using an integer for mux config
is that it assumes something about how many pins, configs etc
that may exist on such a system. This should be stated
explicitly in the bindings atleast so we know what restrictions
we build into them. String-based function+group matching has
no such restrictions.

Yours,
Linus Walleij

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

[Sascha Hauer]

On Tue, Jan 13, 2015 at 11:05:22AM +0100, Linus Walleij wrote:
> >> I am worried that there is something in your reasoning that sort of
> >> assumes all pin controllers mux pins one-by-one and not in groups.
> >> How do we make it impossible to write a device tree that also
> >> make hardware that do groupwise config viable without ambiguities?
> >
> > Sorry, I don't understand this sentence. What do you mean here?
> >
> > The bindings I suggested are for individual pin based controllers
> > only. I know there are group based controllers, but I don't want to
> > change their bindings. I believe there is no single binding that is
> > good for both types of controllers.
> >
> > I think we must face it that individual pin based controllers are
> > different from group based controllers. That's the main difference
> > between different pin controllers and I think there are good reasons
> > to reflect that in the device tree.
> 
> OK let's work on a binding for this usecase.

Okay.

> 
> > You often talk about ambiguities. Could you give an example what
> > ambiguities you mean?
> 
> What happened was this pins = ; arguments were sometimes
> strings and sometimes integers, that becomes strange to handle
> in code, ambiguous.

I see. I like naming it 'pinmux' because that's what it is: pins and
mux settings. A plain 'pinno' suggests that it contains only pin mubers,
without mux setting. How about 'pin-no-mux'? We also could add an
explicit "pins-are-numbered" property instead of distinguishing this
by property names.

> 
> I'm fuzzily referring to the concept of things being named the
> same way in different device trees, yet lacking commonality,
> confusing a human reader that they may be the same thing,
> even if it is possible to write schemas and parsers handling
> it unambigously, so not ambiguity in the formal logic sense.
> 
> If i later want to refactor the code around this to a central
> parser I cannot do so because it would lead to formal ambiguities
> and is non-doable.

There could be a flag in the pinctroller struct indicating whether the
properties are to be interpreted as strings or as numbers.

> 
> > Note that the way we combine pin/mux in a single define is not new,
> > the i.MX pin controller uses this already and so far I'm not aware of
> > any problems this makes.
> 
> Yeah we never had time to sit down and come up with proper
> generic pin control bindings, we went with custom bindings
> partly because of general disagreements, partly because I
> was new to device tree and honestly had no idea of how
> to skin this cat.
> 
> Now that we get to formalize generic bindings for DT and
> ACPI and whatever alike, I prefer if we make both groupwise
> and per-pin pin controllers as strict and well defined as
> possible.
> 
> One minor problem I have with using an integer for mux config
> is that it assumes something about how many pins, configs etc
> that may exist on such a system. This should be stated
> explicitly in the bindings atleast so we know what restrictions
> we build into them. String-based function+group matching has
> no such restrictions.

No problem, that can be documented. Normally the defines should be used
anyway, not the plain pin numbers.

BTW one thing I really like about integers is the pure binary size. In
barebox I also parse the pinmux settings from the device tree. The
drivers using string matching are multiple times bigger due to the
string tables:

-rw-r--r-- 1 sha ptx  5436 Jan 13 15:00 imx-iomux-v3.o
-rw-r--r-- 1 sha ptx 42060 Jan 13 15:00 pinctrl-tegra30.o

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 v3 2/3] dt-bindings: Add pinctrl bindings for mt65xx/mt81xx.

[Jean-Christophe PLAGNIOL-VILLARD]

> On Jan 14, 2015, at 12:16 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> 
> On Tue, Jan 13, 2015 at 11:05:22AM +0100, Linus Walleij wrote:
>>>> I am worried that there is something in your reasoning that sort of
>>>> assumes all pin controllers mux pins one-by-one and not in groups.
>>>> How do we make it impossible to write a device tree that also
>>>> make hardware that do groupwise config viable without ambiguities?
>>> 
>>> Sorry, I don't understand this sentence. What do you mean here?
>>> 
>>> The bindings I suggested are for individual pin based controllers
>>> only. I know there are group based controllers, but I don't want to
>>> change their bindings. I believe there is no single binding that is
>>> good for both types of controllers.
>>> 
>>> I think we must face it that individual pin based controllers are
>>> different from group based controllers. That's the main difference
>>> between different pin controllers and I think there are good reasons
>>> to reflect that in the device tree.
>> 
>> OK let's work on a binding for this usecase.
> 
> Okay.
> 
>> 
>>> You often talk about ambiguities. Could you give an example what
>>> ambiguities you mean?
>> 
>> What happened was this pins = ; arguments were sometimes
>> strings and sometimes integers, that becomes strange to handle
>> in code, ambiguous.
> 
> I see. I like naming it 'pinmux' because that's what it is: pins and
> mux settings. A plain 'pinno' suggests that it contains only pin mubers,
> without mux setting. How about 'pin-no-mux'? We also could add an
> explicit "pins-are-numbered" property instead of distinguishing this
> by property names.
> 
>> 
>> I'm fuzzily referring to the concept of things being named the
>> same way in different device trees, yet lacking commonality,
>> confusing a human reader that they may be the same thing,
>> even if it is possible to write schemas and parsers handling
>> it unambigously, so not ambiguity in the formal logic sense.
>> 
>> If i later want to refactor the code around this to a central
>> parser I cannot do so because it would lead to formal ambiguities
>> and is non-doable.
> 
> There could be a flag in the pinctroller struct indicating whether the
> properties are to be interpreted as strings or as numbers.
> 
>> 
>>> Note that the way we combine pin/mux in a single define is not new,
>>> the i.MX pin controller uses this already and so far I'm not aware of
>>> any problems this makes.
>> 
>> Yeah we never had time to sit down and come up with proper
>> generic pin control bindings, we went with custom bindings
>> partly because of general disagreements, partly because I
>> was new to device tree and honestly had no idea of how
>> to skin this cat.
>> 
>> Now that we get to formalize generic bindings for DT and
>> ACPI and whatever alike, I prefer if we make both groupwise
>> and per-pin pin controllers as strict and well defined as
>> possible.
>> 
>> One minor problem I have with using an integer for mux config
>> is that it assumes something about how many pins, configs etc
>> that may exist on such a system. This should be stated
>> explicitly in the bindings atleast so we know what restrictions
>> we build into them. String-based function+group matching has
>> no such restrictions.
> 
> No problem, that can be documented. Normally the defines should be used
> anyway, not the plain pin numbers.
> 
> BTW one thing I really like about integers is the pure binary size. In
> barebox I also parse the pinmux settings from the device tree. The
> drivers using string matching are multiple times bigger due to the
> string tables:
> 
> -rw-r--r-- 1 sha ptx  5436 Jan 13 15:00 imx-iomux-v3.o
> -rw-r--r-- 1 sha ptx 42060 Jan 13 15:00 pinctrl-tegra30.o

Agreed with Sascha that’s why I chose integer for at91 too

if you want string just use define instead to make it more readable

Best Regards,
J.
> 
> 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 |
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Linus Walleij]

On Tue, Jan 13, 2015 at 5:16 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Tue, Jan 13, 2015 at 11:05:22AM +0100, Linus Walleij wrote:

>> > You often talk about ambiguities. Could you give an example what
>> > ambiguities you mean?
>>
>> What happened was this pins = ; arguments were sometimes
>> strings and sometimes integers, that becomes strange to handle
>> in code, ambiguous.
>
> I see. I like naming it 'pinmux' because that's what it is: pins and
> mux settings. A plain 'pinno' suggests that it contains only pin mubers,
> without mux setting. How about 'pin-no-mux'? We also could add an
> explicit "pins-are-numbered" property instead of distinguishing this
> by property names.

I kind of like this "pins-are-numbered" thing.

The other property for the pin, whether pinmux or pin-no-mux or
pin-num-and-mux etc is no such big deal, as long as it's
consistent and documented with the generic bindings.

Yours,
Linus Walleij

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

[Yingjoe Chen]

On Fri, 2015-01-16 at 10:53 +0100, Linus Walleij wrote:
> On Tue, Jan 13, 2015 at 5:16 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > On Tue, Jan 13, 2015 at 11:05:22AM +0100, Linus Walleij wrote:
> 
> >> > You often talk about ambiguities. Could you give an example what
> >> > ambiguities you mean?
> >>
> >> What happened was this pins = ; arguments were sometimes
> >> strings and sometimes integers, that becomes strange to handle
> >> in code, ambiguous.
> >
> > I see. I like naming it 'pinmux' because that's what it is: pins and
> > mux settings. A plain 'pinno' suggests that it contains only pin mubers,
> > without mux setting. How about 'pin-no-mux'? We also could add an
> > explicit "pins-are-numbered" property instead of distinguishing this
> > by property names.
> 
> I kind of like this "pins-are-numbered" thing.
> 
> The other property for the pin, whether pinmux or pin-no-mux or
> pin-num-and-mux etc is no such big deal, as long as it's
> consistent and documented with the generic bindings.

Hi Linus,

To make sure I understand it correct, you think something like this is
OK?

	pinctrl@01c20800 {
		compatible = "mediatek,mt8135-pinctrl";
[...]
		pins-are-numbered;

		i2c0_pins_a: i2c0@0 {
			pins1 {
				pins = <MT8135_PIN_100_SDA0__FUNC_SDA0>,
					<MT8135_PIN_101_SCL0__FUNC_SCL0>;
				bias-disable;
			};
		};
[....]
	}

Joe.C

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

[Linus Walleij]

On Fri, Jan 16, 2015 at 11:23 AM, Yingjoe Chen
<yingjoe.chen@mediatek.com> wrote:
> On Fri, 2015-01-16 at 10:53 +0100, Linus Walleij wrote:
>> On Tue, Jan 13, 2015 at 5:16 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
>> > On Tue, Jan 13, 2015 at 11:05:22AM +0100, Linus Walleij wrote:
>>
>> >> > You often talk about ambiguities. Could you give an example what
>> >> > ambiguities you mean?
>> >>
>> >> What happened was this pins = ; arguments were sometimes
>> >> strings and sometimes integers, that becomes strange to handle
>> >> in code, ambiguous.
>> >
>> > I see. I like naming it 'pinmux' because that's what it is: pins and
>> > mux settings. A plain 'pinno' suggests that it contains only pin mubers,
>> > without mux setting. How about 'pin-no-mux'? We also could add an
>> > explicit "pins-are-numbered" property instead of distinguishing this
>> > by property names.
>>
>> I kind of like this "pins-are-numbered" thing.
>>
>> The other property for the pin, whether pinmux or pin-no-mux or
>> pin-num-and-mux etc is no such big deal, as long as it's
>> consistent and documented with the generic bindings.
>
> Hi Linus,
>
> To make sure I understand it correct, you think something like this is
> OK?
>
>         pinctrl@01c20800 {
>                 compatible = "mediatek,mt8135-pinctrl";
> [...]
>                 pins-are-numbered;
>
>                 i2c0_pins_a: i2c0@0 {
>                         pins1 {
>                                 pins = <MT8135_PIN_100_SDA0__FUNC_SDA0>,
>                                         <MT8135_PIN_101_SCL0__FUNC_SCL0>;
>                                 bias-disable;
>                         };
>                 };

As discussed with Sascha Hauer it is ambigous to use "pins" for
a numerical value indicating both a mux setting and a pin. Sascha
suggests using "pinmux" and adding this as a secondary generic
binding for this type of pin controllers that use numbers and #defines
to set up bindings.

We should still move these parsing functions to the core.

See this discussion earlier in this thread:
http://marc.info/?l=linux-kernel&m=142116581226500&w=2

Yours,
Linus Walleij

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

[Sascha Hauer]

Hi Linus,

On Tue, Jan 20, 2015 at 10:45:09AM +0100, Linus Walleij wrote:
> On Fri, Jan 16, 2015 at 11:23 AM, Yingjoe Chen
> <yingjoe.chen@mediatek.com> wrote:
> > On Fri, 2015-01-16 at 10:53 +0100, Linus Walleij wrote:
> >> On Tue, Jan 13, 2015 at 5:16 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> >> > On Tue, Jan 13, 2015 at 11:05:22AM +0100, Linus Walleij wrote:
> >>
> >> >> > You often talk about ambiguities. Could you give an example what
> >> >> > ambiguities you mean?
> >> >>
> >> >> What happened was this pins = ; arguments were sometimes
> >> >> strings and sometimes integers, that becomes strange to handle
> >> >> in code, ambiguous.
> >> >
> >> > I see. I like naming it 'pinmux' because that's what it is: pins and
> >> > mux settings. A plain 'pinno' suggests that it contains only pin mubers,
> >> > without mux setting. How about 'pin-no-mux'? We also could add an
> >> > explicit "pins-are-numbered" property instead of distinguishing this
> >> > by property names.
> >>
> >> I kind of like this "pins-are-numbered" thing.
> >>
> >> The other property for the pin, whether pinmux or pin-no-mux or
> >> pin-num-and-mux etc is no such big deal, as long as it's
> >> consistent and documented with the generic bindings.
> >
> > Hi Linus,
> >
> > To make sure I understand it correct, you think something like this is
> > OK?
> >
> >         pinctrl@01c20800 {
> >                 compatible = "mediatek,mt8135-pinctrl";
> > [...]
> >                 pins-are-numbered;
> >
> >                 i2c0_pins_a: i2c0@0 {
> >                         pins1 {
> >                                 pins = <MT8135_PIN_100_SDA0__FUNC_SDA0>,
> >                                         <MT8135_PIN_101_SCL0__FUNC_SCL0>;
> >                                 bias-disable;
> >                         };
> >                 };
> 
> As discussed with Sascha Hauer it is ambigous to use "pins" for
> a numerical value indicating both a mux setting and a pin. Sascha
> suggests using "pinmux" and adding this as a secondary generic
> binding for this type of pin controllers that use numbers and #defines
> to set up bindings.
> 
> We should still move these parsing functions to the core.

I tried that for the last few days and failed.

Part of the problem is that the core lacks the data structures to put
the information in. There is

struct pinctrl_map_mux {
	const char *group;
	const char *function;
};

but its meaning is SoC specific. Some SoCs combine the pins found in a
dt subnode to one group (i.MX, rockchip, at91) while others make an
individual group from each single pin (Tegra, others?). Also the
function string is SoC specific. Some SoCs just define functions like
"alt1".."altn" which are valid for all groups, others define different
function names for each group.

Another thing is that the binding gives us numbers, but struct
pinctrl_map_mux expects strings, so the numbers would have to be
converted to strings. This is crude since the contents of struct
pinctrl_map_mux are converted from strings back to numbers later from
the pinctrl core with the help of the driver. So we would have to
translate the numbers from the bindings to strings just to convert them
back to numbers before passing them to the driver later.

Given that the best I can come up with is something like:

int pinctrl_parse_pinmux(struct device_node *np, int index,
			 unsigned int *pinno, unsigned int *funcno)
{
	u32 val;
	int ret;

	ret = of_property_read_u32_index(np, "pinmux", index, &val);
	if (ret)
		return ret;

	*pinno = val >> 8
	*funcno = val & 0xff;

	return 0;
}

Is that what you expect from a common parser?

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 v3 2/3] dt-bindings: Add pinctrl bindings for mt65xx/mt81xx.

[Linus Walleij]

On Mon, Jan 26, 2015 at 4:57 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Tue, Jan 20, 2015 at 10:45:09AM +0100, Linus Walleij wrote:

>> As discussed with Sascha Hauer it is ambigous to use "pins" for
>> a numerical value indicating both a mux setting and a pin. Sascha
>> suggests using "pinmux" and adding this as a secondary generic
>> binding for this type of pin controllers that use numbers and #defines
>> to set up bindings.
>>
>> We should still move these parsing functions to the core.
>
> I tried that for the last few days and failed.
>
> Part of the problem is that the core lacks the data structures to put
> the information in. There is
>
> struct pinctrl_map_mux {
>         const char *group;
>         const char *function;
> };
>
> but its meaning is SoC specific. Some SoCs combine the pins found in a
> dt subnode to one group (i.MX, rockchip, at91) while others make an
> individual group from each single pin (Tegra, others?). Also the
> function string is SoC specific. Some SoCs just define functions like
> "alt1".."altn" which are valid for all groups, others define different
> function names for each group.
>
> Another thing is that the binding gives us numbers, but struct
> pinctrl_map_mux expects strings, so the numbers would have to be
> converted to strings. This is crude since the contents of struct
> pinctrl_map_mux are converted from strings back to numbers later from
> the pinctrl core with the help of the driver. So we would have to
> translate the numbers from the bindings to strings just to convert them
> back to numbers before passing them to the driver later.

So can we use a union?

struct pinctrl_map_mux {
        union group {
              const char *groupstr;
              u16 groupno;
        };
        union function {
              const char *functionstr;
              u16 funcno;
        };
};

The augment the code to reference foo->group.groupstr or
foo->group.groupno etc depending on runpath.

Yours,
Linus Walleij