[PATCH v2 01/10] pinctrl: Add pinmux property support to pinctrl-generic

Simon Glass
2020-06-17T03:11:28Z

Hi Sean,

On Sun, 7 Jun 2020 at 19:27, Sean Anderson <seanga2@gmail.com> wrote:
>
> The pinmux property allows for smaller and more compact device trees,
> especially when there are many pins which need to be assigned individually.
> Instead of specifying an array of strings to be parsed as pins and a
> function property, the pinmux property contains an array of integers
> representing pinmux groups. A pinmux group consists of the pin identifier
> and mux settings represented as a single integer or an array of integers.
> Each individual pin controller driver specifies the exact format of a
> pinmux group. As specified in the Linux documentation, a pinmux group may
> be multiple integers long. However, no existing drivers use multi-integer
> pinmux groups, so I have chosen to omit this feature. This makes the
> implementation easier, since there is no need to allocate a buffer to do
> endian conversions.
>
> Support for the pinmux property is done differently than in Linux. As far
> as I can tell, inversion of control is used when implementing support for
> the pins and groups properties to avoid allocating. This results in some
> duplication of effort; every property in a config node is parsed once for
> each pin in that node. This is not such an overhead with pins and groups
> properties, since having multiple pins in one config node does not occur
> especially often. However, the semantics of the pinmux property make such a
> configuration much more appealing. A future patch could parse all config
> properties at once and store them in an array. This would make it easier to
> create drivers which do not function solely as callbacks from
> pinctrl-generic.
>
> This commit increases the size of the sandbox build by approximately 48
> bytes. However, it also decreases the size of the K210 device tree by 2
> KiB from the previous version of this series.
>
> The documentation has been updated from the last Linux commit before it was
> split off into yaml files.
>
> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> ---
>
> Changes in v2:
> - New
>
> .../pinctrl/pinctrl-bindings.txt | 65 ++++++++-
> drivers/pinctrl/pinctrl-generic.c | 125 ++++++++++++++----
> include/dm/pinctrl.h | 21 +--
> 3 files changed, 168 insertions(+), 43 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[..]

> diff --git a/include/dm/pinctrl.h b/include/dm/pinctrl.h
> index 692e5fc8cb..d50af1ce38 100644
> --- a/include/dm/pinctrl.h
> +++ b/include/dm/pinctrl.h
> @@ -34,29 +34,33 @@ struct pinconf_param {
> * depending on your necessity.
> *
> * @get_pins_count: return number of selectable named pins available
> - * in this driver. (necessary to parse "pins" property in DTS)
> + * in this driver. (necessary to parse "pins" property in DTS)

What is happening here? I don't think we want the period.

> * @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)
> + * 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)
> + * 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)
> + * 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)
> + * 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)
> + * 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)
> + * 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. 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)
> + * @pinmux_property_set: enable a pinmux group. @pinmux_group should specify the
> + * pin identifier and mux settings. The exact format of a pinmux group is
> + * left up to the driver. The pin selector for the mux-ed pin should be
> + * returned on success. (necessary to parse the "pinmux" property in DTS)
> * @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
> @@ -90,6 +94,7 @@ struct pinctrl_ops {
> unsigned func_selector);
> int (*pinmux_group_set)(struct udevice *dev, unsigned group_selector,
> unsigned func_selector);
> + int (*pinmux_property_set)(struct udevice *dev, u32 pinmux_group);

I am not a fan of how the comments work in this file and would prefer
that each function is commented individually. If you have the energy,
you could do a follow-up patch.

> unsigned int pinconf_num_params;
> const struct pinconf_param *pinconf_params;
> int (*pinconf_set)(struct udevice *dev, unsigned pin_selector,
> --
> 2.26.2
>

Regards,
Simon