All of lore.kernel.org
 help / color / mirror / Atom feed
From: arnd@arndb.de (Arnd Bergmann)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH V7 1/2] ARM: bcm281xx: Add timer driver (driver portion)
Date: Thu, 28 Mar 2013 19:27:34 +0000	[thread overview]
Message-ID: <201303281927.34327.arnd@arndb.de> (raw)
In-Reply-To: <5154848D.7010400@linaro.org>

On Thursday 28 March 2013, John Stultz wrote:
> On 03/28/2013 09:03 AM, Christian Daudt wrote:

> >>> Maybe we should move the ARM specific ones into
> >>> drivers/clocksource/arm/ ?
> >> About half the IP blocks we use on ARM are also used on at least
> >> one ARM64/AVR32/MIPS/PowerPC/x86/SH/Hexagon/c6x/etc part. Grouping them
> >> by which CPU architecture first starts using them or happens to be
> >> more popular at the time does not seem too helpful here.
> >>
> >> Maybe it's better to have a subdirectory for those clock sources
> >> that are used on any SoC, or have subdirectories based on the
> >> company that created that part, as we do for ethernet drivers.
> >> I wouldn't bother with that until there are a couple of dozen
> >> different clock source drivers.
> 
> So having had a few days to think about this, I think what usually rubs 
> me the wrong way when I get driver/clocksource submissions, is that for 
> 99% of it, they *aren't clocksource drivers*.  Most of the code is 
> *clockevent* driver logic, and then maybe 1-5 lines of actual 
> clocksource code.
> 
> Now, I know the reason for this is often the clocksource and clockevent 
> drivers are backed by the same hardware, and since there's no clockevent 
> directory, might as well have it all in a single file somewhere. But 
> mixing the different subsystem drivers together causes some of the 
> maintenance confusion here.
> 
> So instead of creating drivers/clocksource/arch/ directories, what I'd 
> propose is we create a drivers/clockevent directory to handle the actual 
> clockevent code. I think this would better delineate the lines of 
> responsibility on the gatekeeper side (that being Thomas or maybe 
> someone else who has an interest in the subtleties of how various 
> hardware timers are be broken-by-design ;), and I'd be much happier 
> taking clocksource code where I felt I had a reasonable chance of 
> noticing bugs.

Yes, this sounds like a good idea.

> Thomas: Not that you need more to maintain, but does this seem 
> semi-reasonable? Do we need to find someone else to help here?
> 
> That said, at the end of the day, if I take a bad drivers/clocksource 
> patch, what breaks won't be the timekeeping core, it will be an SoC 
> board.  So I'll have to really rely on the original clocksource driver 
> authors to help vet incoming patches. This is where I think having the 
> SoC tree as a central point for SoC patches has and advantage, as its 
> less likely breakage will sneak upstream via a subsystem tree. But 
> understanding the need for review help, I think I'm ok with taking on 
> more clocksource specific review.

I think the important point for you to realize is that you don't need to
worry about breaking a specific SoC. The patches will come from someone
who is using that hardware in the end. What we need from clock{source,event}
subsystem maintainers is perspective of how things fit into the subsystem
and to provide review like:
- this driver is not using CLOCKSOURCE_OF_DECLARE when it should
- that driver only handles clockevent, don't put it into drivers/clocksource
- I don't like your coding style, it doesn't fit in with the other
  drivers in this subsystem.
- what you do can be implemented much simpler using the generic infrastructure
  introduced by that earlier driver.

	Arnd

WARNING: multiple messages have this Message-ID (diff)
From: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
To: John Stultz <john.stultz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Russell King <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>,
	Christian Daudt <csd-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>,
	devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org,
	"arm-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org"
	<arm-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>,
	Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
Subject: Re: [PATCH V7 1/2] ARM: bcm281xx: Add timer driver (driver portion)
Date: Thu, 28 Mar 2013 19:27:34 +0000	[thread overview]
Message-ID: <201303281927.34327.arnd@arndb.de> (raw)
In-Reply-To: <5154848D.7010400-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

On Thursday 28 March 2013, John Stultz wrote:
> On 03/28/2013 09:03 AM, Christian Daudt wrote:

