Hi Simon,

2015-08-12 23:15 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 is useful when we want to bind a device, but do not need the device pointer.

Could adjust the subject to something shorter, like "dm: core: Allow device_bind() to not return the device"

Done. Thanks.

eturn -EINVAL;

...

@@ -133,7 +134,8 @@ int device_bind(struct udevice *parent, const struct driver *drv,

    if (parent)
            dm_dbg("Bound device %s to %s\n", dev->name, parent->name);

•   *devp = dev;
  

•   if (devp)
  

•           *devp = dev;
  
    dev->flags |= DM_FLAG_BOUND;
  
  

-- 1.9.1

Please can you update the function comments for device_bind()?

Done for device_bind() and others.

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 PINCTRL
  

Can you add some help for these - explaining what each one means and why to enable it.

+config PINMUX

•   bool "Support pin multiplexing controllers"
  

•   depends on PINCTRL_GENERIC
  

+config PINCONF

•   bool "Support pin configuration controllers"
  

•   depends on PINCTRL_GENERIC
  

+config SPL_PINCTRL

•   bool "Support pin controlloers in SPL"
  

•   depends on SPL
  

+config SPL_PINCTRL_GENERIC

•   bool "Support generic pin controllers in SPL"
  

•   depends on SPL_PINCTRL
  

+config SPL_PINMUX

•   bool "Support pin multiplexing controllers in SPL"
  

•   depends on SPL_PINCTRL_GENERIC
  

+config SPL_PINCONF

•   bool "Support pin configuration controllers in SPL"
  

•   depends on SPL_PINCTRL_GENERIC
  

+if PINCTRL || SPL_PINCTRL

+endif

+endmenu 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?

@@ -0,0 +1,2 @@ +obj-y += pinctrl-uclass.o +obj-$(CONFIG_$(SPL_)PINCTRL_GENERIC) += pinctrl-generic.o diff --git a/drivers/pinctrl/pinctrl-generic.c b/drivers/pinctrl/pinctrl-generic.c new file mode 100644 index 0000000..5d5d245 --- /dev/null +++ b/drivers/pinctrl/pinctrl-generic.c @@ -0,0 +1,351 @@ +/*

• • Copyright (C) 2015 Masahiro Yamada yamada.masahiro@socionext.com
• • SPDX-License-Identifier: GPL-2.0+
• */

+#include <common.h> +#include <linux/compat.h> +#include <dm/device.h> +#include <dm/pinctrl.h>

+DECLARE_GLOBAL_DATA_PTR;

+/**

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

• */

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

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

•           return -EINVAL;
  

That is normally used for an invalid device tree arg. How about -ENOSYS?

•   }
  

•   npins = ops->get_pins_count(dev);
  

•   /* See if this pctldev has this pin */
  

•   while (selector < npins) {
  

How about:

for (selector = 0; selector < npins; selected++)

•           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

• */

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

+}

+#if CONFIG_IS_ENABLED(PINMUX) +/**

• • pinmux_func_name_to_selector() - return the function selector for a function
• • @dev: pin controller device
• • @function: the function name to look up
• */

+static int pinmux_func_name_to_selector(struct udevice *dev,

•                                   const char *function)
  

+{

•   const struct pinctrl_ops *ops = dev->driver->ops;
  

•   unsigned nfuncs, selector = 0;
  

•   if (!ops->get_functions_count || !ops->get_function_name) {
  

•           dev_err(dev,
  

•                   "get_functions_count or get_function_name missing\n");
  

•           return -EINVAL;
  

•   }
  

•   nfuncs = ops->get_functions_count(dev);
  

•   /* See if this pctldev has this function */
  

•   while (selector < nfuncs) {
  

•           const char *fname = ops->get_function_name(dev, selector);
  

•           if (!strcmp(function, fname))
  

•                   return selector;
  

•           selector++;
  

•   }
  

•   return -EINVAL;
  

+}

+/**

• • pinmux_enable_setting() - enable pin-mux setting for a certain pin/group
• • @dev: pin controller device
• • @is_group: target of operation (true: pin group, false: pin)
• • @selector: pin selector or group selector, depending on @is_group
• • @func_selector: function selector
• */

+static int pinmux_enable_setting(struct udevice *dev, bool is_group,

•                            unsigned selector, unsigned func_selector)
  

+{

•   const struct pinctrl_ops *ops = dev->driver->ops;
  

•   if (is_group) {
  

•           if (!ops->pinmux_group_set) {
  

•                   dev_err(dev, "pinmux_group_set op missing\n");
  

•                   return -EINVAL;
  

•           }
  

•           return ops->pinmux_group_set(dev, selector, func_selector);
  

•   } else {
  

•           if (!ops->pinmux_set) {
  

•                   dev_err(dev, "pinmux_set op missing\n");
  

•                   return -EINVAL;
  

•           }
  

•           return ops->pinmux_set(dev, selector, func_selector);
  

•   }
  

+} +#else +static int pinmux_func_name_to_selector(struct udevice *dev,

