Re: [spi-devel-general] [PATCH v2 3/6] mtd: m25p80: add support to parse the SPI flash's partitions

Re: [spi-devel-general] [PATCH v2 3/6] mtd: m25p80: add support to parse the SPI flash's partitions

[Grant Likely]

On Tue, Aug 10, 2010 at 2:29 AM, Joakim Tjernlund
<joakim.tjernlund@transmode.se> wrote:
>> On Tue, Aug 10, 2010 at 1:14 AM, David Brownell <david-b@pacbell.net> wrote:
>> >
>> >
>> > --- On Mon, 8/9/10, Grant Likely <grant.likely@secretlab.ca> wrote:
>> >
>> >
>> >> > +       nr_parts =
>> >> of_mtd_parse_partitions(&spi->dev, np, &parts);
>> >
>> > Let's keep OF-specific logic out of drivers like
>> > this one ...  intended to work without OF.
>> >
>> > NAK on adding dependencies like OF to drivers
>> > and other infrastructure that starts generic.
>
> Agreed.
>
>>
>> The OF hooks compile to no-ops when CONFIG_OF is disabled.  I've been
>> very careful about that.
>
> OF should not be in the drivers, one should be able use some other method
> than OF.

If a device is described in the device tree, then the code has to live
*somewhere* to translate the data from the device tree into a form the
driver can use.  If not the driver, then where should that code live?

If it is in the machine support code, then that requires foreknowledge
about all the device specific data that needs to be translated out of
the device tree at device registration time, which also means that the
translation code cannot be a module and it nullifies some of the
advantages of the device tree.  Not to mention the fact that it is
just plain ugly!  :-)

Some of it can go into the infrastructure code, but only for parsing
common properties like irqs, address ranges, and some common bindings
like registering spi and i2c bus child nodes as devices.  (This
*particular* case may fall into this category if add_mtd_device() was
able to call the OF partition parsing hook if a device node pointer
was present; which does make a certain amount of sense, but I defer to
dwmw2 in this case).  However, device-specific properties cannot be
parsed in the infrastructure code because the infrastructure has no
knowledge of device specific bits.

The suggestion has been raised to use something like bus notifiers to
get a hook onto the device registration before the driver is probed so
that the platform_data can be translated in an separate chuck of code,
but down that path lies insanity.  There are all kinds of ordering
issues (like making sure the translation code is called before the
driver probe code), and it is a lot of complexity for what is really a
simple thing.

I've yet to be convinced that device-specific OF calls belongs
anywhere in the kernel other than in the driver that uses the data.

That being said, I completely agree that OF calls do not belong in the
vast majority of the driver code.  Using OF calls after the driver is
bound to the device is generally wrong.  However, they make perfect
sense in the probe hook who's whole purpose is to obtain data about
the device and register it.  The model is actually very simple:

if (dev->platform_data)
      get_platform_data();
else if (dev->of_node)
      get_of_data();

For the actual implementation in this case, my comments reflect what I
think is the clearest implementation.  However, if you'd prefer a
single function call at the start of the probe() hook to return a
flash_partition_data pointer, then I'm okay with that too.

Cheers,
g.

Re: [spi-devel-general] [PATCH v2 3/6] mtd: m25p80: add support to parse the SPI flash's partitions

[Anton Vorontsov]

On Tue, Aug 10, 2010 at 08:56:42AM -0600, Grant Likely wrote:
[...] 
> The suggestion has been raised to use something like bus notifiers to
> get a hook onto the device registration before the driver is probed so
> that the platform_data can be translated in an separate chuck of code,
> but down that path lies insanity.  There are all kinds of ordering
> issues (like making sure the translation code is called before the
> driver probe code), and it is a lot of complexity for what is really a
> simple thing.

There's another option: platform data handlers (they could be
chained, in case if we want to use several methods of obtaining
platform data, i.e. "try raw data, then ACPI, then OF").

See include/linux/spi/mmc_spi.h (mmc_spi_get_pdata)
and drivers/mmc/host/of_mmc_spi.c.

I think we might implement some library to make these handlers
more generic, but even the simple implementation as in mmc_spi
case proves that it is possible to separate OF/ACPI/DMI/whatever
handling from the generic code.

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [spi-devel-general] [PATCH v2 3/6] mtd: m25p80: add support to parse the SPI flash's partitions

[Joakim Tjernlund]

