Re: [PATCH v10 10/10] mtd: maps: gpio-addr-flash: Add support for device-tree devices

[Boris Brezillon]

Hi Ricardo,

On Fri, 5 Oct 2018 08:31:35 +0200
Ricardo Ribalda Delgado <ricardo.ribalda@xxxxxxxxx> wrote:

> Hi Boris
> 
> 
> On Fri, Oct 5, 2018 at 12:21 AM Boris Brezillon
> <boris.brezillon@xxxxxxxxxxx> wrote:
> >
> > Hi Ricardo,
> >
> > On Thu,  4 Oct 2018 16:29:42 +0200
> > Ricardo Ribalda Delgado <ricardo.ribalda@xxxxxxxxx> wrote:
> >  
> > > Allow creating gpio-addr-flash via device-tree and not just via platform
> > > data.
> > >
> > > Mimic what physmap_of_versatile and physmap_of_gemini does to reduce
> > > code duplicity.
> > >
> > > Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@xxxxxxxxx>
> > > ---
> > >  drivers/mtd/maps/Kconfig           |  8 +++
> > >  drivers/mtd/maps/Makefile          |  3 +-
> > >  drivers/mtd/maps/gpio-addr-flash.c | 95 +++++++++++++++++++-----------
> > >  drivers/mtd/maps/gpio-addr-flash.h | 34 +++++++++++
> > >  drivers/mtd/maps/physmap_of_core.c |  5 ++
> > >  drivers/mtd/maps/physmap_of_gpio.c | 21 +++++++
> > >  6 files changed, 129 insertions(+), 37 deletions(-)
> > >  create mode 100644 drivers/mtd/maps/gpio-addr-flash.h
> > >  create mode 100644 drivers/mtd/maps/physmap_of_gpio.c
> > >
> > > diff --git a/drivers/mtd/maps/Kconfig b/drivers/mtd/maps/Kconfig
> > > index afb36bff13a7..427143d42168 100644
> > > --- a/drivers/mtd/maps/Kconfig
> > > +++ b/drivers/mtd/maps/Kconfig
> > > @@ -94,6 +94,14 @@ config MTD_PHYSMAP_OF_GEMINI
> > >         platforms, some detection and setting up parallel mode on the
> > >         external interface.
> > >
> > > +config MTD_PHYSMAP_OF_GPIO
> > > +     bool "GPIO-assisted OF-based physical memory map handling"
> > > +     depends on MTD_PHYSMAP_OF
> > > +     depends on MTD_GPIO_ADDR
> > > +     help
> > > +       This provides some extra DT physmap parsing for flashes that are
> > > +       partially physically addressed and assisted by GPIOs.
> > > +  
> >
> > Hm, so now we have the physmap_of driver which uses a function exposed
> > by the gpio-addr-flash module, but this module is also declaring a
> > platform_driver. It's probably working fine, but it's hard to follow.
> >
> > So, I decided to give it a try and started to rework a bit the physmap,
> > physmap_of and gpio-addr-flash drivers. Here is the result [1] (it's
> > only been compile tested). With this rework we now have a single
> > driver which supports DT and !DT init and can also use GPIOs to
> > extend the physical memory range in case it's not large enough to
> > address the whole memory dev.
> >
> > Let me know what you think of this approach.
> >  
> 
> The code is definitely easier to follow. But I believe you should
> rebase your changes on mines (1-9) and instead of 10 apply your
> patch-set.:
> - "mtd: maps: Merge gpio-addr-flash.c into physmap-core.c" includes
> all my changes and some of them are not obvious:( Port to gpiod,
> gpio_values, win_order, use new apis....)

True.

> - Also there would be 1 (or 2) fixes that should be included in stable
> that we lose using your approach "mtd: maps: gpio-addr-flash: Fix
> ioremapped size", "mtd: physmap_of: Release resources on error"

I'm not sure this is really useful to backport them to stable since
blackfin users never complained about it (plus, blackin arch is now
gone), but okay.

> - And last, but not least, I want some credit for all this work ;)
> After 10 iterations I guess that I deserve more than a Documentation
> change, specially when my code would be on the tree.

Fair enough.

> 
> My other concern is that maybe we are giving too much entity to
> gpio-addr by including it in the core. But if you are fine with it, I
> am fine with it.

Well, the gpio-addr stuff is just here to support platforms that do not
have enough ADDR lines (or iomem address space) to address the whole
device. I see it as an 'extension' of the physmap logic, which is why I
think it makes sense to have this logic in the physmap driver instead
of creating a completely new driver.

> 
> If you want i can do the rebase and start testing on my board. What do
> you think?

That'd be great!

Thanks,

Boris