| Message ID | 20241025104548.1220076-3-fshao@chromium.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Update properties in two ASoC DT bindings | expand |
On Fri, Oct 25, 2024 at 06:44:42PM +0800, Fei Shao wrote: > The associated machine driver is not dependent on the format of DAI link > node names. This means we are allowed to use more descriptive names > instead of indices without impacting functionality. > > Update the binding to accept arbitrary DAI link names with a "-dai-link" > suffix. This is the common pattern used by the target (MT8188) and other > (MT8195, MT8186 etc.) MediaTek-based Chromebooks. We do not want arbitrary names. Why for example green-batman-dai-link should be correct? The existing pattern looks wrong. Please read DT spec and chapter about recommended names. > > Signed-off-by: Fei Shao <fshao@chromium.org> > --- > > .../devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml b/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml > index 701cedfa38d2..2da34b66818f 100644 > --- a/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml > +++ b/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml > @@ -40,7 +40,7 @@ properties: > name defined in the machine driver. > > patternProperties: > - "^dai-link-[0-9]+$": > + ".*-dai-link$": This breaks existing users. > type: object > description: > Container for dai-link level properties and CODEC sub-nodes. > @@ -112,7 +112,7 @@ examples: > "Headphone", "Headphone L", > "Headphone", "Headphone R", > "AIN1", "Headset Mic"; > - dai-link-0 { > + hdmi-dai-link { No. Not really justified. Best regards, Krzysztof
On Mon, Oct 28, 2024 at 4:56 AM Krzysztof Kozlowski <krzk@kernel.org> wrote: > > On Fri, Oct 25, 2024 at 06:44:42PM +0800, Fei Shao wrote: > > The associated machine driver is not dependent on the format of DAI link > > node names. This means we are allowed to use more descriptive names > > instead of indices without impacting functionality. > > > > Update the binding to accept arbitrary DAI link names with a "-dai-link" > > suffix. This is the common pattern used by the target (MT8188) and other > > (MT8195, MT8186 etc.) MediaTek-based Chromebooks. > > We do not want arbitrary names. Why for example green-batman-dai-link > should be correct? The existing pattern looks wrong. Please read DT spec > and chapter about recommended names. At first I was thinking about regex like `^[a-z-]+(-[a-z]+)*-dai-link$` based on the DTS coding style guide, but your example is not suggesting that. and the name like "hs-capture-dai-link" was rejected, so it's not just about enumerating the names either. Or "^dai-link@[0-9a-f]+$"? But they don't tie to particular register addresses... or just for some pseudo indices? I could miss something about DAI links under the sound documentation. Still trying to figure it out. > > > > > Signed-off-by: Fei Shao <fshao@chromium.org> > > --- > > > > .../devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml b/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml > > index 701cedfa38d2..2da34b66818f 100644 > > --- a/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml > > +++ b/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml > > @@ -40,7 +40,7 @@ properties: > > name defined in the machine driver. > > > > patternProperties: > > - "^dai-link-[0-9]+$": > > + ".*-dai-link$": > > This breaks existing users. There's no existing users in upstream, and the only downstream user is the MT8188 Chromebook DT that I'm trying to upstream, which I can fix. This binding was upstreamed exclusively for that DT. Even if we take a step back and assume someone has another DT already following this pattern (and that would be a surprise), this wouldn't break anything except a dtbs_check error, which would be trivial if they don't run that or attempt upstreaming. The driver doesn't use node names to distinguish DAI links. I want to fix this pattern before any actual users are in the tree if possible, but I'm also fine to stick with dai-link-0 to dai-link-10 in the case that nothing is changed and the DT has to follow the existing patterns. Regards, Fei > > > type: object > > description: > > Container for dai-link level properties and CODEC sub-nodes. > > @@ -112,7 +112,7 @@ examples: > > "Headphone", "Headphone L", > > "Headphone", "Headphone R", > > "AIN1", "Headset Mic"; > > - dai-link-0 { > > + hdmi-dai-link { > > No. Not really justified. > > Best regards, > Krzysztof >
On 28/10/2024 12:12, Fei Shao wrote: > On Mon, Oct 28, 2024 at 4:56 AM Krzysztof Kozlowski <krzk@kernel.org> wrote: >> >> On Fri, Oct 25, 2024 at 06:44:42PM +0800, Fei Shao wrote: >>> The associated machine driver is not dependent on the format of DAI link >>> node names. This means we are allowed to use more descriptive names >>> instead of indices without impacting functionality. >>> >>> Update the binding to accept arbitrary DAI link names with a "-dai-link" >>> suffix. This is the common pattern used by the target (MT8188) and other >>> (MT8195, MT8186 etc.) MediaTek-based Chromebooks. >> >> We do not want arbitrary names. Why for example green-batman-dai-link >> should be correct? The existing pattern looks wrong. Please read DT spec >> and chapter about recommended names. > > At first I was thinking about regex like > `^[a-z-]+(-[a-z]+)*-dai-link$` based on the DTS coding style guide, > but your example is not suggesting that. > and the name like "hs-capture-dai-link" was rejected, so it's not just > about enumerating the names either. > Or "^dai-link@[0-9a-f]+$"? But they don't tie to particular register > addresses... or just for some pseudo indices? > I could miss something about DAI links under the sound documentation. > Still trying to figure it out. You did not provide any reasoning why this has to be changed and why usually preferred generic dai-link is not correct. Names are not descriptive. > >> >>> >>> Signed-off-by: Fei Shao <fshao@chromium.org> >>> --- >>> >>> .../devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml b/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml >>> index 701cedfa38d2..2da34b66818f 100644 >>> --- a/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml >>> +++ b/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml >>> @@ -40,7 +40,7 @@ properties: >>> name defined in the machine driver. >>> >>> patternProperties: >>> - "^dai-link-[0-9]+$": >>> + ".*-dai-link$": >> >> This breaks existing users. > > There's no existing users in upstream, and the only downstream user is > the MT8188 Chromebook DT that I'm trying to upstream, which I can fix. > This binding was upstreamed exclusively for that DT. So you have entire commit msg to explain impact. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml b/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml index 701cedfa38d2..2da34b66818f 100644 --- a/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml +++ b/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml @@ -40,7 +40,7 @@ properties: name defined in the machine driver. patternProperties: - "^dai-link-[0-9]+$": + ".*-dai-link$": type: object description: Container for dai-link level properties and CODEC sub-nodes. @@ -112,7 +112,7 @@ examples: "Headphone", "Headphone L", "Headphone", "Headphone R", "AIN1", "Headset Mic"; - dai-link-0 { + hdmi-dai-link { link-name = "ETDM3_OUT_BE"; dai-format = "i2s"; mediatek,clk-provider = "cpu";
The associated machine driver is not dependent on the format of DAI link node names. This means we are allowed to use more descriptive names instead of indices without impacting functionality. Update the binding to accept arbitrary DAI link names with a "-dai-link" suffix. This is the common pattern used by the target (MT8188) and other (MT8195, MT8186 etc.) MediaTek-based Chromebooks. Signed-off-by: Fei Shao <fshao@chromium.org> --- .../devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)