Re: [PATCH 2/3] dt-bindings: SPI: Add Ingenic SFC bindings.

From: Rob Herring <robh@kernel.org>
To: Mike Yang <reimu@sudomaker.com>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
	Mark Brown <broonie@kernel.org>,
	Zhou Yanjie <zhouyanjie@wanyeetech.com>,
	tudor.ambarus@microchip.com, p.yadav@ti.com, michael@walle.cc,
	miquel.raynal@bootlin.com, richard@nod.at, vigneshr@ti.com,
	krzysztof.kozlowski+dt@linaro.org, linux-mtd@lists.infradead.org,
	linux-spi@vger.kernel.org, linux-mips@vger.kernel.org,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
	aidanmacdonald.0x0@gmail.com, tmn505@gmail.com,
	paul@crapouillou.net, dongsheng.qiu@ingenic.com,
	aric.pzqi@ingenic.com, rick.tyliu@ingenic.com,
	jinghui.liu@ingenic.com, sernia.zhou@foxmail.com
Subject: Re: [PATCH 2/3] dt-bindings: SPI: Add Ingenic SFC bindings.
Date: Mon, 25 Jul 2022 12:30:52 -0600	[thread overview]
Message-ID: <20220725183052.GA2392099-robh@kernel.org> (raw)
In-Reply-To: <b52a8e97-3b8e-c67b-4440-2d7428edb4fa@sudomaker.com>

On Sun, Jul 24, 2022 at 04:49:25AM +0800, Mike Yang wrote:
> On 7/24/22 04:07, Krzysztof Kozlowski wrote:
> > On 23/07/2022 21:27, Mark Brown wrote:
> >> On Sun, Jul 24, 2022 at 02:47:14AM +0800, Mike Yang wrote:
> >>> On 7/24/22 01:43, Krzysztof Kozlowski wrote:
> >>>> On 23/07/2022 18:50, Zhou Yanjie wrote:
> >>
> >>>>> No offense, does it really need to be named that way?
> >>>>> I can't seem to find documentation with instructions on this :(
> >>
> >> ...
> >>
> >>>> All bindings are to follow this rule, so I don't understand why you
> >>>> think it is an exception for you?
> >>
> >>> Zhou didn't ask you to make an exception. They have a valid
> >>> point and they're asking why.
> >>
> >>> You may want to avoid further incidents of this kind by stop
> >>> being bossy and actually writing a guideline of naming these
> >>> .yaml files and publish it somewhere online.

I don't like your tone. Patches are welcome to fix deficiencies in 
documentation. Out of the hundreds of bindings a year, I see <5 
documentation patches a year.

The documentation clearly says to run 'make dt_binding_check' and that 
was obviously not followed here. 

> >> Yeah, I do have to say that I was also completely unaware that
> >> there was any enforced convention here.
> > 
> > Indeed, it's not a enforced pattern. But there are many other
> > insignificant ones which we also tend to forget during review, like
> > using words "Device Tree bindings" in title or using unnecessary quotes
> > around "refs" (also in ID of schema). It's not a big deal, but I ask
> > when I notice it.
> 
> Good. Thanks for paying attention to these details.
> 
> 
> >> Zhou already mentioned he was unable find the naming guidelines of these .yaml files.
> >>
> >> Apparently you think it's unacceptable for new contributors of a certain subsystem to use existing code as examples, and/or they're responsible for figuring out what's a good example and what's a bad one in the existing codebase.

Please wrap your lines on replies.

> > 
> > It's everywhere in the kernel, what can I say? If you copy existing
> > code, you might copy poor code...
> 
> Still, it shouldn't be a responsibility of new contributors to 
> determine the quality of an existing piece of code, unless there are 
> clear guidelines (i.e. one should use the new "cs-gpios" attribute in SPI controllers).

Generally the guidance is to look at newer drivers for current best 
practices.

> >>> It might never grow to new devices (because they might be different), so
> >>> that is not really an argument.
> >>
> >> It is an argument. A very valid one.
> >>
> >> "they *might* be different". You may want to get your hands on real hardware and try another word. Or at least read the datasheets instead of believing your imagination.
> >>
> >> I would enjoy duplicating the st,stm32-spi.yaml into st,stm32{f,h}{0..7}-spi.yaml if I'm bored at a Sunday afternoon.
> >>
> >>>
> >>> All bindings are to follow this rule, so I don't understand why you
> >>> think it is an exception for you?
> >>
> >> Zhou didn't ask you to make an exception. They have a valid point and they're asking why.
> > 
> > Hm, everyone has the same valid point and such recommendation is to
> > everyone, although it is nothing serious.
> > 
> >> You may want to avoid further incidents of this kind by stop being bossy and actually writing a guideline of naming these .yaml files and publish it somewhere online.
> > 
> > I did not see any incident here... Process of review includes comments
> > and there is nothing bad happening when you receive a comment. No
> > incident...
> 
> 
> Okay. After careful inspection of the Ingenic datasheets, now I have 
> the conclusion: The Ingenic X1000, X1021, X1500, X1501, X1520, X1600, 
> X1800, X1830, X2000, X2100, X2500 have the same SFC controller.

So if they are all 'the same', then I expect they all have a fallback 
compatible with x1000 and using that for the filename makes sense.

> X1600 has a newer version (let's say v2) of the SFC, and X2000-2500 
> have v3. Others have the original version (let's say v1). Each new 
> version introduced new features such as arbitrary DMA sizes, and the 
> rest features are the same.

So backwards compatible? If so, then they should have x1000 for 
fallback.

> 
> So IMO the name "ingenic,sfc.yaml" is perfectly logical.

Covering all 3 versions? If so and not backwards compatible, then I 
would agree.

Rob

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/