•                                   const char *function)
  

+{

•   return 0;
  

+}

+static int pinmux_enable_setting(struct udevice *dev, bool is_group,

•                            unsigned selector, unsigned func_selector)
  

+{

•   return 0;
  

+} +#endif

+#if CONFIG_IS_ENABLED(PINCONF) +/**

• • pinconf_prop_name_to_param() - return parameter ID for a property name
• • @dev: pin controller device
• • @property: property name in DTS, such as "bias-pull-up", "slew-rate", etc.
• • @default_value: return default value in case no value is specified in DTS
• */

+static int pinconf_prop_name_to_param(struct udevice *dev,

•                                 const char *property, u32 *default_value)
  

+{

•   const struct pinctrl_ops *ops = dev->driver->ops;
  

•   const struct pinconf_param *p, *end;
  

•   if (!ops->pinconf_num_params || !ops->pinconf_params) {
  

•           dev_err(dev, "pinconf_num_params or pinconf_params missing\n");
  

•           return -EINVAL;
  

•   }
  

•   p = ops->pinconf_params;
  

•   end = p + ops->pinconf_num_params;
  

•   /* See if this pctldev supports this parameter */
  

•   while (p < end) {
  

•           if (!strcmp(property, p->property)) {
  

•                   *default_value = p->default_value;
  

•                   return p->param;
  

•           }
  

•           p++;
  

•   }
  

•   return -EINVAL;
  

+}

+/**

• • pinconf_enable_setting() - apply pin configuration for a certain pin/group
• • @dev: pin controller device
• • @is_group: target of operation (true: pin group, false: pin)
• • @selector: pin selector or group selector, depending on @is_group
• • @param: configuration paramter
• • @argument: argument taken by some configuration parameters
• */

+static int pinconf_enable_setting(struct udevice *dev, bool is_group,

•                             unsigned selector, unsigned param,
  

•                             u32 argument)
  

+{

•   const struct pinctrl_ops *ops = dev->driver->ops;
  

•   if (is_group) {
  

•           if (!ops->pinconf_group_set) {
  

•                   dev_err(dev, "pinconf_group_set op missing\n");
  

•                   return -EINVAL;
  

•           }
  

•           return ops->pinconf_group_set(dev, selector, param,
  

•                                         argument);
  

•   } else {
  

•           if (!ops->pinconf_set) {
  

•                   dev_err(dev, "pinconf_set op missing\n");
  

•                   return -EINVAL;
  

•           }
  

•           return ops->pinconf_set(dev, selector, param, argument);
  

•   }
  

+} +#else +static int pinconf_prop_name_to_param(struct udevice *dev,

•                                 const char *property, u32 *default_value)
  

+{

•   return -EINVAL;
  

+}

+static int pinconf_enable_setting(struct udevice *dev, bool is_group,

•                             unsigned selector, unsigned param,
  

•                             u32 argument)
  

+{

•   return 0;
  

+} +#endif

+/**

• • pinctrl_generic_set_state_one() - set state for a certain pin/group
• • Apply all pin-muxing and pin configurations specified by @config
• • for a given pin or pin group.
• • @dev: pin controller device
• • @config: pseudo device pointing to config node
• • @is_group: target of operation (true: pin group, false: pin)
• • @selector: pin selector or group selector, depending on @is_group
• */

+static int pinctrl_generic_set_state_one(struct udevice *dev,

•                                    struct udevice *config,
  

•                                    bool is_group, unsigned selector)
  

+{

•   const void *fdt = gd->fdt_blob;
  

•   int node_offset = config->of_offset;
  

•   const char *propname;
  

•   const void *value;
  

•   int prop_offset, len, func_selector, param, ret;
  

•   u32 arg, default_val;
  

•   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)

•                           arg = fdt32_to_cpu(*(fdt32_t *)value);
  

•                   else
  

•                           arg = default_val;
  

•                   ret = pinconf_enable_setting(dev, is_group,
  

•                                                selector, param, arg);
  

•           }
  

•           if (ret)
  

•                   return ret;
  

•   }
  

•   return 0;
  

+}

+/**

• • pinctrl_generic_set_state_subnode() - apply all settings in config node
• • @dev: pin controller device
• • @config: pseudo device pointing to config node
• */

+static int pinctrl_generic_set_state_subnode(struct udevice *dev,

•                                        struct udevice *config)
  

