Simon,
I've just posted v4.
Sorry for the delay.
2015-08-12 23:16 GMT+09:00 Simon Glass sjg@chromium.org:
Hi Masahiro,
On 10 August 2015 at 10:05, Masahiro Yamada yamada.masahiro@socionext.com wrote:
This creates a new framework for handling of pin control devices, i.e. devices that control different aspects of package pins.
This uclass handles pinmuxing and pin configuration; pinmuxing controls switching among silicon blocks that share certain physical pins, pin configuration handles electronic properties such as pin- biasing, load capacitance etc.
This framework supports the same device tree bindings, but if you do not need full interface support, you can disable some features to reduce memory foot print.
Signed-off-by: Masahiro Yamada yamada.masahiro@socionext.com
drivers/Kconfig | 2 + drivers/Makefile | 1 + drivers/core/device.c | 4 + drivers/pinctrl/Kconfig | 42 +++++ drivers/pinctrl/Makefile | 2 + drivers/pinctrl/pinctrl-generic.c | 351 ++++++++++++++++++++++++++++++++++++++ drivers/pinctrl/pinctrl-uclass.c | 151 ++++++++++++++++ include/dm/pinctrl.h | 218 +++++++++++++++++++++++ include/dm/uclass-id.h | 2 + 9 files changed, 773 insertions(+) create mode 100644 drivers/pinctrl/Kconfig create mode 100644 drivers/pinctrl/Makefile create mode 100644 drivers/pinctrl/pinctrl-generic.c create mode 100644 drivers/pinctrl/pinctrl-uclass.c create mode 100644 include/dm/pinctrl.h
diff --git a/drivers/Kconfig b/drivers/Kconfig index 092bc02..2b9933f 100644 --- a/drivers/Kconfig +++ b/drivers/Kconfig @@ -32,6 +32,8 @@ source "drivers/i2c/Kconfig"
source "drivers/spi/Kconfig"
+source "drivers/pinctrl/Kconfig"
source "drivers/gpio/Kconfig"
source "drivers/power/Kconfig" diff --git a/drivers/Makefile b/drivers/Makefile index a721ec8..9d0a595 100644 --- a/drivers/Makefile +++ b/drivers/Makefile @@ -1,6 +1,7 @@ obj-$(CONFIG_$(SPL_)DM) += core/ obj-$(CONFIG_$(SPL_)CLK) += clk/ obj-$(CONFIG_$(SPL_)LED) += led/ +obj-$(CONFIG_$(SPL_)PINCTRL) += pinctrl/ obj-$(CONFIG_$(SPL_)RAM) += ram/
ifdef CONFIG_SPL_BUILD diff --git a/drivers/core/device.c b/drivers/core/device.c index 634070c..767b7fe 100644 --- a/drivers/core/device.c +++ b/drivers/core/device.c @@ -15,6 +15,7 @@ #include <dm/device.h> #include <dm/device-internal.h> #include <dm/lists.h> +#include <dm/pinctrl.h> #include <dm/platdata.h> #include <dm/uclass.h> #include <dm/uclass-internal.h> @@ -277,6 +278,9 @@ int device_probe_child(struct udevice *dev, void *parent_priv)
dev->flags |= DM_FLAG_ACTIVATED;
/* continue regardless of the result of pinctrl */pinctrl_select_state(dev, "default");ret = uclass_pre_probe_device(dev); if (ret) goto fail;diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig new file mode 100644 index 0000000..3f4f4b3 --- /dev/null +++ b/drivers/pinctrl/Kconfig @@ -0,0 +1,42 @@ +# +# PINCTRL infrastructure and drivers +#
+menu "Pin controllers"
+config PINCTRL
bool "Support pin controllers"+config PINCTRL_GENERIC
bool "Support generic pin controllers"depends on PINCTRLCan you add some help for these - explaining what each one means and why to enable it.
Done.
Feel free to add more detailed explanations you think is necessary. http://patchwork.ozlabs.org/patch/510080/
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile new file mode 100644 index 0000000..a713c7d --- /dev/null +++ b/drivers/pinctrl/Makefile
Does this need a license header?
I do not think it is important for Makefiles.
+/**
- pinctrl_pin_name_to_selector() - return the pin selector for a pin
- @dev: pin controller device
- @pin: the pin name to look up
@return, please fix globally.
Done.
- */
+static int pinctrl_pin_name_to_selector(struct udevice *dev, const char *pin) +{
const struct pinctrl_ops *ops = dev->driver->ops;Please create a #define for this as with spi.h for example.
Done.
unsigned npins, selector = 0;if (!ops->get_pins_count || !ops->get_pin_name) {dev_err(dev, "get_pins_count or get_pin_name missing\n");Should this be debug() or dev_warn()? It would be nice to compile these out unless debugging.
I chose dev_dbg().
return -EINVAL;That is normally used for an invalid device tree arg. How about -ENOSYS?
This is the comment block in U-Boot:
#define ENOSYS 38 /* Function not implemented */
And this is the one in Linux:
/* * This error code is special: arch syscall entry code will return * -ENOSYS if users try to call a syscall that doesn't exist. To keep * failures of syscalls that really do exist distinguishable from * failures due to attempts to use a nonexistent syscall, syscall * implementations should refrain from returning -ENOSYS. */ #define ENOSYS 38 /* Invalid system call number */
From the comment above, I hesitate to use -ENOSYS for normal error cases.
I chose ENOTSUPP for unimplemented functions and it is widely used in Linux's pinctrl drivers.
}npins = ops->get_pins_count(dev);/* See if this pctldev has this pin */while (selector < npins) {How about:
for (selector = 0; selector < npins; selected++)
Good idea. Done.
const char *pname = ops->get_pin_name(dev, selector);if (!strcmp(pin, pname))return selector;selector++;}return -EINVAL;+}
+/**
- pinctrl_group_name_to_selector() - return the group selector for a group
- @dev: pin controller device
- @group: the pin group name to look up
@return
Done.
- */
+static int pinctrl_group_name_to_selector(struct udevice *dev,
const char *group)+{
const struct pinctrl_ops *ops = dev->driver->ops;unsigned ngroups, selector = 0;if (!ops->get_groups_count || !ops->get_group_name) {dev_err(dev, "get_groups_count or get_group_name missing\n");return -EINVAL;}ngroups = ops->get_groups_count(dev);/* See if this pctldev has this group */while (selector < ngroups) {const char *gname = ops->get_group_name(dev, selector);if (!strcmp(group, gname))return selector;selector++;}return -EINVAL;I think this means that the device tree is missing something, in which case this is fine. If not you might consider -ENOENT.
Actually, the pinctrl driver subsystem returns -EINVAL in this case (see linux/drivers/pinctrl/pinmux.c), but looks like you do not prefer -EINVAL here.
I replaced it with -ENOTSUPP.
for (prop_offset = fdt_first_property_offset(fdt, node_offset);prop_offset > 0;prop_offset = fdt_next_property_offset(fdt, prop_offset)) {value = fdt_getprop_by_offset(fdt, prop_offset,&propname, &len);if (!value)return -EINVAL;if (!strcmp(propname, "function")) {func_selector = pinmux_func_name_to_selector(dev,value);if (func_selector < 0)return func_selector;ret = pinmux_enable_setting(dev, is_group,selector,func_selector);} else {param = pinconf_prop_name_to_param(dev, propname,&default_val);if (param < 0)continue; /* just skip unknown properties */if (len > 0)Strictly speaking, len > sizeof(fdt32_t)
Do you mean len >= sizeof(fdt32_t) ?
Assuming so, fixed in v4.
+int pinctrl_generic_set_state(struct udevice *dev, struct udevice *config) +{
struct udevice *child;int ret;ret = pinctrl_generic_set_state_subnode(dev, config);if (ret)return ret;list_for_each_entry(child, &config->child_head, sibling_node) {ret = pinctrl_generic_set_state_subnode(dev, child);if (ret)return ret;I try to keep access to internal lists within driver/core. Consider using device_find_first/next_child instead. Or perhaps create an iterator device_foreach_child().
Done. (but I still think my original code is simpler. Maybe a new iterator is worth creating)
+static int pinctrl_config_one(struct udevice *config) +{
struct udevice *pctldev;const struct pinctrl_ops *ops;pctldev = config;for (;;) {pctldev = dev_get_parent(pctldev);if (!pctldev) {dev_err(config, "could not find pctldev\n");return -EINVAL;}if (pctldev->uclass->uc_drv->id == UCLASS_PINCTRL)break;}ops = pctldev->driver->ops;Use a #define for this as other uclasses do.
assert(ops), or check for NULL and return -ENOSYS.
I chose "check the valid pointer or -ENOTSUPP"
return ops->set_state(pctldev, config);+}
+int pinctrl_select_state(struct udevice *dev, const char *statename) +{
DECLARE_GLOBAL_DATA_PTR;Can we put that at the top of the file?
Done.
+/**
- pinconfig_post-bind() - post binding for PINCONFIG uclass
- Recursively bind its children as pinconfig devices.
What is the use case for recursive binding?
It is allowed to have grouping nodes.
The following is the real use-case (linux/arch/arm/boot/dts/zynq-zc706.dts):
The nodes "gem0-default", "gpio0-default", "i2c0-default" are just grouping their children. The pin-mux, pin-conf settings are described in the lower level nodes.
In this case, binding should be done recursively.
&pinctrl0 { pinctrl_gem0_default: gem0-default { mux { function = "ethernet0"; groups = "ethernet0_0_grp"; };
conf { groups = "ethernet0_0_grp"; slew-rate = <0>; io-standard = <4>; };
conf-rx { pins = "MIO22", "MIO23", "MIO24", "MIO25", "MIO26", "MIO27"; bias-high-impedance; low-power-disable; };
conf-tx { pins = "MIO16", "MIO17", "MIO18", "MIO19", "MIO20", "MIO21"; low-power-enable; bias-disable; };
mux-mdio { function = "mdio0"; groups = "mdio0_0_grp"; };
conf-mdio { groups = "mdio0_0_grp"; slew-rate = <0>; io-standard = <1>; bias-disable; }; };
pinctrl_gpio0_default: gpio0-default { mux { function = "gpio0"; groups = "gpio0_7_grp", "gpio0_46_grp", "gpio0_47_grp"; };
conf { groups = "gpio0_7_grp", "gpio0_46_grp", "gpio0_47_grp"; slew-rate = <0>; io-standard = <1>; };
conf-pull-up { pins = "MIO46", "MIO47"; bias-pull-up; };
conf-pull-none { pins = "MIO7"; bias-disable; }; };
pinctrl_i2c0_default: i2c0-default { mux { groups = "i2c0_10_grp"; function = "i2c0"; };
conf { groups = "i2c0_10_grp"; bias-pull-up; slew-rate = <0>; io-standard = <1>; }; };
pinctrl_sdhci0_default: sdhci0-default { mux { groups = "sdio0_2_grp"; function = "sdio0"; };
conf { groups = "sdio0_2_grp"; slew-rate = <0>; io-standard = <1>; bias-disable; };
mux-cd { groups = "gpio0_14_grp"; function = "sdio0_cd"; };
conf-cd { groups = "gpio0_14_grp"; bias-high-impedance; bias-pull-up; slew-rate = <0>; io-standard = <1>; };
mux-wp { groups = "gpio0_15_grp"; function = "sdio0_wp"; };
conf-wp { groups = "gpio0_15_grp"; bias-high-impedance; bias-pull-up; slew-rate = <0>; io-standard = <1>; }; };
+#define PIN_CONFIG_BIAS_DISABLE 0 +#define PIN_CONFIG_BIAS_HIGH_IMPEDANCE 1 +#define PIN_CONFIG_BIAS_BUS_HOLD 2 +#define PIN_CONFIG_BIAS_PULL_UP 3 +#define PIN_CONFIG_BIAS_PULL_DOWN 4 +#define PIN_CONFIG_BIAS_PULL_PIN_DEFAULT 5 +#define PIN_CONFIG_DRIVE_PUSH_PULL 6 +#define PIN_CONFIG_DRIVE_OPEN_DRAIN 7 +#define PIN_CONFIG_DRIVE_OPEN_SOURCE 8 +#define PIN_CONFIG_DRIVE_STRENGTH 9 +#define PIN_CONFIG_INPUT_ENABLE 10 +#define PIN_CONFIG_INPUT_SCHMITT_ENABLE 11 +#define PIN_CONFIG_INPUT_SCHMITT 12 +#define PIN_CONFIG_INPUT_DEBOUNCE 13 +#define PIN_CONFIG_POWER_SOURCE 14 +#define PIN_CONFIG_SLEW_RATE 15 +#define PIN_CONFIG_LOW_POWER_MODE 16 +#define PIN_CONFIG_OUTPUT 17
Use enum?
Low-level drivers are allowed to have vendor-specific parameters in addition to these generic parameters.
If we defined enum only for these 18 parameters, it would be difficult to do so.
+#define PIN_CONFIG_END 0x7FFF
+#if CONFIG_IS_ENABLED(PINCTRL) +/**
- pinctrl_select_state() - set a device to a given state
- @dev: peripheral device
- @statename: state name, like "default"
- */
+int pinctrl_select_state(struct udevice *dev, const char *statename); +#else +static inline int pinctrl_select_state(struct udevice *dev,
const char *statename)+{
return 0;+} +#endif
+#if CONFIG_IS_ENABLED(PINCTRL_GENERIC)
Do you think PINCTRL_SIMPLE might be a better name?
They have different meanings.
PINCTRL_SIMPLE - switch between full-pinctrl and simple-pinctrl
simple-pinctrl does not require DTS at all
PINCTRL_GENERIC - enable generic DTS interface
This depends on full-pinctrl. This feature is useful to handle generic properties such as "pins", "groups", "functions" in DTS.
+#endif /* __PINCTRL_H */ diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h index c744044..a2fb6de 100644 --- a/include/dm/uclass-id.h +++ b/include/dm/uclass-id.h @@ -44,6 +44,8 @@ enum uclass_id { UCLASS_PCH, /* x86 platform controller hub */ UCLASS_PCI, /* PCI bus */ UCLASS_PCI_GENERIC, /* Generic PCI bus device */
UCLASS_PINCTRL, /* Pinctrl device */Expand - e.g. Pin control and multiplexing device
Done.
UCLASS_PINCONFIG, /* Pinconfig node device */Pin configuration
Done.