Simon,
I've just posted v4. Sorry for the delay. 2015-08-12 23:16 GMT+09:00 Simon Glass <[email protected]>: > Hi Masahiro, > > On 10 August 2015 at 10:05, Masahiro Yamada > <[email protected]> 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 <[email protected]> >> --- >> >> 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 PINCTRL > > Can 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. -- Best Regards Masahiro Yamada _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
