[PATCH] Staging: netlogic: platform_net: Fixed '(' at the EOL

[PATCH] Staging: netlogic: platform_net: Fixed '(' at the EOL

[Naveen Panwar]

Removed '(' from the end of line, coding style issue.

Signed-off-by: Naveen Panwar <naveen.panwar27@gmail.com>
---
 drivers/staging/netlogic/platform_net.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/netlogic/platform_net.c b/drivers/staging/netlogic/platform_net.c
index abf4c71..f45b67a 100644
--- a/drivers/staging/netlogic/platform_net.c
+++ b/drivers/staging/netlogic/platform_net.c
@@ -107,8 +107,8 @@ static struct platform_device *gmac_controller2_init(void *gmac0_addr)
 		.dev.platform_data = &ndata1,
 	};
 
-	gmac4_addr = ioremap(CPHYSADDR(
-		nlm_mmio_base(NETLOGIC_IO_GMAC_4_OFFSET)), 0xfff);
+	gmac4_addr = ioremap(CPHYSADDR
+			(nlm_mmio_base(NETLOGIC_IO_GMAC_4_OFFSET)), 0xfff);
 	ndata1.serdes_addr = gmac4_addr;
 	ndata1.pcs_addr	= gmac4_addr;
 	ndata1.mii_addr	= gmac0_addr;
@@ -134,8 +134,8 @@ static void xls_gmac_init(void)
 {
 	int mac;
 	struct platform_device *xlr_net_dev1;
-	void __iomem *gmac0_addr = ioremap(CPHYSADDR(
-		nlm_mmio_base(NETLOGIC_IO_GMAC_0_OFFSET)), 0xfff);
+	void __iomem *gmac0_addr = ioremap(CPHYSADDR
+			(nlm_mmio_base(NETLOGIC_IO_GMAC_0_OFFSET)), 0xfff);
 
 	static struct xlr_net_data ndata0 = {
 		.rfr_station	= FMN_STNID_GMACRFR_0,
@@ -153,8 +153,8 @@ static void xls_gmac_init(void)
 	ndata0.mii_addr	= gmac0_addr;
 
 	/* Passing GPIO base for serdes init. Only needed on sgmii ports */
-	gpio_addr = ioremap(CPHYSADDR(
-		nlm_mmio_base(NETLOGIC_IO_GPIO_OFFSET)), 0xfff);
+	gpio_addr = ioremap(CPHYSADDR
+			(nlm_mmio_base(NETLOGIC_IO_GPIO_OFFSET)), 0xfff);
 	ndata0.gpio_addr = gpio_addr;
 	ndata0.cpu_mask = nlm_current_node()->coremask;
 
@@ -214,8 +214,8 @@ static void xlr_gmac_init(void)
 		.id		= 0,
 		.dev.platform_data = &ndata0,
 	};
-	ndata0.mii_addr = ioremap(CPHYSADDR(
-		nlm_mmio_base(NETLOGIC_IO_GMAC_0_OFFSET)), 0xfff);
+	ndata0.mii_addr = ioremap(CPHYSADDR
+			(nlm_mmio_base(NETLOGIC_IO_GMAC_0_OFFSET)), 0xfff);
 
 	ndata0.cpu_mask = nlm_current_node()->coremask;
 
-- 
2.7.4

Re: [PATCH] Staging: netlogic: platform_net: Fixed '(' at the EOL

[Joe Perches]

On Sun, 2018-01-14 at 23:47 +0530, Naveen Panwar wrote:
> Removed '(' from the end of line, coding style issue.
[]
> diff --git a/drivers/staging/netlogic/platform_net.c b/drivers/staging/netlogic/platform_net.c
[]
> @@ -107,8 +107,8 @@ static struct platform_device *gmac_controller2_init(void *gmac0_addr)
>  		.dev.platform_data = &ndata1,
>  	};
>  
> -	gmac4_addr = ioremap(CPHYSADDR(
> -		nlm_mmio_base(NETLOGIC_IO_GMAC_4_OFFSET)), 0xfff);
> +	gmac4_addr = ioremap(CPHYSADDR
> +			(nlm_mmio_base(NETLOGIC_IO_GMAC_4_OFFSET)), 0xfff);

My first reaction is this is ugly.

I suggest
	gmac4_addr = ioremap(CPHYSADDR(nlm_mmio_base(NETLOGIC_IO_GMAC_4_OFFSET)),
			     0xfff);

