Re: [PATCH 11/11] i2c: omap: enhance pinctrl support

[Kevin Hilman <khilman-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>]

+Linus Walleij (pinctrl maintainer)

Hebbar Gururaja <gururaja.hebbar-l0cyMroinI0@public.gmane.org> writes:

> Amend the I2C omap pin controller to optionally take a pin control
> handle and set the state of the pins to:
>
> - "default" on boot, resume and before performing an i2c transfer
> - "idle" after initial default, after resume default, and after each
> i2c xfer
> - "sleep" on suspend()
>
> By optionally putting the pins into sleep state in the suspend callback
> we can accomplish two things.
> - One is to minimize current leakage from pins and thus save power,
> - second, we can prevent the IP from driving pins output in an
> uncontrolled manner, which may happen if the power domain drops the
> domain regulator.
>
> Note:
> A .suspend & .resume callback is added which simply puts the pins to sleep
> state upon suspend & are moved to default & idle state upon resume.
>
> If any of the above pin states are missing in dt, a warning message
> about the missing state is displayed.
> If certain pin-states are not available, to remove this warning message
> pass respective state name with null phandler.
>
> (Changes based on i2c-nomadik.c)
>
> Signed-off-by: Hebbar Gururaja <gururaja.hebbar-l0cyMroinI0@public.gmane.org>
> Cc: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
> Cc: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>
> Cc: linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org

[...]

> @@ -1123,14 +1138,47 @@ omap_i2c_probe(struct platform_device *pdev)
>  		dev->set_mpu_wkup_lat = pdata->set_mpu_wkup_lat;
>  	}
>  
> -	dev->pins = devm_pinctrl_get_select_default(&pdev->dev);
> -	if (IS_ERR(dev->pins)) {
> -		if (PTR_ERR(dev->pins) == -EPROBE_DEFER)
> +	dev->pinctrl = devm_pinctrl_get(&pdev->dev);
> +	if (!IS_ERR(dev->pinctrl)) {
> +		dev->pins_default = pinctrl_lookup_state(dev->pinctrl,
> +							 PINCTRL_STATE_DEFAULT);

This part is already done by probe in driver core, why does it need to
be done again.  dev->pins->default_state should already have this.
(c.f. pinctrl_bind_pins() in drivers/base/pinctrl.c)

But that brings up a bigger question about whether or not we should be
doing the rest of this (idle/sleep) pin management in the drivers or in
the driver core as well.  I would much prefer it be handled by the
driver core.

In fact, since these are all PM related events, it should probably be
handled by the PM core and seems pretty straight forward to do so.

Kevin