Re: [PATCH V2 1/4] dt-bindings: display: panel: Update NewVision NV3051D compatibles

From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
To: Chris Morgan <macromorgan@hotmail.com>
Cc: devicetree@vger.kernel.org, conor+dt@kernel.org,
	krzysztof.kozlowski+dt@linaro.org, neil.armstrong@linaro.org,
	sam@ravnborg.org, sebastian.reichel@collabora.com,
	dri-devel@lists.freedesktop.org,
	Chris Morgan <macroalpha82@gmail.com>,
	linux-rockchip@lists.infradead.org, robh+dt@kernel.org
Subject: Re: [PATCH V2 1/4] dt-bindings: display: panel: Update NewVision NV3051D compatibles
Date: Fri, 10 Nov 2023 15:52:19 +0100	[thread overview]
Message-ID: <ad64aa78-1874-4a92-8a79-6f45bb19dbe2@linaro.org> (raw)
In-Reply-To: <SN6PR06MB5342A696F25065F9C253154CA5AEA@SN6PR06MB5342.namprd06.prod.outlook.com>

On 10/11/2023 15:28, Chris Morgan wrote:
> On Fri, Nov 10, 2023 at 02:11:58PM +0100, Krzysztof Kozlowski wrote:
>> On 09/11/2023 22:50, Chris Morgan wrote:
>>> From: Chris Morgan <macromorgan@hotmail.com>
>>>
>>> Update the NewVision NV3051D compatible strings by adding a new panel,
>>> the powkiddy,rk2023-panel, and removing another entry, the
>>> anbernic,rg353v-panel. The rg353v-panel is exactly identical to the
>>> rg353p-panel and is not currently in use by any existing device tree.
>>> The rk2023-panel is similar to the rg353p-panel but has slightly
>>> different timings.
>>>
>>> I originally wrote the driver checking for the newvision,nv3051d
>>> compatible string which worked fine when there was only 1 panel type.
>>> When I added support for the 351v-panel I *should* have changed how the
>>> compatible string was handled, but instead I simply added a check in the
>>> probe function to look for the secondary string of
>>> "anbernic,rg351v-panel". Now that I am adding the 3rd panel type of
>>> "powkiddy,rk2023-panel" I am correcting the driver to do it the right
>>> way by checking for the specific compatibles.
>>
>> I don't understand how any of this driver behavior is a reason to drop
>> rg353v. You wrote two paragraphs to justify this removal, but I feel the
>> only reason is that rg353v is just not needed, because it is duplicating
>> rg353p? Is this right? You actually did not write it explicitly...
> 
> Sorry if I wasn't clear, I did note that the rg353p-panel is exactly
> identical to the rg353v-panel. Should I add additional details beyond
> that to clarify?

The entire paragraph about driver feels redundant. Your first paragraph
should still say why you remove it.

Best regards,
Krzysztof