Re: [PATCH V3 2/2] pata_arasan_cf: Adding support for arasan compact flash host controller

[Tejun Heo <tj@kernel.org>]

On Tue, Feb 22, 2011 at 02:32:39PM +0530, Viresh Kumar wrote:
> The Arasan CompactFlash Device Controller has three basic modes of
> operation: PC card ATA using I/O mode, PC card ATA using memory mode, PC card
> ATA using true IDE modes.
> 
> Currently driver supports only True IDE mode.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@st.com>

Looks mostly good to me now but just few more things.

> +static inline void dma_complete(struct arasan_cf_dev *acdev)
> +{
> +	struct ata_queued_cmd *qc = acdev->qc;
> +
> +	acdev->qc = NULL;
> +	ata_sff_interrupt(acdev->irq, acdev->host);
> +
> +	if (unlikely(qc->err_mask) && ata_is_dma(qc->tf.protocol))
> +		ata_ehi_push_desc(&qc->ap->link.eh_info, "DMA Failed: Timeout");
> +}

ata_ehi_push_desc() needs to be called under host lock.  In case I
missed other places, all functions which modify port/host states need
to be called under host lock.  Please make sure they are.

> +void arasan_cf_error_handler(struct ata_port *ap)
> +{
> +	struct arasan_cf_dev *acdev = ap->host->private_data;
> +
> +	cancel_work_sync(&acdev->work);
> +	cancel_delayed_work_sync(&acdev->dwork);
> +	return ata_sff_error_handler(ap);
> +}

Please add some comments explaining what's going on with DMA
processing as this driver is quite unusual.  Here and in the issue
path.

> +static int __devinit arasan_cf_probe(struct platform_device *pdev)
> +{
...
> +	ap->flags |= ATA_FLAG_PIO_POLLING;

Maybe ATA_FLAG_NO_ATAPI too?

Thanks.

-- 
tejun