[PATCH v3] libata: pata_samsung_cf: Add Samsung PATA controller driver

Sergei Shtylyov wrote:

Hello.

Kukjin Kim wrote:

quoted

quoted

quoted

From: Abhilash Kesavan <redacted>
Adds support for the Samsung PATA controller. This driver is based on

the

quoted

quoted

quoted

Libata subsystem and references the earlier patches sent for IDE

subsystem.

quoted

Hi,

quoted

Thanks for your comments.

quoted

quoted

quoted

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

[...]

quoted

quoted

quoted

diff --git a/drivers/ata/pata_samsung_cf.c

b/drivers/ata/pata_samsung_cf.c

quoted

quoted

quoted

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

quoted

    File names in the heading comment are discouraged.

quoted

Hmm. I used like that in other device drivers.

    Nevertheless, it's quite an old rule already.

quoted

Ok..will remove the file name in the heading comment.

quoted

quoted

[...]

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)

quoted

quoted

quoted

+{
+	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

quoted

    In fact, for 8-bit (command) timing you should program the slowest

mode

of

quoted

quoted

the two drives. However, with CF, you probably only have only one drive

per

quoted

quoted

channel...

quoted

Below code looks OK ?

    No, it doesn't.

quoted

	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

quoted

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.

OK, no check for invalid mode then. Now using 'ata_timing_compute' to get the
quantized timings t1, t2 and t2i and then writing them to PIO_TIME registers.
PIO_TIME [0:3]  t1      [4:11] t2      [12:19] t2i.

quoted

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

quoted

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

quoted

Will remove the check.

quoted

quoted

quoted

+static int __devinit pata_s3c_probe(struct platform_device *pdev)

[...]

quoted

quoted

quoted

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

quoted

quoted

    Probably should call devm_request_mem_region() if you're using
devm_ioremap()...

quoted

Will change to devm_request_mem_region.

    This function does exist, if I don't mistake...

quoted

quoted

quoted

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

quoted

quoted

    Doesn't using devm_kzalloc() guarantee that the memory will be freed

up

quoted

quoted

automatically?

quoted

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

quoted

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

quoted

quoted

    2 empty lines -- broken patch?

quoted

Seems OK at my end.

    The sent patch had them, nevertheless.

MBR, Sergei

Thanks.

Best regards,
Kgene.
--
Kukjin Kim [off-list ref], Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.