| Message ID | 20220607202058.8304-2-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1,1/2] driver core: Introduce device_find_first_child() helper | expand |
On Tue, Jun 7, 2022 at 10:22 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > We have already a helper to get the first child device, use it and > drop custom approach. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Nice cleanup. Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > drivers/spi/spi.c | 9 ++------- > 1 file changed, 2 insertions(+), 7 deletions(-) > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > index ea09d1b42bf6..87dc8773108b 100644 > --- a/drivers/spi/spi.c > +++ b/drivers/spi/spi.c > @@ -2613,11 +2613,6 @@ int spi_slave_abort(struct spi_device *spi) > } > EXPORT_SYMBOL_GPL(spi_slave_abort); > > -static int match_true(struct device *dev, void *data) > -{ > - return 1; > -} > - > static ssize_t slave_show(struct device *dev, struct device_attribute *attr, > char *buf) > { > @@ -2625,7 +2620,7 @@ static ssize_t slave_show(struct device *dev, struct device_attribute *attr, > dev); > struct device *child; > > - child = device_find_child(&ctlr->dev, NULL, match_true); > + child = device_find_first_child(&ctlr->dev); > return sprintf(buf, "%s\n", > child ? to_spi_device(child)->modalias : NULL); > } > @@ -2644,7 +2639,7 @@ static ssize_t slave_store(struct device *dev, struct device_attribute *attr, > if (rc != 1 || !name[0]) > return -EINVAL; > > - child = device_find_child(&ctlr->dev, NULL, match_true); > + child = device_find_first_child(&ctlr->dev); > if (child) { > /* Remove registered slave */ > device_unregister(child); > -- > 2.35.1 >
On Tue, Jun 07, 2022 at 11:20:58PM +0300, Andy Shevchenko wrote: > We have already a helper to get the first child device, use it and > drop custom approach. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/spi/spi.c | 9 ++------- > 1 file changed, 2 insertions(+), 7 deletions(-) > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > index ea09d1b42bf6..87dc8773108b 100644 > --- a/drivers/spi/spi.c > +++ b/drivers/spi/spi.c > @@ -2613,11 +2613,6 @@ int spi_slave_abort(struct spi_device *spi) > } > EXPORT_SYMBOL_GPL(spi_slave_abort); > > -static int match_true(struct device *dev, void *data) > -{ > - return 1; > -} > - > static ssize_t slave_show(struct device *dev, struct device_attribute *attr, > char *buf) > { > @@ -2625,7 +2620,7 @@ static ssize_t slave_show(struct device *dev, struct device_attribute *attr, > dev); > struct device *child; > > - child = device_find_child(&ctlr->dev, NULL, match_true); > + child = device_find_first_child(&ctlr->dev); > return sprintf(buf, "%s\n", > child ? to_spi_device(child)->modalias : NULL); > } Horrible naming convention asside, what is this really showing? I do not see this documented in Documentation/ABI/ anywhere, so can it just be dropped entirely? Ah, it's in Documentation/spi/spi-summary.rst not where it belongs... Looks like "any" of the child devices could match here, so it's just finding the first one by default. So you aren't explicitly asking for the real first device, you could return the last one as well, and it would still work as there is just "one" device in this list from what I can tell. So is does this really deserve a new driver core api call? thanks, greg k-h
On Wed, Jun 08, 2022 at 01:36:17PM +0200, Greg Kroah-Hartman wrote: > On Tue, Jun 07, 2022 at 11:20:58PM +0300, Andy Shevchenko wrote: > > We have already a helper to get the first child device, use it and > > drop custom approach. > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > --- > > drivers/spi/spi.c | 9 ++------- > > 1 file changed, 2 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > > index ea09d1b42bf6..87dc8773108b 100644 > > --- a/drivers/spi/spi.c > > +++ b/drivers/spi/spi.c > > @@ -2613,11 +2613,6 @@ int spi_slave_abort(struct spi_device *spi) > > } > > EXPORT_SYMBOL_GPL(spi_slave_abort); > > > > -static int match_true(struct device *dev, void *data) > > -{ > > - return 1; > > -} > > - > > static ssize_t slave_show(struct device *dev, struct device_attribute *attr, > > char *buf) > > { > > @@ -2625,7 +2620,7 @@ static ssize_t slave_show(struct device *dev, struct device_attribute *attr, > > dev); > > struct device *child; > > > > - child = device_find_child(&ctlr->dev, NULL, match_true); > > + child = device_find_first_child(&ctlr->dev); > > return sprintf(buf, "%s\n", > > child ? to_spi_device(child)->modalias : NULL); > > } > > Horrible naming convention asside, what is this really showing? I do > not see this documented in Documentation/ABI/ anywhere, so can it just > be dropped entirely? > > Ah, it's in Documentation/spi/spi-summary.rst not where it belongs... > > Looks like "any" of the child devices could match here, so it's just > finding the first one by default. So you aren't explicitly asking for > the real first device, you could return the last one as well, and it > would still work as there is just "one" device in this list from what I > can tell. > > So is does this really deserve a new driver core api call? As I said I noticed more places like this (*) and the problem is that I can't simply use device_match_any() because of the different prototype. I agree that all thing should be using _any instead of _first. *) e.g. ptp_ocp.
On Wed, Jun 08, 2022 at 02:49:14PM +0300, Andy Shevchenko wrote: > On Wed, Jun 08, 2022 at 01:36:17PM +0200, Greg Kroah-Hartman wrote: > > On Tue, Jun 07, 2022 at 11:20:58PM +0300, Andy Shevchenko wrote: > > > We have already a helper to get the first child device, use it and > > > drop custom approach. > > > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > --- > > > drivers/spi/spi.c | 9 ++------- > > > 1 file changed, 2 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > > > index ea09d1b42bf6..87dc8773108b 100644 > > > --- a/drivers/spi/spi.c > > > +++ b/drivers/spi/spi.c > > > @@ -2613,11 +2613,6 @@ int spi_slave_abort(struct spi_device *spi) > > > } > > > EXPORT_SYMBOL_GPL(spi_slave_abort); > > > > > > -static int match_true(struct device *dev, void *data) > > > -{ > > > - return 1; > > > -} > > > - > > > static ssize_t slave_show(struct device *dev, struct device_attribute *attr, > > > char *buf) > > > { > > > @@ -2625,7 +2620,7 @@ static ssize_t slave_show(struct device *dev, struct device_attribute *attr, > > > dev); > > > struct device *child; > > > > > > - child = device_find_child(&ctlr->dev, NULL, match_true); > > > + child = device_find_first_child(&ctlr->dev); > > > return sprintf(buf, "%s\n", > > > child ? to_spi_device(child)->modalias : NULL); > > > } > > > > Horrible naming convention asside, what is this really showing? I do > > not see this documented in Documentation/ABI/ anywhere, so can it just > > be dropped entirely? > > > > Ah, it's in Documentation/spi/spi-summary.rst not where it belongs... > > > > Looks like "any" of the child devices could match here, so it's just > > finding the first one by default. So you aren't explicitly asking for > > the real first device, you could return the last one as well, and it > > would still work as there is just "one" device in this list from what I > > can tell. > > > > So is does this really deserve a new driver core api call? > > As I said I noticed more places like this (*) and the problem is that I can't > simply use device_match_any() because of the different prototype. Why not exactly? match_true() above and device_match_any() have the same signature from what I can tell: static int match_true(struct device *dev, void *data) int device_match_any(struct device *dev, const void *unused) What am I missing, the const? > I agree that all thing should be using _any instead of _first. Yes, so let's fix it please, don't propagate bad patterns. thanks, greg k-h
On Wed, Jun 08, 2022 at 02:03:32PM +0200, Greg Kroah-Hartman wrote: > On Wed, Jun 08, 2022 at 02:49:14PM +0300, Andy Shevchenko wrote: ... > Why not exactly? match_true() above and device_match_any() have the > same signature from what I can tell: > static int match_true(struct device *dev, void *data) > int device_match_any(struct device *dev, const void *unused) > > What am I missing, the const? Yep! Compiler is very unhappy about it. > > I agree that all thing should be using _any instead of _first. > > Yes, so let's fix it please, don't propagate bad patterns. Will do, thanks!
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index ea09d1b42bf6..87dc8773108b 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -2613,11 +2613,6 @@ int spi_slave_abort(struct spi_device *spi) } EXPORT_SYMBOL_GPL(spi_slave_abort); -static int match_true(struct device *dev, void *data) -{ - return 1; -} - static ssize_t slave_show(struct device *dev, struct device_attribute *attr, char *buf) { @@ -2625,7 +2620,7 @@ static ssize_t slave_show(struct device *dev, struct device_attribute *attr, dev); struct device *child; - child = device_find_child(&ctlr->dev, NULL, match_true); + child = device_find_first_child(&ctlr->dev); return sprintf(buf, "%s\n", child ? to_spi_device(child)->modalias : NULL); } @@ -2644,7 +2639,7 @@ static ssize_t slave_store(struct device *dev, struct device_attribute *attr, if (rc != 1 || !name[0]) return -EINVAL; - child = device_find_child(&ctlr->dev, NULL, match_true); + child = device_find_first_child(&ctlr->dev); if (child) { /* Remove registered slave */ device_unregister(child);
We have already a helper to get the first child device, use it and drop custom approach. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/spi/spi.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)