Hi,
I'm sorry I've been busy all this time and I hadn't got the opportunity
(untill now) to check this.
But now I am committed full-time on this.
Sergei Shtylyov escreveu:
João Ramos wrote:
Here is the revised patch according to your comments.
Most significant changes are:
[...]
- Removed useless blank lines and white-spaces (Ryan);
Not all of them yet. :-)
I've seen your comments and I fixed those blank lines.
Sorry, old programming habits ;-) .
Regards,
João
Patch follows.
diff -urN linux-2.6.29.orig/arch/arm/mach-ep93xx/core.c
linux-2.6.29/arch/arm/mach-ep93xx/core.c
--- linux-2.6.29.orig/arch/arm/mach-ep93xx/core.c 2009-03-23
23:12:14.000000000 +0000
+++ linux-2.6.29/arch/arm/mach-ep93xx/core.c 2009-03-30
13:52:04.000000000 +0100
@@ -537,6 +537,51 @@
platform_device_register(&ep93xx_i2c_device);
}
+static struct resource ep93xx_ide_resources[] = {
+ [0] = {
Your code uses 4-space indentation instead of single-tab -- this
style is not acceptable. I'd advise to run the patch thru
scripts/checkpatch.pl to find and fix the coding style mistakes.
The problem is mainly my mailer settings: it deforms the patch output.
Perhaps next time I should send the patch as an attachment?
Either way, i've run the patch through both Lindent.pl and checkpatch.pl
and fixed some identation issues.
[...]
+/*
+ * IDE Interface register map default state
+ * (shutdown)
+ */
+static void
+ep93xx_ide_pio_clean_regs(unsigned long base)
+{
+ /* disable IDE interface initially, ensuring that no device is
selected */
+ writel((IDECTRL_CS0N | IDECTRL_CS1N), IDE_REGISTER(IDECTRL));
+
+ /* clear IDE registers */
+ writel(0, IDE_REGISTER(IDECFG));
Writing 0 to this register generates an invalid cycle on IDE bus if
I don't mistake (CS0/1 and DIOR/W all asserted), are you sure you want
this?
Zero (0) is the default value for the IDE Configuration Register, as
stated in EP93xx User's Guide (Section 27, page 11).
Besides, CS0/1 and DIOR/W lines are controlled through IDE Control Register.
In that case, perhaps it would be best to have it also set to it's
default value, deasserting DIOR/W as well:
writel((IDECTRL_CS0N | IDECTRL_CS1N | IDECTRL_DIORN | IDECTRL_DIOWN),
IDE_REGISTER(IDECTRL));
+/*
+ * EP93xx IDE PIO low-level hardware initialization routine
+ */
+static void
+ep93xx_ide_pio_init_hwif(ide_hwif_t *hwif)
+{
+ unsigned long base = hwif->config_data;
+
+ /* enforce reset state */
+ ep93xx_ide_pio_clean_regs(base);
+
+ /* set gpio port E, G and H for IDE */
+ ep93xx_ide_gpio_on_ide();
+
+ /*
+ * configure IDE interface:
+ * - IDE Master Enable
+ * - Polled IO Operation
+ * - PIO Mode 4 (16.67 MBps)
+ * - 1 Wait State (10 ns)
+ */
+ writel(IDECFG_IDEEN | IDECFG_PIO | IDECFG_PIO_MODE_4 |
+ ((1 << 8) & IDECFG_WST), IDE_REGISTER(IDECFG));
+}
+
+/*
+ * EP93xx IDE PIO low-level byte-read procedure.
+ */
+static u8
+ep93xx_ide_pio_readb(unsigned long base, unsigned long addr)
+{
+ u32 reg;
+
+ /*
+ * initial state: DIORn=1, DIOWn=1
+ * write address out
+ * select CS configuration
+ */
+ reg = ((addr & 0x07) << 2) | ((addr >> 3) & 0x03) | IDECTRL_DIORN |
+ IDECTRL_DIOWN;
+ writel(reg, IDE_REGISTER(IDECTRL));
+
+ /*
+ * bring DIORn low
+ */
+ reg &= ~IDECTRL_DIORN;
+ writel(reg, IDE_REGISTER(IDECTRL));
+
+ /*
+ * bring DIORn high
+ */
+ reg |= IDECTRL_DIORN;
+ writel(reg, IDE_REGISTER(IDECTRL));
+
+ /*
+ * read IDE Data Input Register
+ */
+ return (readl(IDE_REGISTER(IDEDATAIN)) & 0xFF);
Masking is pointless, it's achieved by truncation to 'u8'.
+}
I don't see what warrants minimum active/recovery time here. And I
don't see how you're supporitng PIO modes 3/4 as you're not checking
-IORDY.
So, you're saying I should check for the IORDY value at the beginning
and end of each transfer?
What exactly do you mean by 'warrant minimum active/recovery time'?
Should I insert manual delays to ensure timing issues?
[...]
+/*
+ * EP93xx IDE PIO low-level word buffer-read procedure
+ */
+static void
+ep93xx_ide_pio_readsw(unsigned long base, unsigned long addr, void
*buf,
+ unsigned int len)
+{
+ u16 *data = (u16 *)buf;
+
+ for (;len;len--)
Put spaces after ; please.
+ *data++ = ep93xx_ide_pio_readw(base, addr);
Is this little-endian only code? If not, it's not valid in the big
endian mode...
Should I instead use array-indexing code such as:
for (; len; len--, i++)
data[i] = ep93xx_ide_pio_readw(base, addr);
And the same for 'ep93xx_ide_pio_writesw()' function?
[...]
+static struct ide_port_info ep93xx_ide_pio_port_info = {
+ .name = MODULE_NAME,
+ .init_hwif = ep93xx_ide_pio_init_hwif,
+ .tp_ops = &ep93xx_ide_pio_tp_ops,
+ .host_flags = IDE_HFLAG_SINGLE | IDE_HFLAG_NO_DMA |
IDE_HFLAG_MMIO |
+ IDE_HFLAG_NO_IO_32BIT | IDE_HFLAG_NO_UNMASK_IRQS,
+ .pio_mask = ATA_PIO4,
I'm not seeing the proper support for any of the PIO modes in your
driver as yet...
If you mind to explain to me how to, I would appreciate...
+static int __devinit ep93xx_ide_pio_probe(struct platform_device *pdev)
+{
+ int retval;
+
+ void __iomem *ide_base;
+
+ struct resource *mem_res;
+ struct resource *irq_res;
+
+ hw_regs_t hw;
+ hw_regs_t *hws[] = {&hw, NULL, NULL, NULL};
+
+ struct ide_host *host;
+
+ /*
+ * collect resources from platform_device structure
+ */
+ mem_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!mem_res) {
+ dev_err(&pdev->dev, "could not retrieve device memory
resources!\n");
+ retval = -ENXIO;
+ goto fail_return;
+ }
+
+ irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
You could also use platform_get_irq()...
Fixed.
+ if (!irq_res) {
+ dev_err(&pdev->dev, "could not retrieve device IRQ
resource!\n");
+ retval = -ENXIO;
+ goto fail_return;
+ }
+ + /* request IDE controller registers memory area */
+ if (!request_mem_region(mem_res->start, mem_res->end -
mem_res->start + 1,
+ MODULE_NAME)) {
+ dev_err(&pdev->dev, "could not request memory resources!\n");
+ retval = -EBUSY;
+ goto fail_return;
Pointless goto/label, you could just instead use:
return retval;
Fixed.
Best regards,
João Ramos
--
************************************************************************
João Ramos <joao.ramos@xxxxxxx>
INOV INESC Inovação - ESTG Leiria
Escola Superior de Tecnologia e Gestão de Leiria
Edíficio C1, Campus 2
Morro do Lena, Alto do Vieiro
Leiria
2411-901 Leiria
Portugal
Tel: +351244843424
Fax: +351244843424
************************************************************************
--
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
