[Pdbg] [PATCH v2 13/16] libpdbg: Remove old dt_prop functions

Amitay Isaacs amitay at ozlabs.org
Wed Nov 7 17:52:51 AEDT 2018


If a function returns 0/-1 for success/failure, I think returning bool
makes code easier to read.  I guess it's just the matter of style.

On Wed, 2018-11-07 at 16:39 +1100, Alistair Popple wrote:
> The dt_prop functions were copied over from Skiboot. Rework and
> rename
> these to make use of the existing libpdbg property functions. These
> should not have been used by external projects so maintaining
> backwards compatibility is not a concern.
> 
> Signed-off-by: Alistair Popple <alistair at popple.id.au>
> ---
>  libpdbg/bmcfsi.c  | 37 ++++++++++++++++++++++++-------------
>  libpdbg/cfam.c    |  2 +-
>  libpdbg/device.c  | 38 +-------------------------------------
>  libpdbg/device.h  |  7 -------
>  libpdbg/i2c.c     |  5 ++++-
>  libpdbg/libpdbg.c | 16 ++++++++++++++++
>  libpdbg/libpdbg.h |  1 +
>  libpdbg/p9chip.c  |  4 +++-
>  8 files changed, 50 insertions(+), 60 deletions(-)
> 
> diff --git a/libpdbg/bmcfsi.c b/libpdbg/bmcfsi.c
> index b57e029..a233106 100644
> --- a/libpdbg/bmcfsi.c
> +++ b/libpdbg/bmcfsi.c
> @@ -42,7 +42,7 @@
>   * address offset */
>  struct gpio_pin {
>  	uint32_t offset;
> -	int bit;
> +	uint32_t bit;
>  };
>  
>  enum gpio {
> @@ -71,7 +71,7 @@ enum fsi_result {
>  	FSI_ERR_C = 0x3,
>  };
>  
> -static int clock_delay  = 0;
> +static uint32_t clock_delay  = 0;
>  
>  #define FSI_DATA0_REG	0x1000
>  #define FSI_DATA1_REG	0x1001
> @@ -459,17 +459,28 @@ int bmcfsi_probe(struct pdbg_target *target)
>  	}
>  
>  	if (!gpio_reg) {
> -		gpio_pins[GPIO_FSI_CLK].offset =
> dt_prop_get_u32_index(target, "fsi_clk", 0);
> -		gpio_pins[GPIO_FSI_CLK].bit =
> dt_prop_get_u32_index(target, "fsi_clk", 1);
> -		gpio_pins[GPIO_FSI_DAT].offset =
> dt_prop_get_u32_index(target, "fsi_dat", 0);
> -		gpio_pins[GPIO_FSI_DAT].bit =
> dt_prop_get_u32_index(target, "fsi_dat", 1);
> -		gpio_pins[GPIO_FSI_DAT_EN].offset =
> dt_prop_get_u32_index(target, "fsi_dat_en", 0);
> -		gpio_pins[GPIO_FSI_DAT_EN].bit =
> dt_prop_get_u32_index(target, "fsi_dat_en", 1);
> -		gpio_pins[GPIO_FSI_ENABLE].offset =
> dt_prop_get_u32_index(target, "fsi_enable", 0);
> -		gpio_pins[GPIO_FSI_ENABLE].bit =
> dt_prop_get_u32_index(target, "fsi_enable", 1);
> -		gpio_pins[GPIO_CRONUS_SEL].offset =
> dt_prop_get_u32_index(target, "cronus_sel", 0);
> -		gpio_pins[GPIO_CRONUS_SEL].bit =
> dt_prop_get_u32_index(target, "cronus_sel", 1);
> -		clock_delay = dt_prop_get_u32(target, "clock_delay");
> +		assert(!(pdbg_target_u32_index(target, "fsi_clk", 0,
> +			 &gpio_pins[GPIO_FSI_CLK].offset)));
> +		assert(!(pdbg_target_u32_index(target, "fsi_clk", 1,
> +			 &gpio_pins[GPIO_FSI_CLK].bit)));
> +		assert(!(pdbg_target_u32_index(target, "fsi_dat", 0,
> +			 &gpio_pins[GPIO_FSI_DAT].offset)));
> +		assert(!(pdbg_target_u32_index(target, "fsi_dat", 1,
> +			 &gpio_pins[GPIO_FSI_DAT].bit)));
> +		assert(!(pdbg_target_u32_index(target, "fsi_dat_en", 0,
> +			 &gpio_pins[GPIO_FSI_DAT_EN].offset)));
> +		assert(!(pdbg_target_u32_index(target, "fsi_dat_en", 1,
> +			 &gpio_pins[GPIO_FSI_DAT_EN].bit)));
> +		assert(!(pdbg_target_u32_index(target, "fsi_enable", 0,
> +			 &gpio_pins[GPIO_FSI_ENABLE].offset)));
> +		assert(!(pdbg_target_u32_index(target, "fsi_enable", 1,
> +			 &gpio_pins[GPIO_FSI_ENABLE].bit)));
> +		assert(!(pdbg_target_u32_index(target, "cronus_sel", 0,
> +			 &gpio_pins[GPIO_CRONUS_SEL].offset)));
> +		assert(!(pdbg_target_u32_index(target, "cronus_sel", 1,
> +			 &gpio_pins[GPIO_CRONUS_SEL].bit)));
> +		assert(!(pdbg_target_u32_property(target,
> "clock_delay",
> +			 &clock_delay)));
>  
>  		/* We only have to do this init once per backend */
>  		gpio_reg = mmap(NULL, getpagesize(),
> diff --git a/libpdbg/cfam.c b/libpdbg/cfam.c
> index 67ab22e..c9bdf3b 100644
> --- a/libpdbg/cfam.c
> +++ b/libpdbg/cfam.c
> @@ -317,7 +317,7 @@ static int cfam_hmfsi_probe(struct pdbg_target
> *target)
>  	int rc;
>  
>  	/* Enable the port in the upstream control register */
> -	port = dt_prop_get_u32(target, "port");
> +	assert(!(pdbg_target_u32_property(target, "port", &port)));
>  	fsi_read(fsi_parent, 0x3404, &value);
>  	value |= 1 << (31 - port);
>  	if ((rc = fsi_write(fsi_parent, 0x3404, value))) {
> diff --git a/libpdbg/device.c b/libpdbg/device.c
> index 5cd8302..288891b 100644
> --- a/libpdbg/device.c
> +++ b/libpdbg/device.c
> @@ -501,14 +501,7 @@ struct pdbg_target
> *dt_find_compatible_node(struct pdbg_target *root,
>  	return NULL;
>  }
>  
> -u32 dt_prop_get_u32(const struct pdbg_target *node, const char
> *prop)
> -{
> -	const struct dt_property *p = dt_require_property(node, prop,
> 4);
> -
> -	return dt_property_get_cell(p, 0);
> -}
> -
> -static u32 dt_prop_get_u32_def(const struct pdbg_target *node, const
> char *prop, u32 def)
> +static uint32_t dt_prop_get_u32_def(const struct pdbg_target *node,
> const char *prop, u32 def)
>  {
>          const struct dt_property *p = dt_find_property(node, prop);
>  
> @@ -518,35 +511,6 @@ static u32 dt_prop_get_u32_def(const struct
> pdbg_target *node, const char *prop,
>          return dt_property_get_cell(p, 0);
>  }
>  
> -u32 dt_prop_get_u32_index(const struct pdbg_target *node, const char
> *prop, u32 index)
> -{
> -	const struct dt_property *p = dt_require_property(node, prop,
> -1);
> -
> -	return dt_property_get_cell(p, index);
> -}
> -
> -const void *dt_prop_get(const struct pdbg_target *node, const char
> *prop)
> -{
> -	const struct dt_property *p = dt_require_property(node, prop,
> -1);
> -
> -	return p->prop;
> -}
> -
> -const void *dt_prop_get_def(const struct pdbg_target *node, const
> char *prop,
> -			    void *def)
> -{
> -	const struct dt_property *p = dt_find_property(node, prop);
> -
> -	return p ? p->prop : def;
> -}
> -
> -u32 dt_prop_get_cell(const struct pdbg_target *node, const char
> *prop, u32 cell)
> -{
> -	const struct dt_property *p = dt_require_property(node, prop,
> -1);
> -
> -	return dt_property_get_cell(p, cell);
> -}
> -
>  static enum pdbg_target_status str_to_status(const char *status)
>  {
>  	if (!strcmp(status, "enabled")) {
> diff --git a/libpdbg/device.h b/libpdbg/device.h
> index 29224a2..ac265e9 100644
> --- a/libpdbg/device.h
> +++ b/libpdbg/device.h
> @@ -37,11 +37,4 @@ struct pdbg_target *dt_find_compatible_node(struct
> pdbg_target *root,
>  	for (node = NULL; 			        \
>  	     (node = dt_find_compatible_node(root, node, compat)) !=
> NULL;)
>  
> -/* Simplified accessors */
> -u32 dt_prop_get_u32(const struct pdbg_target *node, const char
> *prop);
> -u32 dt_prop_get_u32_index(const struct pdbg_target *node, const char
> *prop, u32 index);
> -const void *dt_prop_get(const struct pdbg_target *node, const char
> *prop);
> -const void *dt_prop_get_def(const struct pdbg_target *node, const
> char *prop,
> -			    void *def);
> -
>  #endif /* __DEVICE_H */
> diff --git a/libpdbg/i2c.c b/libpdbg/i2c.c
> index 9cb6271..f1c6ea9 100644
> --- a/libpdbg/i2c.c
> +++ b/libpdbg/i2c.c
> @@ -130,7 +130,10 @@ int i2c_target_probe(struct pdbg_target *target)
>  	const char *bus;
>  	int addr;
>  
> -	bus = dt_prop_get_def(&pib->target, "bus", "/dev/i2c4");
> +	bus = pdbg_target_property(&pib->target, "bus", NULL);
> +	if (!bus)
> +		bus = "/dev/i2c4";
> +
>  	addr = pdbg_target_address(&pib->target, NULL);
>  	assert(addr);
>  
> diff --git a/libpdbg/libpdbg.c b/libpdbg/libpdbg.c
> index 07947cb..b59590d 100644
> --- a/libpdbg/libpdbg.c
> +++ b/libpdbg/libpdbg.c
> @@ -150,6 +150,22 @@ int pdbg_target_u32_property(struct pdbg_target
> *target, const char *name, uint3
>          return 0;
>  }
>  
> +int pdbg_target_u32_index(struct pdbg_target *target, const char
> *name, int index, uint32_t *val)
> +{
> +        size_t len;
> +        void *p;
> +
> +        p = pdbg_get_target_property(target, name, &len);
> +        if (!p)
> +                return -1;
> +
> +        assert(len >= (index+1)*sizeof(u32));

May be replace u32 with uint32_t?

> +
> +        /* Always aligned, so this works. */
> +        *val = be32toh(((const u32 *)p)[index]);

Why do we need const cast here?  Can p be defined as uint32_t *?

> +        return 0;
> +}
> +
>  uint32_t pdbg_target_chip_id(struct pdbg_target *target)
>  {
>          uint32_t id;
> diff --git a/libpdbg/libpdbg.h b/libpdbg/libpdbg.h
> index f8fe3e8..6108af9 100644
> --- a/libpdbg/libpdbg.h
> +++ b/libpdbg/libpdbg.h
> @@ -73,6 +73,7 @@ void pdbg_target_set_property(struct pdbg_target
> *target, const char *name, cons
>  /* Get the given property and return the size */
>  void *pdbg_target_property(struct pdbg_target *target, const char
> *name, size_t *size);
>  int pdbg_target_u32_property(struct pdbg_target *target, const char
> *name, uint32_t *val);
> +int pdbg_target_u32_index(struct pdbg_target *target, const char
> *name, int index, uint32_t *val);
>  uint64_t pdbg_target_address(struct pdbg_target *target, uint64_t
> *size);
>  
>  /* Old deprecated for names for the above. Do not use for new
> projects
> diff --git a/libpdbg/p9chip.c b/libpdbg/p9chip.c
> index 2411103..9d43725 100644
> --- a/libpdbg/p9chip.c
> +++ b/libpdbg/p9chip.c
> @@ -118,8 +118,10 @@ static struct thread_state
> p9_get_thread_status(struct thread *thread)
>  static int p9_thread_probe(struct pdbg_target *target)
>  {
>  	struct thread *thread = target_to_thread(target);
> +	uint32_t tid;
>  
> -	thread->id = dt_prop_get_u32(target, "tid");
> +	assert(!pdbg_target_u32_property(target, "tid", &tid));
> +	thread->id = tid;
>  	thread->status = p9_get_thread_status(thread);
>  
>  	return 0;
> -- 
> 2.11.0
> 

Amitay.
-- 

I've sometimes thought of marrying, and then I've thought again. - Noel
Coward



More information about the Pdbg mailing list