Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 25 Jul 2022 12:30:52 -0600
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.
Message-ID: <20220725183052.GA2392099-robh@kernel.org>
References: <1658508510-15400-1-git-send-email-zhouyanjie@wanyeetech.com>
 <1658508510-15400-3-git-send-email-zhouyanjie@wanyeetech.com>
 <487a93c4-3301-aefd-abba-aabf4cb8ec90@linaro.org>
 <37062a5d-9da3-fbaf-89bd-776f32be36d9@wanyeetech.com>
 <d1a0dd15-3621-14e9-b931-417cefaab017@linaro.org>
 <b5505a46-ce76-d0aa-009e-81d9ba16e1d5@sudomaker.com>
 <YtxLoPOykLDTzTn9@sirena.org.uk>
 <f05045fa-9ecd-d312-0eaa-5d19498453fc@linaro.org>
 <b52a8e97-3b8e-c67b-4440-2d7428edb4fa@sudomaker.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <b52a8e97-3b8e-c67b-4440-2d7428edb4fa@sudomaker.com>
Precedence: bulk

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