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

[Boris Brezillon]

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

> On Fri, Oct 5, 2018 at 9:08 AM Boris Brezillon
> <boris.brezillon@xxxxxxxxxxx> wrote:
> >
> > 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!  
> 
> Can you start by picking 1-8 from my patchset so i do not have to
> resend it again and again while we work on your changes?

Done.