On 09/07/2012 07:55 PM, Tony Lindgren wrote:
> * Peter Ujfalusi <peter.ujfalusi@xxxxxx> [120907 08:13]:
>> On 09/06/2012 10:10 PM, Tony Lindgren wrote:
>>>
>>> Is it now safe to assume that we always have width of three if
>>> pinctrl-single,bits is specified? The reason I'm asking is..
>>>
>>>> @@ -657,18 +664,29 @@ static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs,
>>>> {
>>>> struct pcs_func_vals *vals;
>>>> const __be32 *mux;
>>>> - int size, rows, *pins, index = 0, found = 0, res = -ENOMEM;
>>>> + int size, params, rows, *pins, index = 0, found = 0, res = -ENOMEM;
>>>> struct pcs_function *function;
>>>>
>>>> - mux = of_get_property(np, PCS_MUX_NAME, &size);
>>>> - if ((!mux) || (size < sizeof(*mux) * 2)) {
>>>> - dev_err(pcs->dev, "bad data for mux %s\n",
>>>> - np->name);
>>>> + mux = of_get_property(np, PCS_MUX_PINS_NAME, &size);
>>>> + if (mux) {
>>>> + params = 2;
>>>> + } else {
>>>> + mux = of_get_property(np, PCS_MUX_BITS_NAME, &size);
>>>> + if (!mux) {
>>>> + dev_err(pcs->dev, "no valid property for %s\n",
>>>> + np->name);
>>>> + return -EINVAL;
>>>> + }
>>>> + params = 3;
>>>> + }
>>>
>>> ..because here we could assume the default value for params is 2
>>> if pinctrl-single,pins is specified, and otherwise params is 3
>>> if pinctrl-single,bits is specified for the controller. That would
>>> avoid querying a potentially non-exiting property for each entry.
>>>
>>>> @@ -686,6 +704,10 @@ static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs,
>>>> val = be32_to_cpup(mux + index++);
>>>> vals[found].reg = pcs->base + offset;
>>>> vals[found].val = val;
>>>> + if (params == 3) {
>>>> + val = be32_to_cpup(mux + index++);
>>>> + vals[found].mask = val;
>>>> + }
>>>>
>>>> pin = pcs_get_pin_by_offset(pcs, offset);
>>>> if (pin < 0) {
>>>
>>> Here params too would be then set during probe already.
>>
>> I'm afraid you lost me here...
>> We only know if the user specified the mux configuration with
>> pinctrl-single,pins or pinctrl-single,bits in this function.
>
> Sorry right, yeah we don't know that at probe time currently.
>
> I'd like to have something that specifies the controller type so
> we don't need to mix two types of controllers and test for
> non-existing properties when parsing the pins.
>
> How about we require something specified for the pinctrl driver
> in the "one-bit-per-mux" case like pinctrl-single,bit-per-mux?
>
> And then in the pinctrl-single probe we set params = 3 if
> pinctrl-single,bit-per-mux is specified. And if no
> pinctrl-single,bit-per-mux is specified then set params = 2.
>
> That way pcs_parse_one_pinctrl_entry() can just test for params.
>
> Sorry I don't have a better name in mind than bit-per-mux..
I'm not sure if this would make things more prone to error or make the DTS or
the code more clean in any ways.
Both pinctrl-single,pins and pinctrl-single,bits works on top of the same
pinctrl-single area.
In most cases we are going to use pinctrl-single,pins for the mux
configuration and only in few places we need to fall back to pinctrl-single,bits.
With this patch we will have maximum of two query to find out which type is
used, while if we add the 'bit-per-mux' property we will always have need to
have two of_get_property() call to figure out what is used.
IMHO in this way we could also have copy-paste errors coming at us when adding
the mux configurations for the pins and at the end we also need to do same
safety/sanity checks if the user really provided the correct type with
correlation to the 'bit-per-mux'.
This would just complicate the code.
The existence of pinctrl-single,pins or pinctrl-single,bits property alone
gives enough information for the number of parameters.
>
>> One thing I could do to make the code a bit better to look at is:
>> int params = 2;
>>
>> mux = of_get_property(np, PCS_MUX_PINS_NAME, &size);
>> if (!mux) {
>> mux = of_get_property(np, PCS_MUX_BITS_NAME, &size);
>> if (!mux) {
>> dev_err(pcs->dev, "no valid property for %s\n",
>> np->name);
>> return -EINVAL;
>> }
>> params = 3;
>> }
>>
>> This might make the code a bit more compact but at the same time one might
>> need to spend few more seconds to understand it.
>
> Yes well there's no need to do of_get_property second guessing
> if we already know params from the pinctrl-single.c probe time.
>
> I think we're safe to assume that we don't need to mix parsing
> two different types of configuration for the same controller
> as they can always be set up as separate controllers.
>
> Regards,
>
> Tony
>
--
Péter
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
