All of lore.kernel.org
 help / color / mirror / Atom feed
From: mingo@kernel.org (Ingo Molnar)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH] clockevents: Add (missing) default case for switch blocks
Date: Fri, 20 Feb 2015 10:36:59 +0100	[thread overview]
Message-ID: <20150220093659.GA23469@gmail.com> (raw)
In-Reply-To: <20150220084807.GJ21418@twins.programming.kicks-ass.net>


* Peter Zijlstra <peterz@infradead.org> wrote:

> > So this whole approach looks fragile for several reasons:
> > 
> >    - 'mode setting' callbacks are just bad by design
> >      because they mix several functions into the same entry
> >      point, complicating the handler functions 
> >      unnecessarily. We should reduce complexity, not expand 
> >      on it.
> > 
> >    - now by adding 'default' you hide from drivers the
> >      ability to easily discover whether it has been updated
> >      to some new core clockevents mode setting feature or
> >      not.
> 
> So this patch was a follow on from bd624d75db21 
> ("clockevents: Introduce mode specific callbacks").
> 
> That patch changes the set_mode() interface; and provides 
> per mode functions.

So why is a 'default' mode needed then? It makes the 
addition of new modes to the legacy handler easier, which 
looks backwards.

> New (and updated) drivers should not use ->set_mode() 
> anymore, but it was felt that we do not want to go do 
> flag day changes.

I fully agree that we don't want flag day changes, but make 
it really apparent that it's an obsolete interface:

  - rename it to set_mode_obsolete()

  - try to convert as many of the easy cases as possible - 
    the overwhelming majority of mode setting functions 
    look reasonably simple.

  - get rid of the mode enum in the core, and rename the 
    mode bits to CLOCK_EVT_MODE_OBSOLETE_XXX.

etc.

> And it allows for adding optional modes; not every driver 
> needs to go implement all mode functions if there is a 
> sane default action.
> 
> But it does mean we need to be able to add values to the 
> enum.

So I'm confused: if we are using proper callbacks (like my 
example outlined) , why is a 'mode enum' needed at all?

Thanks,

	Ingo

WARNING: multiple messages have this Message-ID (diff)
From: Ingo Molnar <mingo@kernel.org>
To: Peter Zijlstra <peterz@infradead.org>
Cc: Viresh Kumar <viresh.kumar@linaro.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>,
	linaro-kernel@lists.linaro.org, Kevin Hilman <khilman@linaro.org>,
	Preeti U Murthy <preeti@linux.vnet.ibm.com>,
	Daniel Lezcano <daniel.lezcano@linaro.org>,
	linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	Frederic Weisbecker <fweisbec@gmail.com>,
	linaro-networking@linaro.org, Steven Miao <realmz6@gmail.com>,
	Mark Salter <msalter@redhat.com>, Michal Simek <monstr@monstr.eu>,
	Ralf Baechle <ralf@linux-mips.org>,
	Ley Foon Tan <lftan@altera.com>, Jonas Bonn <jonas@southpole.se>,
	"David S. Miller" <davem@davemloft.net>,
	Jeff Dike <jdike@addtoit.com>, Guan Xuetao <gxt@mprc.pku.edu.cn>
Subject: Re: [PATCH] clockevents: Add (missing) default case for switch blocks
Date: Fri, 20 Feb 2015 10:36:59 +0100	[thread overview]
Message-ID: <20150220093659.GA23469@gmail.com> (raw)
In-Reply-To: <20150220084807.GJ21418@twins.programming.kicks-ass.net>


* Peter Zijlstra <peterz@infradead.org> wrote:

> > So this whole approach looks fragile for several reasons:
> > 
> >    - 'mode setting' callbacks are just bad by design
> >      because they mix several functions into the same entry
> >      point, complicating the handler functions 
> >      unnecessarily. We should reduce complexity, not expand 
> >      on it.
> > 
> >    - now by adding 'default' you hide from drivers the
> >      ability to easily discover whether it has been updated
> >      to some new core clockevents mode setting feature or
> >      not.
> 
> So this patch was a follow on from bd624d75db21 
> ("clockevents: Introduce mode specific callbacks").
> 
> That patch changes the set_mode() interface; and provides 
> per mode functions.

So why is a 'default' mode needed then? It makes the 
addition of new modes to the legacy handler easier, which 
looks backwards.

> New (and updated) drivers should not use ->set_mode() 
> anymore, but it was felt that we do not want to go do 
> flag day changes.

I fully agree that we don't want flag day changes, but make 
it really apparent that it's an obsolete interface:

  - rename it to set_mode_obsolete()

  - try to convert as many of the easy cases as possible - 
    the overwhelming majority of mode setting functions 
    look reasonably simple.

  - get rid of the mode enum in the core, and rename the 
    mode bits to CLOCK_EVT_MODE_OBSOLETE_XXX.

etc.

> And it allows for adding optional modes; not every driver 
> needs to go implement all mode functions if there is a 
> sane default action.
> 
> But it does mean we need to be able to add values to the 
> enum.

So I'm confused: if we are using proper callbacks (like my 
example outlined) , why is a 'mode enum' needed at all?

Thanks,

	Ingo

  reply	other threads:[~2015-02-20  9:36 UTC|newest]

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-02-20  6:32 [PATCH] clockevents: Add (missing) default case for switch blocks Viresh Kumar
2015-02-20  6:32 ` Viresh Kumar
2015-02-20  8:38 ` Ingo Molnar
2015-02-20  8:38   ` Ingo Molnar
2015-02-20  8:48   ` Peter Zijlstra
2015-02-20  8:48     ` Peter Zijlstra
2015-02-20  9:36     ` Ingo Molnar [this message]
2015-02-20  9:36       ` Ingo Molnar
2015-02-20 10:12       ` Viresh Kumar
2015-02-20 10:12         ` Viresh Kumar
2015-02-20 10:52         ` Ingo Molnar
2015-02-20 10:52           ` Ingo Molnar
2015-02-20 11:37       ` Peter Zijlstra
2015-02-20 11:37         ` Peter Zijlstra
2015-02-20 11:41         ` Ingo Molnar
2015-02-20 11:41           ` Ingo Molnar
2015-02-20 11:56           ` Viresh Kumar
2015-02-20 11:56             ` Viresh Kumar
2015-02-20 13:22             ` Ingo Molnar
2015-02-20 13:22               ` Ingo Molnar
2015-02-20 13:58               ` Viresh Kumar
2015-02-20 13:58                 ` Viresh Kumar
2015-02-20 14:04                 ` Ingo Molnar
2015-02-20 14:04                   ` Ingo Molnar
2015-02-23  5:33                   ` Viresh Kumar
2015-02-23  5:33                     ` Viresh Kumar
2015-02-23 16:37                     ` Ingo Molnar
2015-02-23 16:37                       ` Ingo Molnar
2015-02-24 11:11                       ` viresh kumar
2015-02-24 11:11                         ` viresh kumar
2015-02-24 14:54                         ` Ingo Molnar
2015-02-24 14:54                           ` Ingo Molnar
2015-02-24 15:12                           ` Viresh Kumar
2015-02-24 15:12                             ` Viresh Kumar

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=20150220093659.GA23469@gmail.com \
    --to=mingo@kernel.org \
    --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.