On Wed, 24 Feb 2010, Eric W. Biederman wrote:
> Yinghai Lu <yinghai@xxxxxxxxxx> writes:
>
> > will have
> > void (*ack)(struct irq_desc *desc);
> > void (*mask)(struct irq_desc *desc);
> > void (*mask_ack)(struct irq_desc *desc);
> > void (*unmask)(struct irq_desc *desc);
> > void (*eoi)(struct irq_desc *desc);
> >
> > so for sparseirq with raidix tree, we don't call extra
> > irq_to_desc, and could use desc directly
>
> Overall this looks pretty decent. This look pretty complete.
> How many platforms did you manage to compile test this on?
>
> I have found a couple of issues (see below).
>
> A few times you change a bit more than is necessary which is a bit
> spooky in a patch this far ranging.
>
> Reading through this patch to review it took an uncomfortably long
> time.
And you didn't even catch all problem spots.
While I like the idea, I really hate the convert wholesale approach.
303 files changed, 3127 insertions(+), 2191 deletions(-)
is fine for a sed script renaming action, but definitely _not_ for
patches which require semantical changes to the affected code.
The right way to do that is
1) add new function pointers, which take irq_desc as their argument
2) make the generic code use them when the old function pointer is
NULL
3) convert the irq_chip implementations one by one with separate
patches
4) Remove the old function pointers and switch the generic code
fully over to use the new ones
Yes, it's more work, but it's less error prone and easier to review,
as the maintainers just have to look at the patch which affects their
particular area.
Thanks,
tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-arch" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
