[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