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

On Thu, Oct 8, 2009 at 8:33 AM, Anton Vorontsov
[off-list ref] wrote:

On Thu, Oct 08, 2009 at 04:04:32PM +0200, Wolfram Sang wrote:

quoted

As Anton introduced archdata support, I wondered if this is a suitable w=

ay to

quoted

handle the platform_data/devicetree_property-dualism (at least for some
drivers).

Yes, we handle OF in a similar way for mmc_spi driver. Though,

[...]

quoted

--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c

@@ -22,6 +22,9 @@

[...]

quoted

+#ifdef CONFIG_OF_I2C
+#include <linux/of.h>
+#endif

[..]

quoted

+#ifdef CONFIG_OF_I2C
+static void at24_get_ofdata(struct i2c_client *client, struct at24_plat=

form_data *chip)

quoted

+{
+ =A0 =A0 const u32 *val;
+ =A0 =A0 struct device_node *node =3D dev_archdata_get_node(&client->de=

v.archdata);

quoted

+
+ =A0 =A0 if (node) {
+ =A0 =A0 =A0 =A0 =A0 =A0 if (of_get_property(node, "read-only", NULL))
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 chip->flags |=3D AT24_FLAG_REA=

DONLY;

quoted

+ =A0 =A0 =A0 =A0 =A0 =A0 val =3D of_get_property(node, "pagesize", NULL=

);

quoted

+ =A0 =A0 =A0 =A0 =A0 =A0 if (val)
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 chip->page_size =3D *val;
+ =A0 =A0 }
+}
+#else
+static void at24_get_ofdata(struct i2c_client *client, struct at24_plat=

form_data *chip)

quoted

+{ }
+#endif

#ifdefs are ugly in .c files. I'd suggest to move the OF code
into a separate file. As an example, see

Please don't.  It is such a small amount of code, and I far prefer to
see drivers self contained in a single .c file.  #ifdefs are fine IMHO
when it is a top level block, and not inside a function block.  In the
example you give, I do like the move toward focusing on the pdata
structure; but the patch ads a *lot* of code for something very
simple.  And then we'll need to do the same think for every driver
which will ever be described in the device tree.  It's the right
direction, but still not right.  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.

g.

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

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. ;-)

[...]

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 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.

Thanks,

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

On Thu, Oct 8, 2009 at 9:10 AM, Anton Vorontsov
[off-list ref] wrote:

On Thu, Oct 08, 2009 at 08:53:46AM -0600, Grant Likely wrote:
[...]

quoted

Please don't. =A0It 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.

[...]

quoted

Driver writers shouldn't have to
write anything more than a tiny function to populate pdata from the
device tree. =A0Managing that pdata instance needs to be done with
common infrastructure (but I don't have a firm idea about how it
should look yet). =A0In 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.

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

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.

quoted

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/  |

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/  |

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

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/  |

On Fri, Oct 9, 2009 at 7:43 AM, Nate Case [off-list ref] wrote:

On Fri, 2009-10-09 at 07:14 +0200, Wolfram Sang wrote:

quoted

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

=A0 =A0 =A0 =A0 gpio: pca953x: Get platform_data from OpenFirmware
=A0 =A0 =A0 =A0 (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. =A0So I submitted that commit as an attempt at somethin=

g

very simple and unobtrusive to the device driver itself. =A0It 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.

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