On Sun, 11 Feb 2024 12:28:24 +0300 Andrey Skvortsov andrej.skvortzov@gmail.com wrote:
Hi Andrey,
thanks for taking care of this upstream!
In newer 1.2 PinePhone board revisions LIS3MDL magnetometer was replaced by AF8133J. They use the same PB1 pin in different modes.
LIS3MDL uses it as an gpio input to handle interrupt. AF8133J uses it as an gpio output as a reset signal.
It wasn't possible at runtime to enable both device tree nodes and detect supported sensor at probe time.
AF8133J has reset pin (PB1) connected to the SoC. By default AF8133J is in a reset state and don't respond to probe request on I2C bus. Extra code would be needed to handle reset signal. Therefore this code uses LIS3MDL magnetometer instead of AF8133J.
Thanks for the research and explanation, that makes it much easier to reason about!
Introducing new dts 1.2b with AF8133J sensor would require probing in SPL. That would lead to pulling in into SPL I2C controller driver, RSB controller driver, introducing new AXP803 driver to power-up sensors for probe. It's working, but SPL is pretty size-constrained on A64 and doesn't have much space. Therefore fdt fixup is done in U-Boot proper without introducing new board revision and new dts.
I agree on that, especially if it's indeed just a matter of flipping two "status" properties.
Signed-off-by: Andrey Skvortsov andrej.skvortzov@gmail.com
AF8133J's driver isn't upstreamed yet, but it will be soon. Therefore this is RFC patch. I'd like to know whether selected approach will be accepted in u-boot before submiting coresponding dts changes to the Linux kernel. Any feedback on this change would be very welcome.
So I think this is the right approach: Since the SPL has no use of this device, and it's a rather small DT change, we definitely want to do this in U-Boot proper. And I believe that U-Boot (proper) is indeed the best place to do those adjustments, since it's built for one board anyway, so anything board specific belongs into there (and not into TF-A or the SPL, which are more tailored to one *SoC*). Also we are not so size sensitive in proper. And I also like the fact that it's protected by a board specific definition, and even better that it's an already existing one.
Oh, and I am not really convinced this is the right approach here, but maybe have a look at doc/usage/cmd/extension.rst, for another related mechanism.
Some smaller comments below ...
board/sunxi/board.c | 26 ++++++++++++++++++++++++++ configs/pinephone_defconfig | 1 + 2 files changed, 27 insertions(+)
diff --git a/board/sunxi/board.c b/board/sunxi/board.c index 1305302060..a4bfa24400 100644 --- a/board/sunxi/board.c +++ b/board/sunxi/board.c @@ -15,6 +15,7 @@ #include <dm.h> #include <env.h> #include <hang.h> +#include <i2c.h> #include <image.h> #include <init.h> #include <log.h> @@ -920,6 +921,28 @@ static void bluetooth_dt_fixup(void *blob) "local-bd-address", bdaddr, ETH_ALEN, 1); }
+#ifdef CONFIG_PINEPHONE_DT_SELECTION
Can you move that line into the function? That would for one avoid the protection in the caller below, and secondly make it easier to add other boards, if a need arises for them. The two #defines don't hurt or change the code, so just keep them outside.
+#define PINEPHONE_LIS3MDL_I2C_ADDR 0x1E +#define PINEPHONE_LIS3MDL_I2C_BUS 1 /* I2C1 */
+static void board_dt_fixup(void *blob) +{
- struct udevice *bus, *dev;
(have the #ifdef here)
- if (!fdt_node_check_compatible(blob, 0, "pine64,pinephone-1.2"))
Please have curly braces around that.
So I'd say please send the (disabled) DT node addition patch to the kernel MLs, then send this patch to U-Boot.
Cheers, Andre
if (!uclass_get_device_by_seq(UCLASS_I2C, PINEPHONE_LIS3MDL_I2C_BUS, &bus)) {dm_i2c_probe(bus, PINEPHONE_LIS3MDL_I2C_ADDR, 0, &dev);fdt_set_status_by_compatible(blob, "st,lis3mdl-magn",dev ? FDT_STATUS_OKAY : FDT_STATUS_DISABLED);fdt_set_status_by_compatible(blob, "voltafield,af8133j",dev ? FDT_STATUS_DISABLED : FDT_STATUS_OKAY);}+} +#endif
int ft_board_setup(void *blob, struct bd_info *bd) { int __maybe_unused r; @@ -934,6 +957,9 @@ int ft_board_setup(void *blob, struct bd_info *bd)
bluetooth_dt_fixup(blob);
+#ifdef CONFIG_PINEPHONE_DT_SELECTION
- board_dt_fixup(blob);
+#endif #ifdef CONFIG_VIDEO_DT_SIMPLEFB r = sunxi_simplefb_setup(blob); if (r) diff --git a/configs/pinephone_defconfig b/configs/pinephone_defconfig index eebc676901..457e7ee1e7 100644 --- a/configs/pinephone_defconfig +++ b/configs/pinephone_defconfig @@ -12,6 +12,7 @@ CONFIG_MMC_SUNXI_SLOT_EXTRA=2 CONFIG_PINEPHONE_DT_SELECTION=y # CONFIG_SYS_MALLOC_CLEAR_ON_INIT is not set CONFIG_OF_LIST="sun50i-a64-pinephone-1.1 sun50i-a64-pinephone-1.2" +CONFIG_SYS_I2C_MVTWSI=y CONFIG_LED_STATUS=y CONFIG_LED_STATUS_GPIO=y CONFIG_LED_STATUS0=y