lkml.org 
[lkml]   [2011]   [Feb]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2] ata: add CONFIG_SATA_HOST config option
Hello.

On 14-02-2011 11:36, Bartlomiej Zolnierkiewicz wrote:

>>> Add CONFIG_SATA_HOST config option (for selecting SATA Host
>>> support) to make setup easier on PATA-only systems.

>>> Additionally move SATA-specific code to libata-sata.c which
>>> allows us to save ~11.5k of the output code size (x86-64) on
>>> PATA-only systems for CONFIG_SATA_HOST=n:

>>> CONFIG_SATA_HOST=y:
>>> text data bss dec hex filename
>>> 44283 6576 57 50916 c6e4 drivers/ata/libata-core.o
>>> 29054 16 2 29072 7190 drivers/ata/libata-eh.o
>>> 20085 0 19 20104 4e88 drivers/ata/libata-sff.o
>>> 8699 0 0 8699 21fb drivers/ata/libata-sata.o

>>> CONFIG_SATA_HOST=n:
>>> text data bss dec hex filename
>>> 43754 6576 57 50387 c4d3 drivers/ata/libata-core.o
>>> 26775 16 2 26793 68a9 drivers/ata/libata-eh.o
>>> 20144 0 19 20163 4ec3 drivers/ata/libata-sff.o

>>> Signed-off-by: Bartlomiej Zolnierkiewicz<bzolnier@gmail.com>

>> [...]

>>> Index: b/drivers/ata/libata-core.c
>>> ===================================================================
>>> --- a/drivers/ata/libata-core.c
>>> +++ b/drivers/ata/libata-core.c
>>
>> [...]
>>>
>>> @@ -573,34 +494,26 @@ void ata_tf_to_fis(const struct ata_task
>>> fis[19] = 0;
>>> }

>> Hm, what about ata_tf_to_fis()? It shouldn't be needed by non-SATA stuff,
>> moreover it'should be only needed by non-SFF controllers...

> in theory yes but it is actually used by libata-scsi.c

Ah, didn't look here -- I didn't have the full kernel source to grep in...

>>> +#ifndef CONFIG_SATA_HOST
>>> +int sata_link_scr_lpm(struct ata_link *link, enum ata_lpm_policy policy,
>>> + bool spm_wakeup)
>>> {
>>> - tf->command = fis[2]; /* status */
>>> - tf->feature = fis[3]; /* error */
>>> -
>>> - tf->lbal = fis[4];
>>> - tf->lbam = fis[5];
>>> - tf->lbah = fis[6];
>>> - tf->device = fis[7];
>>> -
>>> - tf->hob_lbal = fis[8];
>>> - tf->hob_lbam = fis[9];
>>> - tf->hob_lbah = fis[10];
>>> -
>>> - tf->nsect = fis[12];
>>> - tf->hob_nsect = fis[13];
>>> + return -EOPNOTSUPP;
>>> +}
>>> +EXPORT_SYMBOL_GPL(sata_link_scr_lpm);
>>
>> Shouldn't there be empty line here?

> ata_piix needs it

I didn't doubt -- I just asked about empty line between functions. :-)

>>> +int sata_std_hardreset(struct ata_link *link, unsigned int *class,
>>> + unsigned long deadline)
>>> +{
>>> + return -EOPNOTSUPP;
>>> +}
>>> +EXPORT_SYMBOL_GPL(sata_std_hardreset);

>> ... and here?

> ditto, and it is quite useful to have possibility of building ata_piix
> w/ CONFIG_SATA_HOST=n

Of course... but I just asked about empty line. :-)

>>> Index: b/drivers/ata/libata-sata.c
>>> ===================================================================
>>> --- /dev/null
>>> +++ b/drivers/ata/libata-sata.c
>>> @@ -0,0 +1,1242 @@
>>
>> [...]
>>>
>>> +int sata_set_spd(struct ata_link *link)
>>> +{
>>> + u32 scontrol;
>>> + int rc;
>>> +
>>> + if ((rc = sata_scr_read(link, SCR_CONTROL,&scontrol)))

>> I guess checkpatch.pl protests here...

> it does ;)

> [...]

>>> + goto out;
>>> +
>>> + scontrol = (scontrol& 0x0f0) | 0x301;
>>> +
>>> + if ((rc = sata_scr_write_flush(link, SCR_CONTROL, scontrol)))

>> And here...
>> I understand that you're only moving the code. Perhaps it's worth fixing
>> checkpatch.pl's complaints beforehand -- I was going to look at it...

> Because code is moved I preferred to leave CodingStyle changes alone
> but I also think that it would be useful to fix them one day (though I
> deferred the work for later due to pragmatic reasons)..

OK...

> Thanks,
> Bartlomiej

WBR, Sergei


\
 
 \ /
  Last update: 2011-02-14 13:05    [W:0.635 / U:0.468 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site