Re: [RFC PATCH 00/16] OMAP: GPMC: Restructure OMAP GPMC driver (NAND) : DT binding change proposal

[Roger Quadros <rogerq@ti.com>]

Javier,

On 05/23/2014 12:40 PM, Javier Martinez Canillas wrote:
> Hello Roger,
> 
> On Fri, May 23, 2014 at 10:16 AM, Roger Quadros <rogerq@ti.com> wrote:
>> Ezequiel & Javier,
>>
>> On 05/22/2014 05:46 PM, Ezequiel Garcia wrote:
>>> On 22 May 01:51 PM, Javier Martinez Canillas wrote:
>>>> On Thu, May 22, 2014 at 10:12 AM, Roger Quadros <rogerq@ti.com> wrote:
>>>>>> On 21 May 02:20 PM, Roger Quadros wrote:
>>>>>>>
>>>>>>> For DT boot:
>>>>>>> - The GPMC controller node should have a chip select (CS) node for each used
>>>>>>>   chip select. The CS node must have a child device node for each device
>>>>>>>   attached to that chip select. Properties for that child are GPMC agnostic.
>>>>>>>
>>>>>>>   i.e.
>>>>>>>      gpmc {
>>>>>>>              cs0 {
>>>>>>>                      nand0 {
>>>>>>>                      }
>>>>>>>              };
>>>>>>>
>>>>>>>              cs1 {
>>>>>>>                      nor0 {
>>>>>>>                      }
>>>>>>>              }
>>>>>>>              ...
>>>>>>>      };
>>>>>>>
>>>>>>
>>>>>> While I agree that the GPMC driver is a bit messy, I'm not sure it's possible
>>>>>> to go through such a complete devicetree binding re-design (breaking backwards
>>>>>> compatibility) now that the binding is already in production.
>>>>>
>>>>> Why not? especially if the existing bindings are poorly dones. Is anyone using these
>>>>> bindings burning the DT into ROM and can't change it when they update the kernel?
>>>>>
>>>>
>>>> While I do agree that your DT bindings are much better than the
>>>> current ones, there is a policy that DT bindings are an external API
>>>> and once are released with a kernel are set in stone and can't be
>>>> changed.
>>>>
>>>
>>> Exactly. The DT binding is considered an ABI. Thus, invariant across kernel
>>> versions. Users can't be coherced into a DTB update after a kernel update.
>>>
>>> That said, I don't really care if you break compatilibity in this case.
>>> Rather, I'm suggesting that you make sure this change is going to be accepted
>>> upstream, before doing any more work. The DT maintainers are reluctant to do
>>> so.
>>
>> Appreciate your concern.
>>
>> Would be really nice if you can review patches 1-12. They have nothing to do with DT changes.
>> Thanks.
>>
> 
> Overall your patches looks good to me. But I think it's better to wait
> until Tony removes the legacy board files for OMAP2+ since AFAIU at
> least the following patches could be dropped or trimmed down when
> board files are gone:
> 
> [RFC PATCH 04/16] ARM: OMAP2+: gpmc: use platform data to configure CS
> space and poplulate
> [RFC PATCH 06/16] ARM: OMAP2+: gpmc: add NAND specific setup
> [RFC PATCH 07/16] ARM: OMAP2+: nand: Update gpmc_nand_init() to use
> generic_gpmc_init()
> 
> Patches 1-3 and 5 are independent and can be applied in the meantime
> as a preparation for further changes following board files removal.
> 
> I really like patches 9-12 since those moves some NAND add-hoc code to
> the NAND driver where it really belongs. I think that similar changes
> can be made for OneNAND and push the special case handling code from
> GPMC driver to drivers/mtd/onenand/omap2.c.
> 
> Other devices (nor, ethernet, uart, etc) are already using
> gpmc_probe_generic_child() so I hope we can isolate the NAND and
> OneNAND specific changes and just use a single probe function for all
> child devices and possibly get even need the enum gpmc_omap_type you
> are adding on your struct gpmc_omap_cs_data.

Yes, I was thinking the same.

> 
> So what do you think if as a first step we add the platform data as
> you propose with all the commons timings and settings there, move all
> the possible code to NAND and OneNAND drivers and try to use a single
> configuration function for all child devices?

Yes, I agree.
> 
> Then once board files are gone we can do further cleanup in the driver
> and then we can discuss about changing the DT bindings. Maybe we can
> even change it while keeping backwards compatibility? Although I'm not
> sure about the last point I think that at least is worth to discuss
> it.

OK.

cheers,
-roger