Re: [PATCH 2/2] ARM: dts: da850: add a node for the LCD controller

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
To: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Mark Rutland <mark.rutland@arm.com>,
	Karl Beldan <karl.beldan@gmail.com>,
	linux-devicetree <devicetree@vger.kernel.org>,
	Kevin Hilman <khilman@baylibre.com>,
	Michael Turquette <mturquette@baylibre.com>,
	Sekhar Nori <nsekhar@ti.com>,
	Russell King <linux@armlinux.org.uk>,
	linux-drm <dri-devel@lists.freedesktop.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Peter Ujfalusi <peter.ujfalusi@ti.com>,
	Rob Herring <robh+dt@kernel.org>,
	Karl Beldan <kbeldan@baylibre.com>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Maxime Ripard <maxime.ripard@free-electrons.com>,
	Jyri Sarha <jsarha@ti.com>, Frank Rowand <frowand.list@gmail.com>,
	arm-soc <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH 2/2] ARM: dts: da850: add a node for the LCD controller
Date: Mon, 17 Oct 2016 16:01:21 +0200	[thread overview]
Message-ID: <CAMpxmJXT8hcSc0-svbwAq5r_JOFsYV852gtE0vuXL8hHykXnRQ@mail.gmail.com> (raw)
In-Reply-To: <daf49046-ad0a-d9e2-d744-1e06085c6585@ti.com>

2016-10-17 14:29 GMT+02:00 Tomi Valkeinen <tomi.valkeinen@ti.com>:
> On 17/10/16 14:40, Laurent Pinchart wrote:
>> Hello,
>>
>> On Monday 17 Oct 2016 10:33:58 Tomi Valkeinen wrote:
>>> On 17/10/16 10:12, Sekhar Nori wrote:
>>>> On Monday 17 October 2016 11:26 AM, Tomi Valkeinen wrote:
>>>>> On 15/10/16 20:42, Sekhar Nori wrote:
>>>>>>> diff --git a/arch/arm/boot/dts/da850.dtsi
>>>>>>> b/arch/arm/boot/dts/da850.dtsi
>>>>>>> index f79e1b9..32908ae 100644
>>>>>>> --- a/arch/arm/boot/dts/da850.dtsi
>>>>>>> +++ b/arch/arm/boot/dts/da850.dtsi
>>>>>>> @@ -399,6 +420,14 @@
>>>>>>>                                  <&edma0 0 1>;
>>>>>>>                          dma-names = "tx", "rx";
>>>>>>>                  };
>>>>>>> +
>>>>>>> +                display: display@213000 {
>>>>>>> +                        compatible = "ti,am33xx-tilcdc", "ti,da850-tilcdc";
>>>>>>
>>>>>> This should instead be:
>>>>>>
>>>>>> compatible = "ti,da850-tilcdc", "ti,am33xx-tilcdc";
>>>>>>
>>>>>> as the closest match should appear first in the list.
>>>>>
>>>>> Actually I don't think that's correct. The LCDC on da850 is not
>>>>> compatible with the LCDC on AM335x. I think it should be just
>>>>> "ti,da850-tilcdc".
>>>>
>>>> So if "ti,am33xx-tilcdc" is used, the display wont work at all? If thats
>>>> the case, I wonder how the patch passed testing. Bartosz?
>>>
>>> AM3 has "version 2" of LCDC, whereas DA850 is v1. They are quite
>>> similar, but different.
>>>
>>> The driver gets the version number from LCDC's register, and acts based
>>> on that, so afaik the compatible string doesn't really affect the
>>> functionality (as long as it matches).
>>>
>>> But even if it works with the current driver, I don't think
>>> "ti,am33xx-tilcdc" and "ti,da850-tilcdc" are compatible in the HW level.
>>
>> If the hardware provides IP revision information, how about just "ti,lcdc" ?
>
> Maybe, and I agree that's the "correct" way, but looking at the history,
> it's not just once or twice when we've suddenly found out some
> difference or bug or such in an IP revision, or the integration to a
> SoC, that can't be found based on the IP revision.
>
> That's why I feel it's usually safer to have the SoC revision there in
> the compatible string.
>
> That said, we have only a few different old SoCs with LCDC (compared to,
> say, OMAP DSS) so in this case perhaps just "ti,lcdc" would be fine.
>
>  Tomi
>

I Sekhar is ok with this, I'll send a follow-up patch for that.

Thanks,
Bartosz
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel