Re: [RFC] misc/at24: add experimental OF support for the generic eeprom driver

Re: [RFC] misc/at24: add experimental OF support for the generic eeprom driver

[Grant Likely]

On Thu, Oct 8, 2009 at 9:10 AM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> On Thu, Oct 08, 2009 at 08:53:46AM -0600, Grant Likely wrote:
> [...]
>> Please don't.  It is such a small amount of code,
>
> It's *always* a small amound of code, at a start. Then we get
> floppy disk drivers and the tty layer. ;-)

Holy straw man argument Batman!

But the focus is still on creating pdata.  If a translator gets too
big, then sure, split it into a separate file.  Until then, there I
see no good reason to do so now.

>
> [...]
>> Driver writers shouldn't have to
>> write anything more than a tiny function to populate pdata from the
>> device tree.  Managing that pdata instance needs to be done with
>> common infrastructure (but I don't have a firm idea about how it
>> should look yet).  In the mean time I think Wolfram's approach has
>> lower impact.
>
> If I wasn't a PPC/OF guy to some degree, I'd hate PPC/OF people
> for bringing arch-specific details into a generic code... :-P

No, this goes beyond PPC/OF.  The real issue is that it is no longer a
safe assumption that pdata will be a static data structure in platform
code.  The number of possible data sources is going to get larger, not
smaller.  OF is just one.  UEFI is another.  Translating that data
into pdata will be the problem that comes up over and over again.
However, translation code is still driver specific, so it belongs with
the driver that it translates code for.

So, in my opinion, translation code must:
1. be *tiny* -- should be trivial to add to a driver without impacting
common code
2. live with the driver that it translates data for; ideally in the
same .c file for drivers that are small.

> No matter how small the OF code is, I believe we shouldn't put it
> into the generic code. Take a look at mmc_spi case again, it can be
> easily extended to any arch, because there is no arch-specific stuff,
> but a "get/put" pattern for platform data.

I'm not disagreeing with you that the arch specific stuff should be
logically separated from the generic code.  But I don't agree that it
belongs in a separate file.  And I also think that the mmc_spi
implementation uses too much code.  There must be a better way.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [RFC] misc/at24: add experimental OF support for the generic eeprom driver

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 1250 bytes --]

> No, this goes beyond PPC/OF.  The real issue is that it is no longer a
> safe assumption that pdata will be a static data structure in platform
> code.  The number of possible data sources is going to get larger, not
> smaller.  OF is just one.  UEFI is another.  Translating that data
> into pdata will be the problem that comes up over and over again.
> However, translation code is still driver specific, so it belongs with
> the driver that it translates code for.
> 
> So, in my opinion, translation code must:
> 1. be *tiny* -- should be trivial to add to a driver without impacting
> common code
> 2. live with the driver that it translates data for; ideally in the
> same .c file for drivers that are small.

I am with Grant on these points. It is more than just PPC.

> > No matter how small the OF code is, I believe we shouldn't put it
> > into the generic code. Take a look at mmc_spi case again, it can be
> > easily extended to any arch, because there is no arch-specific stuff,
> > but a "get/put" pattern for platform data.

Will check this tomorrow.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 150 bytes --]

_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [RFC] misc/at24: add experimental OF support for the generic eeprom driver

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 775 bytes --]

> Will check this tomorrow.

And while doing this and figuring the pro/cons of those methods, I stumbled over this commit:

	gpio: pca953x: Get platform_data from OpenFirmware
	(1965d30356c1c65660ba3330927671cfe81acdd5)

It looks to me that it missed all people involved in OF/DT-development and now we
have undocumented and IMO questionable properties in the kernel.

Conclusions I draw:

a) we better solve the pdata-problem rather sooner than later ;)
b) we need to spread the word about devicetree-discuss
c) more documentation may help, too

I know, 'send patches'...

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 150 bytes --]

_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [RFC] misc/at24: add experimental OF support for the generic eeprom driver

[Grant Likely]

On Thu, Oct 8, 2009 at 11:14 PM, Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
>> Will check this tomorrow.
>
> And while doing this and figuring the pro/cons of those methods, I stumbled over this commit:
>
>        gpio: pca953x: Get platform_data from OpenFirmware
>        (1965d30356c1c65660ba3330927671cfe81acdd5)
>
> It looks to me that it missed all people involved in OF/DT-development and now we
> have undocumented and IMO questionable properties in the kernel.

Hi Nate,

For your future reference, patches that look at the device tree should
also cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org so that new bindings can
be reviewed and common mistakes can be avoided.  It is expected that
new device tree bindings are accompanied with documentation describing
what the binding is for and how it should be used (see
Documentation/powerpc/dts-bindings).

I know this change is already in mainline, but can you please post the
device tree fragment that you're using to describe this chip?  I want
to make sure we don't get stuck with things in the kernel that will be
hard to maintain in the long term.

Thanks,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [RFC] misc/at24: add experimental OF support for the generic eeprom driver

[Nate Case]

On Thu, 2009-10-08 at 23:40 -0600, Grant Likely wrote:
> For your future reference, patches that look at the device tree should
> also cc: devicetree-discuss@lists.ozlabs.org so that new bindings can
> be reviewed and common mistakes can be avoided.  It is expected that
> new device tree bindings are accompanied with documentation describing
> what the binding is for and how it should be used (see
> Documentation/powerpc/dts-bindings).
> 
> I know this change is already in mainline, but can you please post the
> device tree fragment that you're using to describe this chip?  I want
> to make sure we don't get stuck with things in the kernel that will be
> hard to maintain in the long term.

Hi Grant,

Sorry for neglecting to include devicetree-discuss on that one.  I was
in fact aware of this list, and subscribe to it.  I really just forgot
in this case.

I also have a documentation patch for it that went along with it, but it
wasn't ready in time and so it's been sitting in our local patch queue.
I can submit that soon,  but it probably makes sense for Wolfram to
voice whatever his concerns were about "questionable" properties before
I document what's there.

Here's an example device tree node for this case:

               gpio1: gpio@18 {
                        compatible = "nxp,pca9557";
                        reg = <0x18>;
                        #gpio-cells = <2>;
                        gpio-controller;
                        polarity = <0x00>;
               };

In this case, the linux,gpio-base property wasn't specified.  But, the
use case is identical to the pdata->gpio_base field.  "polarity" is used
for specifying polarity inversion for each line, and is in the same
format of the 'polarity inversion' register on the chip.  My reasoning
in the property naming was as follows:

    linux,gpio-base: Linux-specific as it relates to internal GPIO
                     numbering.  So, it's prefixed with "linux,"
    polarity: Dictated by how hardware is wired up, so it's needed
              regardless of the OS.

- Nate

Re: [RFC] misc/at24: add experimental OF support for the generic eeprom driver

[Grant Likely]

On Fri, Oct 9, 2009 at 8:01 AM, Nate Case <ncase@xes-inc.com> wrote:
> On Thu, 2009-10-08 at 23:40 -0600, Grant Likely wrote:
>> For your future reference, patches that look at the device tree should
>> also cc: devicetree-discuss@lists.ozlabs.org so that new bindings can
>> be reviewed and common mistakes can be avoided.  It is expected that
>> new device tree bindings are accompanied with documentation describing
>> what the binding is for and how it should be used (see
>> Documentation/powerpc/dts-bindings).
>>
>> I know this change is already in mainline, but can you please post the
>> device tree fragment that you're using to describe this chip?  I want
>> to make sure we don't get stuck with things in the kernel that will be
>> hard to maintain in the long term.
>
> Hi Grant,
>
> Sorry for neglecting to include devicetree-discuss on that one.  I was
> in fact aware of this list, and subscribe to it.  I really just forgot
> in this case.

No worries, it happens.

> I also have a documentation patch for it that went along with it, but it
> wasn't ready in time and so it's been sitting in our local patch queue.
> I can submit that soon,  but it probably makes sense for Wolfram to
> voice whatever his concerns were about "questionable" properties before
> I document what's there.

Yes, please post it as soon as you can.

> Here's an example device tree node for this case:
>
>               gpio1: gpio@18 {
>                        compatible = "nxp,pca9557";
>                        reg = <0x18>;
>                        #gpio-cells = <2>;
>                        gpio-controller;
>                        polarity = <0x00>;
>               };
>
> In this case, the linux,gpio-base property wasn't specified.  But, the
> use case is identical to the pdata->gpio_base field.  "polarity" is used
> for specifying polarity inversion for each line, and is in the same
> format of the 'polarity inversion' register on the chip.  My reasoning
> in the property naming was as follows:
>
>    linux,gpio-base: Linux-specific as it relates to internal GPIO
>                     numbering.  So, it's prefixed with "linux,"

This would be the 'questionable' property.  :-)  The device tree is
supposed to be OS agnostic, so I get the heebee geebees when I see new
'linux,<blah>' properties defined because it means Linux internal
implementation details are getting leaked out into the data blob.  The
problem leakage is that the device tree should not be impacted by
internal Linux code changes.

In this particular case, specifying the exact GPIO base number doesn't
really belong in the device tree because Linux is able to assign and
manage GPIO numbers on its own just like all other OF GPIO controller
drivers currently do.  In fact, that goes for pretty much all
enumeration, not just GPIO.  In general, a particular device instance
(uart, gpio, phy, whatever) should be resolved from its node in the
device tree, and not by some arbitrary number assigned by the .dts
author.  The problem with arbitrary numbers is they don't expose the
linkage between nodes in the same way a 'phandle' does (A phandle can
be searched for.  An arbitrary number assignment, good luck?), and
they don't expose intended usage (its just a meaningless number, and
it only works because userspace just happens to 'agree' to use the
same number).

>    polarity: Dictated by how hardware is wired up, so it's needed
>              regardless of the OS.

Typically GPIO drivers have been handling this by using #gpio-cells
set to 2, and using the 2nd cell for flags.  The priority can be
encoded as a flag.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [RFC] misc/at24: add experimental OF support for the generic eeprom driver

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 873 bytes --]


> I can submit that soon,  but it probably makes sense for Wolfram to
> voice whatever his concerns were about "questionable" properties before
> I document what's there.

Please don't feel offended. The things I noticed are:

a) no documentation

b) 'polarity' is a direct mapping to the register which IMO is a hint to look
closer. I haven't checked in detil, but maybe the active_low-flag could be used
for this?

I mainly got alarmed that properties were mainlined without being reviewed; as
the device-tree is based on convention (which is hard to change afterwards), I
try to make sure this will not so easily happen again (thus the
get_maintainer-patch on lkml).

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 150 bytes --]

_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [RFC] misc/at24: add experimental OF support for the generic eeprom driver

[Nate Case]

On Fri, 2009-10-09 at 07:14 +0200, Wolfram Sang wrote:
> And while doing this and figuring the pro/cons of those methods, I
> stumbled over this commit:
> 
>         gpio: pca953x: Get platform_data from OpenFirmware
>         (1965d30356c1c65660ba3330927671cfe81acdd5)

Aside from any issues you have with the properties themselves, what's
your take on this approach?

Personally, I just got tired of waiting for someone else to solve the
pdata/OF problem.  So I submitted that commit as an attempt at something
very simple and unobtrusive to the device driver itself.  It seems
pretty clean to me, but I'm curious to see if others have any better
ideas.

- Nate

Re: [RFC] misc/at24: add experimental OF support for the generic eeprom driver

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 595 bytes --]

> Aside from any issues you have with the properties themselves, what's
> your take on this approach?

Well, my approach for AT24 looked very similar to your approach. In fact, even
the motivation was the same as yours :) Well, the outcome of this is the
current thread and no definite solution yet. The archdata surely helps for this
issue, it just seems that a bit more generalization is needed.

Kind regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 150 bytes --]

_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [RFC] misc/at24: add experimental OF support for the generic eeprom driver

[Grant Likely]

On Fri, Oct 9, 2009 at 7:43 AM, Nate Case <ncase@xes-inc.com> wrote:
> On Fri, 2009-10-09 at 07:14 +0200, Wolfram Sang wrote:
>> And while doing this and figuring the pro/cons of those methods, I
>> stumbled over this commit:
>>
>>         gpio: pca953x: Get platform_data from OpenFirmware
>>         (1965d30356c1c65660ba3330927671cfe81acdd5)
>
> Aside from any issues you have with the properties themselves, what's
> your take on this approach?

As I mentioned in an earlier email, I don't think quite the right form
has been found yet, but I like the direction things are moving.

> Personally, I just got tired of waiting for someone else to solve the
> pdata/OF problem.  So I submitted that commit as an attempt at something
> very simple and unobtrusive to the device driver itself.  It seems
> pretty clean to me, but I'm curious to see if others have any better
> ideas.

Yup, that's good.  Between Anton's, Wolfram's and your work things are
going the right way.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [RFC] misc/at24: add experimental OF support for the generic eeprom driver

[Anton Vorontsov]

