Hi Afzal, On 05/07/2012 05:57 AM, Mohammed, Afzal wrote: > Hi Jon, > > On Fri, May 04, 2012 at 21:50:16, Hunter, Jon wrote: >>> As mentioned in the cover letter, >>> >>> "Additional features that currently boards in mainline does not make >>> use of like, waitpin interrupt handling, changes to leverage revision >>> 6 IP differences has not been incorporated." >>> >>> Priority in this series is to convert into a driver, get all boards working >>> on mainline. Once all boards are working with gpmc driver, these features >>> which are not required for currently supported boards can be added later. >> >> Yes, but I meant why 2 and not say 5? Anyway, I think that this should >> be marked with a comment like a TODO so it is clear that this needs to >> be re-visited. > > Ok, it will be marked with TODO > >>>> I think that it make more sense to have the wait-pin information part of >>>> the gpmc_cs_data structure for the following reasons ... >>> >>> Waitpin information is indeed a part of cs as far as boards are concerned, >>> it is only that it gets propogated to device >> >> Why does the device's driver care? From my point of view, the wait-pin >> is just part of the interface setup/configuration. The external device >> may require this and assumes that this has been setup along with the CS >> and interface timing, but the device's driver should not care. Remember >> this is just a ready signal used to stall the access. Once configured, >> software should be unaware of it. > > By device, it is referred to gpmc device which only gpmc driver is aware, > the peripheral device's driver is not at all aware. > >>>> Also, any reason why waitpin_polarity is an int? I see you define LOW as >>>> -1, but I not sure why LOW cannot be 0 as 0 is programmed into the >>>> register for an active low wait signal. >>> >>> Only intention is not to alter default waitpin polarity value, i.e. if >>> any board does make use of it w/o knowledge of Kernel, I don't want to >>> break it, there may be an argument saying that the board code is buggy, >>> while if some board does so, it is, but don't want to break working one. >>> >>> Here unless user explicitly set the flag, it does nothing on polarity >> >> Ok. Do such scenario's exist today? Please note that board code will be >> removed in the future and device-tree will replace. So if there are no >> cases today, I would not be concerned. Unless this could be something >> that has already been configured by the bootloader. However, in that >> case would be even call this function? > > Let me check this, here only intent was to play safe, as I have > only two boards to test. > >>>>> +int gpmc_cs_reconfigure(char *name, int id, struct gpmc_cs_data *cs) >>>> >>>> What scenario is this function used in? May be worth adding a comment >>>> about the function. >>> >>> Ok, it was required for OneNAND, as it needs to reconfigure >> >> Ok, but why is that? Unless this is something generic about one-nand I >> don't comprehend. I have a high-level understanding of one-nand, but I >> am not familiar with the specifics that require it to be reconfigured. > > Not too familiar with OneNAND, from the code it has been understood > that to set into sync mode, first it may have to set to async mode, read > frequency from OneNAND, then setup sync mode for that frequency. > > >>>>> + gpmc_setup_writeprotect(gpmc); >>>> >>>> Write protect is a pin and there is only one. Like the waitpins and CS >>>> signals this needs to be associated with a device. It would make sense >>>> that this is associated with the cs data. >>> >>> As far as platform are concerned, it is associated with cs, it is only >>> that while configuring CS, it is propagated such that it is done once. >> >> Hmmm ... but here it looks like if write-protect is used you are going >> to turn it on. A feature like this seems that it does need to be handled >> by the device's driver. Enabling and disabling write-protect are >> functions that I would expect are exported to the device's driver and >> not directly enabled in the gpmc driver during probe. However, maybe is >> could be valid for the write protect to be enabled by default which >> could make sense. However, I don't see anywhere else in the driver to >> control it. >> >> Shouldn't we warn on multiple devices trying to use write-protect when >> parsing their configuration? > > Even if a single device sets write protect, kernel will log it. That does not seem sufficient. It should be flagged as an error. Write protect is a resource like the CS and waitpins and so I would have thought that it should be reserved in the same way. Jon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