or using a temporary or a #define for
	CPHYSADDR(nlm_mmio_base(NETLOGIC_IO_GMAC_4_OFFSET))

or add a new define like:

#define MMIO_CPHYSADDR(addr) (CPHYSADDR(nlm_mmio_base(addr)))

and change all the netlogic uses of CPHYSADDR so this one
could be

	gmac4_addr = ioremap(MMIO_CPHYSADDR(NETLOGIC_IO_GMAC_4_OFFSET), 0xfff);

etc...

Re: [PATCH] Staging: netlogic: platform_net: Fixed '(' at the EOL

[Al Viro]

On Sun, Jan 14, 2018 at 11:47:11PM +0530, Naveen Panwar wrote:
> Removed '(' from the end of line, coding style issue.

The one and only reason for warnings is that they point to
places more likely to be dodgy.  There is no inherent value
in having e.g. checkpatch.pl STFU, all wanking about uniformity
of style nonwithstanding.

If you end up figuring out how to manipulate some code so that
some warning would go away, something is wrong.  Either the
warning is bogus (which it might very well be - the point of
a warning is "might be worth looking here" and considering
the... level of sophistication of some of those, you can't
expect to get no false alarms) or there *is* something
dodgy in the vicinity.  Dodgy beyond the "this heuristic
triggers", that is.

The first thing to do when dealing with any warning is to
look around and see if it has caught something interesting.
In this case the warning points to excessively long expressions.
With your patch they *still* are just as long, but split in
more unnatural way, making checkpatch.pl miss them.

All of them appear to have the same form - CPHYSADDR applied to
nlm_mmio_base result.  Moreover, looking around that file
shows that all uses of CPHYSADDR and nlm_mmio_base in it
are parts of such expressions.

Most of those expressions are too long and hard to read, so
cleaning them up would probably be a good idea.  And seeing
that composition of these functions keeps occuring in that
sucker, making that composition an inlined helper appears
to be called for.

So
static inline <some type> something(<some type> offset)
{
	return CPHYSADDR(nlm_mmio_base(offset));
}

and turn those into

	gmac4_addr = ioremap(something(NETLOGIC_IO_GMAC_4_OFFSET), 0xfff);

	void __iomem *gmac0_addr = ioremap(something(NETLOGIC_IO_GMAC_0_OFFSET),
					   0xfff);
 
	gpio_addr = ioremap(something(NETLOGIC_IO_GPIO_OFFSET), 0xfff);

	ndata0.mii_addr = ioremap(something(NETLOGIC_IO_GMAC_0_OFFSET), 0xfff);

and turn
        res->start = CPHYSADDR(nlm_mmio_base(offset));
in xlr_resource_init() into
        res->start = something(offset);
as well.  What should the types be?  Result is either fed to ioremap(), or
stored in struct resource 'start' field.  The latter is resource_size_t;
the former varies among the architectures.  Some take resource_size_t,
some - phys_addr_t, some - unsigned long.  On mips (that driver appears to
be mips-only) ioremap takes phys_addr_t; the difference is cosmetical,
anyway, since resource_size_t is typedefed to phys_addr_t on all architectures.

So let it return phys_addr_t.  As for the argument type, looks like it's
an explicit constant or, in case of xlr_resource_init(), an int.  Grepping
shows that all constants involved do fit into int, so we arrive at

static inline phys_addr_t something(int offset)
{
        return CPHYSADDR(nlm_mmio_base(offset));
}

probably best placed just before the first use.  OTOH, nlm_mmio_base() is
declared as taking uint32_t, so that might be a better fit for argument.
All constants fed there are between 0 and 2^31, so the only question is
what values does xlr_resource_init() get passed in 'offset' argument.
Callers are
                xlr_resource_init(&xlr_net1_res[mac * 2],
                                  xlr_gmac_offsets[mac + 4],
                                  xlr_gmac_irqs[mac + 4]);
                xlr_resource_init(&xlr_net0_res[0], xlr_gmac_offsets[0],
                                  xlr_gmac_irqs[0]);
                        xlr_resource_init(&xlr_net0_res[mac * 2],
                                          xlr_gmac_offsets[mac],  
                                        xlr_gmac_irqs[mac]);
                xlr_resource_init(&xlr_net0_res[mac * 2], xlr_gmac_offsets[mac],
                                  xlr_gmac_irqs[mac]);

