Re: [PATCH 08/14] net: axienet: Removed checkpatch errors/warnings

[Michal Simek]

On 02/13/2014 04:51 PM, Joe Perches wrote:
> On Thu, 2014-02-13 at 08:19 +0100, Michal Simek wrote:
>> On 02/13/2014 01:31 AM, Joe Perches wrote:
>>> On Wed, 2014-02-12 at 16:55 +0100, Michal Simek wrote:
> 
> Hi again Michal.
> 
>>>> +		netdev_warn(lp->ndev,
>>>> +			 "Could not find clock ethernet controller property.");
>>>
>>> here too. (and alignment)
>>
>> This is problematic. I would like to keep 80 char limits and keeping
>> this align just break it. That's why I was using tab alignment.
>> Probably the solution is just to shorten message.
> 
> (overly long, tiresomely trivial stuff below)
> 
> Your choice.  I'm not an 80 column zealot but
> please don't shorten the message just to fit
> 80 columns if it impacts intelligibility.

I am trying to keep 80 chars and follow subsystem
standards.

> Generally, I'd write this something like:
> 
> 		netdev_warn(lp->ndev,
> 			    "Could not find clock ethernet controller property\n");
> 
> (without the period) which is 83 columns.

ok.

> checkpatch makes exceptions for 80 column line
> length maximums for format strings.

yes but testing systems reports it because that 80 chars
is still default value.


> 
> I've no real issue if you indent it back one.
> 
> fyi: this is 77 columns
> 
> 		netdev_warn(lp->ndev,
> 			    "No clock ethernet controller property found\n");
> 
> About the message itself.
> 
> You dropped the "axienet_mdio_setup" function name.
> 
> I believe the dmesg output will look something like:
> 
> xilinx_temac 0000:01:00.0 (unregistered net_device): Could not find clock ethernet controller property.
> xilinx_temac 0000:01:00.0 (unregistered net_device): Setting MDIO clock divisor to default 29
> 
> Because these 2 messages are effectively linked,
> my preference would be to emit them on a single line,
> 
> Something like:
> 
> xilinx_temac 0000:01:00.0 (unregistered net_device): of_get_property("clock-frequency") not found - setting MDIO clock divisor to default 29
> 
> or
> 
> 		netdev_warn(lp->ndev,
> 			    "of_get_property(\"clock-frequency\") not found - setting MDIO clock divisor to default %u\n",
> 			    DEFAULT_CLOCK_DIVISOR);
> 

But then you are breaking 80 char limits a lot.

Thanks,
Michal


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/