On Thu, Oct 08, 2009 at 09:48:50AM -0600, Grant Likely wrote:
> On Thu, Oct 8, 2009 at 9:10 AM, Anton Vorontsov
[...]
> > It's *always* a small amound of code, at a start. Then we get
> > floppy disk drivers and the tty layer. ;-)
> 
> Holy straw man argument Batman!
> 
> But the focus is still on creating pdata.  If a translator gets too
> big, then sure, split it into a separate file.  Until then, there I
> see no good reason to do so now.

Luckily, I'm not at24 driver maintainer (Wolfram himself is ;-),
but as a maintainer of driver "Foo", I would not want to see
completely unfamiliar "Bar" in my shiny driver.

Another plus is that you can bypass (or almost bypass) subsystem
maintainers when merging OF-specific patches (since he/she couldn't
possibly care less about all these weird arch internals. But again,
this doesn't work for this particular driver since Wolfram is the
maintainer :-).

> > If I wasn't a PPC/OF guy to some degree, I'd hate PPC/OF people
> > for bringing arch-specific details into a generic code... :-P
> 
> No, this goes beyond PPC/OF.  The real issue is that it is no longer a
> safe assumption that pdata will be a static data structure in platform
> code.  The number of possible data sources is going to get larger, not
> smaller.  OF is just one.  UEFI is another.  Translating that data
> into pdata will be the problem that comes up over and over again.
> However, translation code is still driver specific,
> so it belongs with the driver that it translates code for.

Wait... The translation code depends on a platform, and on a
platform_data structure, the same as non-OF arch-specific code
depends on it. How is it different from a static platform data
in the arch/ code? We don't put static platform data into the
drivers and surround them with ugly #ifdefs+machine_is()...

> So, in my opinion, translation code must:
> 1. be *tiny*

Yeah, dream on. ;-) It's tiny when all you have is of_get_property(),
I'd like to see the code when you'll have GPIOs, IRQs, and platform-
specific fixups.

You might say that at24 doesn't need that stuff, but it does.
Suppose AT24's WP pin is connected to a GPIO, and without
'read-only' property I'd like the driver to pull the pin low,
and vice versa: with 'read-only' specifier, WP should be tied
high. Or if WP is controlled by a switch/jumper, GPIO can be
used to read current WP state.

> -- should be trivial to add to a driver without impacting
> common code

This is doable, yes.

> > No matter how small the OF code is, I believe we shouldn't put it
> > into the generic code. Take a look at mmc_spi case again, it can be
> > easily extended to any arch, because there is no arch-specific stuff,
> > but a "get/put" pattern for platform data.
> 
> I'm not disagreeing with you that the arch specific stuff should be
> logically separated from the generic code.  But I don't agree that it
> belongs in a separate file.  And I also think that the mmc_spi
> implementation uses too much code.  There must be a better way.

I wonder how you'd shrink the mmc_spi bindings, can you elaborate?

Thanks,

-- 
Anton Vorontsov
email: cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
irc://irc.freenode.net/bd2

Re: [RFC] misc/at24: add experimental OF support for the generic eeprom driver

[Grant Likely]

On Thu, Oct 8, 2009 at 4:20 PM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> On Thu, Oct 08, 2009 at 09:48:50AM -0600, Grant Likely wrote:
>> But the focus is still on creating pdata.  If a translator gets too
>> big, then sure, split it into a separate file.  Until then, there I
>> see no good reason to do so now.
>
> Luckily, I'm not at24 driver maintainer (Wolfram himself is ;-),
> but as a maintainer of driver "Foo", I would not want to see
> completely unfamiliar "Bar" in my shiny driver.

It sounds like your saying that data parsing isn't really the same as
driver code, and I don't think that is true.  Device data parsing is
equally as important as the functional behaviour.  A device driver
isn't complete unless it does both.  Right now most drivers only
understand LInux's internal representation (pdata) because that is the
only data source they've needed to this point.  When new data sources
appear (device tree), it is completely appropriate for the driver to
be modified to understand the new data format (with all the caveats of
coding it in a logical way with translation decoupled from function to
keep impact at a bare minimum).

To use your example, a driver author who states "I only use Bar; so I
don't ever want to see Foo code" is probably being a bit short sighted
with regards to portability.

> Another plus is that you can bypass (or almost bypass) subsystem
> maintainers when merging OF-specific patches (since he/she couldn't
> possibly care less about all these weird arch internals. But again,
> this doesn't work for this particular driver since Wolfram is the
> maintainer :-).