glikely@secretlab.ca wrote on 2010/08/10 16:56:42:
>
> On Tue, Aug 10, 2010 at 2:29 AM, Joakim Tjernlund
> <joakim.tjernlund@transmode.se> wrote:
> >> On Tue, Aug 10, 2010 at 1:14 AM, David Brownell <david-b@pacbell.net> wrote:
> >> >
> >> >
> >> > --- On Mon, 8/9/10, Grant Likely <grant.likely@secretlab.ca> wrote:
> >> >
> >> >
> >> >> > +       nr_parts =
> >> >> of_mtd_parse_partitions(&spi->dev, np, &parts);
> >> >
> >> > Let's keep OF-specific logic out of drivers like
> >> > this one ...  intended to work without OF.
> >> >
> >> > NAK on adding dependencies like OF to drivers
> >> > and other infrastructure that starts generic.
> >
> > Agreed.
> >
> >>
> >> The OF hooks compile to no-ops when CONFIG_OF is disabled.  I've been
> >> very careful about that.
> >
> > OF should not be in the drivers, one should be able use some other method
> > than OF.
>
> If a device is described in the device tree, then the code has to live
> *somewhere* to translate the data from the device tree into a form the
> driver can use.  If not the driver, then where should that code live?
>
> If it is in the machine support code, then that requires foreknowledge
> about all the device specific data that needs to be translated out of
> the device tree at device registration time, which also means that the
> translation code cannot be a module and it nullifies some of the
> advantages of the device tree.  Not to mention the fact that it is
> just plain ugly!  :-)
>
> Some of it can go into the infrastructure code, but only for parsing
> common properties like irqs, address ranges, and some common bindings
> like registering spi and i2c bus child nodes as devices.  (This
> *particular* case may fall into this category if add_mtd_device() was
> able to call the OF partition parsing hook if a device node pointer
> was present; which does make a certain amount of sense, but I defer to
> dwmw2 in this case).  However, device-specific properties cannot be
> parsed in the infrastructure code because the infrastructure has no
> knowledge of device specific bits.

I am no expert at this at all, but I can give you an example what I don't
want. Look at spi_mpc8xxx.c, earlier it was possible to define your own
CS functions and register them from board code. After OF:ication one can
not and the current OF methods aren't expressive enough to do what I need.
The spi subsystem has a general framework for dealing with SPI devices
and I think OF should be built on top of the native methods, at the
very least not disable those methods like spi_mpc8xxx.c does.

I will shut up now :)

  Regards
         Jocke

Re: [spi-devel-general] [PATCH v2 3/6] mtd: m25p80: add support to parse the SPI flash's partitions

[Grant Likely]

On Tue, Aug 10, 2010 at 11:23 AM, Joakim Tjernlund
<joakim.tjernlund@transmode.se> wrote:
> glikely@secretlab.ca wrote on 2010/08/10 16:56:42:
>>
>> On Tue, Aug 10, 2010 at 2:29 AM, Joakim Tjernlund
>> <joakim.tjernlund@transmode.se> wrote:
>> >> On Tue, Aug 10, 2010 at 1:14 AM, David Brownell <david-b@pacbell.net> wrote:
>> >> >
>> >> >
>> >> > --- On Mon, 8/9/10, Grant Likely <grant.likely@secretlab.ca> wrote:
>> >> >
>> >> >
>> >> >> > +       nr_parts =
>> >> >> of_mtd_parse_partitions(&spi->dev, np, &parts);
>> >> >
>> >> > Let's keep OF-specific logic out of drivers like
>> >> > this one ...  intended to work without OF.
>> >> >
>> >> > NAK on adding dependencies like OF to drivers
>> >> > and other infrastructure that starts generic.
>> >
>> > Agreed.
>> >
>> >>
>> >> The OF hooks compile to no-ops when CONFIG_OF is disabled.  I've been
>> >> very careful about that.
>> >
>> > OF should not be in the drivers, one should be able use some other method
>> > than OF.
>>
>> If a device is described in the device tree, then the code has to live
>> *somewhere* to translate the data from the device tree into a form the
>> driver can use.  If not the driver, then where should that code live?
>>
>> If it is in the machine support code, then that requires foreknowledge
>> about all the device specific data that needs to be translated out of
>> the device tree at device registration time, which also means that the
>> translation code cannot be a module and it nullifies some of the
>> advantages of the device tree.  Not to mention the fact that it is
>> just plain ugly!  :-)
>>
>> Some of it can go into the infrastructure code, but only for parsing
>> common properties like irqs, address ranges, and some common bindings
>> like registering spi and i2c bus child nodes as devices.  (This
>> *particular* case may fall into this category if add_mtd_device() was
>> able to call the OF partition parsing hook if a device node pointer
>> was present; which does make a certain amount of sense, but I defer to
>> dwmw2 in this case).  However, device-specific properties cannot be
>> parsed in the infrastructure code because the infrastructure has no
>> knowledge of device specific bits.
>
> I am no expert at this at all, but I can give you an example what I don't
> want. Look at spi_mpc8xxx.c, earlier it was possible to define your own
> CS functions and register them from board code. After OF:ication one can
> not and the current OF methods aren't expressive enough to do what I need.
> The spi subsystem has a general framework for dealing with SPI devices
> and I think OF should be built on top of the native methods, at the
> very least not disable those methods like spi_mpc8xxx.c does.

