All of lore.kernel.org
 help / color / mirror / Atom feed
From: Arnd Bergmann <arnd@arndb.de>
To: Pawel Moll <pawel.moll@arm.com>
Cc: arm@kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, Jean Delvare <jdelvare@suse.de>,
	Guenter Roeck <linux@roeck-us.net>,
	lm-sensors@lm-sensors.org
Subject: Re: [lm-sensors] [PATCH 09/12] hwmon: vexpress: Use devm helper for hwmon device registration
Date: Tue, 11 Feb 2014 20:57:46 +0000	[thread overview]
Message-ID: <201402112157.46774.arnd@arndb.de> (raw)
In-Reply-To: <1392138636-29240-10-git-send-email-pawel.moll@arm.com>

On Tuesday 11 February 2014, Pawel Moll wrote:
> -#define VEXPRESS_HWMON_ATTRS(_name, _label_attr, _input_attr)  \
> -struct attribute *vexpress_hwmon_attrs_##_name[] = {           \
> -       &dev_attr_name.attr,                                    \
> -       &dev_attr_##_label_attr.attr,                           \
> -       &sensor_dev_attr_##_input_attr.dev_attr.attr,           \
> -       NULL                                                    \
> -}
> +#define VEXPRESS_HWMON_ATTR_GROUPS(_name, _label_attr, _input_attr)    \
> +static struct attribute *vexpress_hwmon_##_name##_attrs[] = {          \
> +       &dev_attr_##_label_attr.attr,                                   \
> +       &sensor_dev_attr_##_input_attr.dev_attr.attr,                   \
> +       NULL                                                            \
> +};                                                                     \
> +ATTRIBUTE_GROUPS(vexpress_hwmon_##_name)
>  
>  #if !defined(CONFIG_REGULATOR_VEXPRESS)
>  static DEVICE_ATTR(in1_label, S_IRUGO, vexpress_hwmon_label_show, NULL);
>  static SENSOR_DEVICE_ATTR(in1_input, S_IRUGO, vexpress_hwmon_u32_show,
>                 NULL, 1000);
> -static VEXPRESS_HWMON_ATTRS(volt, in1_label, in1_input);
> -static struct attribute_group vexpress_hwmon_group_volt = {
> -       .attrs = vexpress_hwmon_attrs_volt,
> -};
> +VEXPRESS_HWMON_ATTR_GROUPS(volt, in1_label, in1_input);

Maybe it would be a good time to get rid of the obfuscation here and just
open-code the attribute groups rather than defining another macro.

I understand you want to save a few source lines, but it really helps
readability if you can search for the attribute groups when they
are referenced later. Just because the common driver model code uses
macros with string concatenation doesn't mean it's a good idea to
do the same in a device driver.

	Arnd

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

WARNING: multiple messages have this Message-ID (diff)
From: arnd@arndb.de (Arnd Bergmann)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH 09/12] hwmon: vexpress: Use devm helper for hwmon device registration
Date: Tue, 11 Feb 2014 21:57:46 +0100	[thread overview]
Message-ID: <201402112157.46774.arnd@arndb.de> (raw)
In-Reply-To: <1392138636-29240-10-git-send-email-pawel.moll@arm.com>

