| Message ID | 20240115132009.101718-1-raphael.gallais-pou@foss.st.com |
|---|---|
| Headers | show |
| Series | Introduce STM32 LVDS driver | expand |
On Mon, Jan 15, 2024 at 02:20:04PM +0100, Raphael Gallais-Pou wrote: > Add "st,stm32mp25-lvds" compatible. > > Signed-off-by: Raphael Gallais-Pou <raphael.gallais-pou@foss.st.com> > --- > Depends on: "dt-bindings: stm32: add clocks and reset binding for > stm32mp25 platform" by Gabriel Fernandez > > Changes in v3: > - Clarify commit dependency > - Fix includes in the example > - Fix YAML > - Add "clock-cells" description > - s/regroups/is composed of/ > - Changed compatible to show SoC specificity > > Changes in v2: > - Switch compatible and clock-cells related areas > - Remove faulty #include in the example. > - Add entry in MAINTAINERS > --- > .../bindings/display/st,stm32-lvds.yaml | 119 ++++++++++++++++++ Filename matching compatible. > +properties: > + compatible: > + const: st,stm32mp25-lvds > +examples: > + - | > + #include <dt-bindings/clock/st,stm32mp25-rcc.h> > + #include <dt-bindings/reset/st,stm32mp25-rcc.h> > + > + lvds: lvds@48060000 { > + compatible = "st,stm32-lvds"; Wrong compatible.
On 1/15/24 16:46, Rob Herring wrote: > On Mon, Jan 15, 2024 at 02:20:04PM +0100, Raphael Gallais-Pou wrote: >> Add "st,stm32mp25-lvds" compatible. >> >> Signed-off-by: Raphael Gallais-Pou <raphael.gallais-pou@foss.st.com> >> --- >> Depends on: "dt-bindings: stm32: add clocks and reset binding for >> stm32mp25 platform" by Gabriel Fernandez >> >> Changes in v3: >> - Clarify commit dependency >> - Fix includes in the example >> - Fix YAML >> - Add "clock-cells" description >> - s/regroups/is composed of/ >> - Changed compatible to show SoC specificity >> >> Changes in v2: >> - Switch compatible and clock-cells related areas >> - Remove faulty #include in the example. >> - Add entry in MAINTAINERS >> --- >> .../bindings/display/st,stm32-lvds.yaml | 119 ++++++++++++++++++ > Filename matching compatible. Hi Rob, I was unsure about this. The driver will eventually support several SoCs with different compatibles, wouldn't this be more confusing ? I also wanted to keep the similarity with the "st,stm32-<ip>.yaml" name for the DRM STM drivers. Would that be possible ? Regards, Raphaël > >> +properties: >> + compatible: >> + const: st,stm32mp25-lvds > >> +examples: >> + - | >> + #include <dt-bindings/clock/st,stm32mp25-rcc.h> >> + #include <dt-bindings/reset/st,stm32mp25-rcc.h> >> + >> + lvds: lvds@48060000 { >> + compatible = "st,stm32-lvds"; > Wrong compatible.
On 15/01/2024 17:51, Raphael Gallais-Pou wrote: > > On 1/15/24 16:46, Rob Herring wrote: >> On Mon, Jan 15, 2024 at 02:20:04PM +0100, Raphael Gallais-Pou wrote: >>> Add "st,stm32mp25-lvds" compatible. >>> A nit, subject: drop second/last, redundant "dt-bindings for". The "dt-bindings" prefix is already stating that these are bindings. See also: https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18 >>> Signed-off-by: Raphael Gallais-Pou <raphael.gallais-pou@foss.st.com> >>> --- >>> Depends on: "dt-bindings: stm32: add clocks and reset binding for >>> stm32mp25 platform" by Gabriel Fernandez >>> >>> Changes in v3: >>> - Clarify commit dependency >>> - Fix includes in the example >>> - Fix YAML >>> - Add "clock-cells" description >>> - s/regroups/is composed of/ >>> - Changed compatible to show SoC specificity >>> >>> Changes in v2: >>> - Switch compatible and clock-cells related areas >>> - Remove faulty #include in the example. >>> - Add entry in MAINTAINERS >>> --- >>> .../bindings/display/st,stm32-lvds.yaml | 119 ++++++++++++++++++ >> Filename matching compatible. > > Hi Rob, > > > I was unsure about this. > > The driver will eventually support several SoCs with different compatibles, > wouldn't this be more confusing ? No. "Eventually" might never happen. > I also wanted to keep the similarity with the "st,stm32-<ip>.yaml" name for the > DRM STM drivers. Would that be possible ? But why? The consistency we want is the filename matching compatible, not matching other filenames. If you have here multiple devices, document them *now*. > > > Regards, > > Raphaël I hope you did not ignore rest of the comments... We expect some sort of "ack/ok/I'll fix/whatever" message and you wrote nothing further. Best regards, Krzysztof
On 1/16/24 08:42, Krzysztof Kozlowski wrote: > On 15/01/2024 17:51, Raphael Gallais-Pou wrote: >> On 1/15/24 16:46, Rob Herring wrote: >>> On Mon, Jan 15, 2024 at 02:20:04PM +0100, Raphael Gallais-Pou wrote: >>>> Add "st,stm32mp25-lvds" compatible. >>>> > A nit, subject: drop second/last, redundant "dt-bindings for". The > "dt-bindings" prefix is already stating that these are bindings. > See also: > https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18 > >>>> Signed-off-by: Raphael Gallais-Pou <raphael.gallais-pou@foss.st.com> >>>> --- >>>> Depends on: "dt-bindings: stm32: add clocks and reset binding for >>>> stm32mp25 platform" by Gabriel Fernandez >>>> >>>> Changes in v3: >>>> - Clarify commit dependency >>>> - Fix includes in the example >>>> - Fix YAML >>>> - Add "clock-cells" description >>>> - s/regroups/is composed of/ >>>> - Changed compatible to show SoC specificity >>>> >>>> Changes in v2: >>>> - Switch compatible and clock-cells related areas >>>> - Remove faulty #include in the example. >>>> - Add entry in MAINTAINERS >>>> --- >>>> .../bindings/display/st,stm32-lvds.yaml | 119 ++++++++++++++++++ >>> Filename matching compatible. >> Hi Rob, >> >> >> I was unsure about this. >> >> The driver will eventually support several SoCs with different compatibles, >> wouldn't this be more confusing ? > No. "Eventually" might never happen. > >> I also wanted to keep the similarity with the "st,stm32-<ip>.yaml" name for the >> DRM STM drivers. Would that be possible ? > But why? The consistency we want is the filename matching compatible, > not matching other filenames. If you have here multiple devices, > document them *now*. Hi Krzysztof, |There is no multiple devices, so I will stick to the "st,stm32mp25-lvds" pattern for now.| > >> >> Regards, >> >> Raphaël > I hope you did not ignore rest of the comments... We expect some sort of > "ack/ok/I'll fix/whatever" message and you wrote nothing further. Although I did not acknowledged what has been said previously, I always take into account every comment on my patches. I understand that it can lead to some confusion. So rest assured that I did not ignore Rob's and Dmitry's review. Regards, Raphaël > > Best regards, > Krzysztof >