I don't want OF parsing to bypass subsystem or driver maintainers.
:-)  I think they should be involved in reviewing and acking
translator code.

>> > If I wasn't a PPC/OF guy to some degree, I'd hate PPC/OF people
>> > for bringing arch-specific details into a generic code... :-P
>>
>> No, this goes beyond PPC/OF.  The real issue is that it is no longer a
>> safe assumption that pdata will be a static data structure in platform
>> code.  The number of possible data sources is going to get larger, not
>> smaller.  OF is just one.  UEFI is another.  Translating that data
>> into pdata will be the problem that comes up over and over again.
>> However, translation code is still driver specific,
>> so it belongs with the driver that it translates code for.
>
> Wait... The translation code depends on a platform, and on a
> platform_data structure, the same as non-OF arch-specific code
> depends on it.

The translation code depends on the data source.  That may be OF.  It
may be UEFI.  It may be another driver (think a PCI driver
instantiating a set of child platform devices).  It may be a kernel
hacker (who hard codes it into the platform code).  It has nothing to
do with arch/.  (and ignore the whole of_platform bus stuff; that was
a bad idea from the outset (not that we knew that at the time).  I
don't intend to port of_platform to other architectures).

> How is it different from a static platform data
> in the arch/ code? We don't put static platform data into the
> drivers and surround them with ugly #ifdefs+machine_is()...

Of course we don't put static platform data into the drivers; because
static platform data is platform specific, and therefore belongs with
the platform.  An OF translator is different from a static pdata
because it is driver specific code instead of platform specific code.
Platform specific code is only applicable for the platform, so of
course it belongs with the platform code.  Driver specific code
belongs with the driver because it isn't applicable to anything else.
The whole point of the device tree is that it allows driver specific
code to be written that understands the data describing the device
instead of having a programmer hard code it.

>
>> So, in my opinion, translation code must:
>> 1. be *tiny*
>
> Yeah, dream on. ;-) It's tiny when all you have is of_get_property(),
> I'd like to see the code when you'll have GPIOs, IRQs, and platform-
> specific fixups.

It's still pretty tiny, because it still needs to populate a pdata
structure.  99% of the time there won't be any platform specific
fixups either.... In the odd case when there *are* platform specific
hacks, then of course the pdata should be created by platform code,
and not driver code.  One way to do this is to have platform code hook
into the notifier call chain for a bus and watch for devices it needs
to meddle with.  When one shows up, register the custom pdata before
the driver gets probed.  But that is the special case, which doesn't
need to impact the common case.

> You might say that at24 doesn't need that stuff, but it does.
> Suppose AT24's WP pin is connected to a GPIO, and without
> 'read-only' property I'd like the driver to pull the pin low,
> and vice versa: with 'read-only' specifier, WP should be tied
> high. Or if WP is controlled by a switch/jumper, GPIO can be
> used to read current WP state.

I still don't see a problem.  If it can be described in pdata, then a
translator function can populate it from the device tree data.  It
still isn't huge.  Besides, just because it *might* someday become
huge doesn't mean that it should be segregated into another file now.
It can always be moved later if it becomes too big.

>> -- should be trivial to add to a driver without impacting
>> common code
>
> This is doable, yes.
>
>> > No matter how small the OF code is, I believe we shouldn't put it
>> > into the generic code. Take a look at mmc_spi case again, it can be
>> > easily extended to any arch, because there is no arch-specific stuff,
>> > but a "get/put" pattern for platform data.
>>
>> I'm not disagreeing with you that the arch specific stuff should be
>> logically separated from the generic code.  But I don't agree that it
>> belongs in a separate file.  And I also think that the mmc_spi
>> implementation uses too much code.  There must be a better way.
>
> I wonder how you'd shrink the mmc_spi bindings, can you elaborate?

To start; eliminate all the pdata management code and write some
library routines to do that instead.  Next, I'd refactor the code to
separate out the GPIO handling stuff because the GPIO handling really
isn't related to OF at all (that code could just as easily be used by
a static pdata structure definition).  All that should be left is the
meat of the mmc_spi_get_pdata() function which parses the device tree
and populates pdata.

I agree that the infrastructure to do what I'm suggesting doesn't
exist yet; but I say this because I think it is the direction that
device tree support needs to go.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.