All of lore.kernel.org
 help / color / mirror / Atom feed
From: rpurdie@rpsys.net (Richard Purdie)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v2] leds: provide helper to register "leds-gpio" devices
Date: Wed, 06 Apr 2011 06:38:17 -0700	[thread overview]
Message-ID: <1302097097.22904.41.camel@rex> (raw)
In-Reply-To: <20110406123310.GD13963@pengutronix.de>

On Wed, 2011-04-06 at 14:33 +0200, Uwe Kleine-K?nig wrote:
> On Wed, Apr 06, 2011 at 04:52:18AM -0700, Richard Purdie wrote:
> > I guess the motivation might be that if a given platform has many
> > different LED configurations, you're trying to remove the ones you don't
> > need from memory? Given all the above I'd suggest that this function
> > should really be added to the platform device code if anywhere and
> "platform device code" means e.g. arch/arm/plat-mxc or drivers/base
> here?

Yes.

> > doesn't belong in the LED subsystem as its not an LED specific problem.
> yeap, that's it. Note that this thread[1] started on the linux-arm-kernel
> mailing list with a imx-specific version of this function. With the
> background of Linus' recent rant against churn in arch/arm several
> people pointed out that this can better go to a more generic place where
> not only arm/imx, but also arm/omap and even powerpc can use the same
> code. So the (IMHO) obvious place to put the code is near the driver.
> 
> And yes, the problem is not LED specific, but a function that creates
> and registers a leds-gpio device *is* LED specific. A while back I
> thought about introducing something like drivers/register/*, but I'm
> sure this won't scale. Either you need a header per device type (or at
> subsystem) or a single header. Both look ugly in my eyes. Having the
> prototype in include/linux/leds.h seems the right thing, because
> platform code needs to include that anyhow to provide a struct
> gpio_led_platform_data.
> 
> I don't consider something wrong here, because the Linux device model
> requires that you have a driver and a device. Both have to match and the
> device (usually) is created at boot time. Because of the needed match
> it's natural to have device use (aka driver) and device creation at the
> same place. Because of the boot time thing the code needs to be
> built-in.

It should only be built-in on platforms that both use leds-gpio and want
to use this function at boot time. This is not the description of
leds-core.c.

I understand the issue and the desire to move it into common code, I
think that is good but I don't think you've found the most appropriate
place yet.

I'm tempted to suggest making the function a static inline in leds.h (or
create an leds-gpio.h and move the struct definition there too).

Cheers,

Richard
-- 
Linux Foundation
http://www.yoctoproject.org/

WARNING: multiple messages have this Message-ID (diff)
From: Richard Purdie <rpurdie@rpsys.net>
To: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	H Hartley Sweeten <hartleys@visionengravers.com>,
	Russell King - ARM Linux <linux@arm.linux.org.uk>,
	Fabio Estevam <fabio.estevam@freescale.com>,
	Sascha Hauer <s.hauer@pengutronix.de>,
	kernel@pengutronix.de
Subject: Re: [PATCH v2] leds: provide helper to register "leds-gpio" devices
Date: Wed, 06 Apr 2011 06:38:17 -0700	[thread overview]
Message-ID: <1302097097.22904.41.camel@rex> (raw)
In-Reply-To: <20110406123310.GD13963@pengutronix.de>

On Wed, 2011-04-06 at 14:33 +0200, Uwe Kleine-König wrote:
> On Wed, Apr 06, 2011 at 04:52:18AM -0700, Richard Purdie wrote:
> > I guess the motivation might be that if a given platform has many
> > different LED configurations, you're trying to remove the ones you don't
> > need from memory? Given all the above I'd suggest that this function
> > should really be added to the platform device code if anywhere and
> "platform device code" means e.g. arch/arm/plat-mxc or drivers/base
> here?

Yes.

> > doesn't belong in the LED subsystem as its not an LED specific problem.
> yeap, that's it. Note that this thread[1] started on the linux-arm-kernel
> mailing list with a imx-specific version of this function. With the
> background of Linus' recent rant against churn in arch/arm several
> people pointed out that this can better go to a more generic place where
> not only arm/imx, but also arm/omap and even powerpc can use the same
> code. So the (IMHO) obvious place to put the code is near the driver.
> 
> And yes, the problem is not LED specific, but a function that creates
> and registers a leds-gpio device *is* LED specific. A while back I
> thought about introducing something like drivers/register/*, but I'm
> sure this won't scale. Either you need a header per device type (or at
> subsystem) or a single header. Both look ugly in my eyes. Having the
> prototype in include/linux/leds.h seems the right thing, because
> platform code needs to include that anyhow to provide a struct
> gpio_led_platform_data.
> 
> I don't consider something wrong here, because the Linux device model
> requires that you have a driver and a device. Both have to match and the
> device (usually) is created at boot time. Because of the needed match
> it's natural to have device use (aka driver) and device creation at the
> same place. Because of the boot time thing the code needs to be
> built-in.

It should only be built-in on platforms that both use leds-gpio and want
to use this function at boot time. This is not the description of
leds-core.c.

I understand the issue and the desire to move it into common code, I
think that is good but I don't think you've found the most appropriate
place yet.

I'm tempted to suggest making the function a static inline in leds.h (or
create an leds-gpio.h and move the struct definition there too).

Cheers,

Richard
-- 
Linux Foundation
http://www.yoctoproject.org/


  reply	other threads:[~2011-04-06 13:38 UTC|newest]

Thread overview: 76+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-04-04 17:06 [PATCH v2 1/2] ARM: mxc: Introduce imx_add_gpio_leds Fabio Estevam
2011-04-04 17:06 ` [PATCH v2 2/2] ARM: mx5/mx53_evk.c: Add LED support Fabio Estevam
2011-04-04 17:19 ` [PATCH v2 1/2] ARM: mxc: Introduce imx_add_gpio_leds Sascha Hauer
2011-04-04 17:42   ` Uwe Kleine-König
2011-04-04 17:52     ` Russell King - ARM Linux
2011-04-04 18:06       ` H Hartley Sweeten
2011-04-04 18:17         ` Russell King - ARM Linux
2011-04-04 21:52           ` H Hartley Sweeten
2011-04-05  7:30             ` Uwe Kleine-König
2011-04-05  7:40               ` Russell King - ARM Linux
2011-04-05  7:43                 ` Uwe Kleine-König
2011-04-05  7:47                   ` Russell King - ARM Linux
2011-04-05  7:51                     ` Uwe Kleine-König
2011-04-05  7:59                       ` Russell King - ARM Linux
2011-04-05  8:32                         ` Uwe Kleine-König
2011-04-05  8:43                           ` Russell King - ARM Linux
2011-04-05  8:46                             ` Uwe Kleine-König
2011-04-05  8:37               ` [PATCH] leds: provide helper to register "leds-gpio" devices Uwe Kleine-König
2011-04-05  8:37                 ` Uwe Kleine-König
2011-04-05 16:13                 ` Fabio Estevam
2011-04-05 16:13                   ` Fabio Estevam
2011-04-05 16:29                   ` Fabio Estevam
2011-04-05 16:29                     ` Fabio Estevam
2011-04-05 18:12                     ` H Hartley Sweeten
2011-04-05 18:12                       ` H Hartley Sweeten
2011-04-05 16:33                 ` Russell King - ARM Linux
2011-04-05 16:33                   ` Russell King - ARM Linux
2011-04-05 20:24                   ` [PATCH v2] " Uwe Kleine-König
2011-04-05 20:24                     ` Uwe Kleine-König
2011-04-06 11:45                     ` Fabio Estevam
2011-04-06 11:45                       ` Fabio Estevam
2011-04-06 11:52                     ` Richard Purdie
2011-04-06 11:52                       ` Richard Purdie
2011-04-06 12:33                       ` Uwe Kleine-König
2011-04-06 12:33                         ` Uwe Kleine-König
2011-04-06 13:38                         ` Richard Purdie [this message]
2011-04-06 13:38                           ` Richard Purdie
2011-04-11 20:35                           ` [PATCH v3] " Uwe Kleine-König
2011-04-11 20:35                             ` Uwe Kleine-König
2011-04-12 21:48                             ` Russell King - ARM Linux
2011-04-12 21:48                               ` Russell King - ARM Linux
2011-04-13  6:23                               ` Uwe Kleine-König
2011-04-13  6:23                                 ` Uwe Kleine-König
2011-05-06 21:03                                 ` Richard Purdie
2011-05-06 21:03                                   ` Richard Purdie
2011-05-09  8:00                                   ` Uwe Kleine-König
2011-05-09  8:00                                     ` Uwe Kleine-König
2011-04-26 15:08                             ` Uwe Kleine-König
2011-04-26 15:08                               ` Uwe Kleine-König
2011-05-06  8:25                               ` Uwe Kleine-König
2011-05-06  8:25                                 ` Uwe Kleine-König
2011-05-09 22:02                             ` Andrew Morton
2011-05-09 22:02                               ` Andrew Morton
2011-05-09 22:17                               ` Russell King - ARM Linux
2011-05-09 22:17                                 ` Russell King - ARM Linux
2011-05-10  6:45                                 ` Uwe Kleine-König
2011-05-10  6:45                                   ` Uwe Kleine-König
2011-05-10  7:31                               ` Uwe Kleine-König
2011-05-10  7:31                                 ` Uwe Kleine-König
2011-05-10  8:50                                 ` [PATCH v4] " Uwe Kleine-König
2011-05-10  8:50                                   ` Uwe Kleine-König
2011-05-10  8:50                                   ` [PATCH] [wip] ARM: imx: register "leds-gpio" device using new helper function Uwe Kleine-König
2011-05-10  8:50                                     ` Uwe Kleine-König
2011-05-10 22:26                                     ` H Hartley Sweeten
2011-05-10 22:26                                       ` H Hartley Sweeten
2011-05-11  6:22                                       ` Uwe Kleine-König
2011-05-11  6:22                                         ` Uwe Kleine-König
2011-05-10 23:02                                     ` H Hartley Sweeten
2011-05-10 23:02                                       ` H Hartley Sweeten
2011-04-19 23:19                   ` [PATCH] leds: provide helper to register "leds-gpio" devices Andrew Morton
2011-04-19 23:19                     ` Andrew Morton
2011-04-19 23:24                     ` Russell King - ARM Linux
2011-04-19 23:24                       ` Russell King - ARM Linux
2011-04-19 23:50                       ` Andrew Morton
2011-04-19 23:50                         ` Andrew Morton
2011-04-05 16:41               ` [PATCH v2 1/2] ARM: mxc: Introduce imx_add_gpio_leds H Hartley Sweeten

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=1302097097.22904.41.camel@rex \
    --to=rpurdie@rpsys.net \
    --cc=linux-arm-kernel@lists.infradead.org \
    /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.