Re: [PATCH v5 09/16] pwm: tegra: Add device tree support

[Grant Likely <grant.likely@secretlab.ca>]

On Wed, 04 Apr 2012 12:32:04 -0600, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 04/03/2012 11:00 PM, Thierry Reding wrote:
> > * Grant Likely wrote:
> >> On Tue, 3 Apr 2012 19:55:11 +0200, Thierry Reding <thierry.reding@avionic-design.de> wrote:
> >>> * Stephen Warren wrote:
> >>>> On 04/02/2012 02:37 AM, Thierry Reding wrote:
> >>>>> * Stephen Warren wrote:
> >>>>>> On 03/28/2012 08:33 AM, Thierry Reding wrote:
> >>>>>>> Add auxdata to instantiate the PWFM controller from a device tree,
> >>>>>>> include the corresponding nodes in the dtsi files for Tegra 20 and
> >>>>>>> Tegra 30 and add binding documentation.
> >>>>>>>
> >>>>>>> Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
> >>>>>>> Acked-by: Stephen Warren <swarren@wwwdotorg.org>
> >>>>>>
> >>>>>>> diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
> >>>>>> ...
> >>>>>>> +#ifdef CONFIG_OF
> >>>>>>> +static struct of_device_id tegra_pwm_of_match[] = {
> >>>>>>> +	{ .compatible = "nvidia,tegra20-pwm" },
> >>>>>>> +	{ .compatible = "nvidia,tegra30-pwm" },
> >>>>>>
> >>>>>> Could you swap those two lines, so that tegra30-pwm matches first. It
> >>>>>> makes no difference at present, but might in the future if the driver
> >>>>>> actually has to differentiate the two SoCs.
> >>>>>
> >>>>> I thought the matching order was determined by the compatible property in the
> >>>>> device tree, not the OF match table of the driver.
> >>>>
> >>>> At least logically, yes. However, of_match_device() appears to iterate
> >>>> over each match table entry, checking whether it matches any string in
> >>>> the compatible flag. Perhaps this could be considered a bug?
> >>>
> >>> It certainly is counter-intuitive. Maybe Grant or Rob can comment?
> >>
> >> Yes, it is a bug.  The order of of_device_id should be entirely
> >> irrelevant, and the order in the DT compatible property should
> >> determine which match entry is returned.
> > 
> > I've had a look at the code and it looks like a fix will not be entirely
> > trivial. I think moving out the compatible check out of the while loop in
> > of_match_node() and separately iterate over all strings in the compatible
> > property would be the easiest. That will also prioritize the compatible
> > match over matches by name and type but I think that's exactly what we
> > want. From a quick look it certainly seems like the large majority of
> > drivers match by compatible anyway.
> > 
> > Do you want me to prepare a patch or can you take care of it?

yes please.

g.