[PATCH v2 10/12] sun5i: add support for DIP detection to CHIP board

[Maxime Ripard]

Hi,

On Tue, Mar 02, 2021 at 10:58:11AM +0100, Kory Maincent wrote:
> Add the extension_board_scan specific function to scan the information
> of the EEPROM on one-wire and fill the extension struct.
> 
> Signed-off-by: Kory Maincent <kory.maincent at bootlin.com>
> ---
>  arch/arm/mach-sunxi/Kconfig |   2 +
>  board/sunxi/Makefile        |   1 +
>  board/sunxi/chip.c          | 104 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 107 insertions(+)
>  create mode 100644 board/sunxi/chip.c
> 
> diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig
> index 4160d52501..2b6ba873ff 100644
> --- a/arch/arm/mach-sunxi/Kconfig
> +++ b/arch/arm/mach-sunxi/Kconfig
> @@ -1020,7 +1020,9 @@ endif
>  
>  config CHIP_DIP_SCAN
>  	bool "Enable DIPs detection for CHIP board"
> +	select SUPPORT_EXTENSION_SCAN
>  	select W1
>  	select W1_GPIO
>  	select W1_EEPROM
>  	select W1_EEPROM_DS24XXX
> +	imply CMD_EXTENSION
> diff --git a/board/sunxi/Makefile b/board/sunxi/Makefile
> index c4e13f8c38..d96b7897b6 100644
> --- a/board/sunxi/Makefile
> +++ b/board/sunxi/Makefile
> @@ -11,3 +11,4 @@ obj-$(CONFIG_SUN7I_GMAC)	+= gmac.o
>  obj-$(CONFIG_MACH_SUN4I)	+= dram_sun4i_auto.o
>  obj-$(CONFIG_MACH_SUN5I)	+= dram_sun5i_auto.o
>  obj-$(CONFIG_MACH_SUN7I)	+= dram_sun5i_auto.o
> +obj-$(CONFIG_CHIP_DIP_SCAN)	+= chip.o

The split of that patch and the previous one is weird: you're adding a
Kconfig symbol doing close to nothing in patch 9, and you add the logic
behind it here

It would be more natural to have the Kconfig option and the code here,
and 

> diff --git a/board/sunxi/chip.c b/board/sunxi/chip.c
> new file mode 100644
> index 0000000000..fc3d14e497
> --- /dev/null
> +++ b/board/sunxi/chip.c
> @@ -0,0 +1,104 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * (C) Copyright 2021
> + * Köry Maincent, Bootlin, <kory.maincent at bootlin.com>
> + * Based on initial code from Maxime Ripard
> + */
> +
> +#include <common.h>
> +#include <malloc.h>
> +#include <dm.h>
> +#include <w1.h>
> +#include <w1-eeprom.h>
> +#include <dm/device-internal.h>
> +
> +#include <asm/arch/gpio.h>
> +
> +#include <extension_board.h>
> +
> +#ifdef CONFIG_CMD_EXTENSION
> +
> +#define for_each_1w_device(b, d) \
> +	for (device_find_first_child(b, d); *d; device_find_next_child(d))

The name is inconsistent with the rest of the 1-wire acronyms in the
file (1w vs w1)

Also, reusing macro arguments is fairly dangerous but since it's used
only once we don't really need that macro?

Maxime
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210302/d0b2f7d8/attachment.sig>