so in all cases we have xlr_gmac_offsets[<something>] passed.  What's
going on with that array?  Initialized as
static u32 xlr_gmac_offsets[] = {
        NETLOGIC_IO_GMAC_0_OFFSET, NETLOGIC_IO_GMAC_1_OFFSET,
        NETLOGIC_IO_GMAC_2_OFFSET, NETLOGIC_IO_GMAC_3_OFFSET,
        NETLOGIC_IO_GMAC_4_OFFSET, NETLOGIC_IO_GMAC_5_OFFSET,
        NETLOGIC_IO_GMAC_6_OFFSET, NETLOGIC_IO_GMAC_7_OFFSET
};
and never modified, apparently.  OK, that pretty much settles it -
xlr_resource_init() should be getting u32 instead of int.  It's harmless
(the constants here are in the same range), but the things will be easier
for reader that way:

static inline phys_addr_t something(u32 offset)
{
        return CPHYSADDR(nlm_mmio_base(offset));
}

static void xlr_resource_init(struct resource *res, u32 offset, int irq)
{
        res->name = "gmac";

        res->start = something(offset);

While we are at it, 'irq' argument appears to be always taken from
xlr_gmac_irqs[], which is also u32.  So I'd probably do 
static void xlr_resource_init(struct resource *res, u32 offset, u32 irq)
instead.

Now, what do we call that helper?  Definition of CPHYSADDR is
/*
 * Returns the physical address of a CKSEGx / XKPHYS address
 */
#define CPHYSADDR(a)            ((_ACAST32_(a)) & 0x1fffffff)
so we are getting physical addresses here.  Getting too creative is
probably not worth it, so let's just call it 'physaddr' and call it
quits.  Grepping doesn't catch any existing functions with such
name, so we should be OK.  So,

static inline phys_addr_t physaddr(u32 offset)
{
        return CPHYSADDR(nlm_mmio_base(offset));
}

static void xlr_resource_init(struct resource *res, u32 offset, u32 irq)
{
        res->name = "gmac";

        res->start = physaddr(offset);
...
and turn the lines caught by that warning in the first place into
	gmac4_addr = ioremap(physaddr(NETLOGIC_IO_GMAC_4_OFFSET), 0xfff);
etc.  All those calls do look sane - there might have been brainos hidden
by hard-to-read expressions, but nothing earth-shattering has turned up.
Which means that we'd probably squeezed all we were going to get out of
that warning.  Now all we need is a sane commit message.  We'd introduced
a helper for physical address calculation that used to be open-coded in
a bunch of places in netlogic/platform_net.c, and adjusted the types of
arguments of xlr_resource_init() there.  So I'd go with something along
the lines of

[netlogic] introduce a helper for physical address calculation

Several places in drivers/staging/netlogic/platform_net.c open-code the
identical logics for calculating physical addresses of memory-mapped
objects on the device.  Put that into an inlined helper and use it.
Simplifies a bunch of expressions in there...

Adjust the types of arguments of xlr_resource_init() - 'offset' and 'irq'
are actually unsigned.  The values passed to those fit into 0..2G range,
so the behaviour is unchanged, but it's more consistent that way.

... and if you feel like crediting checkpatch.pl, possibly add something
about that thing having pointed to excessively long expressions in the
first place.

Re: [PATCH] Staging: netlogic: platform_net: Fixed '(' at the EOL

[Joe Perches]

On Sun, 2018-01-14 at 20:10 +0000, Al Viro wrote:
> On Sun, Jan 14, 2018 at 11:47:11PM +0530, Naveen Panwar wrote:
> > Removed '(' from the end of line, coding style issue.
> 
> The one and only reason for warnings is that they point to
> places more likely to be dodgy.  There is no inherent value
> in having e.g. checkpatch.pl STFU, all wanking about uniformity
> of style nonwithstanding.

[ long and complete response removed, available at
  https://patchwork.kernel.org/patch/10162725/ ]

It was very generous of you to spend so much time on that
informative and thorough reply Al.

My own response was _much_ more terse.

What I wonder is if that sort of guided response can be
setup as documentation for kernel-newbies / janitors so
that future new submitters can have better ideas as to
what code can and should be improved instead of getting
simple and sometimes ill-advised whitespace changes.

cheers, Joe

Re: [PATCH] Staging: netlogic: platform_net: Fixed '(' at the EOL

[Naveen Panwar]

[-- Attachment #1: Type: text/plain, Size: 1381 bytes --]

Hi Guys,

That was really nice, since this is my first patch I just sort of gave it a
go but I'm really delighted to
see the response from the community.

And yes including it in the Documentation is a really great Idea I'll try
to make the patch again with the suggested things.

Thanks and Best Regards

Thank You & Best Regards
Naveen Panwar

On Mon, Jan 15, 2018 at 1:52 AM, Joe Perches <joe@perches.com> wrote:

> On Sun, 2018-01-14 at 20:10 +0000, Al Viro wrote:
> > On Sun, Jan 14, 2018 at 11:47:11PM +0530, Naveen Panwar wrote:
> > > Removed '(' from the end of line, coding style issue.
> >
> > The one and only reason for warnings is that they point to
> > places more likely to be dodgy.  There is no inherent value
> > in having e.g. checkpatch.pl STFU, all wanking about uniformity
> > of style nonwithstanding.
>
> [ long and complete response removed, available at
>   https://patchwork.kernel.org/patch/10162725/ ]
>
> It was very generous of you to spend so much time on that
> informative and thorough reply Al.
>
> My own response was _much_ more terse.
>
> What I wonder is if that sort of guided response can be
> setup as documentation for kernel-newbies / janitors so
> that future new submitters can have better ideas as to
> what code can and should be improved instead of getting
> simple and sometimes ill-advised whitespace changes.
>
> cheers, Joe
>
>

[-- Attachment #2: Type: text/html, Size: 2218 bytes --]

Re: [PATCH] Staging: netlogic: platform_net: Fixed '(' at the EOL

[Naveen Panwar]

[-- Attachment #1: Type: text/plain, Size: 1652 bytes --]

Hi Guys,

I submitted a new patch with the suggestions from Al Viro, did you guys
check it?

Thank You & Best Regards
Naveen Panwar

On Mon, Jan 15, 2018 at 11:31 AM, Naveen Panwar <naveen.panwar27@gmail.com>
wrote:

> Hi Guys,
>
> That was really nice, since this is my first patch I just sort of gave it
> a go but I'm really delighted to
> see the response from the community.
>
> And yes including it in the Documentation is a really great Idea I'll try
> to make the patch again with the suggested things.
>
> Thanks and Best Regards
>
> Thank You & Best Regards
> Naveen Panwar
>
> On Mon, Jan 15, 2018 at 1:52 AM, Joe Perches <joe@perches.com> wrote:
>
>> On Sun, 2018-01-14 at 20:10 +0000, Al Viro wrote:
>> > On Sun, Jan 14, 2018 at 11:47:11PM +0530, Naveen Panwar wrote:
>> > > Removed '(' from the end of line, coding style issue.
>> >
>> > The one and only reason for warnings is that they point to
>> > places more likely to be dodgy.  There is no inherent value
>> > in having e.g. checkpatch.pl STFU, all wanking about uniformity
>> > of style nonwithstanding.
>>
>> [ long and complete response removed, available at
>>   https://patchwork.kernel.org/patch/10162725/ ]
>>
>> It was very generous of you to spend so much time on that
>> informative and thorough reply Al.
>>
>> My own response was _much_ more terse.
>>
>> What I wonder is if that sort of guided response can be
>> setup as documentation for kernel-newbies / janitors so
>> that future new submitters can have better ideas as to
>> what code can and should be improved instead of getting
>> simple and sometimes ill-advised whitespace changes.
>>
>> cheers, Joe
>>
>>
>

[-- Attachment #2: Type: text/html, Size: 3065 bytes --]

Re: [PATCH] Staging: netlogic: platform_net: Fixed '(' at the EOL

[Dan Carpenter]

On Tue, Jan 16, 2018 at 07:33:03PM +0530, Naveen Panwar wrote:
> Hi Guys,
> 
> I submitted a new patch with the suggestions from Al Viro, did you guys
> check it?
> 

The list seems to reject your patches.  It rejected the first one as
well.

regards,
dan carpenter

Re: [PATCH] Staging: netlogic: platform_net: Fixed '(' at the EOL

[Greg KH]

On Tue, Jan 16, 2018 at 07:33:03PM +0530, Naveen Panwar wrote:
> Hi Guys,
> 
> I submitted a new patch with the suggestions from Al Viro, did you guys
> check it?

I do not see any patch from my in my queue :(