Re: [PATCH v1] dt-bindings: dsp: mediatek: add mt8186 dsp document

[Tinghan Shen]

Hi Krzysztof,

On Thu, 2022-06-02 at 14:26 +0200, Krzysztof Kozlowski wrote:
> On 02/06/2022 13:53, Tinghan Shen wrote:
> > Hi Krzysztof,
> > 
> > On Thu, 2022-06-02 at 12:45 +0200, Krzysztof Kozlowski wrote:
> > > On 02/06/2022 12:19, Tinghan Shen wrote:
> > > > Hi Krzysztof,
> > > > 
> > > > On Thu, 2022-06-02 at 09:40 +0200, Krzysztof Kozlowski wrote:
> > > > > On 02/06/2022 08:44, Tinghan Shen wrote:
> > > > > > > > +  mbox-names:
> > > > > > > > +    items:
> > > > > > > > +      - const: mbox0
> > > > > > > > +      - const: mbox1
> > > > > > > 
> > > > > > > These should be rather some meaningful names, e.g. "rx" and "tx".
> > > > > > 
> > > > > > The mbox name has to align with the adsp ipc driver.
> > > > > > The adsp ipc driver is using 'mbox%d' for mailbox channels.
> > > > > > 
> > > > > > 
> > > > > > 
> > > > 
> > > > 
> > 
> > 
https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/commit/?id=9db69df4bdd37eb1f65b6931ee067fb15b9a4d5c__;!!CTRNKA9wMg0ARbw!1TmempNkQhC5QuLBhyfWo_AC97MoLuWipsGV-LPaW9RKNPheU7Bgc-eboNi1JA1nC5I$
> > > > > >  
> > > > > > 
> > > > > > 	chan_name = kasprintf(GFP_KERNEL, "mbox%d", i);
> > > > > > 
> > > > > > 	/* ...snip... */
> > > > > > 
> > > > > > 	adsp_chan->ch = mbox_request_channel_byname(cl, chan_name);
> > > > > > 
> > > > > > Is it ok to continue using these names?
> > > > > 
> > > > > It is a bit confusing... how did that driver got merged recently without
> > > > > bindings? Why bindings are separate?
> > > > > 
> > > > > The bindings always come together in one patchset with the driver
> > > > > implementing them. Bindings are though a separate patch, yet still
> > > > > followed by the driver which uses them.
> > > > > 
> > > > > I do not see any compatibles in that driver, which suggests there is no
> > > > > other binding using it. If that's correct, then you need to change the
> > > > > driver.
> > > > > 
> > > > 
> > > > The mtk-adsp-ipc driver's sole function is to encapsulate the operations 
> > > > of mailbox framework from adsp ipc users. The mtk-adsp-ipc is not defined 
> > > > in the dts file and we don't need it to be defined. The creation of mtk-adsp-ipc 
> > > > device is requested by adsp ipc users via the use of 'platform_device_register_data'[1].
> > > > 
> > > > the driver implemented the mailbox framework is 'mtk-adsp-mailbox'[2]. it has 
> > > > corresponding hardwares and a yaml file[3] to describe it.
> > > 
> > > I don't understand how is this related. We talk here about the
> > > mbox-names for this bindings file. You replied, that these bindings are
> > > already used by something, but now you say that they are not? So why do
> > > you need to change anything in any driver?
> > > 
> > > Simple question - do the bindings here "add mt8186 dsp document" are
> > > used by any specific Linux driver already?
> > 
> > This bindings, 'add mt8186 dsp document', are used by the SOF sound driver of MT8186[1]. 
> > 
> > I'm sorry for miss leading you in previous reply. I was thought that you're 
> > asking why the mtk-adsp-ipc driver got merged without bindings. So, I tried 
> > to explain why mtk-adsp-ipc doesn't have bindings.
> 
> Then my question is kind of still valid:
> How did "mt8186 SOF" driver got merged recently without bindings? Why
> bindings are separate?
> 
> You cannot just sneak in usage of bindings in a driver, then submit
> bindings and say "we already have an user!". No, the bindings come with
> the driver. Always.
> 
> Linked patch [1] brings undocumented compatible mediatek,mt8186-dsp, so
> you should see big fat warning when running checkpatch. So this points
> that you did not run checkpatch which is another not acceptable
> submission. :(
> 
> [1]
> https://lore.kernel.org/all/20220422055659.8738-2-tinghan.shen@xxxxxxxxxxxx/
> 

I apologize for breaking the rules and sending inappropriate patches.

I was thought that it was acceptable to send community reviewed patches in a series, 
then followed the bindings at another patch. I was believed that separating un-reviewed
binding patch from reviewed driver patches would aid in patch acceptance.
Now, I see I make a big mistake. I'm sorry.

Best regards,
TingHan