On Sun, Mar 16, 2014 at 10:16:09PM -0400, Gary Rookard wrote: > > > On Sun, 16 Mar 2014, Gary Rookard wrote: > > > > > > >On Sun, 16 Mar 2014, Mark Einon wrote: > > > >>On Sun, Mar 16, 2014 at 02:19:51PM -0400, Gary Rookard wrote: > >>>I unline split some quoted strings. > >> > >>Hi, > >> > >>Why? What issue does it fix? > >> > >-- > >Hi, > > > >It fixes a checkpatch error/warning "quoted string split across > >lines" or something phrased very simular. Is there > >a problem with. > > > -- > Hi again, > > Well firstly I do know that there is a trade off going on with the patch > that way, but for me, the general rule of thumb is that not sometimes > but most of the time you want your strings avalable for grep. And with > them split across lines it is hard -> impossible to accomplish. Thus > line length becomes meaningless or should I say not quit as bad as > it sounds. If you look they are sill shorter then alot of the code > in that > region of the file. > -- Hi Gary, Ok. Fixing the split line issue is good - there's no problems with what you're trying to do there. However: * The commit message should explain both what you're doing, and why you're doing it - this makes reviewing the patch and referencing in future far easier. For a checkpatch fix, you would normally expect checkpatch to be mentioned and the issues that are being addressed - cut and pasting the checkpatch output would work in this case, as it's not too verbose. (Also applies to your other patch). * You've fixed one checkpatch issue, but introduced another - meaning that * another patch is probably needed. Why not make the code more readable while * you're at it? Cheers, Mark > > >>> > >>>Signed-off-by: Gary Alan Rookard <garyrookard@xxxxxxxxx> > >>> > >>>--- > >>>On branch staging-next > >>> drivers/staging/bcm/CmHost.c | 12 ++++-------- > >>> 1 file changed, 4 insertions(+), 8 deletions(-) > >>> > >>>diff --git a/drivers/staging/bcm/CmHost.c b/drivers/staging/bcm/CmHost.c > >>>index 2d1f94d..4cb59d7 100644 > >>>--- a/drivers/staging/bcm/CmHost.c > >>>+++ b/drivers/staging/bcm/CmHost.c > >>>@@ -975,8 +975,7 @@ static VOID DumpCmControlPacket(PVOID pvBuffer) > >>> psfCSType->cCPacketClassificationRule.u8EthernetDestMacAddressLength); > >>> > >>> BCM_DEBUG_PRINT(Adapter, DBG_TYPE_OTHERS, DUMP_CONTROL, > >>>- DBG_LVL_ALL, "u8EthernetSourceMACAddress[6]: " > >>>- "%pM", psfCSType->cCPacketClassificationRule. > >>>+ DBG_LVL_ALL, "u8EthernetSourceMACAddress[6]: %pM", > >>>psfCSType->cCPacketClassificationRule. > >>> u8EthernetSourceMACAddress); > >> > >>You've also gone way over the 80 char limit for the line, for no > >>real reason. > >>if anything, I would consider making > >>psfCSType->cCPacketClassificationRule.u8EthernetSourceMACAddress > >>more readable. > >>Similarly for the changes below. > >> > >>Cheers, > >> > >>Mark > >> > >>> > >>> BCM_DEBUG_PRINT(Adapter, DBG_TYPE_OTHERS, DUMP_CONTROL, > >>>DBG_LVL_ALL, "u8EthertypeLength: 0x%02X ", > >>>@@ -1092,8 +1091,7 @@ static VOID DumpCmControlPacket(PVOID pvBuffer) > >>> psfCSType->cCPacketClassificationRule.u8ProtocolSourcePortRangeLength); > >>> > >>> BCM_DEBUG_PRINT(Adapter, DBG_TYPE_OTHERS, DUMP_CONTROL, > >>>- DBG_LVL_ALL, "u8ProtocolSourcePortRange[4]: " > >>>- "0x%*ph ", 4, psfCSType-> > >>>+ DBG_LVL_ALL, "u8ProtocolSourcePortRange[4]: 0x%*ph ", 4, > >>>psfCSType-> > >>> cCPacketClassificationRule. > >>> u8ProtocolSourcePortRange); > >>> > >>>@@ -1101,8 +1099,7 @@ static VOID DumpCmControlPacket(PVOID pvBuffer) > >>> psfCSType->cCPacketClassificationRule.u8ProtocolDestPortRangeLength); > >>> > >>> BCM_DEBUG_PRINT(Adapter, DBG_TYPE_OTHERS, DUMP_CONTROL, > >>>- DBG_LVL_ALL, "u8ProtocolDestPortRange[4]: " > >>>- "0x%*ph ", 4, psfCSType-> > >>>+ DBG_LVL_ALL, "u8ProtocolDestPortRange[4]: 0x%*ph ", 4, > >>>psfCSType-> > >>> cCPacketClassificationRule. > >>> u8ProtocolDestPortRange); > >>> > >>>@@ -1118,8 +1115,7 @@ static VOID DumpCmControlPacket(PVOID pvBuffer) > >>> psfCSType->cCPacketClassificationRule.u8EthernetDestMacAddressLength); > >>> > >>> BCM_DEBUG_PRINT(Adapter, DBG_TYPE_OTHERS, DUMP_CONTROL, > >>>- DBG_LVL_ALL, "u8EthernetSourceMACAddress[6]: " > >>>- "%pM", psfCSType->cCPacketClassificationRule. > >>>+ DBG_LVL_ALL, "u8EthernetSourceMACAddress[6]: %pM", > >>>psfCSType->cCPacketClassificationRule. > >>> u8EthernetSourceMACAddress); > >>> > >>> BCM_DEBUG_PRINT(Adapter, DBG_TYPE_OTHERS, DUMP_CONTROL, > >>>DBG_LVL_ALL, "u8EthertypeLength: 0x%02X ", > >>>psfCSType->cCPacketClassificationRule.u8EthertypeLength); > >>>-- > >>>1.9.0 > >>> > >>>_______________________________________________ > >>>devel mailing list > >>>devel@xxxxxxxxxxxxxxxxxxxxxx > >>>http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel > >> > > _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
