| Message ID | 20240909083530.14695-3-andrei.simion@microchip.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Adjust Stream Name and DT Bindings Updates | expand |
On 09/09/2024 10:35, Andrei Simion wrote: > From: Codrin Ciubotariu <codrin.ciubotariu@microchip.com> > > Add 'sound-name-prefix' property to differentiate between interfaces in > DPCM use-cases. Property is optional. > > [andrei.simion@microchip.com: Adjust the commit title and message. > Reword the description for 'sound-name-prefix'.] > > Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com> > Signed-off-by: Andrei Simion <andrei.simion@microchip.com> > --- > .../bindings/sound/microchip,sama7g5-i2smcc.yaml | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/Documentation/devicetree/bindings/sound/microchip,sama7g5-i2smcc.yaml b/Documentation/devicetree/bindings/sound/microchip,sama7g5-i2smcc.yaml > index fb630a184350..ad34df67c7c0 100644 > --- a/Documentation/devicetree/bindings/sound/microchip,sama7g5-i2smcc.yaml > +++ b/Documentation/devicetree/bindings/sound/microchip,sama7g5-i2smcc.yaml > @@ -52,6 +52,13 @@ properties: > - const: gclk > minItems: 1 > > + sound-name-prefix: > + pattern: "^I2SMCC[0-9]$" This does not look correct. Name/prefix can be anything matching real hardware, why are you restricting it? How can you predict all names? > + $ref: /schemas/types.yaml#/definitions/string > + description: > + Unique prefixes for the sink/source names of the component, ensuring > + distinct identification among multiple instances. You are duplicating property definitions. This is not needed at all. Maybe your schema misses $ref to common schema. Best regards, Krzysztof
On 09.09.2024 11:38, Krzysztof Kozlowski wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On 09/09/2024 10:35, Andrei Simion wrote: >> From: Codrin Ciubotariu <codrin.ciubotariu@microchip.com> >> >> Add 'sound-name-prefix' property to differentiate between interfaces in >> DPCM use-cases. Property is optional. >> >> [andrei.simion@microchip.com: Adjust the commit title and message. >> Reword the description for 'sound-name-prefix'.] >> >> Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com> >> Signed-off-by: Andrei Simion <andrei.simion@microchip.com> >> --- >> .../bindings/sound/microchip,sama7g5-i2smcc.yaml | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/sound/microchip,sama7g5-i2smcc.yaml b/Documentation/devicetree/bindings/sound/microchip,sama7g5-i2smcc.yaml >> index fb630a184350..ad34df67c7c0 100644 >> --- a/Documentation/devicetree/bindings/sound/microchip,sama7g5-i2smcc.yaml >> +++ b/Documentation/devicetree/bindings/sound/microchip,sama7g5-i2smcc.yaml >> @@ -52,6 +52,13 @@ properties: >> - const: gclk >> minItems: 1 >> >> + sound-name-prefix: >> + pattern: "^I2SMCC[0-9]$" > > This does not look correct. Name/prefix can be anything matching real > hardware, why are you restricting it? How can you predict all names? > Based on the datasheet, the SoC(s) have the following naming conventions: - sama7g5: I2SMCC0 and I2SMCC1 - sam9x60/sam9x75: I2SMCC To accommodate these variations, I propose using a more relaxed pattern: "^I2SMCC(0-9)?$". This pattern allows for both the fixed prefix and an optional single digit at the end. What are your thoughts on this approach? >> + $ref: /schemas/types.yaml#/definitions/string >> + description: >> + Unique prefixes for the sink/source names of the component, ensuring >> + distinct identification among multiple instances. > > You are duplicating property definitions. This is not needed at all. > Maybe your schema misses $ref to common schema. > I understand the concern about duplicating property definitions. In the current file, I have referenced `dai-common` as shown here: https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/sound/microchip%2Csama7g5-i2smcc.yaml#L74C1-L75C27 Could you please confirm if this reference is correctly implemented, or suggest any adjustments needed to align with the common schema? Best Regards, Andrei Simion > > Best regards, > Krzysztof >
On 09/09/2024 14:28, Andrei.Simion@microchip.com wrote: >>> minItems: 1 >>> >>> + sound-name-prefix: >>> + pattern: "^I2SMCC[0-9]$" >> >> This does not look correct. Name/prefix can be anything matching real >> hardware, why are you restricting it? How can you predict all names? >> > Based on the datasheet, the SoC(s) have the following naming conventions: > - sama7g5: I2SMCC0 and I2SMCC1 > - sam9x60/sam9x75: I2SMCC > > To accommodate these variations, I propose using a more relaxed pattern: "^I2SMCC(0-9)?$". > This pattern allows for both the fixed prefix and an optional single digit at the end. > What are your thoughts on this approach? I understand this does not differ per board, because it is component of the SoC, yet still I do not see any value in enforcing name. > > >>> + $ref: /schemas/types.yaml#/definitions/string >>> + description: >>> + Unique prefixes for the sink/source names of the component, ensuring >>> + distinct identification among multiple instances. >> >> You are duplicating property definitions. This is not needed at all. >> Maybe your schema misses $ref to common schema. >> > > I understand the concern about duplicating property definitions. > In the current file, I have referenced `dai-common` as shown here: > https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/sound/microchip%2Csama7g5-i2smcc.yaml#L74C1-L75C27 > > Could you please confirm if this reference is correctly implemented, Yeah, the dai-common $ref is correct, so you should not need it. Do you see any warning? > or suggest any adjustments needed to align with the common schema? I claim nothing has to be done and entire patch is redundant or not much helpful. Your commit msg did not explain *why* this is needed and what problem you are fixing, so what I can say? I don't know why should be aligned because I do not understand the problem being fixed. Best regards, Krzysztof
On 09/09/2024 10:38, Krzysztof Kozlowski wrote: > On 09/09/2024 10:35, Andrei Simion wrote: >> From: Codrin Ciubotariu <codrin.ciubotariu@microchip.com> >> >> Add 'sound-name-prefix' property to differentiate between interfaces in >> DPCM use-cases. Property is optional. >> >> [andrei.simion@microchip.com: Adjust the commit title and message. >> Reword the description for 'sound-name-prefix'.] >> >> Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com> >> Signed-off-by: Andrei Simion <andrei.simion@microchip.com> >> --- >> .../bindings/sound/microchip,sama7g5-i2smcc.yaml | 7 +++++++ >> 1 file changed, 7 insertions(+) One more point for future (I missed this before) - be sure you CC necessary lists. Use get_maintainers or b4 for this. Skipping DT list means skipping automation, so this would be a NAK anyway. Best regards, Krzysztof
On 10.09.2024 10:17, Krzysztof Kozlowski wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On 09/09/2024 10:38, Krzysztof Kozlowski wrote: >> On 09/09/2024 10:35, Andrei Simion wrote: >>> From: Codrin Ciubotariu <codrin.ciubotariu@microchip.com> >>> >>> Add 'sound-name-prefix' property to differentiate between interfaces in >>> DPCM use-cases. Property is optional. >>> >>> [andrei.simion@microchip.com: Adjust the commit title and message. >>> Reword the description for 'sound-name-prefix'.] >>> >>> Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com> >>> Signed-off-by: Andrei Simion <andrei.simion@microchip.com> >>> --- >>> .../bindings/sound/microchip,sama7g5-i2smcc.yaml | 7 +++++++ >>> 1 file changed, 7 insertions(+) > > One more point for future (I missed this before) - be sure you CC > necessary lists. Use get_maintainers or b4 for this. Skipping DT list > means skipping automation, so this would be a NAK anyway. > yes, I will be much more careful. thank you. Best regards, Andrei Simion > Best regards, > Krzysztof >
diff --git a/Documentation/devicetree/bindings/sound/microchip,sama7g5-i2smcc.yaml b/Documentation/devicetree/bindings/sound/microchip,sama7g5-i2smcc.yaml index fb630a184350..ad34df67c7c0 100644 --- a/Documentation/devicetree/bindings/sound/microchip,sama7g5-i2smcc.yaml +++ b/Documentation/devicetree/bindings/sound/microchip,sama7g5-i2smcc.yaml @@ -52,6 +52,13 @@ properties: - const: gclk minItems: 1 + sound-name-prefix: + pattern: "^I2SMCC[0-9]$" + $ref: /schemas/types.yaml#/definitions/string + description: + Unique prefixes for the sink/source names of the component, ensuring + distinct identification among multiple instances. + dmas: items: - description: TX DMA Channel