Re: [PATCH] of/pdt: allow DT device matching by fixing 'name'brokenness

[Andres Salomon]

On Wed, 23 Feb 2011 12:43:52 -0700
Grant Likely <grant.likely@xxxxxxxxxxxx> wrote:

> On Fri, Feb 18, 2011 at 07:06:59PM -0800, Andres Salomon wrote:
> > On Fri, 18 Feb 2011 23:42:57 +0000
> > Daniel Drake <dsd@xxxxxxxxxx> wrote:
> > 
> > > On 16 February 2011 22:44, David Woodhouse <dwmw2@xxxxxxxxxxxxx>
> > > wrote:
> > > > On Wed, 2011-02-16 at 22:28 +0000, Daniel Drake wrote:
> > > >>
> > > >> +static int __init add_common_platform_devices(void)
> > > >> +{
> > > >> + Â Â Â struct platform_device *pdev;
> > > >> +
> > > >> + Â Â Â pdev = platform_device_register_simple("olpc-battery",
> > > >> -1, NULL, 0);
> > > >> + Â Â Â if (IS_ERR(pdev))
> > > >> + Â Â Â Â Â Â Â return PTR_ERR(pdev);
> > > >> +
> > > >> + Â Â Â return 0;
> > > >> +}
> > > >> +
> > > >
> > > > Still kind of sucks that you have to do this, and can't bind to
> > > > something in the device-tree.
> > > 
> > > OK, feel free to put this patch on hold for now. I started
> > > looking at the device tree approach today. It looks doable but
> > > first we have to fix a DT bug/inconsistency that is preventing us
> > > from correctly binding to the tree's devices.
> > > 
> > > Daniel
> > 
> > 
> > Mea culpa.  The patch below fixes a bug I introduced earlier.
> > Cc'ing the sparc folks, as this probably affects them
> > (although I would think that it fixes broken behavior for them..?)
> 
> Wait; why are you binding to a device based on name?  Binding by name
> and/or device_type is strongly discouraged for new code.  Use
> compatible instead.
> 

Daniel posted a separate patch showing his code, would you mind
commenting on that?  I noticed he didn't cc you though, here's the
patch:

https://patchwork.kernel.org/patch/574901/


> As for this patch, comments below...
> 
> > From: Andres Salomon <dilinger@xxxxxxxxxx>
> > 
> > Commit e2f2a93b changed dp->name from using the 'name' property to
> > using package-to-path.  This fixed /proc/device-tree creation by
> > eliminating conflicts between names (the 'name' property provides
> > names like 'battery', whereas package-to-path provides names like
> > '/foo/bar/battery@0', which we stripped to 'battery@0').  However,
> > it also breaks of_device_id table matching.
> > 
> > The fix that we _really_ wanted was to keep dp->name based upon
> > the name property ('battery'), but based dp->full_name upon
> > package-to-path ('battery@0').  This patch does just that.
> > 
> > Signed-off-by: Andres Salomon <dilinger@xxxxxxxxxx>
> > Reported-by: Daniel Drake <dsd@xxxxxxxxxx>
> 
> From what I can tell, this only affects OLPC, correct?  It looks like
> SPARC's implementation of of_pdt_node_name() will sidestep most of
> this name retrieval code.
> 


This affects sparc as well; it changes behavior back for dp->name to
what it used to be.  dp->full_name behavior is still the same for sparc.


> > ---
> >  drivers/of/pdt.c |   42 +++++++++++++++++++-----------------------
> >  1 files changed, 19 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c
> > index 28295d0..b39d584 100644
> > --- a/drivers/of/pdt.c
> > +++ b/drivers/of/pdt.c
> > @@ -48,7 +48,7 @@ static inline void irq_trans_init(struct
> > device_node *dp) { } 
> >  static inline const char *of_pdt_node_name(struct device_node *dp)
> >  {
> > -	return dp->name;
> > +	return NULL;
> >  }
> 
> Rather than using this hook; perhaps the sparc .path_component_name
> should be enabled for all architectures.  It is a useful data item to
> keep a pointer to, and it would simplify the of_pdt code.
> 
> >  

Yeah, I considered that, but pkg2path works even better for our
purposes.


I'll combine patches and resend, thanks.
--
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/