ARM: davinci: complete the conversion to using the reset framework

From: Bartosz Golaszewski <[email protected]>

Commit 7af1bb19f1d7 ("reset: add support for non-DT systems") introduced reset control lookup mechanism for boards that still use board files.

The routine used to register lookup entries takes the corresponding reset_controlled_dev structure as argument.

It's been determined however that for the first user of this new interface - davinci psc driver - it will be easier to register the lookup entries using the reset controller device name.

This patch changes the way lookup entries are added.

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 drivers/reset/core.c             | 33 +++++++++++++++++++++++++++++----
 include/linux/reset-controller.h |  8 +++++---
 2 files changed, 34 insertions(+), 7 deletions(-)

drivers/reset/core.c +0 -0

index 06fa4907afc4..f37048e55336 100644

drivers/reset/core.c +20 -1

 				 struct reset_control_lookup *lookup,
 				 unsigned int num_entries)
 {

@@ -174,7 +174,7 @@ void reset_controller_add_lookup(struct reset_controller_dev *rcdev,
 			continue;
 		}
 
-		entry->rcdev = rcdev;
+		entry->provider = provider;
 		list_add_tail(&entry->list, &reset_lookup_list);
 	}
 	mutex_unlock(&reset_lookup_mutex);

@@ -526,11 +526,30 @@ struct reset_control *__of_reset_control_get(struct device_node *node,
 }
 EXPORT_SYMBOL_GPL(__of_reset_control_get);
 
+static struct reset_controller_dev *
+__reset_controller_by_name(const char *name)
+{
+	struct reset_controller_dev *rcdev;
+
+	lockdep_assert_held(&reset_list_mutex);
+
+	list_for_each_entry(rcdev, &reset_controller_list, list) {
+		if (!rcdev->dev)
+			continue;
+
+		if (!strcmp(name, dev_name(rcdev->dev)))
+			return rcdev;
+	}
+
+	return NULL;
+}
+
 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;
+	struct reset_controller_dev *rcdev;
 	const char *dev_id = dev_name(dev);
 	struct reset_control *rstc = NULL;
 

drivers/reset/core.c +3 -0

+			}
+
+			rstc = __reset_control_get_internal(rcdev,
 							    lookup->index,
 							    shared);
 			mutex_unlock(&reset_list_mutex);

include/linux/reset-controller.h +0 -0

index 25698f6c1fae..1a6c25d825d3 100644
@@ -30,14 +30,14 @@ struct of_phandle_args;
  * struct reset_control_lookup - represents a single lookup entry
  *
  * @list: internal list of all reset lookup entries

include/linux/reset-controller.h +2 -0

 	unsigned int index;
 	const char *dev_id;
 	const char *con_id;

