Sergei Shtylyov wrote: > > 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. > 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. > >>> + > >>> + /* 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 Thanks. Best regards, Kgene. -- Kukjin Kim <kgene.kim@xxxxxxxxxxx>, Senior Engineer, SW Solution Development Team, Samsung Electronics Co., Ltd. -- 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