Hi Bartosz,
thank you for the update.
On Fri, 2018-02-23 at 12:39 +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszew...@baylibre.com>
>
> The reset framework only supports device-tree. There are some platforms
> however, which need to use it even in legacy, board-file based mode.
>
> An example of such architecture is the DaVinci family of SoCs which
> supports both device tree and legacy boot modes and we don't want to
> introduce any regressions.
>
> We're currently working on converting the platform from its hand-crafted
> clock API to using the common clock framework. Part of the overhaul will
> be representing the chip's power sleep controller's reset lines using
> the reset framework.
>
> This changeset extends the core reset code with a new reset lookup
> entry structure. It contains data allowing the reset core to associate
> reset lines with devices by comparing the dev_id and con_id strings.
>
> It also provides a function allowing drivers to register lookup entries
> with the framework.
>
> The new lookup function is only called as a fallback in case the
> of_node field is NULL and doesn't change anything for current users.
>
> Tested with a dummy reset driver with several lookup entries.
>
> An example lookup table registration from a driver can be found below:
>
> static struct reset_control_lookup foobar_reset_lookup[] = {
> RESET_LOOKUP("foo.0", "foo", 15),
> RESET_LOOKUP("bar.0", NULL, 5),
> };
>
> foobar_probe()
> {
> ...
>
> reset_controller_add_lookup(&rcdev, foobar_reset_lookup,
> ARRAY_SIZE(foobar_reset_lookup));
>
> ...
> }
>
> Cc: Sekhar Nori <nsek...@ti.com>
> Cc: Kevin Hilman <khil...@baylibre.com>
> Cc: David Lechner <da...@lechnology.com>
> Signed-off-by: Bartosz Golaszewski <bgolaszew...@baylibre.com>
> ---
> v1 -> v2:
> - renamed the new function to __reset_control_get_from_lookup()
> - added a missing break; when a matching entry is found
> - rearranged the code in __reset_control_get() - we can no longer get to the
> return at the bottom, so remove it and return from
> __reset_control_get_from_lookup() if __of_reset_control_get() fails
> - return -ENOENT from reset_contol_get() if we can't find a matching entry,
> prevously returned -EINVAL referred to the fact that we passed a device
> without the of_node which is no longer an error condition
> - add a comment about needing a sentinel in the lookup table
>
> v2 -> v3:
> - added the reset id number field to the lookup struct so that we don't need
> to rely on the array index
>
> v3 -> v4:
> - separated the driver and lookup table registration logic by adding a
> function meant to be called by machine-specific code that adds a lookup
> table to the internal list
> - the correct reset controller is now found by first finding the lookup
> table associated with it, then finding the actual reset controller by
> the associated device
>
> v4 -> v5:
> - since the first user of this will be the davinci clk driver and it
> already registers clock lookup from within the driver code - allow
> drivers to register lookups with the assumption that the code can be
> extended to make it possible to register entries from machine code as
> well
How do you imagine this may be extended? By adding an rddev_devid field
to the lookup, similarly to the pwm_lookup?
I suppose reset_controller_add_lookup could then be called with a NULL
rcdev to register lookups by id.
> - simplify the code - only expose a single lookup structure and a simply
> registration function
> - add the RESET_LOOKUP macro for brevity
>
> drivers/reset/core.c | 65
> +++++++++++++++++++++++++++++++++++++++-
> include/linux/reset-controller.h | 28 +++++++++++++++++
> 2 files changed, 92 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> index da4292e9de97..75e54a05147a 100644
> --- a/drivers/reset/core.c
> +++ b/drivers/reset/core.c
> @@ -23,6 +23,9 @@
> static DEFINE_MUTEX(reset_list_mutex);
> static LIST_HEAD(reset_controller_list);
>
> +static DEFINE_MUTEX(reset_lookup_mutex);
> +static LIST_HEAD(reset_lookup_list);
> +
> /**
> * struct reset_control - a reset control
> * @rcdev: a pointer to the reset controller device
> @@ -148,6 +151,30 @@ int devm_reset_controller_register(struct device *dev,
> }
> EXPORT_SYMBOL_GPL(devm_reset_controller_register);
>
> +/**
> + * reset_controller_add_lookup - register a set of lookup entries
> + * @rcdev: initialized reset controller device owning the reset line
> + * @lookup: array of reset lookup entries
> + * @num_entries: number of entries in the lookup array
> + */
> +void reset_controller_add_lookup(struct reset_controller_dev *rcdev,
> + struct reset_control_lookup *lookup,
> + unsigned int num_entries)
> +{
> + struct reset_control_lookup *entry;
> + unsigned int i;
> +
> + mutex_lock(&reset_lookup_mutex);
> + for (i = 0; i < num_entries; i++) {
> + entry = &lookup[i];
> +
> + entry->rcdev = rcdev;
> + list_add_tail(&entry->list, &reset_lookup_list);
> + }
> + mutex_unlock(&reset_lookup_mutex);
> +}
> +EXPORT_SYMBOL_GPL(reset_controller_add_lookup);
> +
What about reset_controller_remove_lookup?
> static inline struct reset_control_array *
> rstc_to_array(struct reset_control *rstc) {
> return container_of(rstc, struct reset_control_array, base);
> @@ -493,6 +520,42 @@ struct reset_control *__of_reset_control_get(struct
> device_node *node,
> }
> EXPORT_SYMBOL_GPL(__of_reset_control_get);
>
> +static struct reset_control *
> +__reset_control_get_from_lookup(struct device *dev, const char *con_id,
> + bool shared, bool optional)
> +{
> + const struct reset_control_lookup *lookup;
> + const char *dev_id = dev_name(dev);
> + struct reset_control *rstc = NULL;
> +
> + if (!dev)
> + return ERR_PTR(-EINVAL);
> +
> + mutex_lock(&reset_lookup_mutex);
> +
> + list_for_each_entry(lookup, &reset_lookup_list, list) {
> + if (strcmp(lookup->dev_id, dev_id))
This will crash if (lookup->dev_id == NULL).
> + continue;
> +
> + if ((!con_id && !lookup->con_id) ||
> + !strcmp(con_id, lookup->con_id)) {
This will crash if only one of con_id or lookup->con_id is NULL.
Call strcmp only if both strings are valid.
> + mutex_lock(&reset_list_mutex);
> + rstc = __reset_control_get_internal(lookup->rcdev,
> + lookup->index,
> + shared);
> + mutex_unlock(&reset_list_mutex);
> + break;
> + }
> + }
> +
> + mutex_unlock(&reset_lookup_mutex);
> +
> + if (!rstc)
> + return optional ? NULL : ERR_PTR(-ENOENT);
> +
> + return rstc;
> +}
> +
> struct reset_control *__reset_control_get(struct device *dev, const char *id,
> int index, bool shared, bool optional)
> {
> @@ -500,7 +563,7 @@ struct reset_control *__reset_control_get(struct device
> *dev, const char *id,
> return __of_reset_control_get(dev->of_node, id, index, shared,
> optional);
>
> - return optional ? NULL : ERR_PTR(-EINVAL);
> + return __reset_control_get_from_lookup(dev, id, shared, optional);
> }
> EXPORT_SYMBOL_GPL(__reset_control_get);
>
> diff --git a/include/linux/reset-controller.h
> b/include/linux/reset-controller.h
> index adb88f8cefbc..25698f6c1fae 100644
> --- a/include/linux/reset-controller.h
> +++ b/include/linux/reset-controller.h
> @@ -26,6 +26,30 @@ struct module;
> struct device_node;
> struct of_phandle_args;
>
> +/**
> + * struct reset_control_lookup - represents a single lookup entry
> + *
> + * @list: internal list of all reset lookup entries
> + * @rcdev: reset controller device controlling this reset line
> + * @index: ID of the reset controller in the reset controller device
> + * @dev_id: name of the device associated with this reset line
> + * @con_id name of the reset line (can be NULL)
> + */
> +struct reset_control_lookup {
> + struct list_head list;
> + struct reset_controller_dev *rcdev;
> + unsigned int index;
> + const char *dev_id;
> + const char *con_id;
> +};
> +
> +#define RESET_LOOKUP(_dev_id, _con_id, _index)
> \
> + { \
> + .dev_id = _dev_id, \
> + .con_id = _con_id, \
> + .index = _index, \
> + }
> +
> /**
> * struct reset_controller_dev - reset controller entity that might
> * provide multiple reset controls
> @@ -58,4 +82,8 @@ struct device;
> int devm_reset_controller_register(struct device *dev,
> struct reset_controller_dev *rcdev);
>
> +void reset_controller_add_lookup(struct reset_controller_dev *rcdev,
> + struct reset_control_lookup *lookup,
> + unsigned int num_entries);
> +
> #endif
regards
Philipp
