Re: [PATCH v10 05/15] dt-bindings: soc: amd: amd,pensando-elbasr: Add AMD Pensando SoC System Controller

From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
To: Brad Larson <blarson@amd.com>
Cc: adrian.hunter@intel.com, alcooperx@gmail.com,
	andy.shevchenko@gmail.com, arnd@arndb.de,
	brendan.higgins@linux.dev, briannorris@chromium.org,
	brijeshkumar.singh@amd.com, broonie@kernel.org,
	catalin.marinas@arm.com, davidgow@google.com,
	devicetree@vger.kernel.org, fancer.lancer@gmail.com,
	gerg@linux-m68k.org, gsomlo@gmail.com, krzk@kernel.org,
	krzysztof.kozlowski+dt@linaro.org, lee.jones@linaro.org,
	lee@kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, linux-mmc@vger.kernel.org,
	linux-spi@vger.kernel.org, p.yadav@ti.com,
	p.zabel@pengutronix.de, piotrs@cadence.com,
	rdunlap@infradead.org, robh+dt@kernel.org, samuel@sholland.org,
	skhan@linuxfoundation.org, suravee.suthikulpanit@amd.com,
	thomas.lendacky@amd.com, tonyhuang.sunplus@gmail.com,
	ulf.hansson@linaro.org, vaishnav.a@ti.com, will@kernel.org,
	yamada.masahiro@socionext.com
Subject: Re: [PATCH v10 05/15] dt-bindings: soc: amd: amd,pensando-elbasr: Add AMD Pensando SoC System Controller
Date: Thu, 9 Mar 2023 09:46:59 +0100	[thread overview]
Message-ID: <2691debf-0596-c265-468f-bb66d19f6db0@linaro.org> (raw)
In-Reply-To: <20230307021822.71245-1-blarson@amd.com>

On 07/03/2023 03:18, Brad Larson wrote:
> On 06/03/2023 09:35, Krzysztof Kozlowski wrote:
>>> On 06/03/2023 05:07, Brad Larson wrote:
>>>> Support the AMD Pensando SoC Controller which is a SPI connected device
>>>> providing a miscellaneous set of essential board control/status registers.
>>>> This device is present in all Pensando SoC based designs.
>>>>
>>>> Signed-off-by: Brad Larson <blarson@amd.com>
>>>> ---
>>>>
>>>> v10 changes:
>>>> - Property renamed to amd,pensando-ctrl
>>>> - Driver is renamed and moved to soc/drivers/amd affecting binding
>>>> - Delete cs property, driver handles device node creation from parent num-cs
>>>>   fixing schema reg error in a different way
>>>>
>>>> v9 changes:
>>>> - Instead of four nodes, one per chip-select, a single
>>>>   node is used with reset-cells in the parent.
>>>> - No MFD API is used anymore in the driver so it made
>>>>   sense to move this to drivers/spi.
>>>> - This driver is common for all Pensando SoC based designs
>>>>   so changed the name to pensando-sr.c to not make it Elba
>>>>   SoC specific.
>>>> - Added property cs for the chip-select number which is used
>>>>   by the driver to create /dev/pensr0.<cs>
>>>>
>>>> ---
>>>>  .../bindings/soc/amd/amd,pensando-ctrl.yaml   | 60 +++++++++++++++++++
>>>>  1 file changed, 60 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/soc/amd/amd,pensando-ctrl.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/soc/amd/amd,pensando-ctrl.yaml b/Documentation/devicetree/bindings/soc/amd/amd,pensando-ctrl.yaml
>>>> new file mode 100644
>>>> index 000000000000..36694077b2e6
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/soc/amd/amd,pensando-ctrl.yaml
>>>
>>> Your subject suggests this is pensando-elbasr but you write everywhere
>>> pensando-ctrl. Confusing. Pick one.
>>
>> Actually pensando-ctrl is for sure not correct, because it misses the
>> name of the SoC (you call it everywhere "elba").
> 
> The reason I dropped elba as part of the name is this driver and its associated 
> SPI attached device (cpld or fpga depending on the board design) will be used
> across a series of SoCs starting with Elba.  Implying its Elba specific is misleading.

Compatibles must be specific.
https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L42
If this is SoC part it must match SoC. What is misleading in this? Why
Pensando is different than all other SoCs (I am really getting tired
everytime asking why people think their solution is special)?

If this is not part of the SoC, then your commit msg is misleading.
Maybe bindings as well, so rework it.

Best regards,
Krzysztof