Add single register pin controller driver
Subsystems (2)
PATCH v2 1/1 Add single register pin controller driver
This patch adds a pin controller driver supporting devices using a single configuration register per pin.
---
Changes in v2:
- add a comment on function single_configure_pins() explaining the function and its parameters
- use fdt_getprop() to simplify retrieval of property 'pinctrl-single,pins' from FDT
- change the return value of single_ofdata_to_platdata() to -EINVAL in case of failure
- change the driver name from pcs_pinctrl to single_pinctrl
- minor reformatting to comply with coding style
drivers/pinctrl/Kconfig | 10 +++ drivers/pinctrl/Makefile | 1 + drivers/pinctrl/pinctrl-single.c | 143 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 154 insertions(+) create mode 100644 drivers/pinctrl/pinctrl-single.c
drivers/pinctrl/Kconfig +10 -0
@@ -175,6 +175,16 @@ config PIC32_PINCTRL by a device tree node which contains both GPIO defintion and pin control functions. +config PINCTRL_SINGLE + bool "Single register pin-control and pin-multiplex driver" + depends on DM + help + This enables pinctrl driver for systems using a single register for + pin configuration and multiplexing. TI's AM335X SoCs are examples of + such systems. + Depending on the platform make sure to also enable OF_TRANSLATE and + eventually SPL_OF_TRANSLATE to get correct address translations. + endif source "drivers/pinctrl/meson/Kconfig"
drivers/pinctrl/Makefile +1 -0
@@ -16,3 +16,4 @@ obj-$(CONFIG_PIC32_PINCTRL) += pinctrl_pic32.o obj-$(CONFIG_PINCTRL_EXYNOS) += exynos/ obj-$(CONFIG_PINCTRL_MESON) += meson/ obj-$(CONFIG_PINCTRL_MVEBU) += mvebu/ +obj-$(CONFIG_PINCTRL_SINGLE) += pinctrl-single.o
drivers/pinctrl/pinctrl-single.c new file +143 -0
143 lines not on this page · raw message
Signature · 1 line
2.7.4
PATCH v2 1/1 Add single register pin controller driver
Hi.
2017-02-07 22:30 GMT+09:00 Felix Brack <[email protected]>:
drivers/pinctrl/pinctrl-single.c line 82
Quoted 9 lines
drivers/pinctrl/pinctrl-single.c new file +9
-0+ +static int single_set_state_simple (struct udevice *dev, +struct udevice *periph) +{ +const void *fdt = gd->fdt_blob; +const struct single_fdt_pin_cfg *prop; +int len; + + prop =fdt_getprop (fdt, periph->of_offset,"pinctrl-single,pins" , &len);
This seems wrong to me.
The "periph" is a peripheral device (like UART, eMMC, USB, etc.).
So, you are parsing the "pinctrl-single,pins" property in the peripheral device, like
uart: uart {
pinctrl-single,pins = <
.....
>;
};In pinctrl, peripheral nodes should have a phandle to a child of the pinctrl device.
As you see Linux, the DT should look like this:
uart: uart {
pinctrl-0 = <&uart_pins>;
};
pinctrl {
uart_pins: uart_pins {
pinctrl-single,pins = <
....
>;
};
};Signature · 2 lines
Best Regards Masahiro Yamada
PATCH v2 1/1 Add single register pin controller driver
Hello Masahiro,
This is my understanding of the parameters 'dev' and 'perihp' passed to the function implementing set_state_simple().
Quoted 20 lines · Masahiro Yamada
Hi.
2017-02-07 22:30 GMT+09:00 Felix Brack <[email protected]>:
drivers/pinctrl/pinctrl-single.c line 82
Quoted 9 lines
drivers/pinctrl/pinctrl-single.c new file +9
-0+ +static int single_set_state_simple (struct udevice *dev, +struct udevice *periph) +{ +const void *fdt = gd->fdt_blob; +const struct single_fdt_pin_cfg *prop; +int len; + + prop =fdt_getprop (fdt, periph->of_offset,"pinctrl-single,pins" , &len);This seems wrong to me.
Quoted 2 lines
The "periph" is a peripheral device (like UART, eMMC, USB, etc.).
In the case above 'dev' represents the pin controller node of the DT and 'periph' represents the DT node holding the pin configuration information for a specific peripheral like i2c0, not the peripheral itself. I know that neither 'dev' nor 'periph' _are_ device tree nodes objects. This is why I use the word 'represent'.
Quoted 12 lines
So, you are parsing the "pinctrl-single,pins" property in the peripheral device, like
uart: uart { pinctrl-single,pins = < ..... >; };
I don't think so (see below).
Quoted 7 lines
In pinctrl, peripheral nodes should have a phandle to a child of the pinctrl device.
As you see Linux, the DT should look like this:
Quoted 18 lines
uart: uart {
pinctrl-0 = <&uart_pins>;
};
pinctrl {
uart_pins: uart_pins {
pinctrl-single,pins = <
....
>;
};
};This is how it works. single_set_state_simple() would be called with 'dev' representing 'pinctrl' node and 'periph' representing 'uart_pins' node. I would have to parse 'periph' for 'pinctrl-single,pins'.
Quoted 2 lines
I use this pin controller on a AM335x based board. Here are the relevant parts from the device tree.
From 'am33xx.dtsi', representing the pin controller itself:
am33xx_pinmux: pinmux at 800 {
compatible = "pinctrl-single";
reg = <0x800 0x238>;
...
}From a a device tree source file, representing the configuration of the pins for the peripheral i2c0:
&am33xx_pinmux {
pinctrl-names = "default";
pinctrl-0 = <&i2c0_pins>;
i2c0_pins: pinmux_i2c0_pins {
pinctrl-single,pins = <
AM33XX_IOPAD(0x988, PIN_INPUT_PULLUP | MUX_MODE0) /* i2c0_sda.i2c0_sda */
AM33XX_IOPAD(0x98c, PIN_INPUT_PULLUP | MUX_MODE0) /* i2c0_scl.i2c0_scl */
>;
};
};With this, single_set_state_simple() gets called with 'dev' representing 'pinmux at 800' and 'periph' representing 'pinmux_i2c0_pins' which makes perfect sense to me.
Again I would have to parse 'periph' for 'pinctrl-single,pins'.
regards Felix
PATCH v2 1/1 Add single register pin controller driver
Hi.
Quoted 16 lines
Quoted 16 lines
2017-02-07 22:30 GMT+09:00 Felix Brack <[email protected]>:
drivers/pinctrl/pinctrl-single.c line 82
Quoted 9 lines
drivers/pinctrl/pinctrl-single.c new file +9
-0+ +static int single_set_state_simple (struct udevice *dev, +struct udevice *periph) +{ +const void *fdt = gd->fdt_blob; +const struct single_fdt_pin_cfg *prop; +int len; + + prop =fdt_getprop (fdt, periph->of_offset,"pinctrl-single,pins" , &len);This seems wrong to me.
Quoted 7 lines
Quoted 2 lines
The "periph" is a peripheral device (like UART, eMMC, USB, etc.).
In the case above 'dev' represents the pin controller node of the DT and 'periph' represents the DT node holding the pin configuration information for a specific peripheral like i2c0, not the peripheral itself.
I recommend you to read pinctrl-uclass.c once again.
Did you track how "periph" was passed across the following function calls?
device_probe()
-> pinctrl_select_state()
-> pinctrl_select_state_simple()
-> ops->set_state_simple()Signature · 2 lines
Best Regards Masahiro Yamada
PATCH v2 1/1 Add single register pin controller driver
Hello Masahiro,
Quoted 22 lines · Masahiro Yamada
Hi.
Quoted 19 lines
Quoted 18 lines
2017-02-07 22:30 GMT+09:00 Felix Brack <[email protected]>:
drivers/pinctrl/pinctrl-single.c line 82
Quoted 9 lines
drivers/pinctrl/pinctrl-single.c new file +9
-0+ +static int single_set_state_simple (struct udevice *dev, +struct udevice *periph) +{ +const void *fdt = gd->fdt_blob; +const struct single_fdt_pin_cfg *prop; +int len; + + prop =fdt_getprop (fdt, periph->of_offset,"pinctrl-single,pins" , &len);This seems wrong to me.
The "periph" is a peripheral device (like UART, eMMC, USB, etc.).
Quoted 8 lines
Quoted 4 lines
In the case above 'dev' represents the pin controller node of the DT and 'periph' represents the DT node holding the pin configuration information for a specific peripheral like i2c0, not the peripheral itself.
I recommend you to read
pinctrl-uclass.conce again.
I did.
Quoted 3 lines
Did you track how "periph" was passed across the following function calls?
I did.
Quoted 1 line
device_probe()
From 'pinctrl-uclass.c::device_probe()':
---
/*
* Process pinctrl for everything except the root device, and
* continue regardless of the result of pinctrl. Don't process pinctrl
* settings for pinctrl devices since the device may not yet be
* probed.
*/
if (dev->parent && device_get_uclass_id(dev) != UCLASS_PINCTRL)
pinctrl_select_state(dev, "default");
---
The comment says it all. This is where 'periph' originates.
Quoted 5 lines
->
pinctrl_select_state()-> pinctrl_select_state_simple() -> ops->set_state_simple()
Also the outcome reported by 'dm tree' makes sense to me:
Class Probed Name
----------------------------------------
root [ + ] root_driver simple_bus [ + ] `-- ocp simple_bus [ + ] |-- l4_wkup at 44c00000 simple_bus [ + ] | `-- scm at 210000 pinctrl [ + ] | `-- pinmux at 800 pinconfig [ + ] | |-- pinmux_i2c0_pins pinconfig [ + ] | |-- pinmux_i2c1_pins pinconfig [ ] | |-- pinmux_i2c2_pins pinconfig [ + ] | |-- pinmux_spi1_pins pinconfig [ ] | |-- pinmux_uart0_pins pinconfig [ ] | |-- pinmux_uart1_pins pinconfig [ + ] | |-- pinmux_uart3_pins pinconfig [ + ] | |-- pinmux_clkout2_pin pinconfig [ ] | |-- cpsw_default pinconfig [ ] | |-- davinci_mdio_default pinconfig [ + ] | |-- pinmux_mmc1_pins pinconfig [ + ] | |-- pinmux_mmc2_pins pinconfig [ ] | |-- lcd_pins_s0 pinconfig [ ] | `-- pinmux_dcan0_pins serial [ ] |-- serial at 44e09000 serial [ ] |-- serial at 48022000 serial [ + ] |-- serial at 481a6000 i2c [ ] |-- i2c at 44e0b000 i2c [ + ] |-- i2c at 4802a000 i2c_generic [ + ] | |-- generic_60 i2c_generic [ + ] | `-- generic_50 i2c [ ] `-- i2c at 4819c000
regards Felix
PATCH v2 1/1 Add single register pin controller driver
Hi Felix,
2017-02-09 18:47 GMT+09:00 Felix Brack <[email protected]>:
Quoted 13 lines
Quoted 2 lines
device_probe()From 'pinctrl-uclass.c::device_probe()':
---
/*
* Process pinctrl for everything except the root device, and
* continue regardless of the result of pinctrl. Don't process pinctrl
* settings for pinctrl devices since the device may not yet be
* probed.
*/
if (dev->parent&&device_get_uclass_id(dev)!=UCLASS_PINCTRL)
Quoted 4 lines
pinctrl_select_state(dev, "default");---
The comment says it all. This is where 'periph' originates.
That's right.
Then, the "dev" in device_probe() represents
the peripheral, such as i2c.
Not a pin-config node, like i2c0_pins.
Signature · 2 lines
Best Regards Masahiro Yamada
PATCH v2 1/1 Add single register pin controller driver
Hi All,
2017-02-10 13:35 GMT+09:00 Masahiro Yamada <[email protected]>:
Quoted 5 lines
That's right.
Then, the "dev" indevice_probe()represents
the peripheral, such as i2c.Not a pin-config node, like
i2c0_pins.
Masahiro, have a look at the call to set_state_simple from pinctrl_select_state_simple (in drivers/pinctrl/pinctrl-uclass.c). It is at this point that the pinctrl node becomes the 'dev' parameter, and the former 'dev' then becomes the second 'periph' parameter.
---
/*
* For simplicity, assume the first device of PINCTRL uclass
* is the correct one. This is most likely OK as there is
* usually only one pinctrl device on the system. */
ret = uclass_get_device(UCLASS_PINCTRL, 0, &pctldev); if (ret)
return ret;
ops = pinctrl_get_ops(pctldev);
if (!ops->set_state_simple) {
dev_dbg(dev, "set_state_simple op missing\n");
return -ENOSYS;
}return ops->set_state_simple(pctldev, dev);
---
--
Kind regards,
James Balean