+{

•   const void *fdt = gd->fdt_blob;
  

•   int node = config->of_offset;
  

•   const char *subnode_target_type = "pins";
  

•   bool is_group = false;
  

•   const char *name;
  

•   int strings_count, selector, i, ret;
  

•   strings_count = fdt_count_strings(fdt, node, subnode_target_type);
  

•   if (strings_count < 0) {
  

•           subnode_target_type = "groups";
  

•           is_group = true;
  

•           strings_count = fdt_count_strings(fdt, node,
  

•                                             subnode_target_type);
  

•           if (strings_count < 0)
  

•                   return -EINVAL;
  

•   }
  

•   for (i = 0; i < strings_count; i++) {
  

•           ret = fdt_get_string_index(fdt, node, subnode_target_type,
  

•                                      i, &name);
  

•           if (ret < 0)
  

•                   return -EINVAL;
  

•           if (is_group)
  

•                   selector = pinctrl_group_name_to_selector(dev, name);
  

•           else
  

•                   selector = pinctrl_pin_name_to_selector(dev, name);
  

•           if (selector < 0)
  

•                   return selector;
  

•           ret = pinctrl_generic_set_state_one(dev, config,
  

•                                               is_group, selector);
  

•           if (ret)
  

•                   return ret;
  

•   }
  

•   return 0;
  

+}

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

•   }
  

•   return 0;
  

+}

diff --git a/drivers/pinctrl/pinctrl-uclass.c b/drivers/pinctrl/pinctrl-uclass.c new file mode 100644 index 0000000..ab3c146 --- /dev/null +++ b/drivers/pinctrl/pinctrl-uclass.c @@ -0,0 +1,151 @@ +/*

• • Copyright (C) 2015 Masahiro Yamada yamada.masahiro@socionext.com
• • SPDX-License-Identifier: GPL-2.0+
• */

+#include <common.h> +#include <libfdt.h> +#include <linux/err.h> +#include <linux/list.h> +#include <dm/device.h> +#include <dm/lists.h> +#include <dm/pinctrl.h> +#include <dm/uclass.h>

+DECLARE_GLOBAL_DATA_PTR;

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

•   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?

•   const void *fdt = gd->fdt_blob;
  

•   int node = dev->of_offset;
  

•   char propname[32]; /* long enough */
  

•   const fdt32_t *list;
  

•   uint32_t phandle;
  

•   int config_node;
  

•   struct udevice *config;
  

•   int state, size, i, ret;
  

•   state = fdt_find_string(fdt, node, "pinctrl-names", statename);
  

•   if (state < 0) {
  

•           char *end;
  

•           /*
  

•            * If statename is not found in "pinctrl-names",
  

•            * assume statename is just the integer state ID.
  

•            */
  

•           state = simple_strtoul(statename, &end, 10);
  

•           if (*end)
  

•                   return -EINVAL;
  

•   }
  

•   snprintf(propname, sizeof(propname), "pinctrl-%d", state);
  

•   list = fdt_getprop(fdt, node, propname, &size);
  

•   if (!list)
  

•           return -EINVAL;
  

•   size /= sizeof(*list);
  

•   for (i = 0; i < size; i++) {
  

•           phandle = fdt32_to_cpu(*list++);
  

•           config_node = fdt_node_offset_by_phandle(fdt, phandle);
  

•           if (config_node < 0) {
  

•                   dev_err(dev, "prop %s index %d invalid phandle\n",
  

•                           propname, i);
  

•                   return -EINVAL;
  

•           }
  

•           ret = uclass_get_device_by_of_offset(UCLASS_PINCONFIG,
  

•                                                config_node, &config);
  

•           if (ret)
  

•                   return ret;
  

•           ret = pinctrl_config_one(config);
  

•           if (ret)
  

•                   return ret;
  

•   }
  

•   return 0;
  

+}

+/**

• • pinconfig_post-bind() - post binding for PINCONFIG uclass
• • Recursively bind its children as pinconfig devices.

What is the use case for recursive binding?

• • @dev: pinconfig device
• */

+static int pinconfig_post_bind(struct udevice *dev) +{

•   const void *fdt = gd->fdt_blob;
  

•   int offset = dev->of_offset;
  

•   const char *name;
  

•   int ret;
  

•   for (offset = fdt_first_subnode(fdt, offset);
  

•        offset > 0;
  

•        offset = fdt_next_subnode(fdt, offset)) {
  

•           name = fdt_get_name(fdt, offset, NULL);
  

•           if (!name)
  

•                   return -EINVAL;
  

•           ret = device_bind_driver_to_node(dev, "pinconfig", name,
  

•                                            offset, NULL);
  

•           if (ret)
  

•                   return ret;
  

•   }
  

•   return 0;
  

+}

