| Message ID | 20210302095813.3273-11-kory.maincent@bootlin.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Add support for extension boards detection and DT overlays application | expand |
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@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@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
Hello Maxime, Thanks for your review. On Tue, 2 Mar 2021 17:05:38 +0100 Maxime Ripard <maxime@cerno.tech> wrote: > 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 Ok I will merge them into one commit. > > > 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@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) Yes, I didn't notice this. > > Also, reusing macro arguments is fairly dangerous but since it's used > only once we don't really need that macro? It was to increase the readability of the loop, but if you judge it not pertinent I can remove it.
On Tue, Mar 02, 2021 at 05:14:44PM +0100, Köry Maincent wrote: > Hello Maxime, > > Thanks for your review. > > On Tue, 2 Mar 2021 17:05:38 +0100 > Maxime Ripard <maxime@cerno.tech> wrote: > > > 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 > > Ok I will merge them into one commit. Sorry my sentence was incomplete, I meant to say and enable the Kconfig symbol in another patch Maxime
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 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@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)) + +#define dip_convert(field) \ + ( \ + (sizeof(field) == 1) ? field : \ + (sizeof(field) == 2) ? be16_to_cpu(field) : \ + (sizeof(field) == 4) ? be32_to_cpu(field) : \ + -1 \ + ) + +#define DIP_MAGIC 0x50494843 /* CHIP */ + +struct dip_w1_header { + u32 magic; /* CHIP */ + u8 version; /* spec version */ + u32 vendor_id; + u16 product_id; + u8 product_version; + char vendor_name[32]; + char product_name[32]; + u8 rsvd[36]; /* rsvd for future spec versions */ + u8 data[16]; /* user data, per-dip specific */ +} __packed; + +int extension_board_scan(struct list_head *extension_list) +{ + struct extension *dip; + struct dip_w1_header w1_header; + struct udevice *bus, *dev; + u32 vid; + u16 pid; + int ret; + + int num_dip = 0; + + sunxi_gpio_set_pull(SUNXI_GPD(2), SUNXI_GPIO_PULL_UP); + + ret = w1_get_bus(0, &bus); + if (ret) { + printf("one wire interface not found\n"); + return 0; + } + + for_each_1w_device(bus, &dev) { + if (w1_get_device_family(dev) != W1_FAMILY_DS2431) + continue; + + ret = device_probe(dev); + if (ret) { + printf("Couldn't probe device %s: error %d", + dev->name, ret); + continue; + } + + w1_eeprom_read_buf(dev, 0, (u8 *)&w1_header, sizeof(w1_header)); + + if (w1_header.magic != DIP_MAGIC) + continue; + + vid = dip_convert(w1_header.vendor_id); + pid = dip_convert(w1_header.product_id); + + printf("DIP: %s (0x%x) from %s (0x%x)\n", + w1_header.product_name, pid, + w1_header.vendor_name, vid); + + dip = calloc(1, sizeof(struct extension)); + if (!dip) { + printf("Error in memory allocation\n"); + return num_dip; + } + + snprintf(dip->overlay, sizeof(dip->overlay), "dip-%x-%x.dtbo", + vid, pid); + strncpy(dip->name, w1_header.product_name, 32); + strncpy(dip->owner, w1_header.vendor_name, 32); + list_add_tail(&dip->list, extension_list); + num_dip++; + } + return num_dip; +} + +#endif /* CONFIG_CMD_EXTENSION */
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@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