Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states

[Tony Lindgren]

* Tony Lindgren <tony@xxxxxxxxxxx> [130718 00:31]:
> * Stephen Warren <swarren@xxxxxxxxxxxxx> [130717 14:21]:
> > On 07/16/2013 03:05 AM, Tony Lindgren wrote:
> > > +int pinctrl_check_dynamic(struct device *dev, struct pinctrl_state *st1,
> > > +			  struct pinctrl_state *st2)
> > > +{
> > > +	struct pinctrl_setting *s1, *s2;
> > > +
> > > +	list_for_each_entry(s1, &st1->settings, node) {
> > ...
> > > +		list_for_each_entry(s2, &st2->settings, node) {
> > ...
> > > +			if (pctldev1 != pctldev2) {
> > > +				dev_dbg(dev, "pctldev must be the same for states\n");
> > > +				return -EINVAL;
> > > +			}
> > 
> > I don't think we should require that; it's perfectly legal at the moment
> > for some device's pinctrl configuration to include settings from
> > multiple different pin controllers.
> 
> Yes that's fine for pinctrl_select(), but let's not do that for
> runtime muxing.

Hmm reading this again, you're right, there should not be anything
preventing mixing multiple controllers as long as the states match.
 
> > > +			for (i = 0; i < num_pins1; i++) {
> > > +				int pin1 = pins1[i];
> > > +
> > > +				for (j = 0; j < num_pins2; j++) {
> > > +					int pin2 = pins2[j];
> > > +
> > > +					if (pin1 == pin2) {
> > > +						found++;
> > > +						break;
> > > +					}
> > > +				}
> > > +			}
> > > +
> > > +			if (found != num_pins1) {
> > 
> > I guess this make sure that every entry in the dynamic state is in the
> > state state, but not vice-versa; the static state can affect more stuff
> > than the dynamic state?
> > 
> > If so, shouldn't that check be if (found != num_pins2)?
> 
> The check is that idle_state and sleep_state pins must match the
> active_state pins. This is intentionally different from the current
> pinctrl_select() logic.

And here things will get messed up if the order of settings is diffrent..
So yes, pinctrl_check_dynamic() needs more work.

Regards,

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