> >>> Maybe we should move the ARM specific ones into
> >>> drivers/clocksource/arm/ ?
> >> About half the IP blocks we use on ARM are also used on at least
> >> one ARM64/AVR32/MIPS/PowerPC/x86/SH/Hexagon/c6x/etc part. Grouping them
> >> by which CPU architecture first starts using them or happens to be
> >> more popular at the time does not seem too helpful here.
> >>
> >> Maybe it's better to have a subdirectory for those clock sources
> >> that are used on any SoC, or have subdirectories based on the
> >> company that created that part, as we do for ethernet drivers.
> >> I wouldn't bother with that until there are a couple of dozen
> >> different clock source drivers.
> 
> So having had a few days to think about this, I think what usually rubs 
> me the wrong way when I get driver/clocksource submissions, is that for 
> 99% of it, they *aren't clocksource drivers*.  Most of the code is 
> *clockevent* driver logic, and then maybe 1-5 lines of actual 
> clocksource code.
> 
> Now, I know the reason for this is often the clocksource and clockevent 
> drivers are backed by the same hardware, and since there's no clockevent 
> directory, might as well have it all in a single file somewhere. But 
> mixing the different subsystem drivers together causes some of the 
> maintenance confusion here.
> 
> So instead of creating drivers/clocksource/arch/ directories, what I'd 
> propose is we create a drivers/clockevent directory to handle the actual 
> clockevent code. I think this would better delineate the lines of 
> responsibility on the gatekeeper side (that being Thomas or maybe 
> someone else who has an interest in the subtleties of how various 
> hardware timers are be broken-by-design ;), and I'd be much happier 
> taking clocksource code where I felt I had a reasonable chance of 
> noticing bugs.

Yes, this sounds like a good idea.

> Thomas: Not that you need more to maintain, but does this seem 
> semi-reasonable? Do we need to find someone else to help here?
> 
> That said, at the end of the day, if I take a bad drivers/clocksource 
> patch, what breaks won't be the timekeeping core, it will be an SoC 
> board.  So I'll have to really rely on the original clocksource driver 
> authors to help vet incoming patches. This is where I think having the 
> SoC tree as a central point for SoC patches has and advantage, as its 
> less likely breakage will sneak upstream via a subsystem tree. But 
> understanding the need for review help, I think I'm ok with taking on 
> more clocksource specific review.

I think the important point for you to realize is that you don't need to
worry about breaking a specific SoC. The patches will come from someone
who is using that hardware in the end. What we need from clock{source,event}
subsystem maintainers is perspective of how things fit into the subsystem
and to provide review like:
- this driver is not using CLOCKSOURCE_OF_DECLARE when it should
- that driver only handles clockevent, don't put it into drivers/clocksource
- I don't like your coding style, it doesn't fit in with the other
  drivers in this subsystem.
- what you do can be implemented much simpler using the generic infrastructure
  introduced by that earlier driver.

	Arnd

  reply	other threads:[~2013-03-28 19:27 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-03-13 21:27 [PATCH V7 1/2] ARM: bcm281xx: Add timer driver (driver portion) Christian Daudt
2013-03-13 21:27 ` Christian Daudt
2013-03-13 21:27 ` [PATCH V7 2/2] ARM: bcm281xx: Add timer driver (DT portion) Christian Daudt
2013-03-13 21:27   ` Christian Daudt
2013-03-28 16:07   ` Christian Daudt
2013-03-28 16:07     ` Christian Daudt
2013-04-02 20:43     ` Olof Johansson
2013-04-02 20:43       ` Olof Johansson
2013-04-02 20:41   ` Olof Johansson
2013-04-02 20:41     ` Olof Johansson
2013-03-13 23:29 ` [PATCH V7 1/2] ARM: bcm281xx: Add timer driver (driver portion) John Stultz
2013-03-13 23:29   ` John Stultz
2013-03-14  9:16   ` Thomas Gleixner
2013-03-14  9:16     ` Thomas Gleixner
2013-03-14 16:03     ` Arnd Bergmann
2013-03-14 16:03       ` Arnd Bergmann
2013-03-28 16:03       ` Christian Daudt
2013-03-28 16:03         ` Christian Daudt
2013-03-28 17:57         ` John Stultz
2013-03-28 17:57           ` John Stultz
2013-03-28 19:27           ` Arnd Bergmann [this message]
2013-03-28 19:27             ` Arnd Bergmann
2013-03-28 18:01 ` John Stultz
2013-03-28 18:01   ` John Stultz

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=201303281927.34327.arnd@arndb.de \
    --to=arnd@arndb.de \
    --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.