On 21/11/2023 18:15, Peter Griffin wrote: > Hi Rob, > > Thanks for your review. > > On Tue, 21 Nov 2023 at 15:16, Rob Herring <robh@xxxxxxxxxx> wrote: >> >> On Mon, Nov 20, 2023 at 09:20:27PM +0000, Peter Griffin wrote: >>> Specifying samsung,uart-fifosize in both DT and driver static data is error >>> prone and relies on driver probe order and dt aliases to be correct. >>> >>> Additionally on many Exynos platforms these are (USI) universal serial >>> interfaces which can be uart, spi or i2c, so it can change per board. >>> >>> For google,gs101-uart and exynosautov9-uart make samsung,uart-fifosize a >>> required property. For these platforms fifosize now *only* comes from DT. >>> >>> It is hoped other Exynos platforms will also switch over time. >> >> Then allow the property on them. > > Not sure I fully understand your comment. Can you elaborate? Do you > mean leave the 'samsung,uart-fifosize' as an optional property like it > is currently even for the platforms that now require it to be present > to function correctly? > > I deliberately restricted the yaml change to only require this > property for the SoCs that already set the 'samsung,uart-fifosize' dt > property. As setting the property and having the driver use what is > specified in DT also requires a corresponding driver update (otherwise > fifosize gets overwritten by the driver static data, and then becomes > dependent on probe order, dt aliases etc). The rationale was drivers > 'opt in' and add themselves to the compatibles in this patch as they > migrate away from obtaining fifo size from driver static data to > obtaining it from DT. Your code diff looks like you are adding the property only to these models. > >> >>> >>> Signed-off-by: Peter Griffin <peter.griffin@xxxxxxxxxx> >>> --- >>> .../bindings/serial/samsung_uart.yaml | 17 +++++++++++++++++ >>> 1 file changed, 17 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/serial/samsung_uart.yaml b/Documentation/devicetree/bindings/serial/samsung_uart.yaml >>> index ccc3626779d9..22a1edadc4fe 100644 >>> --- a/Documentation/devicetree/bindings/serial/samsung_uart.yaml >>> +++ b/Documentation/devicetree/bindings/serial/samsung_uart.yaml >>> @@ -133,6 +133,23 @@ allOf: >>> - const: uart >>> - const: clk_uart_baud0 >>> >>> + - if: >>> + properties: >>> + compatible: >>> + contains: >>> + enum: >>> + - google,gs101-uart >>> + - samsung,exynosautov9-uart >>> + then: >>> + properties: >>> + samsung,uart-fifosize: >>> + description: The fifo size supported by the UART channel. >>> + $ref: /schemas/types.yaml#/definitions/uint32 >>> + enum: [16, 64, 256] >> >> We already have 'fifo-size' in several drivers. Use that. Please move >> its type/description definitions to serial.yaml and make drivers just do >> 'fifo-size: true' if they use it. > > What do you suggest we do for the samsung,uart-fifosize property that > is being used upstream? Nothing, your diff is just wrong. Or at least nothing needed. Just drop all this properties: here and only make it required for Google GS101. > >> >>> + >>> + required: >>> + - samsung,uart-fifosize >> >> A new required property is an ABI break. Please explain why that is okay >> in the commit message. >> > > I can update the commit message to make clear there is an ABI break. > As mentioned above the platforms where this is now required are either > already setting the property or are new in this series. Is that > sufficient justification? Yes, but only first case. You need to order your patches correctly - first is ABI break expecting ExynopsAutov9 to provide FIFO size in DTS with its explanation. Second commit is adding GS101 where there is no ABI break. Best regards, Krzysztof
