Re: [PATCH v2] ata: add CONFIG_SATA_HOST config option

[Sergei Shtylyov <sshtylyov@mvista.com>]

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