Re: [PATCH] staging: keucr: smscsi: fixed coding style issues

[Dan Carpenter]

On Tue, Mar 22, 2011 at 10:21:29PM +0100, Ralf Thielow wrote:
> Fixed coding style issues.

This patch description is not very good.

> diff --git a/drivers/staging/keucr/smscsi.c b/drivers/staging/keucr/smscsi.c
> index 6211686..821a162 100644
> --- a/drivers/staging/keucr/smscsi.c
> +++ b/drivers/staging/keucr/smscsi.c
> @@ -9,76 +9,78 @@
>  #include "usb.h"
>  #include "scsiglue.h"
>  #include "transport.h"
> -//#include "smcommon.h"
> +/* #include "smcommon.h" */

Just delete this comment.

>  #include "smil.h"
>  
> -int SM_SCSI_Test_Unit_Ready (struct us_data *us, struct scsi_cmnd *srb);
> -int SM_SCSI_Inquiry         (struct us_data *us, struct scsi_cmnd *srb);
> -int SM_SCSI_Mode_Sense      (struct us_data *us, struct scsi_cmnd *srb);
> -int SM_SCSI_Start_Stop      (struct us_data *us, struct scsi_cmnd *srb);
> -int SM_SCSI_Read_Capacity   (struct us_data *us, struct scsi_cmnd *srb);
> -int SM_SCSI_Read            (struct us_data *us, struct scsi_cmnd *srb);
> -int SM_SCSI_Write           (struct us_data *us, struct scsi_cmnd *srb);
> -
> -extern struct SSFDCTYPE     Ssfdc;
> -extern struct ADDRESS       Media;
> -extern PBYTE                SMHostAddr;
> -extern DWORD                ErrXDCode;

Why move these to a seperate header file?  That should have been
explained in the changelog.  You forgot to include the header file.
Also you didn't compile this crap...  :/

> -
> -//----- SM_SCSIIrp() --------------------------------------------------
> +/* ----- SM_SCSIIrp() -------------------------------------------------- */

There are a lot of comments here that should just be deleted.  For
example this one is worthless.  If it were in kernel doc format then
it would be worthwhile, but as it is, you may as well just delete it.

All the commented out debug code should just be deleted as well.  Maybe
send 2 patches.  The first just deletes things and the second fixes up
style issues.

>  int SM_SCSIIrp(struct us_data *us, struct scsi_cmnd *srb)
>  {
>  	int    result;
>  
>  	us->SrbStatus = SS_SUCCESS;
> -	switch (srb->cmnd[0])
> -	{
> -		case TEST_UNIT_READY :  result = SM_SCSI_Test_Unit_Ready (us, srb);  break; //0x00
> -		case INQUIRY         :  result = SM_SCSI_Inquiry         (us, srb);  break; //0x12
> -		case MODE_SENSE      :  result = SM_SCSI_Mode_Sense      (us, srb);  break; //0x1A
> -		case READ_CAPACITY   :  result = SM_SCSI_Read_Capacity   (us, srb);  break; //0x25
> -		case READ_10         :  result = SM_SCSI_Read            (us, srb);  break; //0x28
> -		case WRITE_10        :  result = SM_SCSI_Write           (us, srb);  break; //0x2A

If you want you could just leave this as is.  It's readable.
checkpatch.pl is there to serve you and not the other way
around.

> -
> -		default:
> -			us->SrbStatus = SS_ILLEGAL_REQUEST;
> -			result = USB_STOR_TRANSPORT_FAILED;
> -			break;
> +	switch (srb->cmnd[0]) {
> +	case TEST_UNIT_READY:
> +		result = SM_SCSI_Test_Unit_Ready(us, srb);
> +		break; /* 0x00 */
                       ^^^^^^^^^^

This should have been on the same line as TEST_UNIT_READY.  But actually
it's a worthless comment.  Delete if you want.  Or leave it as it was as
I said before.

> -//----- SM_SCSI_Test_Unit_Ready() --------------------------------------------------
> +/* ----- SM_SCSI_Test_Unit_Ready() ----------------------------------------- */
>  int SM_SCSI_Test_Unit_Ready(struct us_data *us, struct scsi_cmnd *srb)
>  {
> -	//printk("SM_SCSI_Test_Unit_Ready\n");
> +	/* printk("SM_SCSI_Test_Unit_Ready\n"); */

This is what I mean by debug code that should just be deleted.

> @@ -98,49 +100,52 @@ int SM_SCSI_Read_Capacity(struct us_data *us, struct scsi_cmnd *srb)
>  	WORD    bl_len;
>  	BYTE    buf[8];
>  
> -	printk("SM_SCSI_Read_Capacity\n");
> +	printk(KERN_INFO, "SM_SCSI_Read_Capacity\n");

Use KERN_DEBUG or pr_debug().  In the end, we'll have to delete these
before it gets moved out of staging.  There was a similar issue in a 
different change you made.

These are behavior changes and behavior changes should always be
mentioned in the changelod.

>  
>  	bl_len = 0x200;
>  	bl_num = Ssfdc.MaxLogBlocks * Ssfdc.MaxSectors * Ssfdc.MaxZones - 1;
> -	//printk("MaxLogBlocks = %x\n", Ssfdc.MaxLogBlocks);
> -	//printk("MaxSectors   = %x\n", Ssfdc.MaxSectors);
> -	//printk("MaxZones     = %x\n", Ssfdc.MaxZones);
> -	//printk("bl_num       = %x\n", bl_num);
> +	/* printk("MaxLogBlocks = %x\n", Ssfdc.MaxLogBlocks);
> +	printk("MaxSectors   = %x\n", Ssfdc.MaxSectors);
> +	printk("MaxZones     = %x\n", Ssfdc.MaxZones);
> +	printk("bl_num       = %x\n", bl_num); */

This is not the right way to do multi-line comments.  Read CodingStyle.
>  
>  	us->bl_num = bl_num;
> -	printk("bl_len = %x\n", bl_len);
> -	printk("bl_num = %x\n", bl_num);
> -
> -	//srb->request_bufflen = 8;
> -	buf[0] = (bl_num>>24) & 0xff;
> -	buf[1] = (bl_num>>16) & 0xff;
> -	buf[2] = (bl_num>> 8) & 0xff;
> -	buf[3] = (bl_num>> 0) & 0xff;
> -	buf[4] = (bl_len>>24) & 0xff;
> -	buf[5] = (bl_len>>16) & 0xff;
> -	buf[6] = (bl_len>> 8) & 0xff;
> -	buf[7] = (bl_len>> 0) & 0xff;
> -	
> +	printk(KERN_INFO, "bl_len = %x\n", bl_len);
> +	printk(KERN_INFO, "bl_num = %x\n", bl_num);

DEBUG.

Etc.

regards,
dan carpenter
--
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/