Re: [PATCH 2/3] ide: add at91_ide driver

[Stanislaw Gruszka <stf_xl@wp.pl>]

Wednesday 04 February 2009 17:04:27 Sergei Shtylyov napisał(a):
> >>> extern void __init at91_add_device_cf(struct at91_cf_data *data);
> >>> 
> >>>+ /* Compact Flash True IDE mode */
> >>>+struct at91_ide_data {
> >>>+	u8	irq_pin;		/* the same meaning as for CF */
> 
> >>  I again have to express my dislike about not passing IRQ the usual 
> >>way. Also, see my comments to the platform code.
> 
> > Yes, I know, I don't like to argue. Only reasoning to use platform irq resource
> > seams to be: "because other drivers do". However we have exception - at91_cf
> > also use board->irq_pin, so maybe this driver could also do ?
> 
>     Then why have the memory resource when we can calculate it from the chip 
> select?  
Great idea, I very like it :) But memory is a platform (cpu) resource, however
board dependend.

> (I'm not asking you to do that, since the platfrom device resources   
> are user-visible thru /proc/iomem -- even if the driver is not enabled.)
Let's distinguish platform (cpu) resources and board resources.

If you take a look at arch/arm/mach-at91/*_devices.c files, 
IORESOURCE_IRQ are used for interrupts from devices that are integrated
on the chip. Board specific irq pins (like in at91_cf, at91_ether) are not
passed to platform driver via platform_resource but via board data.

> >>>diff --git a/drivers/ide/at91_ide.c b/drivers/ide/at91_ide.c
> >>>new file mode 100644
> >>>index 0000000..3a1f7e0
> >>>--- /dev/null
> >>>+++ b/drivers/ide/at91_ide.c
> >>>@@ -0,0 +1,496 @@
> >>>+/*
> >>>+ * IDE host driver for AT91SAM9 Static Memory Controller
> 
> >>   Why not call the driver 'at91sam9_ide'?
> 
> >>>+/*
> >>>+ * AT91 Static Memory Controller
> 
> >>  AT91SAM9.
> 
> > Ok, currently only SAM9 can be used with driver. However I think adding
> > support to AT91RM9200 to this driver will be not much effort.
> 
>     Can you answer the simple question: why we should try to support two 
> incompatible chips with a single driver? Because the driver name will be 
> shorter? :-)
Very funny. I think patch adding RM9200 support to this driver will have less
than 50 lines changeset, whereas writing new driver would be about 500
lines.

> > I don't think
> > someone will want to write new driver for RM9200 insted using this one.
> 
>     You're right, nobody will want that... because AT91RM9200 as is has *no 
> support for True IDE mode*. ;-)
Atmel documents are confusing. AT91RM9200 datasheet tells there is no
True IDE support, but RM9200 hard drive application note (which I send you
a link before) tell it is.

> >>   Frankly speaking, I don't see why you're reading this register back 
> >>if you already know what needs to be set there -- as you've done it in 
> >>init_smc_mode().
> 
> > This function is not only used at initialization. It is also used when PIO mode
> > is changed.
> 
>     So what? You can just write the fixed mode bits into the register every 
> time without readback.

Ok, I see now.

Cheers
Stanislaw Gruszka