[Pdbg] [PATCH v4 05/19] libpdbg: Use PDBG_BACKEND_DRIVER to specify drivers to load

[Alistair Popple]

On Thursday, 30 April 2020 11:22:22 AM AEST Amitay Isaacs wrote:
> On Thu, 2020-04-30 at 10:07 +1000, Alistair Popple wrote:
> > On Tuesday, 21 April 2020 2:16:41 PM AEST Amitay Isaacs wrote:
> > > When PDBG_BACKEND_DTB is specified, backend may not be set.  This
> > > means
> > > only the drivers registered with PDBG_DEFAULT_BACKEND will be
> > > loaded.
> > > To be able to match backend specific drivers for targets in system
> > > tree,
> > > use the backend specified in PDBG_BACKEND_DRIVER environment
> > > variable.
> > > 
> > > Signed-off-by: Amitay Isaacs <amitay at ozlabs.org>
> > > ---
> > > 
> > >  libpdbg/hwunit.c | 37 +++++++++++++++++++++++++++++++++++--
> > >  1 file changed, 35 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/libpdbg/hwunit.c b/libpdbg/hwunit.c
> > > index 074ddef..3d6a05d 100644
> > > --- a/libpdbg/hwunit.c
> > > +++ b/libpdbg/hwunit.c
> > > @@ -14,6 +14,7 @@
> > > 
> > >   * limitations under the License.
> > >   */
> > > 
> > > +#include <stdlib.h>
> > > 
> > >  #include <string.h>
> > >  #include <assert.h>
> > > 
> > > @@ -70,12 +71,44 @@ static const struct hw_unit_info
> > > *find_compatible(enum
> > > pdbg_backend backend, return NULL;
> > > 
> > >  }
> > > 
> > > +static enum pdbg_backend get_backend_driver(void)
> > > +{
> > > +	const char *tmp;
> > > +	enum pdbg_backend backend = PDBG_DEFAULT_BACKEND;
> > > +
> > > +	tmp = getenv("PDBG_BACKEND_DRIVER");
> > > +	if (tmp) {
> > > +		if (!strcmp(tmp, "fsi"))
> > > +			backend = PDBG_BACKEND_FSI;
> > > +		else if (!strcmp(tmp, "i2c"))
> > > +			backend = PDBG_BACKEND_I2C;
> > > +		else if (!strcmp(tmp, "kernel"))
> > > +			backend = PDBG_BACKEND_KERNEL;
> > > +		else if (!strcmp(tmp, "fake"))
> > > +			backend = PDBG_BACKEND_FAKE;
> > > +		else if (!strcmp(tmp, "host"))
> > > +			backend = PDBG_BACKEND_HOST;
> > > +		else if (!strcmp(tmp, "cronus"))
> > > +			backend = PDBG_BACKEND_CRONUS;
> > > +	}
> > > +
> > > +	return backend;
> > > +}
> > > +
> > > 
> > >  const struct hw_unit_info *pdbg_hwunit_find_compatible(const char
> > > 
> > > *compat_list, uint32_t len)
> > > 
> > >  {
> > > 
> > > -	const struct hw_unit_info *p;
> > > +	const struct hw_unit_info *p = NULL;
> > > +	enum pdbg_backend backend = pdbg_get_backend();
> > > +
> > > +	if (backend == PDBG_DEFAULT_BACKEND) {
> > > +		backend = get_backend_driver();
> > 
> > The flow here was initially a little confusing for me to figure out
> > what was
> > going on here and why, although I understand now - you hit this case
> > when an
> > external backend DTB is specified and you need to figure out the
> > driver
> > collection to use.
> > 
> > That makes sense but I think this code belongs with the rest of the
> > initialisation code in libpdbg/dtb.c. Can we initialise the backend
> > correctly
> > with the rest of the environment variables in pdbg_default_dtb() such
> > that
> > pdbg_get_backend() just returns the "correct" thing?
> 
> Sure.  I was also not happy about having to do separate checks
> elsewhere in the code.
> 
> Are you ok with the environment variable PDBG_BACKEND_DRIVER?  I
> thought of using PDBG_BACKEND earlier, but we are specifying the
> backend device tree, so that seemed odd to also specify PDBG_BACKEND.
> That's why PDBG_BACKEND_DRIVER, which is backend setting for selecting
> a collection of drivers.

Yeah, I think PDBG_BACKEND_DRIVER makes the most sense. Thanks.
 
- Alistair

> Let me know what you prefer.
> 
> > - Alistair
> > 
> > > +		if (backend != PDBG_DEFAULT_BACKEND)
> > > +			p = find_compatible(backend, compat_list, len);
> > > +	} else {
> > > +		p = find_compatible(backend, compat_list, len);
> > > +	}
> > > 
> > > -	p = find_compatible(pdbg_get_backend(), compat_list, len);
> > > 
> > >  	if (!p)
> > >  	
> > >  		p = find_compatible(PDBG_DEFAULT_BACKEND, compat_list,
> > > 
> > > len);
> 
> Amitay.