+/**

• • pinconfig_post-bind() - post binding for PINCTRL uclass
• • Check the mandatory operation and bind its children as pinconfig devices.
• • @dev: pinctrl device
• */

+static int pinctrl_post_bind(struct udevice *dev) +{

•   const struct pinctrl_ops *ops = dev->driver->ops;
  

•   if (!ops || !ops->set_state) {
  

•           dev_err(dev, "set_state op missing\n");
  

•           return -EINVAL;
  

•   }
  

•   return pinconfig_post_bind(dev);
  

+}

+UCLASS_DRIVER(pinctrl) = {

•   .id = UCLASS_PINCTRL,
  

•   .post_bind = pinctrl_post_bind,
  

•   .name = "pinctrl",
  

+};

+UCLASS_DRIVER(pinconfig) = {

•   .id = UCLASS_PINCONFIG,
  

•   .post_bind = pinconfig_post_bind,
  

•   .name = "pinconfig",
  

+};

+U_BOOT_DRIVER(pinconfig_generic) = {

•   .name = "pinconfig",
  

•   .id = UCLASS_PINCONFIG,
  

+}; diff --git a/include/dm/pinctrl.h b/include/dm/pinctrl.h new file mode 100644 index 0000000..42008da --- /dev/null +++ b/include/dm/pinctrl.h @@ -0,0 +1,218 @@ +/*

• • Copyright (C) 2015 Masahiro Yamada yamada.masahiro@socionext.com
• • SPDX-License-Identifier: GPL-2.0+
• */

+#ifndef __PINCTRL_H +#define __PINCTRL_H

+/**

• • struct pinconf_param - pin config parameters
• • @property: property name in DT nodes
• • @param: ID for this config parameter
• • @default_value: default value for this config parameter used in case

• • no value is specified in DT nodes
    

• */

+struct pinconf_param {

•   const char * const property;
  

•   unsigned int param;
  

•   u32 default_value;
  

+};

+/**

• • struct pinctrl_ops - pin control operations, to be implemented by
• • pin controller drivers.
• • The @set_state is the only mandatory operation. You can implement your
• • pinctrl driver with its own @set_state. In this case, the other callbacks
• • are not required. Otherwise, generic pinctrl framework is also available;
• • use pinctrl_generic_set_state for @set_state, and implement other operations
• • depending on your necessity.
• • @get_pins_count: return number of selectable named pins available

• • in this driver.  (necessary to parse "pins" property in DTS)
    

• • @get_pin_name: return the pin name of the pin selector,

• • called by the core to figure out which pin it shall do
    

• • operations to.  (necessary to parse "pins" property in DTS)
    

• • @get_groups_count: return number of selectable named groups available

• • in this driver.  (necessary to parse "groups" property in DTS)
    

• • @get_group_name: return the group name of the group selector,

• • called by the core to figure out which pin group it shall do
    

• • operations to.  (necessary to parse "groups" property in DTS)
    

• • @get_functions_count: return number of selectable named functions available

• • in this driver.  (necessary for pin-muxing)
    

• • @get_function_name: return the function name of the muxing selector,

• • called by the core to figure out which mux setting it shall map a
    

• • certain device to.  (necessary for pin-muxing)
    

• • @pinmux_set: enable a certain muxing function with a certain pin.

• • The @func_selector selects a certain function whereas @pin_selector
    

• • selects a certain pin to be used. On simple controllers one of them
    

• • may be ignored.  (necessary for pin-muxing against a single pin)
    

• • @pinmux_group_set: enable a certain muxing function with a certain pin

• • group.  The @func_selector selects a certain function whereas
    

• • @group_selector selects a certain set of pins to be used. On simple
    

• • controllers one of them may be ignored.
    

• • (necessary for pin-muxing against a pin group)
    

• • @pinconf_num_params: number of driver-specific parameters to be parsed

• • from device trees  (necessary for pin-configuration)
    

• • @pinconf_params: list of driver_specific parameters to be parsed from

• • device trees  (necessary for pin-configuration)
    

• • @pinconf_set: configure an individual pin with a given parameter.

• • (necessary for pin-configuration against a single pin)
    

• • @pinconf_group_set: configure all pins in a group with a given parameter.

• • (necessary for pin-configuration against a pin group)
    

• • @set_state: enable pin-mux and pinconf settings specified by @config, a

• • pseudo device pointing a config node. (mandatory for all drivers)
    

• */

