Re: [PATCH 08/12] doc: binding: pwrseq-usb-generic: add binding doc for generic usb power sequence driver

[Peter Chen <hzpeterchen@gmail.com>]

On Tue, Jun 21, 2016 at 04:26:53PM -0500, Rob Herring wrote:
> On Tue, Jun 21, 2016 at 10:11:17AM +0800, Peter Chen wrote:
> > On Mon, Jun 20, 2016 at 11:16:07AM -0500, Rob Herring wrote:
> > > On Mon, Jun 20, 2016 at 07:26:51PM +0800, Peter Chen wrote:
> > > > On Fri, Jun 17, 2016 at 12:16:48PM -0500, Rob Herring wrote:
> > > > > On Fri, Jun 17, 2016 at 5:09 AM, Peter Chen <peter.chen@nxp.com> wrote:
> > > > > > Add binding doc for generic usb power sequence driver, and update
> > > > > > generic usb device binding-doc accordingly.
> 
> [...]
> 
> > > > 		clocks = <&clks IMX6SX_CLK_CKO>;
> > > > 
> > > > 		#address-cells = <1>;
> > > > 		#size-cells = <0>;
> > > > 		ethernet: asix@1 {
> > > > 			compatible = "usbb95,1708";
> > > > 			reg = <1>;
> > > > 
> > > > 			power-sequence;
> > > > 			reset-gpios = <&gpio4 6 GPIO_ACTIVE_LOW>; /* ethernet_rst */
> > > > 			reset-duration-us = <15>;
> > > > 			clocks = <&clks IMX6SX_CLK_IPG>;
> > > > 		};
> > > > 	};
> > > > };
> > > > 
> > > > If the node has property "power-sequence", the pwrseq core will create
> > > > related platform device, and the driver under pwrseq driver will handle
> > > > power sequence stuffs. 
> > > 
> > > This I have issue with. If you are creating a platform device here, you 
> > > are trying to work-around limitations in the linux driver model.
> > 
> > My current solution like below, but it seems you didn't agree with that.
> > I just double confirm here, if you don't, I give up the solution for
> > using generic power sequence framework.
> > 
> > In drivers/usb/core/hub.c
> > 
> > 	for_each_child_of_node(parent->of_node, node) {
> > 		hdev_pwrseq = pwrseq_alloc(node, "usb_pwrseq_generic");
> > 		if (!IS_ERR_OR_NULL(hdev_pwrseq)) {
> > 			pwrseq_node = kzalloc(sizeof(pwrseq_node), GFP_KERNEL);
> > 			if (!pwrseq_node) {
> > 				ret = -ENOMEM;
> > 				goto err1;
> > 			}
> > 			/* power on sequence */
> > 			ret = pwrseq_pre_power_on(hdev_pwrseq);
> 
> Why does this function need to do anything more than:
> 
> - Check if the child has a "power-sequence" property
> - Get the "reset-gpios" GPIO
> - Assert reset for specified/default time
> - Deassert reset
> 

Besides gpios, it may exist clock and regulator operation, and the
operation may be failed. I thought these operations can be easy
done belongs to a device, but now, let me try this straight-forward
way, thanks.

Peter
> Then continue on as normal. That seems straight-forward to me.
> 
> There is no reason you need a platform device in the mix. Perhaps trying 
> to move the MMC pwr-seq code is pointless as it adds needless 
> complexity.
> 
> [...]
> 
> > > Either 
> > > we need some sort of pre-probe hook to the drivers to call or each 
> > > parent node driver is responsible for checking and calling pwr-seq 
> > > functions for child nodes. e.g. The host controller calls pwr-seq for 
> > > the hub, the hub driver calls the power seq for the asix chip. Soon as 
> > > we have a case too complex for the generic pwr-seq, we're going to need 
> > > the pre-probe hook as I don't want to see a continual expansion of 
> > > generic pwr-seq binding for ever more complex cases.
> > > 
> > 
> > How the driver know what it needs to handle (eg, gpio, clock) if there
> > is no device for it? The most important we need to consider is which
> > device owns there power sequence properties, then the corresponding
> > driver can handle it.
> 
> What can be handled by is defined by presence of power-sequence 
> property. There can be 1 driver for the device. That is the USB hub 
> driver in this example. You should not have 2 "devices".
> 
> Rob

-- 

Best Regards,
Peter Chen