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

[Sergei Shtylyov]

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@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

   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
--
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/