Hi,
On Sun, Feb 13, 2011 at 3:39 PM, Sergei Shtylyov <sshtylyov@xxxxxxxxxx> wrote:
> Hello.
>
> On 11-02-2011 16:33, 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@xxxxxxxxx>
>
> [...]
>
>> 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
>> +#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
>> +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
>> 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)..
Thanks,
Bartlomiej
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
