Hi Sean, On Sun, 7 Jun 2020 at 19:27, Sean Anderson <sean...@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 <sean...@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 <s...@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