Hi, the answers to your questions and comments are embedded below. Because a few changes in the patch are required and also because the current kernel version is now 2.6.27 I am re-doing the patch and it will be included in a second e-mail by itself. Tony Olech Elan Digital Systems Limited On Thu, 2011-01-06 at 13:17 +0000, David Vrabel wrote: > I've not given this driver a detailed review, just skimmed it and > noticed a few things. > Tony Olech wrote: > > + Some SDIO cards will need a firmware file to be loaded and > > + sent to VUB300 chip in order to achieve better data throughput. > > + Download these "Offload Pseudocode" from Elan Digital Systems' > > + web-site http://www.elandigitalsystems.com/support/downloads.php > > + and put them in /lib/firmware. Note that without these additional > > + firmware files the VUB300 chip will still function, but not at > > + the best obtainable data rate. > It would be useful to document (in the driver) what this "offload > pseudocode" does. Looking at the driver it looks like it's pre-fetching > SDIO register values, yes? Yes it does does pre-fetch and pre-set some register values. At this point in time the only available documentation is the interface document referred to in a previous e-mail which can be downloaded from: www.elandigitalsystems.com/eng/drivers/vub300/Linux-v2-10.zip > > +#pragma pack(1) > You probably want __attribute__((packed)) on each structure here. (Not > sure if GCC even supports this #pragma). Thank you for this comment. I have changed the patch accordingly. > > +static DEVICE_ATTR(OperatingMode, S_IRUGO, show_OperatingMode, NULL); > > +static struct attribute *vub300_attrs[] = { > > + &dev_attr_OperatingMode.attr, > > + NULL, > > +}; > Suggest removing this unless it's actually useful. If it is useful, it > should use all lower case for the name and it should be documented in > Documentation/ABI. This sysfs interface is a very useful read-only diagnostic and will enable us to support customers. In particular it identifies the SDIO bus speed actually being used as well as the identity of the offload pseudocode firmware file, if any, that has been downloaded into the VUB300 adapter. I have modified the patch to use lower case for the sysfs file name (even though other sysfs users use camel case) and have added documentation as requested.vub300->irqs_queued > > +static void snoop_block_size_and_bus_width(struct vub300_mmc_host *vub300, > > + u32 cmd_arg) > This doesn't seem necessary. The block size is available in the datavub300->irqs_queued > request and the bus width is supplied to the set_ios() call. Unfortunately you are incorrect in your assertion. The "block size" given in the data request is NOT ALWAYS equal to the function block size. Therefore the snooping of the function block size must remain. > > + if (6 == cmd->opcode) { > > + ResponseType = SDRT_1; > > + vub300->resp_len = 6; > You don't need to check the opcode to determine the response format. Seevub300->irqs_queued > cmd->flags. Unfortunately you are incorrect in your assertion. It is impossible, for example, from a flag setting of 0x15 to determine whether a response type of R5, R6 or R7 is to be expected. Therefore the snooping of the opcode must remain. > > +static void vub300_enable_sdio_irq(struct mmc_host *mmc, int enable) > > +{ /* NOT irq */ > > + struct vub300_mmc_host *vub300 = mmc_priv(mmc);vub300->irqs_queued > > + if (!vub300->interface) > > + return; > > + kref_get(&vub300->kref); > > + if (enable) {vub300->irqs_queued > > + mutex_lock(&vub300->irq_mutex); > > + if (vub300->irqs_queued) { > > + vub300->irqs_queued -= 1; > > + mmc_signal_sdio_irq(vub300->mmc); > This might signal a spurious interrupt as it's not checking if the > interrupt is still asserted. This should be avoided. You are correct in your implied assertion that spurious interrupts cause havoc and ultimately failure in the operation of the device. However this code segment is absolutely required for the correct operation of our offload processing. The key thing to note is that the variable vub300->irqs_queued prevents spurious interrupts. > David -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
