linux-kernel - Re: [PATCH v2 1/9] pinctrl: mvebu: pinctrl driver core

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 23 Aug 2012 11:54:12 -0600
From:	Stephen Warren <swarren@...dotorg.org>
To:	Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>
CC:	Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>,
	Grant Likely <grant.likely@...retlab.ca>,
	Rob Herring <rob.herring@...xeda.com>,
	Rob Landley <rob@...dley.net>,
	Russell King <linux@....linux.org.uk>,
	Lior Amsalem <alior@...vell.com>, Andrew Lunn <andrew@...n.ch>,
	Gregory CLEMENT <gregory.clement@...e-electrons.com>,
	Ben Dooks <ben.dooks@...ethink.co.uk>,
	Linus Walleij <linus.walleij@...aro.org>,
	devicetree-discuss@...ts.ozlabs.org, linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v2 1/9] pinctrl: mvebu: pinctrl driver core

On 08/23/2012 03:45 AM, Sebastian Hesselbarth wrote:
> On 8/22/12, Stephen Warren <swarren@...dotorg.org> wrote:
>> On 08/22/2012 02:22 AM, Sebastian Hesselbarth wrote:
>>> +++ b/drivers/pinctrl/pinctrl-mvebu.c
>>
>>> +static int mvebu_pinctrl_dt_node_to_map(struct pinctrl_dev *pctldev,
>>
>>> +	of_property_for_each_string(np, "marvell,pins", prop, group) {
>>> +		struct mvebu_pinctrl_group *grp =
>>> +			mvebu_pinctrl_find_group_by_name(pctl, group);
>>> +
>>> +		if (!grp) {
>>> +			dev_err(pctl->dev, "unknown pin %s", group);
>>> +			continue;
>>> +		}
>>> +
>>> +		if (!mvebu_pinctrl_find_setting_by_name(pctl, grp, function)) {
>>> +			dev_err(pctl->dev, "unsupported function %s on pin %s",
>>> +				function, group);
>>> +			continue;
>>> +		}
>>
>> The error-checking here isn't strictly necessary; the pinctrl core will
>> error-check all the names if/when the map entries are used. So, you
>> could probably get away with just assigning the pin/function names
>> directly into (*map) and hence remove some code here. Still, it's not a
>> big deal either way.
> 
> Hi Stephen,
> 
> as these error checks are only performed once, I'd like to have them there
> as they give hints when someone uses wrong marvell,pins/function names
> in DT.
> 
>>> +static int __devinit mvebu_pinctrl_dt_parse_function(struct mvebu_pinctrl
>>> *pctl,
>>
>>> +static int __devinit mvebu_pinctrl_dt_parse(struct platform_device
>>> *pdev,
>>> +					struct mvebu_pinctrl *pctl)
>>
>> I don't understand what those two functions do, or a good chunk of
>> probe(). It seems like they're setting up the ability to get/set pin
>> configrations in addition to pin muxing, although if that's so, it seems
>> odd that none of the binding documents specify any way of controlling
>> pin configuration from device tree. Are you expecting drivers to call
>> APIs such as pin_config_set() directly, rather than controlling pin
>> config through DT?
> 
> probe() receives some structs from the SoC specific driver stubs that
> describe, e.g. how many muxable pins are available, which pingroup they
> belong to (marvell,pins), what valid register values are and how they are
> named (marvell,function). Although, all mvebu SoC share a good part of
> pinmux register layout, there are especially for dove some very different
> ways to mux pins. As mvebu pinctrl is a true mux-only driver, i.e. no
> pin drive strength or pin direction configuration, we decided not to have
> single pins controlable but only pingroups even if they comprise only a
> single pin.
> 
> dt_parse() and dt_parse_function() build up structs that get used later on
> in mvebu_pinmux_ops that require indexed functions. I can join them with
> dt_node_to_map() but that would require incremental kzalloc for the
> corresponding array. Or I could (functionally) leave dt_parse() to allocate
> the array and only join dt_parse_function() with dt_node_to_map().

So everything you said makes sense, in that the core driver is
parameterizable and receives data from a SoC-variant-specific driver
indicating which pins/groups/functions are available. I'm still not sure
though why the translation of the pin/group/function structures passed
to probe into other data structures requires accessing the DT at all;
the set of available pins/groups/functions isn't configured through DT,
and doesn't need to be limited to only those options actually used in
DT, so can't you just process all the data that's passed to probe
without interaction with the DT?

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