From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: Aleksey Makarov <aleksey.makarov@linaro.org>
Cc: "linux-acpi@vger.kernel.org" <linux-acpi@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
linux-arm Mailing List <linux-arm-kernel@lists.infradead.org>,
Graeme Gregory <graeme.gregory@linaro.org>,
Russell King <linux@arm.linux.org.uk>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
"Rafael J . Wysocki" <rjw@rjwysocki.net>,
Shannon Zhao <shannon.zhao@linaro.org>,
Vladimir Zapolskiy <vz@mleia.com>, Len Brown <lenb@kernel.org>
Subject: Re: [PATCH v6 1/2] ACPI: introduce a function to find the first physical device
Date: Wed, 20 Jan 2016 17:12:18 +0200 [thread overview]
Message-ID: <CAHp75VcdtU6izThE+Z2ODnYoXk-uhweXzUtFwSze_jJHw3+38g@mail.gmail.com> (raw)
In-Reply-To: <1453300171-25473-2-git-send-email-aleksey.makarov@linaro.org>
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;
> +}
--
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
WARNING: multiple messages have this Message-ID (diff)
From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: Aleksey Makarov <aleksey.makarov@linaro.org>
Cc: "linux-acpi@vger.kernel.org" <linux-acpi@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
linux-arm Mailing List <linux-arm-kernel@lists.infradead.org>,
Graeme Gregory <graeme.gregory@linaro.org>,
Russell King <linux@arm.linux.org.uk>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
"Rafael J . Wysocki" <rjw@rjwysocki.net>,
Shannon Zhao <shannon.zhao@linaro.org>,
Vladimir Zapolskiy <vz@mleia.com>, Len Brown <lenb@kernel.org>
Subject: Re: [PATCH v6 1/2] ACPI: introduce a function to find the first physical device
Date: Wed, 20 Jan 2016 17:12:18 +0200 [thread overview]
Message-ID: <CAHp75VcdtU6izThE+Z2ODnYoXk-uhweXzUtFwSze_jJHw3+38g@mail.gmail.com> (raw)
In-Reply-To: <1453300171-25473-2-git-send-email-aleksey.makarov@linaro.org>
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;
> +}
--
With Best Regards,
Andy Shevchenko
WARNING: multiple messages have this Message-ID (diff)
From: andy.shevchenko@gmail.com (Andy Shevchenko)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v6 1/2] ACPI: introduce a function to find the first physical device
Date: Wed, 20 Jan 2016 17:12:18 +0200 [thread overview]
Message-ID: <CAHp75VcdtU6izThE+Z2ODnYoXk-uhweXzUtFwSze_jJHw3+38g@mail.gmail.com> (raw)
In-Reply-To: <1453300171-25473-2-git-send-email-aleksey.makarov@linaro.org>
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;
> +}
--
With Best Regards,
Andy Shevchenko
next prev parent reply other threads:[~2016-01-20 15:12 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-01-20 14:29 [PATCH v6 0/2] Add AMBA bus probing support to ACPI Aleksey Makarov
2016-01-20 14:29 ` Aleksey Makarov
2016-01-20 14:29 ` [PATCH v6 1/2] ACPI: introduce a function to find the first physical device Aleksey Makarov
2016-01-20 14:29 ` Aleksey Makarov
2016-01-20 15:12 ` Andy Shevchenko [this message]
2016-01-20 15:12 ` Andy Shevchenko
2016-01-20 15:12 ` Andy Shevchenko
2016-01-20 17:00 ` Aleksey Makarov
2016-01-20 17:00 ` Aleksey Makarov
2016-01-20 17:00 ` Aleksey Makarov
2016-01-20 17:25 ` Andy Shevchenko
2016-01-20 17:25 ` Andy Shevchenko
2016-01-20 17:25 ` Andy Shevchenko
2016-02-16 1:21 ` Rafael J. Wysocki
2016-02-16 1:21 ` Rafael J. Wysocki
2016-02-16 1:21 ` Rafael J. Wysocki
2016-02-16 1:20 ` Rafael J. Wysocki
2016-02-16 1:20 ` Rafael J. Wysocki
2016-02-16 12:52 ` Aleksey Makarov
2016-02-16 12:52 ` Aleksey Makarov
2016-02-16 19:20 ` Rafael J. Wysocki
2016-02-16 19:20 ` Rafael J. Wysocki
2016-01-20 14:29 ` [PATCH v6 2/2] ACPI: amba bus probing support Aleksey Makarov
2016-01-20 14:29 ` Aleksey Makarov
2016-02-16 1:23 ` Rafael J. Wysocki
2016-02-16 1:23 ` Rafael J. Wysocki
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAHp75VcdtU6izThE+Z2ODnYoXk-uhweXzUtFwSze_jJHw3+38g@mail.gmail.com \
--to=andy.shevchenko@gmail.com \
--cc=aleksey.makarov@linaro.org \
--cc=graeme.gregory@linaro.org \
--cc=gregkh@linuxfoundation.org \
--cc=lenb@kernel.org \
--cc=linux-acpi@vger.kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@arm.linux.org.uk \
--cc=rjw@rjwysocki.net \
--cc=shannon.zhao@linaro.org \
--cc=vz@mleia.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.