On Fri, Oct 9, 2009 at 9:59 AM, jassi brar <jassisinghbrar@xxxxxxxxx> wrote:
>> +static void wait_for_host_ready(struct s3c_ide_device *ide_dev)
>> +{
>> + u32 count = 1000000;
>> + while (ide_readl(S3C_ATA_FIFO_STATUS) >> 28) {
>> + if (!(count--)) {
>> + dev_err(&ide_dev->pdev->dev,
>> + "ide controller not ready for next taskfile operation");
>> + return;
>> + }
>> + }
>> +}
>
> I think instead of using a magic number for loop counter, it will be
> better to use some timed wait using a counter as in
> http://www.spinics.net/lists/arm-kernel/msg73904.html
I will change this to timed wait as in your patch.
>> +static u8 s3c_ide_INB(ide_hwif_t *hwif, ulong reg)
>> +{
>> + struct s3c_ide_device *ide_dev =
>> + (struct s3c_ide_device *)hwif->hwif_data;
>> + u8 temp;
>> +
>> + wait_for_host_ready(ide_dev);
>> + temp = __raw_readb(reg);
>> + wait_for_host_ready(ide_dev);
>> + temp = __raw_readb(ide_dev->regbase + S3C_ATA_PIO_RDATA);
>> + return temp;
>> +}
>
> any namespace reason for the caps _OUTB and _INB ?
>
I missed that. There was no namespace issue. I will change this to lowercase.
>> + if (ide_dev->irq < 0) {
>> + dev_err(&pdev->dev, "could not obtain irq number\n");
>> + ret = -ENODEV;
>> + goto out;
>> + }
> check for valid irq shud be done immidiately after trying to get it
> and before acquiring any other resource
Ok. I will fix this.
>> +out:
>> + return ret;
>> +}
> Since you call 'goto out' upon some call failure, the label 'out' shud
> undo all the successful operations before hitting the failure.
> It shud rather be out1, out2 etc jumped at from various locations during
> initialization.
I missed this. I will add the undo code here.
Thanks for reviewing the patch. Your comments were very helpful.
Thanks,
Thomas.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
