Hello.
Kukjin Kim wrote:
From: Abhilash Kesavan <a.kesavan@xxxxxxxxxxx>
Adds support for the Samsung PATA controller. This driver is based on the
Libata subsystem and references the earlier patches sent for IDE subsystem.
Hi,
Thanks for your comments.
Signed-off-by: Abhilash Kesavan <a.kesavan@xxxxxxxxxxx>
Signed-off-by: Kukjin Kim <kgene.kim@xxxxxxxxxxx>
[...]
diff --git a/drivers/ata/pata_samsung_cf.c b/drivers/ata/pata_samsung_cf.c
new file mode 100644
index 0000000..fef5515
--- /dev/null
+++ b/drivers/ata/pata_samsung_cf.c
@@ -0,0 +1,608 @@
+/* linux/drivers/ata/pata_samsung_cf.c
File names in the heading comment are discouraged.
Hmm. I used like that in other device drivers.
Nevertheless, it's quite an old rule already.
Ok..will remove the file name in the heading comment.
[...]
+
+ piotime = (t2i << 12) | (t2 << 4) | t1;
+
+ return piotime;
+}
+
+static void pata_s3c_set_piomode(struct ata_port *ap, struct ata_device *adev)
+{
+ int mode = adev->pio_mode - XFER_PIO_0;
+ struct s3c_ide_info *info = ap->host->private_data;
+ ulong ata_cfg = readl(info->ide_addr + S3C_ATA_CFG);
+ ulong piotime;
+
+ /* Calculates timing parameters for PIO mode */
+ piotime = pata_s3c_setup_timing(info, adev);
In fact, for 8-bit (command) timing you should program the slowest mode of
the two drives. However, with CF, you probably only have only one drive per
channel...
Below code looks OK ?
No, it doesn't.
if (ata_timing_compute(adev, adev->pio_mode, &timing, cycle_time, 0))
{
dev_err(ap->dev, "Failed to compute ATA timing\n");
piotime = pata_s3c_setup_timing(info, &initial_timing);
} else {
piotime = pata_s3c_setup_timing(info, &timing);
}
where initial_timing is for PIO0. I have added the below struct
static const struct ata_timing initial_timing =
{XFER_PIO_0, 70, 290, 240, 600, 165, 150, 0, 600, 0};
I'd call ata_timing_find_mode(XFER_PIO_0) rather than duplicating the
ata_timing entry. But really, you shouldn't set any timing for an invalid mode
and, as I said, you won't be passed one, so there's nor much sense in calling
ata_timing_compute() and checking its result; anyway, you'd want to call
ata_timing_find_mode() here instead of ata_timing_compute() because the latter
returns already quantized timings, but initial_timing is not quantized, you'll
have to call ata_timing_compute() in pata_s3c_setup_timing() anyway.
+
+ /* Enables IORDY if mode requires it */
+ if (ata_pio_need_iordy(adev))
+ ata_cfg |= S3C_ATA_CFG_IORDYEN;
+ else
+ ata_cfg &= ~S3C_ATA_CFG_IORDYEN;
+
+ /* Host controller supports upto PIO4 only */
+ if (mode >= 0 && mode <= 4) {
No need to check -- you won't be passed a mode not specified by your
pio_mask.
Will remove the check.
+static int __devinit pata_s3c_probe(struct platform_device *pdev)
[...]
+ if (!request_mem_region(res->start, resource_size(res), DRV_NAME)) {
Probably should call devm_request_mem_region() if you're using
devm_ioremap()...
Will change to devm_request_mem_region.
This function does exist, if I don't mistake...
+release_mem:
+ release_mem_region(res->start, resource_size(res));
+release_device_mem:
+ kfree(info);
Doesn't using devm_kzalloc() guarantee that the memory will be freed up
automatically?
Will remove kfree and release_mem_region because of devm_kzalloc and
devm_request_mem_region usage
I don't know devres librarry capabilities well. Tejun, am I right?
+static struct platform_driver pata_s3c_driver = {
+ .probe = pata_s3c_probe,
2 empty lines -- broken patch?
Seems OK at my end.
The sent patch had them, nevertheless.
MBR, Sergei
--
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
