[PATCH v3 07/10] arm: sunxi: add support for DIP detection to CHIP board

[Köry Maincent]

On Thu, 15 Apr 2021 11:32:48 +0200
Maxime Ripard <maxime at cerno.tech> wrote:

> Hi,
> 
> On Thu, Apr 15, 2021 at 10:38:43AM +0200, Köry Maincent wrote:
> > > >  arch/arm/mach-sunxi/Kconfig |   9 ++++
> > > >  board/sunxi/Makefile        |   1 +
> > > >  board/sunxi/chip.c          | 104 ++++++++++++++++++++++++++++++++++++
> > > >  3 files changed, 114 insertions(+)
> > > >  create mode 100644 board/sunxi/chip.c
> > > > 
> > > > diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig
> > > > index 49ef217f08..2b6ba873ff 100644
> > > > --- a/arch/arm/mach-sunxi/Kconfig
> > > > +++ b/arch/arm/mach-sunxi/Kconfig
> > > > @@ -1017,3 +1017,12 @@ config PINEPHONE_DT_SELECTION
> > > >  	  correct PinePhone hardware revision during boot.
> > > >  
> > > >  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..9be2ede57b
> > > > --- /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    
> > > 
> > > You depend on CMD_EXTENSION already, so that code won't even be compiled
> > > if that configuration option is false  
> > 
> > It is wrong, the build of this code depend on CHIP_DIP_SCAN and this config
> > *imply* CMD_EXTENSION.  
> 
> Indeed :)
> 
> > Therefore it can be build even if CMD_EXTENSION is false.  
> 
> It's not really clear to me what would be the point though? Nothing in
> that file will be compiled if CMD_EXTENSION, so it looks like (from a
> functional point of view) CMD_EXTENSION doesn't bring anything over
> SUPPORT_EXTENSION_SCAN
> 
> Why do we need both?

My point was to separate this part of code in chip.c in case of future
additional code in this file.
Your right, from a functional point it may be better to replace the imply by a
select and remove "#ifdef CONFIG_CMD_EXTENSION".

Regards,

Köry