| Message ID | 1453300171-25473-2-git-send-email-aleksey.makarov@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jan 20, 2016 at 4:29 PM, Aleksey Makarov <aleksey.makarov@linaro.org> wrote: > Factor out the code that finds the first physical device > of a given ACPI device. It is used in several places. > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org> Hmm… Sorry, didn't notice one style issue and there is one is matter of taste below. > --- a/drivers/acpi/acpi_platform.c > +++ b/drivers/acpi/acpi_platform.c > @@ -43,7 +43,6 @@ static const struct acpi_device_id forbidden_id_list[] = { > + pdevinfo.parent = adev->parent ? > + acpi_get_first_physical_node(adev->parent) : NULL; Matter of taste, but I believe if-else looks better here even when consumes +2 LOC. Or, does it fit 80? How wide then? > --- a/drivers/acpi/bus.c > +++ b/drivers/acpi/bus.c > @@ -478,24 +478,35 @@ static void acpi_device_remove_notify_handler(struct acpi_device *device) > Device Matching > -------------------------------------------------------------------------- */ > > -static struct acpi_device *acpi_primary_dev_companion(struct acpi_device *adev, > - const struct device *dev) > +/** > + * acpi_device_fix_parent - Get first physical node of an ACPI device 'node' -> 'device node' Name of the function is wrong. > + * @adev: ACPI device in question > + */ > +struct device *acpi_get_first_physical_node(struct acpi_device *adev) > { > struct mutex *physical_node_lock = &adev->physical_node_lock; > + struct device *node = NULL; > > mutex_lock(physical_node_lock); > - if (list_empty(&adev->physical_node_list)) { > - adev = NULL; > - } else { > - const struct acpi_device_physical_node *node; > > + if (!list_empty(&adev->physical_node_list)) > node = list_first_entry(&adev->physical_node_list, > - struct acpi_device_physical_node, node); > - if (node->dev != dev) > - adev = NULL; > - } > + struct acpi_device_physical_node, node)->dev; I didn't notice this '->dev' thingy. I supposed that the function returns struct acpi_device_physical_node *, not struct device *. Currently the name is not aligned with returned value. > + > mutex_unlock(physical_node_lock); > - return adev; > + > + return node; > +}
Hi Andy, On 20.01.2016 21:12, Andy Shevchenko wrote: > On Wed, Jan 20, 2016 at 4:29 PM, Aleksey Makarov > <aleksey.makarov@linaro.org> wrote: >> Factor out the code that finds the first physical device >> of a given ACPI device. It is used in several places. >> >> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> >> Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org> > > Hmm… Sorry, didn't notice one style issue and there is one is matter > of taste below. > >> --- a/drivers/acpi/acpi_platform.c >> +++ b/drivers/acpi/acpi_platform.c >> @@ -43,7 +43,6 @@ static const struct acpi_device_id forbidden_id_list[] = { > >> + pdevinfo.parent = adev->parent ? >> + acpi_get_first_physical_node(adev->parent) : NULL; > > Matter of taste, but I believe if-else looks better here even when > consumes +2 LOC. > Or, does it fit 80? How wide then? It does not fit 80 chars. And I would prefer to leave ?: here. >> --- a/drivers/acpi/bus.c >> +++ b/drivers/acpi/bus.c >> @@ -478,24 +478,35 @@ static void acpi_device_remove_notify_handler(struct acpi_device *device) >> Device Matching >> -------------------------------------------------------------------------- */ >> >> -static struct acpi_device *acpi_primary_dev_companion(struct acpi_device *adev, >> - const struct device *dev) >> +/** >> + * acpi_device_fix_parent - Get first physical node of an ACPI device > > 'node' -> 'device node' > Name of the function is wrong. I will fix the name of function. The type of returned value is clear from the function definition. >> + * @adev: ACPI device in question >> + */ >> +struct device *acpi_get_first_physical_node(struct acpi_device *adev) >> { >> struct mutex *physical_node_lock = &adev->physical_node_lock; >> + struct device *node = NULL; >> >> mutex_lock(physical_node_lock); >> - if (list_empty(&adev->physical_node_list)) { >> - adev = NULL; >> - } else { >> - const struct acpi_device_physical_node *node; >> >> + if (!list_empty(&adev->physical_node_list)) >> node = list_first_entry(&adev->physical_node_list, >> - struct acpi_device_physical_node, node); >> - if (node->dev != dev) >> - adev = NULL; >> - } >> + struct acpi_device_physical_node, node)->dev; > > I didn't notice this '->dev' thingy. I supposed that the function > returns struct acpi_device_physical_node *, not struct device *. > > Currently the name is not aligned with returned value. It is aligned with the returned value (but not with the type of returned value). So I would prefer to leave it as is. Thank you for review. Aleksey Makarov > >> + >> mutex_unlock(physical_node_lock); >> - return adev; >> + >> + return node; >> +} > >
On Wed, Jan 20, 2016 at 7:00 PM, Aleksey Makarov <aleksey.makarov@linaro.org> wrote: > On 20.01.2016 21:12, Andy Shevchenko wrote: >> >> On Wed, Jan 20, 2016 at 4:29 PM, Aleksey Makarov >> <aleksey.makarov@linaro.org> wrote: >>> >>> Factor out the code that finds the first physical device >>> of a given ACPI device. It is used in several places. >>> >>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> For all your comments, I have to withdraw my tag. You may return it back if Rafael and / or Mika will be okay with these changes. Sorry for inconvenience. >>> Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org> >> >> >> Hmm… Sorry, didn't notice one style issue and there is one is matter >> of taste below.
On Wednesday, January 20, 2016 08:29:26 PM Aleksey Makarov wrote: > Factor out the code that finds the first physical device > of a given ACPI device. It is used in several places. > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> I guess the above doesn't apply any more, does it? > Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org> > --- > drivers/acpi/acpi_platform.c | 19 ++----------------- > drivers/acpi/bus.c | 33 ++++++++++++++++++++++----------- > drivers/acpi/internal.h | 1 + > 3 files changed, 25 insertions(+), 28 deletions(-) > > diff --git a/drivers/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c > index 296b7a1..c3af108 100644 > --- a/drivers/acpi/acpi_platform.c > +++ b/drivers/acpi/acpi_platform.c > @@ -43,7 +43,6 @@ static const struct acpi_device_id forbidden_id_list[] = { > struct platform_device *acpi_create_platform_device(struct acpi_device *adev) > { > struct platform_device *pdev = NULL; > - struct acpi_device *acpi_parent; > struct platform_device_info pdevinfo; > struct resource_entry *rentry; > struct list_head resource_list; > @@ -82,22 +81,8 @@ struct platform_device *acpi_create_platform_device(struct acpi_device *adev) > * attached to it, that physical device should be the parent of the > * platform device we are about to create. > */ > - pdevinfo.parent = NULL; > - acpi_parent = adev->parent; > - if (acpi_parent) { > - struct acpi_device_physical_node *entry; > - struct list_head *list; > - > - mutex_lock(&acpi_parent->physical_node_lock); > - list = &acpi_parent->physical_node_list; > - if (!list_empty(list)) { > - entry = list_first_entry(list, > - struct acpi_device_physical_node, > - node); > - pdevinfo.parent = entry->dev; > - } > - mutex_unlock(&acpi_parent->physical_node_lock); > - } > + pdevinfo.parent = adev->parent ? > + acpi_get_first_physical_node(adev->parent) : NULL; > pdevinfo.name = dev_name(&adev->dev); > pdevinfo.id = -1; > pdevinfo.res = resources; > diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c > index a212cef..832b26d 100644 > --- a/drivers/acpi/bus.c > +++ b/drivers/acpi/bus.c > @@ -478,24 +478,35 @@ static void acpi_device_remove_notify_handler(struct acpi_device *device) > Device Matching > -------------------------------------------------------------------------- */ > > -static struct acpi_device *acpi_primary_dev_companion(struct acpi_device *adev, > - const struct device *dev) > +/** > + * acpi_device_fix_parent - Get first physical node of an ACPI device Please fix the function name here. > + * @adev: ACPI device in question > + */ > +struct device *acpi_get_first_physical_node(struct acpi_device *adev) > { > struct mutex *physical_node_lock = &adev->physical_node_lock; > + struct device *node = NULL; > > mutex_lock(physical_node_lock); > - if (list_empty(&adev->physical_node_list)) { > - adev = NULL; > - } else { > - const struct acpi_device_physical_node *node; > > + if (!list_empty(&adev->physical_node_list)) > node = list_first_entry(&adev->physical_node_list, > - struct acpi_device_physical_node, node); > - if (node->dev != dev) > - adev = NULL; > - } > + struct acpi_device_physical_node, node)->dev; > + No, you don't have to change all that code. Exercise: rework this function with as few lines of code changed as you possibly can. > mutex_unlock(physical_node_lock); > - return adev; > + > + return node; > +} > + > +static struct acpi_device *acpi_primary_dev_companion(struct acpi_device *adev, > + const struct device *dev) > +{ > + const struct device *node = acpi_get_first_physical_node(adev); s/node/phys_dev/ ? > + > + if (node && node == dev) > + return adev; > + > + return NULL; return phys_dev && phys_dev == dev ? adev : NULL; One line of code instead of 4. > } > > /** > diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h > index 11d87bf..ee9312ba 100644 > --- a/drivers/acpi/internal.h > +++ b/drivers/acpi/internal.h > @@ -98,6 +98,7 @@ bool acpi_device_is_present(struct acpi_device *adev); > bool acpi_device_is_battery(struct acpi_device *adev); > bool acpi_device_is_first_physical_node(struct acpi_device *adev, > const struct device *dev); > +struct device *acpi_get_first_physical_node(struct acpi_device *adev); > > /* -------------------------------------------------------------------------- > Device Matching and Notification > Thanks, Rafael
On Wednesday, January 20, 2016 05:12:18 PM Andy Shevchenko wrote: > On Wed, Jan 20, 2016 at 4:29 PM, Aleksey Makarov > <aleksey.makarov@linaro.org> wrote: > > Factor out the code that finds the first physical device > > of a given ACPI device. It is used in several places. > > > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org> > > Hmm… Sorry, didn't notice one style issue and there is one is matter > of taste below. > > > --- a/drivers/acpi/acpi_platform.c > > +++ b/drivers/acpi/acpi_platform.c > > @@ -43,7 +43,6 @@ static const struct acpi_device_id forbidden_id_list[] = { > > > + pdevinfo.parent = adev->parent ? > > + acpi_get_first_physical_node(adev->parent) : NULL; > > Matter of taste, but I believe if-else looks better here even when > consumes +2 LOC. I disagree. Thanks, Rafael
diff --git a/drivers/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c index 296b7a1..c3af108 100644 --- a/drivers/acpi/acpi_platform.c +++ b/drivers/acpi/acpi_platform.c @@ -43,7 +43,6 @@ static const struct acpi_device_id forbidden_id_list[] = { struct platform_device *acpi_create_platform_device(struct acpi_device *adev) { struct platform_device *pdev = NULL; - struct acpi_device *acpi_parent; struct platform_device_info pdevinfo; struct resource_entry *rentry; struct list_head resource_list; @@ -82,22 +81,8 @@ struct platform_device *acpi_create_platform_device(struct acpi_device *adev) * attached to it, that physical device should be the parent of the * platform device we are about to create. */ - pdevinfo.parent = NULL; - acpi_parent = adev->parent; - if (acpi_parent) { - struct acpi_device_physical_node *entry; - struct list_head *list; - - mutex_lock(&acpi_parent->physical_node_lock); - list = &acpi_parent->physical_node_list; - if (!list_empty(list)) { - entry = list_first_entry(list, - struct acpi_device_physical_node, - node); - pdevinfo.parent = entry->dev; - } - mutex_unlock(&acpi_parent->physical_node_lock); - } + pdevinfo.parent = adev->parent ? + acpi_get_first_physical_node(adev->parent) : NULL; pdevinfo.name = dev_name(&adev->dev); pdevinfo.id = -1; pdevinfo.res = resources; diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c index a212cef..832b26d 100644 --- a/drivers/acpi/bus.c +++ b/drivers/acpi/bus.c @@ -478,24 +478,35 @@ static void acpi_device_remove_notify_handler(struct acpi_device *device) Device Matching -------------------------------------------------------------------------- */ -static struct acpi_device *acpi_primary_dev_companion(struct acpi_device *adev, - const struct device *dev) +/** + * acpi_device_fix_parent - Get first physical node of an ACPI device + * @adev: ACPI device in question + */ +struct device *acpi_get_first_physical_node(struct acpi_device *adev) { struct mutex *physical_node_lock = &adev->physical_node_lock; + struct device *node = NULL; mutex_lock(physical_node_lock); - if (list_empty(&adev->physical_node_list)) { - adev = NULL; - } else { - const struct acpi_device_physical_node *node; + if (!list_empty(&adev->physical_node_list)) node = list_first_entry(&adev->physical_node_list, - struct acpi_device_physical_node, node); - if (node->dev != dev) - adev = NULL; - } + struct acpi_device_physical_node, node)->dev; + mutex_unlock(physical_node_lock); - return adev; + + return node; +} + +static struct acpi_device *acpi_primary_dev_companion(struct acpi_device *adev, + const struct device *dev) +{ + const struct device *node = acpi_get_first_physical_node(adev); + + if (node && node == dev) + return adev; + + return NULL; } /** diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h index 11d87bf..ee9312ba 100644 --- a/drivers/acpi/internal.h +++ b/drivers/acpi/internal.h @@ -98,6 +98,7 @@ bool acpi_device_is_present(struct acpi_device *adev); bool acpi_device_is_battery(struct acpi_device *adev); bool acpi_device_is_first_physical_node(struct acpi_device *adev, const struct device *dev); +struct device *acpi_get_first_physical_node(struct acpi_device *adev); /* -------------------------------------------------------------------------- Device Matching and Notification