Re: [PATCH v4 01/39] ARM: OMAP2+: gpmc: driver conversion

[Jon Hunter <jon-hunter@ti.com>]

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