On Tuesday 11 February 2014, Pawel Moll wrote:
> -#define VEXPRESS_HWMON_ATTRS(_name, _label_attr, _input_attr)  \
> -struct attribute *vexpress_hwmon_attrs_##_name[] = {           \
> -       &dev_attr_name.attr,                                    \
> -       &dev_attr_##_label_attr.attr,                           \
> -       &sensor_dev_attr_##_input_attr.dev_attr.attr,           \
> -       NULL                                                    \
> -}
> +#define VEXPRESS_HWMON_ATTR_GROUPS(_name, _label_attr, _input_attr)    \
> +static struct attribute *vexpress_hwmon_##_name##_attrs[] = {          \
> +       &dev_attr_##_label_attr.attr,                                   \
> +       &sensor_dev_attr_##_input_attr.dev_attr.attr,                   \
> +       NULL                                                            \
> +};                                                                     \
> +ATTRIBUTE_GROUPS(vexpress_hwmon_##_name)
>  
>  #if !defined(CONFIG_REGULATOR_VEXPRESS)
>  static DEVICE_ATTR(in1_label, S_IRUGO, vexpress_hwmon_label_show, NULL);
>  static SENSOR_DEVICE_ATTR(in1_input, S_IRUGO, vexpress_hwmon_u32_show,
>                 NULL, 1000);
> -static VEXPRESS_HWMON_ATTRS(volt, in1_label, in1_input);
> -static struct attribute_group vexpress_hwmon_group_volt = {
> -       .attrs = vexpress_hwmon_attrs_volt,
> -};
> +VEXPRESS_HWMON_ATTR_GROUPS(volt, in1_label, in1_input);

Maybe it would be a good time to get rid of the obfuscation here and just
open-code the attribute groups rather than defining another macro.

I understand you want to save a few source lines, but it really helps
readability if you can search for the attribute groups when they
are referenced later. Just because the common driver model code uses
macros with string concatenation doesn't mean it's a good idea to
do the same in a device driver.

	Arnd

WARNING: multiple messages have this Message-ID (diff)
From: Arnd Bergmann <arnd@arndb.de>
To: Pawel Moll <pawel.moll@arm.com>
Cc: arm@kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, Jean Delvare <jdelvare@suse.de>,
	Guenter Roeck <linux@roeck-us.net>,
	lm-sensors@lm-sensors.org
Subject: Re: [PATCH 09/12] hwmon: vexpress: Use devm helper for hwmon device registration
Date: Tue, 11 Feb 2014 21:57:46 +0100	[thread overview]
Message-ID: <201402112157.46774.arnd@arndb.de> (raw)
In-Reply-To: <1392138636-29240-10-git-send-email-pawel.moll@arm.com>

On Tuesday 11 February 2014, Pawel Moll wrote:
> -#define VEXPRESS_HWMON_ATTRS(_name, _label_attr, _input_attr)  \
> -struct attribute *vexpress_hwmon_attrs_##_name[] = {           \
> -       &dev_attr_name.attr,                                    \
> -       &dev_attr_##_label_attr.attr,                           \
> -       &sensor_dev_attr_##_input_attr.dev_attr.attr,           \
> -       NULL                                                    \
> -}
> +#define VEXPRESS_HWMON_ATTR_GROUPS(_name, _label_attr, _input_attr)    \
> +static struct attribute *vexpress_hwmon_##_name##_attrs[] = {          \
> +       &dev_attr_##_label_attr.attr,                                   \
> +       &sensor_dev_attr_##_input_attr.dev_attr.attr,                   \
> +       NULL                                                            \
> +};                                                                     \
> +ATTRIBUTE_GROUPS(vexpress_hwmon_##_name)
>  
>  #if !defined(CONFIG_REGULATOR_VEXPRESS)
>  static DEVICE_ATTR(in1_label, S_IRUGO, vexpress_hwmon_label_show, NULL);
>  static SENSOR_DEVICE_ATTR(in1_input, S_IRUGO, vexpress_hwmon_u32_show,
>                 NULL, 1000);
> -static VEXPRESS_HWMON_ATTRS(volt, in1_label, in1_input);
> -static struct attribute_group vexpress_hwmon_group_volt = {
> -       .attrs = vexpress_hwmon_attrs_volt,
> -};
> +VEXPRESS_HWMON_ATTR_GROUPS(volt, in1_label, in1_input);

Maybe it would be a good time to get rid of the obfuscation here and just
open-code the attribute groups rather than defining another macro.

I understand you want to save a few source lines, but it really helps
readability if you can search for the attribute groups when they
are referenced later. Just because the common driver model code uses
macros with string concatenation doesn't mean it's a good idea to
do the same in a device driver.

	Arnd

  reply	other threads:[~2014-02-11 20:57 UTC|newest]

Thread overview: 112+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-02-11 17:10 [lm-sensors] [PATCH 00/12] Versatile Express updates Pawel Moll
2014-02-11 17:10 ` Pawel Moll
2014-02-11 17:10 ` Pawel Moll
2014-02-11 17:10 ` [PATCH 01/12] misc: vexpress-syscfg: Add udelay-based delay Pawel Moll
2014-02-11 17:10   ` Pawel Moll
2014-02-15 19:19   ` Greg Kroah-Hartman
2014-02-15 19:19     ` Greg Kroah-Hartman
2014-02-11 17:10 ` [PATCH 02/12] power/reset: vexpress: Use udelay instead of timers Pawel Moll
2014-02-11 17:10   ` Pawel Moll
2014-02-11 20:59   ` Arnd Bergmann
2014-02-11 20:59     ` Arnd Bergmann
2014-02-12 11:56     ` Pawel Moll
2014-02-12 11:56       ` Pawel Moll
2014-02-11 17:10 ` [PATCH 03/12] clk: versatile: Split config options for sp810 and vexpress_osc Pawel Moll
2014-02-11 17:10   ` Pawel Moll
2014-02-11 17:10 ` [PATCH 04/12] clocksource: Sched clock source for Versatile Express Pawel Moll
2014-02-11 17:10   ` Pawel Moll
2014-04-16 13:56   ` Rob Herring
2014-04-16 13:56     ` Rob Herring
2014-04-16 14:22     ` Pawel Moll
2014-04-16 14:22       ` Pawel Moll
2014-04-16 14:45       ` Rob Herring
2014-04-16 14:45         ` Rob Herring
2014-04-16 15:05         ` Pawel Moll
2014-04-16 15:05           ` Pawel Moll
2014-05-02 22:14   ` Linus Walleij
2014-05-02 22:14     ` Linus Walleij
2014-05-07  9:57     ` Pawel Moll
2014-05-07  9:57       ` Pawel Moll
2014-05-13  8:47       ` Linus Walleij
2014-05-13  8:47         ` Linus Walleij
2014-02-11 17:10 ` [PATCH 05/12] GPIO: gpio-generic: Add label to platform data Pawel Moll
2014-02-11 17:10   ` Pawel Moll
2014-02-11 17:17   ` Lee Jones
2014-02-11 17:17     ` Lee Jones
2014-02-11 17:20     ` Pawel Moll
2014-02-11 17:20       ` Pawel Moll
2014-02-11 17:29       ` Pawel Moll
2014-02-11 17:29         ` Pawel Moll
2014-02-11 17:46       ` Lee Jones
2014-02-11 17:46         ` Lee Jones
2014-02-11 17:10 ` [PATCH 06/12] mfd: vexpress-sysreg: Add labels to gpio banks Pawel Moll
2014-02-11 17:10   ` Pawel Moll
2014-02-11 17:19   ` Lee Jones
2014-02-11 17:19     ` Lee Jones
2014-02-13 13:08   ` Linus Walleij
2014-02-13 13:08     ` Linus Walleij
2014-02-13 13:11     ` Pawel Moll
2014-02-13 13:11       ` Pawel Moll
2014-02-11 17:10 ` [PATCH 07/12] mfd: syscon: Consider platform data a regmap config name Pawel Moll
2014-02-11 17:10   ` Pawel Moll
2014-02-11 17:24   ` Lee Jones
2014-02-11 17:24     ` Lee Jones
2014-02-12  7:09   ` Alexander Shiyan
2014-02-12  7:09     ` Alexander Shiyan
2014-02-12  8:26     ` Lee Jones
2014-02-12  8:26       ` Lee Jones
2014-02-12 11:06       ` Pawel Moll
2014-02-12 11:06         ` Pawel Moll
2014-02-12 11:18         ` Lee Jones
2014-02-12 11:18           ` Lee Jones
2014-02-12 11:27         ` Alexander Shiyan
2014-02-12 11:27           ` Alexander Shiyan
2014-02-12 11:43           ` Pawel Moll
2014-02-12 11:43             ` Pawel Moll
2014-02-11 17:10 ` [PATCH 08/12] mfd: vexpress-sysreg: Add syscon labels as platform data Pawel Moll
2014-02-11 17:10   ` Pawel Moll
2014-02-11 17:29   ` Lee Jones
2014-02-11 17:29     ` Lee Jones
2014-02-11 17:32     ` Pawel Moll
2014-02-11 17:32       ` Pawel Moll
2014-02-11 17:48       ` Lee Jones
2014-02-11 17:48         ` Lee Jones
2014-02-11 17:52         ` [PATCH v2 1/2] mfd: syscon: Add platform data with a regmap config name Pawel Moll
2014-02-11 17:52           ` Pawel Moll
2014-02-11 17:52           ` [PATCH v2 2/2] mfd: vexpress-sysreg: Add syscon labels as platform data Pawel Moll
2014-02-11 17:52             ` Pawel Moll
2014-02-12 11:20             ` Lee Jones
2014-02-12 11:20               ` Lee Jones
2014-02-11 17:55           ` [PATCH v2 1/2] mfd: syscon: Add platform data with a regmap config name Pawel Moll
2014-02-11 17:55             ` Pawel Moll
2014-02-12 11:19           ` Lee Jones
2014-02-12 11:19             ` Lee Jones
2014-02-11 17:10 ` [lm-sensors] [PATCH 09/12] hwmon: vexpress: Use devm helper for hwmon device registration Pawel Moll
2014-02-11 17:10   ` Pawel Moll
2014-02-11 17:10   ` Pawel Moll
2014-02-11 20:57   ` Arnd Bergmann [this message]
2014-02-11 20:57     ` Arnd Bergmann
2014-02-11 20:57     ` Arnd Bergmann
2014-02-12  2:49   ` [lm-sensors] " Guenter Roeck
2014-02-12  2:49     ` Guenter Roeck
2014-02-12 11:56     ` [lm-sensors] " Pawel Moll
2014-02-12 11:56       ` Pawel Moll
2014-02-12 11:56       ` Pawel Moll
2014-02-12 11:59       ` [lm-sensors] " Pawel Moll
2014-02-12 11:59         ` Pawel Moll
2014-02-12 11:59         ` Pawel Moll
2014-02-12 16:41       ` [lm-sensors] " Guenter Roeck
2014-02-12 16:41         ` Guenter Roeck
2014-02-12 16:41         ` Guenter Roeck
2014-02-11 17:10 ` [PATCH 10/12] ARM: vexpress: remove redundant vexpress_dt_cpus_num to get cpu count Pawel Moll
2014-02-11 17:10   ` Pawel Moll
2014-02-11 17:10 ` [PATCH 11/12] ARM: vexpress: Simplify SMP operations for DT-powered system Pawel Moll
2014-02-11 17:10   ` Pawel Moll
2014-02-11 17:10 ` [PATCH 12/12] ARM: vexpress: move HBI check to sysreg driver Pawel Moll
2014-02-11 17:10   ` Pawel Moll
2014-02-11 19:28 ` [lm-sensors] [PATCH 00/12] Versatile Express updates Arnd Bergmann
2014-02-11 19:28   ` Arnd Bergmann
2014-02-11 19:28   ` Arnd Bergmann
2014-02-12 12:30   ` [lm-sensors] " Pawel Moll
2014-02-12 12:30     ` Pawel Moll
2014-02-12 12:30     ` Pawel Moll

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=201402112157.46774.arnd@arndb.de \
    --to=arnd@arndb.de \
    --cc=arm@kernel.org \
    --cc=jdelvare@suse.de \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@roeck-us.net \
    --cc=lm-sensors@lm-sensors.org \
    --cc=pawel.moll@arm.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.