+struct pinctrl_ops {

•   int (*get_pins_count)(struct udevice *dev);
  

•   const char *(*get_pin_name)(struct udevice *dev, unsigned selector);
  

•   int (*get_groups_count)(struct udevice *dev);
  

•   const char *(*get_group_name)(struct udevice *dev, unsigned selector);
  

•   int (*get_functions_count)(struct udevice *dev);
  

•   const char *(*get_function_name)(struct udevice *dev,
  

•                                    unsigned selector);
  

•   int (*pinmux_set)(struct udevice *dev, unsigned pin_selector,
  

•                     unsigned func_selector);
  

•   int (*pinmux_group_set)(struct udevice *dev, unsigned group_selector,
  

•                           unsigned func_selector);
  

•   unsigned int pinconf_num_params;
  

•   const struct pinconf_param *pinconf_params;
  

•   int (*pinconf_set)(struct udevice *dev, unsigned pin_selector,
  

•                      unsigned param, unsigned argument);
  

•   int (*pinconf_group_set)(struct udevice *dev, unsigned group_selector,
  

•                            unsigned param, unsigned argument);
  

•   int (*set_state)(struct udevice *dev, struct udevice *config);
  

+};

+/**

• • Generic pin configuration paramters
• • @PIN_CONFIG_BIAS_DISABLE: disable any pin bias on the pin, a

• • transition from say pull-up to pull-down implies that you disable
    

• • pull-up in the process, this setting disables all biasing.
    

• • @PIN_CONFIG_BIAS_HIGH_IMPEDANCE: the pin will be set to a high impedance

• • mode, also know as "third-state" (tristate) or "high-Z" or "floating".
    

• • On output pins this effectively disconnects the pin, which is useful
    

• • if for example some other pin is going to drive the signal connected
    

• • to it for a while. Pins used for input are usually always high
    

• • impedance.
    

• • @PIN_CONFIG_BIAS_BUS_HOLD: the pin will be set to weakly latch so that it

• • weakly drives the last value on a tristate bus, also known as a "bus
    

• • holder", "bus keeper" or "repeater". This allows another device on the
    

• • bus to change the value by driving the bus high or low and switching to
    

• • tristate. The argument is ignored.
    

• • @PIN_CONFIG_BIAS_PULL_UP: the pin will be pulled up (usually with high

• • impedance to VDD). If the argument is != 0 pull-up is enabled,
    

• • if it is 0, pull-up is total, i.e. the pin is connected to VDD.
    

• • @PIN_CONFIG_BIAS_PULL_DOWN: the pin will be pulled down (usually with high

• • impedance to GROUND). If the argument is != 0 pull-down is enabled,
    

• • if it is 0, pull-down is total, i.e. the pin is connected to GROUND.
    

• • @PIN_CONFIG_BIAS_PULL_PIN_DEFAULT: the pin will be pulled up or down based

• • on embedded knowledge of the controller hardware, like current mux
    

• • function. The pull direction and possibly strength too will normally
    

• • be decided completely inside the hardware block and not be readable
    

• • from the kernel side.
    

• • If the argument is != 0 pull up/down is enabled, if it is 0, the
    

• • configuration is ignored. The proper way to disable it is to use
    

• • @PIN_CONFIG_BIAS_DISABLE.
    

• • @PIN_CONFIG_DRIVE_PUSH_PULL: the pin will be driven actively high and

• • low, this is the most typical case and is typically achieved with two
    

• • active transistors on the output. Setting this config will enable
    

• • push-pull mode, the argument is ignored.
    

• • @PIN_CONFIG_DRIVE_OPEN_DRAIN: the pin will be driven with open drain (open

• • collector) which means it is usually wired with other output ports
    

• • which are then pulled up with an external resistor. Setting this
    

• • config will enable open drain mode, the argument is ignored.
    

• • @PIN_CONFIG_DRIVE_OPEN_SOURCE: the pin will be driven with open source

• • (open emitter). Setting this config will enable open source mode, the
    

• • argument is ignored.
    

• • @PIN_CONFIG_DRIVE_STRENGTH: the pin will sink or source at most the current

• • passed as argument. The argument is in mA.
    

• • @PIN_CONFIG_INPUT_ENABLE: enable the pin's input. Note that this does not

• • affect the pin's ability to drive output.  1 enables input, 0 disables
    

• • input.
    

• • @PIN_CONFIG_INPUT_SCHMITT_ENABLE: control schmitt-trigger mode on the pin.

• •  If the argument != 0, schmitt-trigger mode is enabled. If it's 0,
    

• •  schmitt-trigger mode is disabled.
    

• • @PIN_CONFIG_INPUT_SCHMITT: this will configure an input pin to run in

• • schmitt-trigger mode. If the schmitt-trigger has adjustable hysteresis,
    

• • the threshold value is given on a custom format as argument when
    

• • setting pins to this mode.
    

• • @PIN_CONFIG_INPUT_DEBOUNCE: this will configure the pin to debounce mode,

• • which means it will wait for signals to settle when reading inputs. The
    

• • argument gives the debounce time in usecs. Setting the
    

• • argument to zero turns debouncing off.
    

• • @PIN_CONFIG_POWER_SOURCE: if the pin can select between different power

• • supplies, the argument to this parameter (on a custom format) tells
    

• • the driver which alternative power source to use.
    

• • @PIN_CONFIG_SLEW_RATE: if the pin can select slew rate, the argument to

• • this parameter (on a custom format) tells the driver which alternative
    

• • slew rate to use.
    

• • @PIN_CONFIG_LOW_POWER_MODE: this will configure the pin for low power

• • operation, if several modes of operation are supported these can be
    

• • passed in the argument on a custom form, else just use argument 1
    

• • to indicate low power mode, argument 0 turns low power mode off.
    

• • @PIN_CONFIG_OUTPUT: this will configure the pin as an output. Use argument

• • 1 to indicate high level, argument 0 to indicate low level. (Please
    

• • see Documentation/pinctrl.txt, section "GPIO mode pitfalls" for a
    

• • discussion around this parameter.)
    

• • @PIN_CONFIG_END: this is the last enumerator for pin configurations, if

• • you need to pass in custom configurations to the pin controller, use
    

• • PIN_CONFIG_END+1 as the base offset.
    

• */

