Re: [PATCH v2 6/9] pinctrl-tz1090: add TZ1090 pinctrl driver

[Linus Walleij]

Hi James, I want to route this patch by Laurent Pinchart, as he's also
adding device tree support for a SoC using generic pinconf.

Creating new bindings for every pin controller defining the same thing
and implementing the same OF parsers is not sane.

We need to:

- Define generic pinconf bindings
- Implement a common parser for these in drivers/pinctrl/pinconf-generic.c

I'll merge your patches to <linux/pinctrl/pinctrl-generic.h> as a
baseline.

On Fri, May 24, 2013 at 6:21 PM, James Hogan <james.hogan@xxxxxxxxxx> wrote:

(...)

> +Optional subnode-properties:
> +- function: A string containing the name of the function to mux to the pin or
> +  group. Valid values for function names are listed below, including which
> +  pingroups can be muxed to them.
> +- tristate: Flag, put pin into high impedance state.
> +- pull-up: Flag, pull pin high.
> +- pull-down: Flag, pull pin low.
> +- bus-hold: Flag, weak latch last value on tristate bus.
> +- schmitt: Integer, enable or disable Schmitt trigger mode for the pins.
> +    0: no hysteresis
> +    1: schmitt trigger
> +- slew-rate: Integer, control slew rate of pins.
> +    0: slow (half frequency)
> +    1: fast
> +- drive-strength: Integer, control drive strength of pins in mA.
> +    2: 2mA
> +    4: 4mA
> +    8: 8mA
> +    12: 12mA

So if you want to use these names they shall be made generic.

Else you have to prefix everything with "tz1090,<property>"

> +/* Describes pinconf properties/flags available from device tree */
> +static const struct cfg_param {
> +       const char *property;
> +       enum pin_config_param param;
> +       bool flag;
> +} cfg_params[] = {
> +       {"tristate",            PIN_CONFIG_BIAS_HIGH_IMPEDANCE,         true},
> +       {"pull-up",             PIN_CONFIG_BIAS_PULL_UP,                true},
> +       {"pull-down",           PIN_CONFIG_BIAS_PULL_DOWN,              true},
> +       {"bus-hold",            PIN_CONFIG_BIAS_BUS_HOLD,               true},
> +       {"schmitt",             PIN_CONFIG_INPUT_SCHMITT_ENABLE,        true},
> +       {"slew-rate",           PIN_CONFIG_SLEW_RATE,                   false},
> +       {"drive-strength",      PIN_CONFIG_DRIVE_STRENGTH,              false},
> +};
> +
> +int tz1090_pinctrl_dt_subnode_to_map(struct device *dev,
> +                                    struct device_node *np,
> +                                    struct pinctrl_map **map,
> +                                    unsigned *reserved_maps,
> +                                    unsigned *num_maps)
> +{
> +       int ret, i;
> +       const char *function;
> +       u32 val;
> +       unsigned long config;
> +       unsigned long *configs = NULL;
> +       unsigned num_configs = 0;
> +       unsigned reserve;
> +       struct property *prop;
> +       const char *group;
> +
> +       ret = of_property_read_string(np, "function", &function);
> +       if (ret < 0) {
> +               /* EINVAL=missing, which is fine since it's optional */
> +               if (ret != -EINVAL)
> +                       dev_err(dev, "could not parse property function\n");
> +               function = NULL;
> +       }
> +
> +       for (i = 0; i < ARRAY_SIZE(cfg_params); i++) {
> +               ret = of_property_read_u32(np, cfg_params[i].property, &val);
> +               /* flags don't have to have a value */
> +               if (ret == -EOVERFLOW && cfg_params[i].flag) {
> +                       val = 1;
> +                       ret = 0;
> +               }
> +               if (!ret) {
> +                       config = pinconf_to_config_packed(cfg_params[i].param,
> +                                                         val);
> +                       ret = add_config(dev, &configs, &num_configs, config);
> +                       if (ret < 0)
> +                               goto exit;
> +               /* EINVAL=missing, which is fine since it's optional */
> +               } else if (ret != -EINVAL) {
> +                       dev_err(dev, "could not parse property %s (%d)\n",
> +                               cfg_params[i].property, ret);
> +               }
> +       }
> +
> +       reserve = 0;
> +       if (function != NULL)
> +               reserve++;
> +       if (num_configs)
> +               reserve++;
> +       ret = of_property_count_strings(np, "pins");
> +       if (ret < 0) {
> +               dev_err(dev, "could not parse property pins\n");
> +               goto exit;
> +       }
> +       reserve *= ret;
> +
> +       ret = reserve_map(dev, map, reserved_maps, num_maps, reserve);
> +       if (ret < 0)
> +               goto exit;
> +
> +       of_property_for_each_string(np, "pins", prop, group) {
> +               if (function) {
> +                       ret = add_map_mux(map, reserved_maps, num_maps,
> +                                         group, function);
> +                       if (ret < 0)
> +                               goto exit;
> +               }
> +
> +               if (num_configs) {
> +                       ret = add_map_configs(dev, map, reserved_maps,
> +                                             num_maps, group, configs,
> +                                             num_configs);
> +                       if (ret < 0)
> +                               goto exit;
> +               }
> +       }
> +
> +       ret = 0;
> +
> +exit:
> +       kfree(configs);
> +       return ret;
> +}

This is looking good. Can you split out the pin config mapping in
some way and put that into pinconf-generic.c?

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/