RE: [PATCH 4/4] pata_samsung: Add Samsung PATA controller driver

[Kukjin Kim <kgene.kim@samsung.com>]

Sergei Shtylyov wrote:
> 
> Hello.

Hi ;-)

> Kukjin Kim wrote:
> 
> >>> From: Abhilash Kesavan <a.kesavan@samsung.com>
> >>> Adds support for the Samsung PATA controller. This driver is based on
the
> >>> Libata subsystem and references the earlier patches sent for IDE
subsystem.
> 
>     Where I those I wonder? :-)
>     It's a pity they didn't get accepted.
> 
Was told that that only new libata based drivers would be accepted.

> >>> Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com>
> >>> Signed-off-by: Kukjin Kim <kgene.kim@samsung.com>
> 
> [...]
> 
> >>> +		for (i = 0; i < words; i++, temp_addr++) {
> >>> +			wait_for_host_ready(info);
> >>> +			writel(*temp_addr, data_addr);
> >>> +		}
> >>> +	}
> 
> >>     Well, if this is pere CF case, 'buflen' can't be odd, but otherwise
> 
>     I meant to type "pure". :-)
> 
considering CF in true-ide mode..should I still add the check?

> >> you should handle that case...
> 
> >>> +	uint pio_teoc[5] = { 240, 43, 10, 70, 25 };
> 
> >>     What timing is this? :-O
> 
> > Should have been t2i rec.
> 
>     If it's t2i timing, it is incorrect for the first 3 PIO modes.
> 
Should have been (240, 93, 40, 70, 25)

> >>> +	if (cpu_type == TYPE_S3C6400) {
> >>> +		ap->ops = &pata_s3c_port_ops;
> >>> +		info->sfr_addr = info->ide_addr + 0x1800;
> >>> +		info->ide_addr = info->ide_addr + 0x1900;
> >>> +		info->fifo_status_reg = 0x94;
> >>> +	} else if (cpu_type == TYPE_S5PC100) {
> >>> +		ap->ops = &pata_s5p_port_ops;
> >>> +		info->sfr_addr = info->ide_addr + 0x1800;
> >>> +		info->ide_addr = info->ide_addr + 0x1900;
> 
> >>     Does make sense to assign those before *if*.
> 
> > Offsets not required for S5PV210/S5PC110.
> 
>     pata_s3c_tune_chipset() certainly requires them.
> 
yes..but for V210/C110 just the ioremapped info->ide_addr with 0 offset is
enough.

> >>> +		info->fifo_status_reg = 0x84;
> >>> +	} else {
> >>> +		ap->ops = &pata_s5p_port_ops;
> 
> >>     You don't assign 'info->ide_addr' here but you'll need it in
> >> pata_s3c_tune_chipset() which will be called thru pata_s5p_port_ops!
> 
> > The address received at ioremap() is enough for S5PV210/S5PC110..no
offset
> > needed.
> 
>     I only have to restate that pata_s3c_tune_chipset() does use
> 'info->ide_addr'. Maybe you shouldn't install this method for
S5PV210/S5PC110?
> Or do you mean thgat offset of 0 will work?
> 
V210/C110 requires 0 offset as compared to 6410 and C100 which require
0x1800.

> >>> +release_mem:
> >>> +	release_mem_region(res->start, res->end - res->start + 1);
> 
> >>     But you didn't call request_mem_region()!
> 
> > I didn't..will remove.
> 
>     But you should call request_mem_region() in one or another form...
> 
OK..will request mem_region() before ioremap()..and then the release_mem's
should be fine.


Thanks.

Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.