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

From: Michal Simek
Date: Fri Feb 14 2014 - 04:22:21 EST


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/