@@ -57,6 +57,7 @@ struct reset_control_lookup {
  * @owner: kernel module of the reset controller driver
  * @list: internal list of reset controller devices
  * @reset_control_head: head of internal list of requested reset controls
+ * @dev: corresponding driver model device struct
  * @of_node: corresponding device tree node as phandle target
  * @of_reset_n_cells: number of cells in reset line specifiers
  * @of_xlate: translation function to translate from specifier as found in the

@@ -68,6 +69,7 @@ struct reset_controller_dev {
 	struct module *owner;
 	struct list_head list;
 	struct list_head reset_control_head;
+	struct device *dev;
 	struct device_node *of_node;
 	int of_reset_n_cells;
 	int (*of_xlate)(struct reset_controller_dev *rcdev,

Hi Bartosz,

Quoted 6 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Commit 7af1bb19f1d7 ("reset: add support for non-DT systems") introduced reset control lookup mechanism for boards that still use board files.

Quoted 6 lines

The routine used to register lookup entries takes the corresponding reset_controlled_dev structure as argument.

It's been determined however that for the first user of this new interface - davinci psc driver - it will be easier to register the lookup entries using the reset controller device name.

Thank you, this is what I expected in the first place.

Quoted 13 lines

This patch changes the way lookup entries are added.

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 drivers/reset/core.c             | 33 +++++++++++++++++++++++++++++----
 include/linux/reset-controller.h |  8 +++++---
 2 files changed, 34 insertions(+), 7 deletions(-)

drivers/reset/core.c +0 -0

index 06fa4907afc4..f37048e55336 100644

drivers/reset/core.c EXPORT_SYMBOL_GPL(devm_reset_controller_register); · line 160

Quoted 10 lines

drivers/reset/core.c +2 -2

@@ -153,11 +153,11 @@ 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
+ * @provider: name of the reset controller provider
  * @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,
+void reset_controller_add_lookup(const char *provider,

Is there any reason not to drop the provider parameter completely? I'd just let the user add the provider device id to the lookup, see below.

Quoted 43 lines

drivers/reset/core.c +20 -1

                               struct reset_control_lookup *lookup,
                               unsigned int num_entries)
 {

@@ -174,7 +174,7 @@ void reset_controller_add_lookup(struct reset_controller_dev *rcdev,
                      continue;
              }
 
-             entry->rcdev = rcdev;
+             entry->provider = provider;
              list_add_tail(&entry->list, &reset_lookup_list);
      }
      mutex_unlock(&reset_lookup_mutex);

@@ -526,11 +526,30 @@ struct reset_control *__of_reset_control_get(struct device_node *node,
 }
 EXPORT_SYMBOL_GPL(__of_reset_control_get);
 
+static struct reset_controller_dev *
+__reset_controller_by_name(const char *name)
+{
+     struct reset_controller_dev *rcdev;
+
+     lockdep_assert_held(&reset_list_mutex);
+
+     list_for_each_entry(rcdev, &reset_controller_list, list) {
+             if (!rcdev->dev)
+                     continue;
+
+             if (!strcmp(name, dev_name(rcdev->dev)))
+                     return rcdev;
+     }
+
+     return NULL;
+}
+
 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;
+     struct reset_controller_dev *rcdev;
      const char *dev_id = dev_name(dev);
      struct reset_control *rstc = NULL;
 

drivers/reset/core.c __reset_control_get_from_lookup(struct device *dev, const char *con_id, · line 572

Quoted 9 lines

drivers/reset/core.c +4 -1

@@ -547,7 +566,13 @@ __reset_control_get_from_lookup(struct device *dev, const char *con_id,
                  ((con_id && lookup->con_id) &&
                   !strcmp(con_id, lookup->con_id))) {
                      mutex_lock(&reset_list_mutex);
-                     rstc = __reset_control_get_internal(lookup->rcdev,
+                     rcdev = __reset_controller_by_name(lookup->provider);
+                     if (!rcdev) {
+                             mutex_unlock(&reset_list_mutex);
+                             continue;

What is the reason to continue here? If we've found a matching lookup that contains a rcdev dev_id for which there is no reset controller, shouldn't we just return an error?

Quoted 14 lines

drivers/reset/core.c +3 -0

+                     }
+
+                     rstc = __reset_control_get_internal(rcdev,
                                                          lookup->index,
                                                          shared);
                      mutex_unlock(&reset_list_mutex);

include/linux/reset-controller.h +0 -0

index 25698f6c1fae..1a6c25d825d3 100644
@@ -30,14 +30,14 @@ struct of_phandle_args;
  * struct reset_control_lookup - represents a single lookup entry
  *
  * @list: internal list of all reset lookup entries

include/linux/reset-controller.h struct of_phandle_args; · line 40

Quoted 10 lines

include/linux/reset-controller.h +2 -2

- * @rcdev: reset controller device controlling this reset line
+ * @provider: name of the 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;
+     const char *provider;

Looks good to me, but I'd also extend RESET_LOOKUP to set the provider instead of passing it to the reset_controller_add_lookup function, similarly to PWM_LOOKUP:

#define RESET_LOOKUP(_provider, _index, _dev_id, _con_id)

2018-03-23 11:24 GMT+01:00 Philipp Zabel <[email protected]>:

Quoted 89 lines

Hi Bartosz,

Quoted 6 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Commit 7af1bb19f1d7 ("reset: add support for non-DT systems") introduced reset control lookup mechanism for boards that still use board files.

Quoted 6 lines

The routine used to register lookup entries takes the corresponding reset_controlled_dev structure as argument.

It's been determined however that for the first user of this new interface - davinci psc driver - it will be easier to register the lookup entries using the reset controller device name.

Thank you, this is what I expected in the first place.

Quoted 13 lines

This patch changes the way lookup entries are added.

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 drivers/reset/core.c             | 33 +++++++++++++++++++++++++++++----
 include/linux/reset-controller.h |  8 +++++---
 2 files changed, 34 insertions(+), 7 deletions(-)

drivers/reset/core.c +0 -0

index 06fa4907afc4..f37048e55336 100644

drivers/reset/core.c EXPORT_SYMBOL_GPL(devm_reset_controller_register); · line 160

Quoted 10 lines

drivers/reset/core.c +2 -2

@@ -153,11 +153,11 @@ 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
+ * @provider: name of the reset controller provider
  * @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,
+void reset_controller_add_lookup(const char *provider,

Is there any reason not to drop the provider parameter completely? I'd just let the user add the provider device id to the lookup, see below.

Quoted 43 lines

drivers/reset/core.c +20 -1

                               struct reset_control_lookup *lookup,
                               unsigned int num_entries)
 {

@@ -174,7 +174,7 @@ void reset_controller_add_lookup(struct reset_controller_dev *rcdev,
                      continue;
              }
 
-             entry->rcdev = rcdev;
+             entry->provider = provider;
              list_add_tail(&entry->list, &reset_lookup_list);
      }
      mutex_unlock(&reset_lookup_mutex);

@@ -526,11 +526,30 @@ struct reset_control *__of_reset_control_get(struct device_node *node,
 }
 EXPORT_SYMBOL_GPL(__of_reset_control_get);
 
+static struct reset_controller_dev *
+__reset_controller_by_name(const char *name)
+{
+     struct reset_controller_dev *rcdev;
+
+     lockdep_assert_held(&reset_list_mutex);
+
+     list_for_each_entry(rcdev, &reset_controller_list, list) {
+             if (!rcdev->dev)
+                     continue;
+
+             if (!strcmp(name, dev_name(rcdev->dev)))
+                     return rcdev;
+     }
+
+     return NULL;
+}
+
 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;
+     struct reset_controller_dev *rcdev;
      const char *dev_id = dev_name(dev);
      struct reset_control *rstc = NULL;
 

Quoted 14 lines

drivers/reset/core.c __reset_control_get_from_lookup(struct device *dev, const char *con_id, · line 572

Quoted 9 lines

drivers/reset/core.c +4 -1

@@ -547,7 +566,13 @@ __reset_control_get_from_lookup(struct device *dev, const char *con_id,
                  ((con_id && lookup->con_id) &&
                   !strcmp(con_id, lookup->con_id))) {
                      mutex_lock(&reset_list_mutex);
-                     rstc = __reset_control_get_internal(lookup->rcdev,
+                     rcdev = __reset_controller_by_name(lookup->provider);
+                     if (!rcdev) {
+                             mutex_unlock(&reset_list_mutex);
+                             continue;

What is the reason to continue here? If we've found a matching lookup that contains a rcdev dev_id for which there is no reset controller, shouldn't we just return an error?

Indeed. This could be used to indicate to drivers that the reset controller may not have yet been probed() or its probe() failed. How about returning -EPROBE_DEFER here?

Bart

Quoted 29 lines

Quoted 14 lines

drivers/reset/core.c +3 -0

+                     }
+
+                     rstc = __reset_control_get_internal(rcdev,
                                                          lookup->index,
                                                          shared);
                      mutex_unlock(&reset_list_mutex);

include/linux/reset-controller.h +0 -0

index 25698f6c1fae..1a6c25d825d3 100644
@@ -30,14 +30,14 @@ struct of_phandle_args;
  * struct reset_control_lookup - represents a single lookup entry
  *
  * @list: internal list of all reset lookup entries

include/linux/reset-controller.h struct of_phandle_args; · line 40

Quoted 10 lines

include/linux/reset-controller.h +2 -2

- * @rcdev: reset controller device controlling this reset line
+ * @provider: name of the 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;
+     const char *provider;

Looks good to me, but I'd also extend RESET_LOOKUP to set the provider instead of passing it to the reset_controller_add_lookup function, similarly to PWM_LOOKUP:

Quoted 2 lines

#define RESET_LOOKUP(_provider, _index, _dev_id, _con_id)

I did it mostly for brevity - I don't mind changing it if you prefer this version.