linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: thomas.petazzoni@free-electrons.com (Thomas Petazzoni)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH 1/3] ARM: at91: move at91 aic driver to drivers/irqchip
Date: Thu, 23 May 2013 13:22:55 +0200	[thread overview]
Message-ID: <20130523132255.3acf4431@skate> (raw)
In-Reply-To: <20130523102601.GI9937@game.jcrosoft.org>

Dear Jean-Christophe PLAGNIOL-VILLARD,

On Thu, 23 May 2013 12:26:01 +0200, Jean-Christophe PLAGNIOL-VILLARD
wrote:

> > Did you read what I write? In arch/arm/mach-at91/pm.c, the only
> > accesses made to the AIC registers are for pr_debug() calls, i.e only
> > to print some debugging messages. I believe those ones can simply be
> > removed to decouple the pm.c code from the irq.c code.
> 
> yes I read and yes I do not want to move code before we do cleanup MANDATORY

Then please explain to our volunteer contributor what sort of cleanup
is required. Simply NACKing without offering any insight as to how
things should be done is completely useless, counter-productive, and a
complete abuse of a maintainer's power over contributors.

> >  * PATCH 1: remove usage of AIC registers for debug messages in pm.c
> NACK I do want this debug, very usefull to debug pm

For the cases where you need to debug this stuff, you can always re-add
some instrumentation in this code to dump whatever you want.

Or, alternatively, we can make the irqchip driver show using
pr_debug() the value of IMR in its ->suspend() hook, the value of IMR &
IPR in its ->resume() hook, and the pm.c code can show the PMC_PCSR on
suspend. This way, you have all the informations you need for debugging.

Having the PM driver and the irqchip driver step on each other register
areas is anyway a bad idea, especially when the only reason to do so is
for two debugging messages that can be implemented differently.

Notice that this solution can always be improved later on if needed,
with further cleanups from you.

But please stop rejecting valid patches from contributors, just because
the code is not coming from you. What Boris is proposing is perfectly
fine, just needs a little bit more work to sort out the header
problems as we discussed, and is a good step in the right direction. And
this step in the right direction doesn't prevent anyone else, including
you, to provide additional patches to move even further in the right
direction.

It would be good if the other AT91 maintainers could raise their voice
here, so that Boris knows whether it is worthwhile doing additional
revisions of this patch set.

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

  reply	other threads:[~2013-05-23 11:22 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-05-23  9:05 [RESEND PATCH 0/3] ARM: at91: move aic driver to drivers/irqchip Boris BREZILLON
2013-05-23  9:05 ` [PATCH 1/3] ARM: at91: move at91 " Boris BREZILLON
2013-05-23  9:06   ` Jean-Christophe PLAGNIOL-VILLARD
2013-05-23  9:51     ` boris brezillon
2013-05-23 10:09       ` Thomas Petazzoni
2013-05-23 10:09         ` Jean-Christophe PLAGNIOL-VILLARD
2013-05-23 10:20           ` Thomas Petazzoni
2013-05-23 10:26             ` Jean-Christophe PLAGNIOL-VILLARD
2013-05-23 11:22               ` Thomas Petazzoni [this message]
2013-05-23 11:58                 ` Jean-Christophe PLAGNIOL-VILLARD
2013-05-23 12:09                   ` Thomas Petazzoni
2013-05-23 13:00                 ` Nicolas Ferre
2013-05-23 10:18   ` Russell King - ARM Linux
2013-05-23 11:59     ` Jean-Christophe PLAGNIOL-VILLARD
2013-05-23 12:13     ` Thomas Petazzoni
2013-05-23 12:42       ` Boris BREZILLON
2013-05-23 12:44       ` boris brezillon
2013-05-23  9:05 ` [PATCH 2/3] ARM: at91: use IRQCHIP_DECLARE instead of machine specific init_irq Boris BREZILLON
2013-05-23  9:05 ` [PATCH 3/3] ARM: at91: use set_handle_irq instead of machine specific handle_irq Boris BREZILLON
  -- strict thread matches above, loose matches on Subject: below --
2013-05-23  8:49 [PATCH 0/3] ARM: at91: move aic driver to drivers/irqchip Boris BREZILLON
2013-05-23  8:49 ` [PATCH 1/3] ARM: at91: move at91 " Boris BREZILLON

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=20130523132255.3acf4431@skate \
    --to=thomas.petazzoni@free-electrons.com \
    --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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).