Re: [PATCH v3 6/9] irqchip: Add the ingenic-tcu-intc driver

[Lee Jones]

On Mon, 22 Jan 2018, Marc Zyngier wrote:

> On 22/01/18 09:26, Lee Jones wrote:
> > On Sat, 20 Jan 2018, Marc Zyngier wrote:
> > 
> >> On Thu, 11 Jan 2018 17:25:45 +0100
> >> Paul Cercueil <paul@xxxxxxxxxxxxxxx> wrote:
> >>
> >>> Hi Marc,
> >>>
> >>>>>  +static int __init ingenic_tcu_intc_of_init(struct device_node 
> >>>>> *node,
> >>>>>  +	struct device_node *parent)
> >>>>>  +{
> >>>>>  +	struct irq_domain *domain;
> >>>>>  +	struct irq_chip_generic *gc;
> >>>>>  +	struct irq_chip_type *ct;
> >>>>>  +	int err, i, num_parent_irqs;
> >>>>>  +	unsigned int parent_irqs[3];  
> >>>>
> >>>> 3 parent interrupts? Really? How do you pick one? Also, given the 
> >>>> useage
> >>>> model below, "int" is the wrong type. Probably should be u32.  
> >>>
> >>> See below.
> >>>
> >>>>>  +	struct regmap *map;
> >>>>>  +
> >>>>>  +	num_parent_irqs = of_property_count_elems_of_size(
> >>>>>  +			node, "interrupts", 4);  
> >>>>
> >>>> Nit: on a single line, as here is nothing that hurts my eyes more than
> >>>> reading something like(
> >>>> this). Also, 4 is better expressed as sizeof(u32).  
> >>>
> >>> That will make checkpatch.pl unhappy :(
> >>
> >> And I don't care about checkpatch. I maintain the irqchip stuff, while
> >> checkpatch doesn't. Hence, I win.
> > 
> > 	num_parent_irqs =
> > 		of_property_count_elems_of_size(node, "interrupts", 4);  
> > 
> > Everybody wins!
> 
> <old_git_rant>
> 
> As I said before, I've stopped using a physical DEC VT100 around 1990,
> and gained the ability to extend my terminal to a bit more that 80
> columns. And even the VT100 could be coerced into using a 132 column mode...
> 
> </old_git_rant>

Right, Greg has spoken about this before.

> Adhering to a convention can be good, but common sense must apply first.
> Splitting an assignment is visually annoying and in that case, it
> doesn't make much sense. I'll happily take a line that goes beyond 80
> cols

I'm not adverse to the idea, but we should agree to do this centrally,
rather than a few of us going rouge.  This way we can go ahead and
change all of the documentation and tooling (inc. Checkpatch) too,
which will save on countless inevitable conversations/patches
attempting to 'fix' non-conforming lines/files.

> and if you really wanted to stay within boundaries, how about
> turning "num_parent_irqs" something shorter?


-- 
Lee Jones
Linaro Services Technical Lead
Linaro.org â Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog