Re: [PATCH RFC] pata_platform: add 8 bit data io support

[Tejun Heo <htejun@gmail.com>]

Hello,

Wang Jian wrote:
> +static void pata_platform_postreset(struct ata_link *link, unsigned int *classes)
> +{
> +	struct ata_port *ap = link->ap;
> +	struct ata_device *dev;
> +	u8 select = ATA_DEVICE_OBS;
> +
> +	/* Call default callback first */
> +	ata_sff_postreset(link, classes);
> +
> +	if (!(ap->flags & ATA_FLAG_8BIT_DATA))
> +		return;
> +
> +	/* Set 8-bit mode. We know we can do that */
> +	ata_link_for_each_dev(dev, link) {
> +		if (dev->devno)
> +			select |= ATA_DEV1;
> +
> +		iowrite8(SETFEATURES_8BIT_ON, ap->ioaddr.feature_addr);
> +		iowrite8(select, ap->ioaddr.device_addr);
> +		iowrite8(ATA_CMD_SET_FEATURES, ap->ioaddr.command_addr);

Aieee... Please don't do this.  I think it best belongs to
ata_dev_configure() or ->dev_config() if you wanna put it in low level
driver.

> @@ -106,7 +159,8 @@ int __devinit __pata_platform_probe(struct device *dev,
>  				    struct resource *ctl_res,
>  				    struct resource *irq_res,
>  				    unsigned int ioport_shift,
> -				    int __pio_mask)
> +				    int __pio_mask,
> +				    unsigned int data_width)
>  {
>  	struct ata_host *host;
>  	struct ata_port *ap;
> @@ -140,6 +194,9 @@ int __devinit __pata_platform_probe(struct device *dev,
>  	ap->pio_mask = __pio_mask;
>  	ap->flags |= ATA_FLAG_SLAVE_POSS;
>  
> +	if (data_width == ATA_DATA_WIDTH_8BIT)
> +		ap->flags |= ATA_FLAG_8BIT_DATA;

It's strange to define ATA_DATA_WIDTH_* constants in ata.h and only
use it in ata_platform.

Overall, I think the bulk of the 8bit PIO implementation should go
into the libata core layer and transfer width should be property of
struct ata_device - probably right above or below pio/dma_mode and
xfer_mode/shift fields.

Thanks.

-- 
tejun