Hello.

Kukjin Kim wrote:

quoted

quoted

From: Abhilash Kesavan <redacted>
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.

quoted

quoted

Signed-off-by: Abhilash Kesavan <redacted>
Signed-off-by: Kukjin Kim <redacted>

[...]

quoted

quoted

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

quoted

    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.

quoted

[...]

quoted

quoted

+
+	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);

quoted

    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.

quoted

quoted

+
+	/* 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) {

quoted

    No need to check -- you won't be passed a mode not specified by your
pio_mask.

Will remove the check.

quoted

quoted

+static int __devinit pata_s3c_probe(struct platform_device *pdev)

[...]

quoted

quoted

+	if (!request_mem_region(res->start, resource_size(res), DRV_NAME)) {

quoted

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

quoted

quoted

+release_mem:
+	release_mem_region(res->start, resource_size(res));
+release_device_mem:
+	kfree(info);

quoted

    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?

quoted

quoted

+static struct platform_driver pata_s3c_driver = {
+	.probe		= pata_s3c_probe,

quoted

    2 empty lines -- broken patch?

Seems OK at my end.

    The sent patch had them, nevertheless.

MBR, Sergei