+#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?

+#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?

+/**

• • pinctrl_generic_set_state() - generic set_state operation
• • @pctldev: pinctrl device
• • @config: config device (pseudo device), pointing a config node in DTS
• */

+int pinctrl_generic_set_state(struct udevice *pctldev, struct udevice *config); +#else +static inline int pinctrl_generic_set_state(struct udevice *pctldev,

•                                       struct udevice *config)
  

+{

•   return 0;
  

+} +#endif

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

•   UCLASS_PINCONFIG,       /* Pinconfig node device */
  

Pin configuration

    UCLASS_PMIC,            /* PMIC I/O device */
    UCLASS_REGULATOR,       /* Regulator device */
    UCLASS_RESET,           /* Reset device */

-- 1.9.1

Regards, Simon

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

Hi Simon,

2015-08-12 23:16 GMT+09:00 Simon Glass sjg@chromium.org:

...

diff --git a/include/dm/pinctrl.h b/include/dm/pinctrl.h index 42008da..b2ba0b4 100644 --- a/include/dm/pinctrl.h +++ b/include/dm/pinctrl.h @@ -85,6 +85,8 @@ struct pinctrl_ops { int (*pinconf_group_set)(struct udevice *dev, unsigned group_selector, unsigned param, unsigned argument); int (*set_state)(struct udevice *dev, struct udevice *config);

•   /* for pinctrl-simple */
  

•   int (*set_state_simple)(struct udevice *dev, struct udevice *periph);
  

So should the other members be #idef'd out? Also, comments on this function?

After my careful consideration, I did not do this.

If we do this,the corresponding members in all drivers must be also #ifdef'd out, including full-pinctrl drivers that do not care about memory footprint.

I do not like adding #ifdefs around to fix build errors found with "make allyesconfig", "make randconfig".

I think it is a general strategy to not #ifdef out struct members.

Hi Marahiro,

On 10 August 2015 at 10:05, Masahiro Yamada yamada.masahiro@socionext.com wrote:

This driver actually does nothing but test pinctrl uclass, and demonstrate how things work.

To try this driver, uncomment /* #define DEBUG */ in the drivers/pinctrl/pinctrl-sandbox.c, and debug messages will be displayed.

DRAM: 128 MiB sandbox pinmux: group = 1 (serial_a), function = 1 (serial) Using default environment

In: cros-ec-keyb Out: lcd Err: lcd Net: Net Initialization Skipped eth0: eth@10002000, eth1: eth@80000000, eth5: eth@90000000 => i2c dev 0 Setting bus to 0 sandbox pinmux: group = 0 (i2c), function = 0 (i2c) sandbox pinconf: group = 0 (i2c), param = 3, arg = 1

Signed-off-by: Masahiro Yamada yamada.masahiro@socionext.com

arch/sandbox/dts/sandbox.dts | 19 +++++ configs/sandbox_defconfig | 5 ++ drivers/pinctrl/Kconfig | 3 + drivers/pinctrl/Makefile | 2 + drivers/pinctrl/pinctrl-sandbox.c | 157 ++++++++++++++++++++++++++++++++++++++ 5 files changed, 186 insertions(+) create mode 100644 drivers/pinctrl/pinctrl-sandbox.c

Reviewed-by: Simon Glass sjg@chromium.org

diff --git a/arch/sandbox/dts/sandbox.dts b/arch/sandbox/dts/sandbox.dts index 8927527..9aad39c 100644 --- a/arch/sandbox/dts/sandbox.dts +++ b/arch/sandbox/dts/sandbox.dts @@ -92,6 +92,8 @@ reg = <0 0>; compatible = "sandbox,i2c"; clock-frequency = <400000>;

•           pinctrl-names = "default";
  

•           pinctrl-0 = <&pinctrl_i2c0>;
            eeprom@2c {
                    reg = <0x2c>;
                    compatible = "i2c-eeprom";
  

@@ -136,6 +138,21 @@ }; };

