Date: Tue, 12 Apr 2016 15:02:45 -0700
From: Tony Lindgren <tony@atomide.com>
To: Frank Rowand <frowand.list@gmail.com>
Cc: Grant Likely <grant.likely@linaro.org>, Rob Herring <robh+dt@kernel.org>,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-omap@vger.kernel.org, Nishanth Menon <nm@ti.com>,
        Tero Kristo <t-kristo@ti.com>, Tom Rini <trini@konsulko.com>
Subject: Re: [PATCH] of: Add generic handling for hardware incomplete fail
 state
Message-ID: <20160412220244.GK5995@atomide.com>
References: <1460486275-12256-1-git-send-email-tony@atomide.com>
 <570D56FE.2070408@gmail.com>
 <20160412203431.GU5995@atomide.com>
 <570D6B7A.3050203@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <570D6B7A.3050203@gmail.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2690
Lines: 69

* Frank Rowand <frowand.list@gmail.com> [160412 14:42]:
> On 4/12/2016 1:34 PM, Tony Lindgren wrote:
> > 
> > OK thanks for the clarification. I don't see why "fail-hw-incomplete"
> > could not be set dynamically during the probe in some cases based
> > on the SoC revision detection for example. So from that point of
> > view using status with the "fail-sss" logic would make more sense.
> 
> If the probe detects that the device should only be power managed
> based on the SoC revision, then it would simply be one more
> test added at the top of probe.  The patch would change from:
> 
>    if (of_device_is_incomplete(pdev->dev.of_node)) {
> 
> to:
> 
>    if (of_device_is_incomplete(pdev->dev.of_node) || socrev == XXX) {
> 
> That code would be the same whether the property involved was
> status or something else.

Yeah that should work if we had a generic way to get the runtime
socrev somehow :) I guess the closest thing is the ARM system_rev.

> >> I would prefer to come up with a new boolean property (with a
> >> standard name that any node binding could choose to implement)
> >> that says something like "only power management is available for
> >> this node, do not attempt to use any other feature of the node".
> > 
> > Heh that's going to be a long property name :) How about
> > unusable-incomplete-idle-only :)
> 
> Or even pm-only.  Maybe I got a little carried away with my
> verbosity. :)

That works for me unless somebody comes up with a better one.
I can only think unusable-for-io, which is no better.

> >> With that change, the bulk of your patch looks good, with
> >> minor changes:
> >>
> >>   __of_device_is_available() would not need to change.
> >>
> >>   __of_device_is_incomplete() would change to check the new
> >>   boolean property.  (And I would suggest renaming it to
> >>   something that conveys it is ok to power manage the
> >>   device, but do not do anything else to the device.)
> > 
> > I'm fine with property too, but the runtime probe fail state
> > changes worry me a bit with that one.
> 
> I don't understand what the concern is.  The change I suggested
> would use exactly the same code for probe as the example patch
> you provided, but just with a slight name change for the function.

I guess I'm just wondering if using property vs status will make
things harder to change during runtime. For example, let's assume
u-boot needs to set some devices to "pm-only" state based on the
SoC revision on a board.

> > I think Rob also preferred to use the status though while we
> > chatted at ELC?
> 
> That is the impression I got too.  We'll have to see if I can
> convince him otherwise.

Yeah let's wait for his comments.

Tony