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

[Amitay Isaacs]

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.

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.
-- 

The secret is to become wise before you get old.