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

[Sean Anderson]

On 6/16/20 11:11 PM, Simon Glass wrote:
> Hi Sean,
> 
> On Sun, 7 Jun 2020 at 19:27, Sean Anderson <seanga2 at 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 at 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 at 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.
> 

I'm just removing the double-spaces. The period here matches how the
rest of the comments also have a period before their parentheses
(despite that not being grammatically correct).

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

Sure, I can have a look at that.

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

--Sean