You're right and it's a problem.  There wasn't a good way to handle
this when powerpc first moved over to use OF.  I think we've got a
good solution now.  See the use of a bus notifier in
arch/powerpc/platforms/512x/pdm360ng.c in Linus' current tree.  Most
of the time that won't be needed, but when a platform specific
callback is required, this seems to be a clean solution.

g.

RE: [spi-devel-general] [PATCH v2 3/6] mtd: m25p80: add support to parse the SPI flash's partitions

[Hu Mingkai-B21284]

> -----Original Message-----
> From: glikely@secretlab.ca [mailto:glikely@secretlab.ca] On Behalf Of Grant
> Likely
> Sent: Wednesday, August 11, 2010 2:27 AM
> To: Joakim Tjernlund
> Cc: David Brownell; David Woodhouse; Gala Kumar-B11780; linux-
> mtd@lists.infradead.org; Hu Mingkai-B21284; spi-devel-
> general@lists.sourceforge.net
> Subject: Re: [spi-devel-general] [PATCH v2 3/6] mtd: m25p80: add support to
> parse the SPI flash's partitions
> 
> On Tue, Aug 10, 2010 at 11:23 AM, Joakim Tjernlund
> <joakim.tjernlund@transmode.se> wrote:
> > glikely@secretlab.ca wrote on 2010/08/10 16:56:42:
> >>
> >> On Tue, Aug 10, 2010 at 2:29 AM, Joakim Tjernlund
> >> <joakim.tjernlund@transmode.se> wrote:
> >> >> On Tue, Aug 10, 2010 at 1:14 AM, David Brownell <david-b@pacbell.net>
> wrote:
> >> >> >
> >> >> >
> >> >> > --- On Mon, 8/9/10, Grant Likely <grant.likely@secretlab.ca> wrote:
> >> >> >
> >> >> >
> >> >> >> > +       nr_parts =
> >> >> >> of_mtd_parse_partitions(&spi->dev, np, &parts);
> >> >> >
> >> >> > Let's keep OF-specific logic out of drivers like this one ...
> >> >> > intended to work without OF.
> >> >> >
> >> >> > NAK on adding dependencies like OF to drivers and other
> >> >> > infrastructure that starts generic.
> >> >
> >> > Agreed.
> >> >
> >> >>
> >> >> The OF hooks compile to no-ops when CONFIG_OF is disabled.  I've
> >> >> been very careful about that.
> >> >
> >> > OF should not be in the drivers, one should be able use some other
> >> > method than OF.
> >>
> >> If a device is described in the device tree, then the code has to
> >> live
> >> *somewhere* to translate the data from the device tree into a form
> >> the driver can use.  If not the driver, then where should that code live?
> >>
> >> If it is in the machine support code, then that requires
> >> foreknowledge about all the device specific data that needs to be
> >> translated out of the device tree at device registration time, which
> >> also means that the translation code cannot be a module and it
> >> nullifies some of the advantages of the device tree.  Not to mention
> >> the fact that it is just plain ugly!  :-)
> >>
> >> Some of it can go into the infrastructure code, but only for parsing
> >> common properties like irqs, address ranges, and some common bindings
> >> like registering spi and i2c bus child nodes as devices.  (This
> >> *particular* case may fall into this category if add_mtd_device() was
> >> able to call the OF partition parsing hook if a device node pointer
> >> was present; which does make a certain amount of sense, but I defer
> >> to
> >> dwmw2 in this case).  However, device-specific properties cannot be
> >> parsed in the infrastructure code because the infrastructure has no
> >> knowledge of device specific bits.
> >
> > I am no expert at this at all, but I can give you an example what I
> > don't want. Look at spi_mpc8xxx.c, earlier it was possible to define
> > your own CS functions and register them from board code. After
> > OF:ication one can not and the current OF methods aren't expressive enough to
> do what I need.
> > The spi subsystem has a general framework for dealing with SPI devices
> > and I think OF should be built on top of the native methods, at the
> > very least not disable those methods like spi_mpc8xxx.c does.
> 
> You're right and it's a problem.  There wasn't a good way to handle this when
> powerpc first moved over to use OF.  I think we've got a good solution now.  See
> the use of a bus notifier in arch/powerpc/platforms/512x/pdm360ng.c in Linus'
> current tree.  Most of the time that won't be needed, but when a platform
> specific callback is required, this seems to be a clean solution.
> 

