| Message ID | 20250105160346.418829-2-ivo.ivanov.ivanov1@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | soc: samsung: usi: implement support for USIv1 | expand |
On Sun, Jan 05, 2025 at 06:03:43PM +0200, Ivaylo Ivanov wrote: > diff --git a/include/dt-bindings/soc/samsung,exynos-usi.h b/include/dt-bindings/soc/samsung,exynos-usi.h > index a01af169d..b7c1406f3 100644 > --- a/include/dt-bindings/soc/samsung,exynos-usi.h > +++ b/include/dt-bindings/soc/samsung,exynos-usi.h > @@ -9,9 +9,9 @@ > #ifndef __DT_BINDINGS_SAMSUNG_EXYNOS_USI_H > #define __DT_BINDINGS_SAMSUNG_EXYNOS_USI_H > > -#define USI_V2_NONE 0 > -#define USI_V2_UART 1 > -#define USI_V2_SPI 2 > -#define USI_V2_I2C 3 > +#define USI_MODE_NONE 0 > +#define USI_MODE_UART 1 > +#define USI_MODE_SPI 2 > +#define USI_MODE_I2C 3 This breaks ABI and does not build. You still need also: #define USI_V2_NONE USI_MODE_NONE and same for the rest. Best regards, Krzysztof
On 1/6/25 08:24, Krzysztof Kozlowski wrote: > On Sun, Jan 05, 2025 at 06:03:43PM +0200, Ivaylo Ivanov wrote: >> diff --git a/include/dt-bindings/soc/samsung,exynos-usi.h b/include/dt-bindings/soc/samsung,exynos-usi.h >> index a01af169d..b7c1406f3 100644 >> --- a/include/dt-bindings/soc/samsung,exynos-usi.h >> +++ b/include/dt-bindings/soc/samsung,exynos-usi.h >> @@ -9,9 +9,9 @@ >> #ifndef __DT_BINDINGS_SAMSUNG_EXYNOS_USI_H >> #define __DT_BINDINGS_SAMSUNG_EXYNOS_USI_H >> >> -#define USI_V2_NONE 0 >> -#define USI_V2_UART 1 >> -#define USI_V2_SPI 2 >> -#define USI_V2_I2C 3 >> +#define USI_MODE_NONE 0 >> +#define USI_MODE_UART 1 >> +#define USI_MODE_SPI 2 >> +#define USI_MODE_I2C 3 > This breaks ABI and does not build. You still need also: > #define USI_V2_NONE USI_MODE_NONE > and same for the rest. Alright, sounds good to me. That way I shouldn't s/USI_V2/USI_MODE/g for the other SoC device trees, right? Should I also mention with a comment that the V2 constants are deprecated? Best regards, Ivaylo > > Best regards, > Krzysztof >
On 06/01/2025 08:09, Ivaylo Ivanov wrote: >>> >>> -#define USI_V2_NONE 0 >>> -#define USI_V2_UART 1 >>> -#define USI_V2_SPI 2 >>> -#define USI_V2_I2C 3 >>> +#define USI_MODE_NONE 0 >>> +#define USI_MODE_UART 1 >>> +#define USI_MODE_SPI 2 >>> +#define USI_MODE_I2C 3 >> This breaks ABI and does not build. You still need also: >> #define USI_V2_NONE USI_MODE_NONE >> and same for the rest. > > Alright, sounds good to me. That way I shouldn't s/USI_V2/USI_MODE/g > for the other SoC device trees, right? Should I also mention with a You can change them as well, because USI_MODE_XXX will be preferred from now on. The DTS commit will just wait one cycle till bindings get accepted. > comment that the V2 constants are deprecated? Yes, this would be good. Thanks for working on this. Best regards, Krzysztof
Hiya, On 1/5/25 4:03 PM, Ivaylo Ivanov wrote: > +#define USI_MODE_NONE 0 > +#define USI_MODE_UART 1 > +#define USI_MODE_SPI 2 > +#define USI_MODE_I2C 3 USI_CONFIG register refers to the protocol selection with USI_I2C, USI_SPI, USI_UART. How about getting rid of the MODE from the name? Cheers, ta
On 1/6/25 09:36, Tudor Ambarus wrote: > Hiya, > > On 1/5/25 4:03 PM, Ivaylo Ivanov wrote: >> +#define USI_MODE_NONE 0 >> +#define USI_MODE_UART 1 >> +#define USI_MODE_SPI 2 >> +#define USI_MODE_I2C 3 > USI_CONFIG register refers to the protocol selection with USI_I2C, > USI_SPI, USI_UART. How about getting rid of the MODE from the name? I thought about that too but I believe that mentioning that these constants are for mode selection in their name is generally a good practice. Let me know if dropping _MODE is really needed. Best regards, Ivaylo > > Cheers, > ta
On 06/01/2025 08:41, Ivaylo Ivanov wrote: > On 1/6/25 09:36, Tudor Ambarus wrote: >> Hiya, >> >> On 1/5/25 4:03 PM, Ivaylo Ivanov wrote: >>> +#define USI_MODE_NONE 0 >>> +#define USI_MODE_UART 1 >>> +#define USI_MODE_SPI 2 >>> +#define USI_MODE_I2C 3 >> USI_CONFIG register refers to the protocol selection with USI_I2C, >> USI_SPI, USI_UART. How about getting rid of the MODE from the name? > > I thought about that too but I believe that mentioning that these constants > are for mode selection in their name is generally a good practice. Let me know > if dropping _MODE is really needed. I am fine with both, MODE feels a bit more descriptive indicating how the USI should be configured. Best regards, Krzysztof
On 1/6/25 8:50 AM, Krzysztof Kozlowski wrote: > On 06/01/2025 08:41, Ivaylo Ivanov wrote: >> On 1/6/25 09:36, Tudor Ambarus wrote: >>> Hiya, >>> >>> On 1/5/25 4:03 PM, Ivaylo Ivanov wrote: >>>> +#define USI_MODE_NONE 0 >>>> +#define USI_MODE_UART 1 >>>> +#define USI_MODE_SPI 2 >>>> +#define USI_MODE_I2C 3 >>> USI_CONFIG register refers to the protocol selection with USI_I2C, >>> USI_SPI, USI_UART. How about getting rid of the MODE from the name? >> >> I thought about that too but I believe that mentioning that these constants >> are for mode selection in their name is generally a good practice. Let me know >> if dropping _MODE is really needed. no strong requirement. > I am fine with both, MODE feels a bit more descriptive indicating how > the USI should be configured. Fine by me to keep MODE in the name. Cheers, ta
diff --git a/Documentation/devicetree/bindings/soc/samsung/exynos-usi.yaml b/Documentation/devicetree/bindings/soc/samsung/exynos-usi.yaml index 5b046932f..cc92a06a3 100644 --- a/Documentation/devicetree/bindings/soc/samsung/exynos-usi.yaml +++ b/Documentation/devicetree/bindings/soc/samsung/exynos-usi.yaml @@ -144,7 +144,7 @@ examples: compatible = "samsung,exynos850-usi"; reg = <0x138200c0 0x20>; samsung,sysreg = <&sysreg_peri 0x1010>; - samsung,mode = <USI_V2_UART>; + samsung,mode = <USI_MODE_UART>; samsung,clkreq-on; /* needed for UART mode */ #address-cells = <1>; #size-cells = <1>; diff --git a/include/dt-bindings/soc/samsung,exynos-usi.h b/include/dt-bindings/soc/samsung,exynos-usi.h index a01af169d..b7c1406f3 100644 --- a/include/dt-bindings/soc/samsung,exynos-usi.h +++ b/include/dt-bindings/soc/samsung,exynos-usi.h @@ -9,9 +9,9 @@ #ifndef __DT_BINDINGS_SAMSUNG_EXYNOS_USI_H #define __DT_BINDINGS_SAMSUNG_EXYNOS_USI_H -#define USI_V2_NONE 0 -#define USI_V2_UART 1 -#define USI_V2_SPI 2 -#define USI_V2_I2C 3 +#define USI_MODE_NONE 0 +#define USI_MODE_UART 1 +#define USI_MODE_SPI 2 +#define USI_MODE_I2C 3 #endif /* __DT_BINDINGS_SAMSUNG_EXYNOS_USI_H */
In the original bindings commit, protocol mode definitions were named with the version of the supported USI (in this case, V2) with the idea of leaving enough room in the future for other versions of this block. This, however, is not how the modes should be modelled. The modes are not version specific and you should not be able to tell USI which version of a mode to use - that has to be handled in the driver - thus encoding this information in the binding is meaningless. Only one constant per mode is needed, so replace USI_V2_ with USI_MODE_ in all constants in the bindings. Signed-off-by: Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com> --- I wasn't sure if it was appropriate to add a Suggested-by tag for Krzysztof because I haven't asked for his permission, so I didn't, but if he wants to add it before merging, please do so! These changes are a bit tricky to approach. My guess was that this would be the best way to put it out - one patch for fixing it in the bindings and trees, then add exynos8895 to the bindings, fiddle with the driver and finally rename the constants in device trees. This breaks compilation if the whole series is not applied, because the driver, the binding and the device trees use the dt-bindings header. If anyone thinks of a better solution to organising the patches, let me know. --- .../devicetree/bindings/soc/samsung/exynos-usi.yaml | 2 +- include/dt-bindings/soc/samsung,exynos-usi.h | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-)