Re: [PATCH 4/5] ide: Add support for TRIM

[Matthew Wilcox <willy@linux.intel.com>]

On Thu, Apr 02, 2009 at 07:58:59PM +0400, Sergei Shtylyov wrote:
>>  +static int idedisk_prepare_discard(struct request_queue *q, struct 
>> request *rq,
>> +							struct bio *bio)
>
>    Weird indentation.

Not at all.  New line, so tab the next line over as far as it will go
without falling off the 80-column limit.  It's fairly common.

To quote Documentation/CodingStyle:

  Statements longer than 80 columns will be broken into sensible chunks.
  Descendants are always substantially shorter than the parent and are placed
  substantially to the right. The same applies to function headers with a long
  argument list. Long strings are as well broken into shorter strings.  The
  only exception to this is where exceeding 80 columns significantly increases
  readability and does not hide information.

>> +{
>> +	ide_task_t *task;
>
>    This patch is already obsolete as 'ide_task_t' is gone. Use 'struct  
> ide_cmd' instead.

Thanks, fixed.

>> +	unsigned size;
>> +	struct page *page = alloc_page(GFP_KERNEL);
>
>    Missing empty line after the declaration block...

Empty line not necessary.

>> +	if (!page)
>> +		goto error;
>
>    Unneeded goto -- why not just return?

Because it then looks the same as the other two cases where we can error
out.

>> +	task->tf_flags   = IDE_TFLAG_LBA48 | IDE_TFLAG_OUT_HOB |
>> +			   IDE_TFLAG_OUT_TF | IDE_TFLAG_OUT_DEVICE |
>
>    The last 3 flags are going to be obsoleted too...

So if I remove them today, the command will still work?