•   pinctrl {
  

•           compatible = "sandbox,pinctrl";
  

•           pinctrl_i2c0: i2c0 {
  

•                   groups = "i2c";
  

•                   function = "i2c";
  

•                   bias-pull-up;
  

•           };
  

•           pinctrl_serial0: uart0 {
  

•                   groups = "serial_a";
  

•                   function = "serial";
  

•           };
  

•   };
  

•   spi@0 {
            #address-cells = <1>;
            #size-cells = <0>;
  

@@ -168,6 +185,8 @@ uart0: serial { compatible = "sandbox,serial"; sandbox,text-colour = "cyan";

•           pinctrl-names = "default";
  

•           pinctrl-0 = <&pinctrl_serial0>;
    };
  
  

Should also have some definitions in test.fdt and a test for pinctrl.

    usb@0 {

diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig index 874a26b..85693f0 100644 --- a/configs/sandbox_defconfig +++ b/configs/sandbox_defconfig @@ -27,6 +27,11 @@ CONFIG_SANDBOX_SERIAL=y CONFIG_TPM_TIS_SANDBOX=y CONFIG_SYS_I2C_SANDBOX=y CONFIG_SANDBOX_SPI=y +CONFIG_PINCTRL=y +CONFIG_PINCTRL_GENERIC=y +CONFIG_PINMUX=y +CONFIG_PINCONF=y +CONFIG_PINCTRL_SANDBOX=y CONFIG_SANDBOX_GPIO=y CONFIG_DM_PMIC=y CONFIG_DM_PMIC_SANDBOX=y diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig index eca83fe..c6229d9 100644 --- a/drivers/pinctrl/Kconfig +++ b/drivers/pinctrl/Kconfig @@ -45,6 +45,9 @@ config SPL_PINCONF

if PINCTRL || SPL_PINCTRL

+config PINCTRL_SANDBOX

•   bool "Sandbox pinctrl driver"
  

help - what does this support / do?

endif

endmenu diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile index a713c7d..35decf4 100644 --- a/drivers/pinctrl/Makefile +++ b/drivers/pinctrl/Makefile @@ -1,2 +1,4 @@ obj-y += pinctrl-uclass.o obj-$(CONFIG_$(SPL_)PINCTRL_GENERIC) += pinctrl-generic.o

+obj-$(CONFIG_PINCTRL_SANDBOX) += pinctrl-sandbox.o diff --git a/drivers/pinctrl/pinctrl-sandbox.c b/drivers/pinctrl/pinctrl-sandbox.c new file mode 100644 index 0000000..1f02bf2 --- /dev/null +++ b/drivers/pinctrl/pinctrl-sandbox.c @@ -0,0 +1,157 @@ +/*

• • SPDX-License-Identifier: GPL-2.0+

(C)?

• */

+/* #define DEBUG */

+#include <common.h> +#include <dm/device.h> +#include <dm/pinctrl.h>

+static const char * const sandbox_pins[] = {

•   "SCL",
  

•   "SDA",
  

•   "TX",
  

•   "RX",
  

+};

+static const char * const sandbox_groups[] = {

•   "i2c",
  

•   "serial_a",
  

•   "serial_b",
  

•   "spi",
  

+};

+static const char * const sandbox_functions[] = {

•   "i2c",
  

•   "serial",
  

•   "spi",
  

+};

+static const struct pinconf_param sandbox_conf_params[] = {

•   { "bias-disable", PIN_CONFIG_BIAS_DISABLE, 0 },
  

•   { "bias-high-impedance", PIN_CONFIG_BIAS_HIGH_IMPEDANCE, 0 },
  

•   { "bias-bus-hold", PIN_CONFIG_BIAS_BUS_HOLD, 0 },
  

•   { "bias-pull-up", PIN_CONFIG_BIAS_PULL_UP, 1 },
  

•   { "bias-pull-down", PIN_CONFIG_BIAS_PULL_DOWN, 1 },
  

•   { "bias-pull-pin-default", PIN_CONFIG_BIAS_PULL_PIN_DEFAULT, 1 },
  

•   { "drive-open-drain", PIN_CONFIG_DRIVE_OPEN_DRAIN, 0 },
  

•   { "drive-open-source", PIN_CONFIG_DRIVE_OPEN_SOURCE, 0 },
  

•   { "drive-strength", PIN_CONFIG_DRIVE_STRENGTH, 0 },
  

•   { "input-enable", PIN_CONFIG_INPUT_ENABLE, 1 },
  

•   { "input-disable", PIN_CONFIG_INPUT_ENABLE, 0 },
  

+};

+static int sandbox_pinctrl_probe(struct udevice *dev) +{

•   return 0;
  

+}

+static int sandbox_pinctrl_remove(struct udevice *dev) +{

•   return 0;
  

+}

Drop those two functions?

+static int sandbox_get_pins_count(struct udevice *dev) +{

•   return ARRAY_SIZE(sandbox_pins);
  

+}

+static const char *sandbox_get_pin_name(struct udevice *dev, unsigned selector) +{

•   return sandbox_pins[selector];
  

+}

+static int sandbox_get_groups_count(struct udevice *dev) +{

•   return ARRAY_SIZE(sandbox_groups);
  

+}

+static const char *sandbox_get_group_name(struct udevice *dev,

•                                     unsigned selector)
  

+{

•   return sandbox_groups[selector];
  

+}

+static int sandbox_get_functions_count(struct udevice *dev) +{

•   return ARRAY_SIZE(sandbox_functions);
  

+}

+static const char *sandbox_get_function_name(struct udevice *dev,

•                                        unsigned selector)
  

+{

•   return sandbox_functions[selector];
  

+}

+static int sandbox_pinmux_set(struct udevice *dev, unsigned pin_selector,

•                         unsigned func_selector)
  

+{

•   debug("sandbox pinmux: pin = %d (%s), function = %d (%s)\n",
  

•         pin_selector, sandbox_get_pin_name(dev, pin_selector),
  

•         func_selector, sandbox_get_function_name(dev, func_selector));
  

•   return 0;
  

+}

+static int sandbox_pinmux_group_set(struct udevice *dev,

•                               unsigned group_selector,
  

•                               unsigned func_selector)
  

+{

•   debug("sandbox pinmux: group = %d (%s), function = %d (%s)\n",
  

•         group_selector, sandbox_get_group_name(dev, group_selector),
  

•         func_selector, sandbox_get_function_name(dev, func_selector));
  

•   return 0;
  

+}

+static int sandbox_pinconf_set(struct udevice *dev, unsigned pin_selector,

•                          unsigned param, unsigned argument)
  

+{

•   debug("sandbox pinconf: pin = %d (%s), param = %d, arg = %d\n",
  

•         pin_selector, sandbox_get_pin_name(dev, pin_selector),
  

•         param, argument);
  

•   return 0;
  

+}

+static int sandbox_pinconf_group_set(struct udevice *dev,

•                                unsigned group_selector,
  

•                                unsigned param, unsigned argument)
  

+{

•   debug("sandbox pinconf: group = %d (%s), param = %d, arg = %d\n",
  

•         group_selector, sandbox_get_group_name(dev, group_selector),
  

•         param, argument);
  

•   return 0;
  

+}

+const struct pinctrl_ops sandbox_pinctrl_ops = {

•   .get_pins_count = sandbox_get_pins_count,
  

•   .get_pin_name = sandbox_get_pin_name,
  

•   .get_groups_count = sandbox_get_groups_count,
  

•   .get_group_name = sandbox_get_group_name,
  

•   .get_functions_count = sandbox_get_functions_count,
  

•   .get_function_name = sandbox_get_function_name,
  

•   .pinmux_set = sandbox_pinmux_set,
  

•   .pinmux_group_set = sandbox_pinmux_group_set,
  

•   .pinconf_num_params = ARRAY_SIZE(sandbox_conf_params),
  

•   .pinconf_params = sandbox_conf_params,
  

•   .pinconf_set = sandbox_pinconf_set,
  

•   .pinconf_group_set = sandbox_pinconf_group_set,
  

•   .set_state = pinctrl_generic_set_state,
  

+};

+static const struct udevice_id sandbox_pinctrl_match[] = {

•   { .compatible = "sandbox,pinctrl" },
  

•   { /* sentinel */ }
  

+};

+U_BOOT_DRIVER(sandbox_pinctrl) = {

•   .name = "sandbox_pinctrl",
  

•   .id = UCLASS_PINCTRL,
  

•   .of_match = sandbox_pinctrl_match,
  

•   .probe = sandbox_pinctrl_probe,
  

•   .remove = sandbox_pinctrl_remove,
  

•   .ops = &sandbox_pinctrl_ops,
  

+};

1.9.1

Regards, Simon