Thanks for your valuable suggestion, but where should I put the bus_register_notifier?
On the drivers/mtd/devices/m25p80.c or drivers/spi/spi.c? I think it should add in the 
flash driver m25p80.c, but it looks like the notifier function will be called many times.
How does it happen?

Here is my demo code patch:
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -753,6 +753,34 @@ static const struct spi_device_id *__devinit jedec_probe(struct spi_device *spi)
 }

+static int m25p80_notifier_call(struct notifier_block *nb,
+                                       unsigned long event, void *__dev)
+{
+       printk("%s %d event = 0x%x:\n", __func__, __LINE__, event);
+       switch (event) {
+       case BUS_NOTIFY_ADD_DEVICE:
+               printk("BUS_NOTIFY_ADD_DEVICE\n");
+               break;
+       case BUS_NOTIFY_DEL_DEVICE:
+               printk("BUS_NOTIFY_DEL_DEVICE\n");
+               break;
+       case BUS_NOTIFY_BOUND_DRIVER:
+               printk("BUS_NOTIFY_BOUND_DRIVER\n");
+               break;
+       case BUS_NOTIFY_UNBIND_DRIVER:
+               printk("BUS_NOTIFY_UNBIND_DRIVER\n");
+               break;
+       case BUS_NOTIFY_UNBOUND_DRIVER:
+               printk("BUS_NOTIFY_UNBOUND_DRIVER\n");
+               break;
+       }
+       return NOTIFY_DONE;
+}
+
+static struct notifier_block m25p80_nb = {
+       .notifier_call = m25p80_notifier_call,
+};
+
 /*
  * board specific setup should have ensured the SPI clock used here
  * matches what the READ command supports, at least until this driver
@@ -766,6 +794,8 @@ static int __devinit m25p_probe(struct spi_device *spi)
        struct flash_info               *info;
        unsigned                        i;

+       bus_register_notifier(&spi_bus_type, &m25p80_nb);
+


Thanks,
Mingkai

RE: [spi-devel-general] [PATCH v2 3/6] mtd: m25p80: add support to parse the SPI flash's partitions

[Hu Mingkai-B21284]

> -----Original Message-----
> From: Anton Vorontsov [mailto:cbouatmailru@gmail.com]
> Sent: Wednesday, August 11, 2010 12:02 AM
> To: Grant Likely
> Cc: Joakim Tjernlund; Gala Kumar-B11780; David Brownell; linux-
> mtd@lists.infradead.org; spi-devel-general@lists.sourceforge.net; Hu Mingkai-
> B21284; David Woodhouse
> Subject: Re: [spi-devel-general] [PATCH v2 3/6] mtd: m25p80: add support to
> parse the SPI flash's partitions
> 
> On Tue, Aug 10, 2010 at 08:56:42AM -0600, Grant Likely wrote:
> [...]
> > The suggestion has been raised to use something like bus notifiers to
> > get a hook onto the device registration before the driver is probed so
> > that the platform_data can be translated in an separate chuck of code,
> > but down that path lies insanity.  There are all kinds of ordering
> > issues (like making sure the translation code is called before the
> > driver probe code), and it is a lot of complexity for what is really a
> > simple thing.
> 
> There's another option: platform data handlers (they could be chained, in case
> if we want to use several methods of obtaining platform data, i.e. "try raw data,
> then ACPI, then OF").
> 
> See include/linux/spi/mmc_spi.h (mmc_spi_get_pdata) and
> drivers/mmc/host/of_mmc_spi.c.
> 
> I think we might implement some library to make these handlers more generic, but
> even the simple implementation as in mmc_spi case proves that it is possible to
> separate OF/ACPI/DMI/whatever handling from the generic code.
> 

It's also a good suggestion, I'll try it.

Thanks,
Mingkai