On Sun, 11 Feb 2024 12:28:24 +0300
Andrey Skvortsov <andrej.skvort...@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.skvort...@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
