[Patch] MPC Adapter: read class attribute from device tree

[Patch] MPC Adapter: read class attribute from device tree

[Michael Lawnick]

For MPC adapter there is no class assigned as it is done in other
adapters. This way no new-style client will ever be instantiated. With
this patch class assignment is read from device tree.
Necessary entry:
class = <1>; /* I2C_CLASS_HWMON (iic.h) */

Based on 2.6.29

Signed-off-by: Michael Lawnick <ml.lawnick-Mmb7MZpHnFY@public.gmane.org>
Cc: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
Cc: Sang, Wolfram <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---
 drivers/i2c/busses/i2c-mpc.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -318,6 +318,7 @@ static int __devinit fsl_i2c_probe(struct of_device
*op, const struct of_device_
 {
 	int result = 0;
 	struct mpc_i2c *i2c;
+	int *of_val;

 	i2c = kzalloc(sizeof(*i2c), GFP_KERNEL);
 	if (!i2c)
@@ -354,6 +355,10 @@ static int __devinit fsl_i2c_probe(struct of_device
*op, const struct of_device_
 	dev_set_drvdata(&op->dev, i2c);

 	i2c->adap = mpc_ops;
+	of_val = of_get_property(op->node, "class", NULL);
+	if(of_val)
+		i2c->adap.class = *of_val;
+
 	i2c_set_adapdata(&i2c->adap, i2c);
 	i2c->adap.dev.parent = &op->dev;

-- 
Michael Lawnick

Re: [Patch] MPC Adapter: read class attribute from device tree

[Wolfgang Grandegger]

Hello,

sorry for jumping in late. I just recently subscribed to this list.

Michael Lawnick wrote:
> For MPC adapter there is no class assigned as it is done in other
> adapters. This way no new-style client will ever be instantiated. With
> this patch class assignment is read from device tree.
> Necessary entry:
> class = <1>; /* I2C_CLASS_HWMON (iic.h) */

Do we really need that? Probing is dangerous and not necessary. Does it
not work with a proper DTS entry? But maybe I have missed something?

> Based on 2.6.29
> 
> Signed-off-by: Michael Lawnick <ml.lawnick-Mmb7MZpHnFY@public.gmane.org>
> Cc: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> Cc: Sang, Wolfram <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-mpc.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
> --- a/drivers/i2c/busses/i2c-mpc.c
> +++ b/drivers/i2c/busses/i2c-mpc.c
> @@ -318,6 +318,7 @@ static int __devinit fsl_i2c_probe(struct of_device
> *op, const struct of_device_
>  {
>  	int result = 0;
>  	struct mpc_i2c *i2c;
> +	int *of_val;
> 
>  	i2c = kzalloc(sizeof(*i2c), GFP_KERNEL);
>  	if (!i2c)
> @@ -354,6 +355,10 @@ static int __devinit fsl_i2c_probe(struct of_device
> *op, const struct of_device_
>  	dev_set_drvdata(&op->dev, i2c);
> 
>  	i2c->adap = mpc_ops;
> +	of_val = of_get_property(op->node, "class", NULL);
> +	if(of_val)
> +		i2c->adap.class = *of_val;
> +
>  	i2c_set_adapdata(&i2c->adap, i2c);
>  	i2c->adap.dev.parent = &op->dev;

The CPM I2C bus driver uses "linux,i2c-class" for that purpose. See:

http://lxr.linux.no/linux+v2.6.29/Documentation/powerpc/dts-bindings/fsl/cpm_qe/cpm/i2c.txt

A consistent binding would be nice and the default should be "0", if we
really need legacy probing.

Furthermore, the new binding needs to be documented and published on the
Devicetree-discuss ML.

Wolfgang.

Re: [Patch] MPC Adapter: read class attribute from device tree

[Michael Lawnick]

Wolfgang Grandegger said the following:
> Hello,
> 
> sorry for jumping in late. I just recently subscribed to this list.
> 
> Michael Lawnick wrote:
>> For MPC adapter there is no class assigned as it is done in other
>> adapters. This way no new-style client will ever be instantiated. With
>> this patch class assignment is read from device tree.
>> Necessary entry:
>> class = <1>; /* I2C_CLASS_HWMON (iic.h) */
> 
> Do we really need that? Probing is dangerous and not necessary. Does it
> not work with a proper DTS entry? But maybe I have missed something?

Our current and our next system consists of two flavors of the same
board with different devices. To minimize maintenance and testing we use
a reduced kernel and load the needed modules at runtime. Furthermore we
will have to handle hot-plugged I2C-devices. Whether this strategy is
best could be discussed in another thread but is rather OT in this
mailing list. Nevertheless loading modules at runtime is legal and
generally supported by LINUX.

Defining all possible (I2C-)devices in DTS would give a mess. E.g. on
one board there will be ~30 temperature sensors, on the other none.
As every DTS entry will force a sysFs subdirectory there would be a
bunch of functionless directories - rather ugly.

If probing of a device is dangerous, it can be defined in DTS anyway and
the device driver can easily omit this part by early return.

Because of the situation above I try to keep the ability of dynamic
instantiation. Jean hesitates, I feel because he sees I2C solely in
static manner.

> 
>> Based on 2.6.29
>> 
>> Signed-off-by: Michael Lawnick <ml.lawnick-Mmb7MZpHnFY@public.gmane.org>
>> Cc: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
>> Cc: Sang, Wolfram <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>> ---
>>  drivers/i2c/busses/i2c-mpc.c |    5 +++++
>>  1 files changed, 5 insertions(+), 0 deletions(-)
>> 
>> diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
>> --- a/drivers/i2c/busses/i2c-mpc.c
>> +++ b/drivers/i2c/busses/i2c-mpc.c
>> @@ -318,6 +318,7 @@ static int __devinit fsl_i2c_probe(struct of_device
>> *op, const struct of_device_
>>  {
>>  	int result = 0;
>>  	struct mpc_i2c *i2c;
>> +	int *of_val;
>> 
>>  	i2c = kzalloc(sizeof(*i2c), GFP_KERNEL);
>>  	if (!i2c)
>> @@ -354,6 +355,10 @@ static int __devinit fsl_i2c_probe(struct of_device
>> *op, const struct of_device_
>>  	dev_set_drvdata(&op->dev, i2c);
>> 
>>  	i2c->adap = mpc_ops;
>> +	of_val = of_get_property(op->node, "class", NULL);
>> +	if(of_val)
>> +		i2c->adap.class = *of_val;
>> +
>>  	i2c_set_adapdata(&i2c->adap, i2c);
>>  	i2c->adap.dev.parent = &op->dev;
> 
> The CPM I2C bus driver uses "linux,i2c-class" for that purpose. See:
> 
> http://lxr.linux.no/linux+v2.6.29/Documentation/powerpc/dts-bindings/fsl/cpm_qe/cpm/i2c.txt

I love this distributed information :-( - I will fix.
> 
> A consistent binding would be nice and the default should be "0", if we
> really need legacy probing.

If linux,i2c-class is not set, 0 will be used.
> 
> Furthermore, the new binding needs to be documented and published on the
> Devicetree-discuss ML.
> 
Hmm, how are they involved if linux,i2c-class is used - it's already
defined as you showed above.

-- 
Kind regards,
Michael

Re: [Patch] MPC Adapter: read class attribute from device tree

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1660 bytes --]


> mailing list. Nevertheless loading modules at runtime is legal and
> generally supported by LINUX.

Ack. And it does so...

> Defining all possible (I2C-)devices in DTS would give a mess. E.g. on
> one board there will be ~30 temperature sensors, on the other none.
> As every DTS entry will force a sysFs subdirectory there would be a
> bunch of functionless directories - rather ugly.

Well, you should have a DTS for every flavour; that is what DTS are
about. You do know that, do you?

Even if not, there would be a slighty messed directory on one specific
device with how many units? Let's take the latest example of extending
the DTS file with OS-specific information - this one is ugly, too, but
has impact _for everyone using dts_ files. Think about the number of
affected systems and you will hopefully understand why such patches are
handled with extreme caution.

> Because of the situation above I try to keep the ability of dynamic
> instantiation. Jean hesitates, I feel because he sees I2C solely in
> static manner.

He is taking care for every I2C communication on every Linux machine out
there. He can't please everyone. On the other hand, you are free to
modify the kernel as you please. It doesn't really have to be in
mainline, does it?

BTW, I hope you saw that the "linux,i2c-class" property is considered
deprecated. Thus, expect a patch from me removing it entirely as soon
as the old binding model went away (there aren't even any users).

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

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

Re: [Patch] MPC Adapter: read class attribute from device tree

[Michael Lawnick]

Wolfram Sang said the following:
>> mailing list. Nevertheless loading modules at runtime is legal and
>> generally supported by LINUX.
> 
> Ack. And it does so...
> 
>> Defining all possible (I2C-)devices in DTS would give a mess. E.g. on
>> one board there will be ~30 temperature sensors, on the other none.
>> As every DTS entry will force a sysFs subdirectory there would be a
>> bunch of functionless directories - rather ugly.
> 
> Well, you should have a DTS for every flavour; that is what DTS are
> about. You do know that, do you?

You like it more complicated? Ok, here it is:
As Jean indicated in his reply, we will have to support hotplug. Most of
the above sensors won't exist a life time on some systems, on others all
could exist. I tried to keep out this issue to avoid a drift to the
discussion that hotplug is not _yet_ supported. I will^Wmust support
hotplug, either by using tree's code or by doing my own modifications.
> 
> Even if not, there would be a slighty messed directory on one specific
> device with how many units? Let's take the latest example of extending
> the DTS file with OS-specific information - this one is ugly, too, but
> has impact _for everyone using dts_ files. Think about the number of
> affected systems and you will hopefully understand why such patches are
> handled with extreme caution.

You talk about my reading of the class entry from DTS?
No, I don't understand how other systems are affected by this.
They don't read this entry of _my_ .dts file.
Seems there is a general misunderstanding...
> 
>> Because of the situation above I try to keep the ability of dynamic
>> instantiation. Jean hesitates, I feel because he sees I2C solely in
>> static manner.
> 
> He is taking care for every I2C communication on every Linux machine out
> there. He can't please everyone. On the other hand, you are free to
> modify the kernel as you please. It doesn't really have to be in
> mainline, does it?
If this would be a private problem, yes.
But I see this as a more general flaw:
Many adapters do initialization of .class in adapter code. A few do not.
I haven't checked, but assume the PPC ones, as they have DTS. Thats Ok,
as long DTS is really used. In at least one other adapter it is: Exactly
Wolfgang Grandegger, who is responsible for removal of the original
default initialization, showed me, that it is done in i2c-cpm!
> 
> BTW, I hope you saw that the "linux,i2c-class" property is considered
> deprecated. Thus, expect a patch from me removing it entirely as soon
> as the old binding model went away (there aren't even any users).
> 
And how do you want to implement setting the class attribute?
Allowing only clients that are mentioned in DTS? How should it work on
systems without DTS?

Re: [Patch] MPC Adapter: read class attribute from device tree

[Wolfgang Grandegger]

Michael Lawnick wrote:
> Wolfram Sang said the following:
>>> mailing list. Nevertheless loading modules at runtime is legal and
>>> generally supported by LINUX.
>> Ack. And it does so...
>>
>>> Defining all possible (I2C-)devices in DTS would give a mess. E.g. on
>>> one board there will be ~30 temperature sensors, on the other none.
>>> As every DTS entry will force a sysFs subdirectory there would be a
>>> bunch of functionless directories - rather ugly.
>> Well, you should have a DTS for every flavour; that is what DTS are
>> about. You do know that, do you?
> 
> You like it more complicated? Ok, here it is:
> As Jean indicated in his reply, we will have to support hotplug. Most of
> the above sensors won't exist a life time on some systems, on others all
> could exist. I tried to keep out this issue to avoid a drift to the
> discussion that hotplug is not _yet_ supported. I will^Wmust support
> hotplug, either by using tree's code or by doing my own modifications.
>> Even if not, there would be a slighty messed directory on one specific
>> device with how many units? Let's take the latest example of extending
>> the DTS file with OS-specific information - this one is ugly, too, but
>> has impact _for everyone using dts_ files. Think about the number of
>> affected systems and you will hopefully understand why such patches are
>> handled with extreme caution.
> 
> You talk about my reading of the class entry from DTS?
> No, I don't understand how other systems are affected by this.
> They don't read this entry of _my_ .dts file.
> Seems there is a general misunderstanding...
>>> Because of the situation above I try to keep the ability of dynamic
>>> instantiation. Jean hesitates, I feel because he sees I2C solely in
>>> static manner.
>> He is taking care for every I2C communication on every Linux machine out
>> there. He can't please everyone. On the other hand, you are free to
>> modify the kernel as you please. It doesn't really have to be in
>> mainline, does it?
> If this would be a private problem, yes.
> But I see this as a more general flaw:
> Many adapters do initialization of .class in adapter code. A few do not.
> I haven't checked, but assume the PPC ones, as they have DTS. Thats Ok,
> as long DTS is really used. In at least one other adapter it is: Exactly
> Wolfgang Grandegger, who is responsible for removal of the original
> default initialization, showed me, that it is done in i2c-cpm!

This property in i2c-cpm existed before the conversion to new style I2C
drivers. I actually proposed the following patch:

 http://ozlabs.org/pipermail/linuxppc-dev/2008-July/060016.html

But after some discussion we came to the conclusion, that legacy device
probing should be avoided for FDT based systems.

Wolfgang.

Re: [Patch] MPC Adapter: read class attribute from device tree

[Michael Lawnick]

Wolfgang Grandegger said the following:
>> Many adapters do initialization of .class in adapter code. A few do not.
>> I haven't checked, but assume the PPC ones, as they have DTS. Thats Ok,
>> as long DTS is really used. In at least one other adapter it is: Exactly
>> Wolfgang Grandegger, who is responsible for removal of the original
>> default initialization, showed me, that it is done in i2c-cpm!
> 
> This property in i2c-cpm existed before the conversion to new style I2C
> drivers. I actually proposed the following patch:
> 
>  http://ozlabs.org/pipermail/linuxppc-dev/2008-July/060016.html
> 
> But after some discussion we came to the conclusion, that legacy device
> probing should be avoided for FDT based systems.
What does this                  ^^^ mean?

I am really without emotion about how it is solved, but dynamic
instantiation should be possible in a clean way. Generating an entry for
every possible device is no clean way, IMHO.
BTW: I can't see what this has to do with legacy, I my case we talk
about new-style ones.

-- 
Michael

Re: [Patch] MPC Adapter: read class attribute from device tree

[Wolfgang Grandegger]

Michael Lawnick wrote:
> Wolfgang Grandegger said the following:
>>> Many adapters do initialization of .class in adapter code. A few do not.
>>> I haven't checked, but assume the PPC ones, as they have DTS. Thats Ok,
>>> as long DTS is really used. In at least one other adapter it is: Exactly
>>> Wolfgang Grandegger, who is responsible for removal of the original
>>> default initialization, showed me, that it is done in i2c-cpm!
>> This property in i2c-cpm existed before the conversion to new style I2C
>> drivers. I actually proposed the following patch:
>>
>>  http://ozlabs.org/pipermail/linuxppc-dev/2008-July/060016.html
>>
>> But after some discussion we came to the conclusion, that legacy device
>> probing should be avoided for FDT based systems.
> What does this                  ^^^ mean?

"flattened device-tree", see

linux-2.6/Documentation/powerpc/booting-without-of.txt

Wolfgang.

Re: [Patch] MPC Adapter: read class attribute from device tree

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 3067 bytes --]


> You like it more complicated? Ok, here it is:

Thanks, that gave me a bigger picture.

> You talk about my reading of the class entry from DTS?
> No, I don't understand how other systems are affected by this.
> They don't read this entry of _my_ .dts file.
> Seems there is a general misunderstanding...

DTS is convention. One convention is to keep it OS-neutral. If you use
something out of this convention like "linux,i2c-class", you motivate
others to follow you. In fact, this is exactly what happened because
i2c-cpm defined such a thing (and people who are watching the DTS not
drifting away seemed to have missed it). If other people use it, other
OS could be forced to evaluate a property which is named  "linux" and
may be, in addition, inadequate for their purpose.

DTS (or better the OF devicetree) faces the danger of so many private
extensions that there is no common thing (= convention) at the end of
the day rendering it unusable. This is why adding new properties is
handled with care (and should always be CCed to devicetree-discuss).

> Many adapters do initialization of .class in adapter code. A few do not.
> I haven't checked, but assume the PPC ones, as they have DTS. Thats Ok,

(Would be nice if you did. Knowledge is better than assumption in this
case)

I think most drivers are old enough to have a .class as it was useful
back then and nobody cared to remove it. Newer drivers, especially
embedded ones, usually don't have it right from the beginning, also on
ARM..., as probing is not wanted.

> as long DTS is really used. In at least one other adapter it is: Exactly
> Wolfgang Grandegger, who is responsible for removal of the original
> default initialization, showed me, that it is done in i2c-cpm!

You mean setting the class variable through DTS? Yeah, this was a good
outcome of this thread. I will submit a patch to remove it soonish. It
just slipped through, should have never been there IMHO.

> And how do you want to implement setting the class attribute?

Not at all. I see ".class" as a reminiscent to overcome some flaws of
the old binding model. If we finally got rid of that we can surely think
of a more elegant way to enforce instanciation of clients run-time. And
with that, probably get rid of .class altogether. D'accord, Jean?

> Allowing only clients that are mentioned in DTS?

For now, yes. This is still the common case (=avoid probing). Until a
mainline solution comes up (which is needed, I agree on that), not so
common cases could simply patch the driver to add a .class field.

> How should it work on systems without DTS?

i2c_board_info? (no offence, sometimes it is hard to tell if you know
all details and still see a valid problem or if you are just missing
details. If the first one is the case, please be a bit more elaborative
on the problem.)

Regards,

   Wolfram

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

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

Re: [Patch] MPC Adapter: read class attribute from device tree

[Jean Delvare]

Hi Wolfram,

On Fri, 24 Apr 2009 10:52:56 +0200, Wolfram Sang wrote:
> > And how do you want to implement setting the class attribute?
> 
> Not at all. I see ".class" as a reminiscent to overcome some flaws of
> the old binding model.

Not really. The old binding model didn't have this concept of class
originally, it was added later but never enforced and only sparsely
used.

Nowadays classes are an extension of the new binding model, used to
restrict the field of application of .detect() callbacks (because we
definitely do not want probing to be enabled by default.)

> If we finally got rid of that we can surely think
> of a more elegant way to enforce instanciation of clients run-time. And
> with that, probably get rid of .class altogether. D'accord, Jean?

In an ideal world, yes. In the real world, no. There are several
thousand PC mainboards out there with devices on the SMBus and we don't
have a list, and in most cases no way to ask the system about them.
Probing is the only realistic way there, so at least I2C_CLASS_HWMON,
I2C_CLASS_DDC and I2C_CLASS_SPD are there to stay. We should be able to
get rid of I2C_CLASS_TV_ANALOG and I2C_CLASS_TV_DIGITAL though.

While classes will stay for PC mainboards, it should be possible (and
desirable) to stop using them completely on embedded systems.

-- 
Jean Delvare

Re: [Patch] MPC Adapter: read class attribute from device tree

[Michael Lawnick]

Jean Delvare said the following:
> While classes will stay for PC mainboards, it should be possible (and
> desirable) to stop using them completely on embedded systems.
> 
This means no probing on embedded systems, doesn't it?
Do you really expect a customized kernel space code to do that? IMHO
there should be at least a way for triggering from user space.

-- 
Regards,
Michael

Re: [Patch] MPC Adapter: read class attribute from device tree

[Michael Lawnick]

Wolfram Sang said the following:
...
>> Allowing only clients that are mentioned in DTS?
> 
> For now, yes. This is still the common case (=avoid probing). Until a
> mainline solution comes up (which is needed, I agree on that), not so
> common cases could simply patch the driver to add a .class field.
> 
Especially it is ugly that devices which are mentioned in DTS but are
not present (because the component isn't plugged in or powered) get an
directory entry in /sys/bus/i2c/devices with bind, unbind, ...
This is my 'problem': I search for a clean way to instantiate on demand
with no zombies in case of missing H/W. I know there are problems in
identification of devices, but to remove all possibilities to try it
seems not a good way for me.

>> How should it work on systems without DTS?
> 
> i2c_board_info? (no offence, sometimes it is hard to tell if you know
> all details and still see a valid problem or if you are just missing
> details. If the first one is the case, please be a bit more elaborative
> on the problem.)

I am new in the sense that I jumped in at 2.4 making some non
i2c-drivers, rejoind at 2.6.24 for i2c-drivers and a non i2c-driver.
Currently I try to catch up to current state to prepare management of
all i2c-devices on our next project.

Even i2c_board_info needs to be filled in and passed to subsystem via a
function call. Which part of kernel is supposed to do that without
changing code beside board specific configuration files?
(I do really mean this as a question.)

-- 
Kind regards,
Michael

Re: [Patch] MPC Adapter: read class attribute from device tree

[Jon Smirl]

On Fri, Apr 24, 2009 at 6:53 AM, Michael Lawnick <ml.lawnick-Mmb7MZpHnFY@public.gmane.org> wrote:
> Wolfram Sang said the following:
> ...
>>> Allowing only clients that are mentioned in DTS?
>>
>> For now, yes. This is still the common case (=avoid probing). Until a
>> mainline solution comes up (which is needed, I agree on that), not so
>> common cases could simply patch the driver to add a .class field.
>>
> Especially it is ugly that devices which are mentioned in DTS but are
> not present (because the component isn't plugged in or powered) get an
> directory entry in /sys/bus/i2c/devices with bind, unbind, ...
> This is my 'problem': I search for a clean way to instantiate on demand
> with no zombies in case of missing H/W. I know there are problems in
> identification of devices, but to remove all possibilities to try it
> seems not a good way for me.

You might want to ask about this on the linux-ppc mailing list.

You're stuck because i2c hardware does not support hotplug. Since the
hardware doesn't support hotplug there's no software support for it in
the kernel. So you want to fall back to polled probing.

But probing is bad because it isn't standardized and coordinated, one
device's probe can cause another device to destroy things (like set a
voltage too high on a motherboard). Because of this probing can't be
turned on in general.

It's possible to get to the i2c bus from user space. You could write a
user space app that polls in your controlled environment. When you
detect the new devices you can load the appropriate modules. Now you
need to instantiate these new devices.

Jean, is it still possible to instantiate i2c devices from the module
command line?

>
>>> How should it work on systems without DTS?
>>
>> i2c_board_info? (no offence, sometimes it is hard to tell if you know
>> all details and still see a valid problem or if you are just missing
>> details. If the first one is the case, please be a bit more elaborative
>> on the problem.)
>
> I am new in the sense that I jumped in at 2.4 making some non
> i2c-drivers, rejoind at 2.6.24 for i2c-drivers and a non i2c-driver.
> Currently I try to catch up to current state to prepare management of
> all i2c-devices on our next project.
>
> Even i2c_board_info needs to be filled in and passed to subsystem via a
> function call. Which part of kernel is supposed to do that without
> changing code beside board specific configuration files?
> (I do really mean this as a question.)
>
> --
> Kind regards,
> Michael
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>



-- 
Jon Smirl
jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

Re: [Patch] MPC Adapter: read class attribute from device tree

[Michael Lawnick]

Jon Smirl said the following:
> On Fri, Apr 24, 2009 at 6:53 AM, Michael Lawnick <ml.lawnick-Mmb7MZpHnFY@public.gmane.org> wrote:
>> Wolfram Sang said the following:
>> ...
>>>> Allowing only clients that are mentioned in DTS?
>>>
>>> For now, yes. This is still the common case (=avoid probing). Until a
>>> mainline solution comes up (which is needed, I agree on that), not so
>>> common cases could simply patch the driver to add a .class field.
>>>
>> Especially it is ugly that devices which are mentioned in DTS but are
>> not present (because the component isn't plugged in or powered) get an
>> directory entry in /sys/bus/i2c/devices with bind, unbind, ...
>> This is my 'problem': I search for a clean way to instantiate on demand
>> with no zombies in case of missing H/W. I know there are problems in
>> identification of devices, but to remove all possibilities to try it
>> seems not a good way for me.
> 
> You might want to ask about this on the linux-ppc mailing list.

I can't see how they should be able to help me more than this list.
> 
> You're stuck because i2c hardware does not support hotplug. Since the
> hardware doesn't support hotplug there's no software support for it in
> the kernel. So you want to fall back to polled probing.

That's what I do. But being able to detect a new device via /dev/i2c is
only halve of the job. The device needs to be identified and a client
instantiated. Doing this job in the user space polling program would
double the code, as detection is already done in the drivers.
> 
> But probing is bad because it isn't standardized and coordinated, one
> device's probe can cause another device to destroy things (like set a
> voltage too high on a motherboard). Because of this probing can't be
> turned on in general.

That's not the problem. The problem is, that in current code state it is
not possible at all to do proper probing on MPC85xx architecture, even
if the possible I2C devices / bus layout would allow it. You must force
instantiation, so if the driver detects an error on .probe and stops,
you have dead entries in sysFs. This would not be the case if there
would be a way to go via .detect
> 
> It's possible to get to the i2c bus from user space. You could write a
> user space app that polls in your controlled environment.

As said above, this is already done in this way.

> When you
> detect the new devices you can load the appropriate modules. Now you
> need to instantiate these new devices.

The modules are already loaded, there is missing a proper way to start
instantiation of *new* clients.
> 
> Jean, is it still possible to instantiate i2c devices from the module
> command line?

This question is missing the point. The module is already up and
running. The situation to handle is that there is powered-on a device
after module load. Now a client for this device needs instantiation. As
code for detection of client-type should IMHO not be part of user space
(as it *is* already part of drivers) I am currently fighting for a way
to trigger a driver's detect function.

--
Kind regards,
Michael

Re: [Patch] MPC Adapter: read class attribute from device tree

[Wolfgang Grandegger]

Jon Smirl wrote:
> On Fri, Apr 24, 2009 at 6:53 AM, Michael Lawnick <ml.lawnick-Mmb7MZpHnFY@public.gmane.org> wrote:
>> Wolfram Sang said the following:
>> ...
>>>> Allowing only clients that are mentioned in DTS?
>>> For now, yes. This is still the common case (=avoid probing). Until a
>>> mainline solution comes up (which is needed, I agree on that), not so
>>> common cases could simply patch the driver to add a .class field.
>>>
>> Especially it is ugly that devices which are mentioned in DTS but are
>> not present (because the component isn't plugged in or powered) get an
>> directory entry in /sys/bus/i2c/devices with bind, unbind, ...
>> This is my 'problem': I search for a clean way to instantiate on demand
>> with no zombies in case of missing H/W. I know there are problems in
>> identification of devices, but to remove all possibilities to try it
>> seems not a good way for me.
> 
> You might want to ask about this on the linux-ppc mailing list.
> 
> You're stuck because i2c hardware does not support hotplug. Since the
> hardware doesn't support hotplug there's no software support for it in
> the kernel. So you want to fall back to polled probing.
> 
> But probing is bad because it isn't standardized and coordinated, one
> device's probe can cause another device to destroy things (like set a
> voltage too high on a motherboard). Because of this probing can't be
> turned on in general.

To decide if a device exists and responds at a specified I2C address, it
would be sufficient to just read some data without causing trouble. Is
that assumption correct or have I missed something? Trying to identify
the device is a different issue and should be avoided.

Wolfgang.

Re: [Patch] MPC Adapter: read class attribute from device tree

[Ben Dooks]

On Mon, Apr 27, 2009 at 11:07:46AM +0200, Wolfgang Grandegger wrote:
> Jon Smirl wrote:
> > On Fri, Apr 24, 2009 at 6:53 AM, Michael Lawnick <ml.lawnick-Mmb7MZpHnFY@public.gmane.org> wrote:
> >> Wolfram Sang said the following:
> >> ...
> >>>> Allowing only clients that are mentioned in DTS?
> >>> For now, yes. This is still the common case (=avoid probing). Until a
> >>> mainline solution comes up (which is needed, I agree on that), not so
> >>> common cases could simply patch the driver to add a .class field.
> >>>
> >> Especially it is ugly that devices which are mentioned in DTS but are
> >> not present (because the component isn't plugged in or powered) get an
> >> directory entry in /sys/bus/i2c/devices with bind, unbind, ...
> >> This is my 'problem': I search for a clean way to instantiate on demand
> >> with no zombies in case of missing H/W. I know there are problems in
> >> identification of devices, but to remove all possibilities to try it
> >> seems not a good way for me.
> > 
> > You might want to ask about this on the linux-ppc mailing list.
> > 
> > You're stuck because i2c hardware does not support hotplug. Since the
> > hardware doesn't support hotplug there's no software support for it in
> > the kernel. So you want to fall back to polled probing.
> > 
> > But probing is bad because it isn't standardized and coordinated, one
> > device's probe can cause another device to destroy things (like set a
> > voltage too high on a motherboard). Because of this probing can't be
> > turned on in general.
> 
> To decide if a device exists and responds at a specified I2C address, it
> would be sufficient to just read some data without causing trouble. Is
> that assumption correct or have I missed something? Trying to identify
> the device is a different issue and should be avoided.

It depends if you know what you are expecting to find there, due to
the limited addressing space used by i2c, a number of devices use the
same address and often do not have any indentification information.

Worse, doing a read of the chip requires knowing whether you need to
issue an register address or not, so this makes life even harder if
you are trying to do some form of autodetect.
 
> Wolfgang.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Ben (ben-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

Re: [Patch] MPC Adapter: read class attribute from device tree

[Jean Delvare]

Hi Jon,

On Fri, 24 Apr 2009 11:38:04 -0400, Jon Smirl wrote:
> You're stuck because i2c hardware does not support hotplug. Since the
> hardware doesn't support hotplug there's no software support for it in
> the kernel. So you want to fall back to polled probing.
> 
> But probing is bad because it isn't standardized and coordinated, one
> device's probe can cause another device to destroy things (like set a
> voltage too high on a motherboard). Because of this probing can't be
> turned on in general.
> 
> It's possible to get to the i2c bus from user space. You could write a
> user space app that polls in your controlled environment. When you
> detect the new devices you can load the appropriate modules. Now you
> need to instantiate these new devices.
> 
> Jean, is it still possible to instantiate i2c devices from the module
> command line?

It is still possible if the driver implements the .detect() callback
(even without any address being probed by default) and uses one of the
I2C_CLIENT_INSMOD* macros. However, these macros are planned for
removal. Their replacement is the sysfs entries I have been discussing
with Michael in the past few weeks and for which he sent a patch for me
to review. This is still on my todo list, but I am very busy these
days :(

-- 
Jean Delvare

Re: [Patch] MPC Adapter: read class attribute from device tree

[Jean Delvare]

On Mon, 27 Apr 2009 12:17:58 +0200, Jean Delvare wrote:
> On Fri, 24 Apr 2009 11:38:04 -0400, Jon Smirl wrote:
> > Jean, is it still possible to instantiate i2c devices from the module
> > command line?
> 
> It is still possible if the driver implements the .detect() callback
> (even without any address being probed by default) and uses one of the
> I2C_CLIENT_INSMOD* macros. However, these macros are planned for
> removal. Their replacement is the sysfs entries I have been discussing
> with Michael in the past few weeks and for which he sent a patch for me
> to review. This is still on my todo list, but I am very busy these
> days :(

For the record, it finally happened. I posted a patch doing this on
the linux-i2c list on May 4th, which can be temporarily seen at:
ftp://ftp.kernel.org/pub/linux/kernel/people/jdelvare/linux-2.6/jdelvare-i2c/i2c-sysfs-interface-to-instantiate-devices.patch

It will be merged in 2.6.31. Potential users are invited to give it a
try and comment by then. Hopefully we can then deprecate the ugly
I2C_CLIENT_INSMOD* macros, and get rid of them 2 kernels later.

Thanks,
-- 
Jean Delvare

Re: [Patch] MPC Adapter: read class attribute from device tree

[Jean Delvare]

On Tue, 21 Apr 2009 11:26:10 +0200, Michael Lawnick wrote:
> Wolfgang Grandegger said the following:
> > Hello,
> > 
> > sorry for jumping in late. I just recently subscribed to this list.
> > 
> > Michael Lawnick wrote:
> >> For MPC adapter there is no class assigned as it is done in other
> >> adapters. This way no new-style client will ever be instantiated. With
> >> this patch class assignment is read from device tree.
> >> Necessary entry:
> >> class = <1>; /* I2C_CLASS_HWMON (iic.h) */
> > 
> > Do we really need that? Probing is dangerous and not necessary. Does it
> > not work with a proper DTS entry? But maybe I have missed something?
> 
> Our current and our next system consists of two flavors of the same
> board with different devices. To minimize maintenance and testing we use
> a reduced kernel and load the needed modules at runtime. Furthermore we
> will have to handle hot-plugged I2C-devices. Whether this strategy is
> best could be discussed in another thread but is rather OT in this
> mailing list. Nevertheless loading modules at runtime is legal and
> generally supported by LINUX.

You apparently still have a hard time differentiating between devices
and drivers. Loading modules at runtime != instantiating devices at
runtime.

(And both can be done, but if you can't define your needs, we won't go
very far.)

> Defining all possible (I2C-)devices in DTS would give a mess. E.g. on
> one board there will be ~30 temperature sensors, on the other none.
> As every DTS entry will force a sysFs subdirectory there would be a
> bunch of functionless directories - rather ugly.

Why don't you define one DTS per board? I'm no embedded expert, but my
understanding is that this is what other developers/designers do.

> If probing of a device is dangerous, it can be defined in DTS anyway and
> the device driver can easily omit this part by early return.

This is correct.

> Because of the situation above I try to keep the ability of dynamic
> instantiation. Jean hesitates, I feel because he sees I2C solely in
> static manner.

Well there are 2 different issues here. First issue is relying on
probing when a DTS would do. Many developers have done that before, and
stopped doing it quickly, for a reason. We simply want to save you from
doing the same mistake.

Second issue is the warm-plugging of I2C devices. Linux doesn't support
this at this point, we are working on it (actually I am working on the
dependencies), but as long as support for basic multiplexing it isn't
there, there's little point in discussing more complex (dynamic)
schemes.

I had been explaining exactly this already, hadn't I?

-- 
Jean Delvare

Re: [Patch] MPC Adapter: read class attribute from device tree

[Michael Lawnick]

Jean Delvare said the following:
> You apparently still have a hard time differentiating between devices
> and drivers. Loading modules at runtime != instantiating devices at
> runtime.
> 
Well, till now I assumed that you can't instantiate a device who's
driver isn't loaded. But I am eager to learn.

> (And both can be done, but if you can't define your needs, we won't go
> very far.)

I thought I had explained very often. I'll do it once again:
We load drivers at runtime directly after boot which does automatic
instantiation. There will occur further devices on bus after some time
(hotplug) and therefore we need later on instantiation without reloading
drivers. We cannot define which devices will exactly be plugged in, it
might even be possible that on same bus address there occur different
devices (as an possible option and not at same time of course). So
device detection is highly appreciated.
Do you need more info?

> 
>> Defining all possible (I2C-)devices in DTS would give a mess. E.g. on
>> one board there will be ~30 temperature sensors, on the other none.
>> As every DTS entry will force a sysFs subdirectory there would be a
>> bunch of functionless directories - rather ugly.
> 
> Why don't you define one DTS per board? I'm no embedded expert, but my
> understanding is that this is what other developers/designers do.
> 
--> my answer to W.G. at 15:05 (<49EDC487.8010201-Mmb7MZpHnFY@public.gmane.org>)

>> Because of the situation above I try to keep the ability of dynamic
>> instantiation. Jean hesitates, I feel because he sees I2C solely in
>> static manner.
> 
> Well there are 2 different issues here. First issue is relying on
> probing when a DTS would do. Many developers have done that before, and
> stopped doing it quickly, for a reason. We simply want to save you from
> doing the same mistake.

If you don't tell me the issue, I won't understand. DTS is no good
option as described multiple times.

> 
> Second issue is the warm-plugging of I2C devices. Linux doesn't support
> this at this point, we are working on it (actually I am working on the
> dependencies), but as long as support for basic multiplexing it isn't
> there, there's little point in discussing more complex (dynamic)
> schemes.
> 
> I had been explaining exactly this already, hadn't I?
> 
I took a look into the code of Rodolfo Giometti and it seemed to be in a
way I would have done it, if I wouldn't have been too late. In my
current working I assume this code as a basis.

Nevertheless my mailed patch has *nothing* to do with multiplexing.
It works on the problem that i2c-mpc has 0 as default class and so there
is no way to instantiate a new-style client that is not mentioned in DTS.

-- 
Michael

Re: [Patch] MPC Adapter: read class attribute from device tree

[Jon Smirl]

On Tue, Apr 21, 2009 at 10:11 AM, Michael Lawnick <ml.lawnick-Mmb7MZpHnFY@public.gmane.org> wrote:
> Jean Delvare said the following:
>> You apparently still have a hard time differentiating between devices
>> and drivers. Loading modules at runtime != instantiating devices at
>> runtime.
>>
> Well, till now I assumed that you can't instantiate a device who's
> driver isn't loaded. But I am eager to learn.
>
>> (And both can be done, but if you can't define your needs, we won't go
>> very far.)
>
> I thought I had explained very often. I'll do it once again:
> We load drivers at runtime directly after boot which does automatic
> instantiation. There will occur further devices on bus after some time
> (hotplug) and therefore we need later on instantiation without reloading
> drivers. We cannot define which devices will exactly be plugged in, it
> might even be possible that on same bus address there occur different
> devices (as an possible option and not at same time of course). So
> device detection is highly appreciated.
> Do you need more info?

Jean, haven't you told me multiple times that hotplug doesn't work for
i2c? How can you tell when a new device has been added to an i2c bus?

There is already code in the kernel that will dynamically load the
correct i2c device drivers based on the device tree. The i2c drivers
don't need to be build into the kernel. Just put drivers for all of
your devices onto the initrd.

-- 
Jon Smirl
jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

Re: [Patch] MPC Adapter: read class attribute from device tree

[Jean Delvare]

On Tue, 21 Apr 2009 12:00:02 -0400, Jon Smirl wrote:
> On Tue, Apr 21, 2009 at 10:11 AM, Michael Lawnick <ml.lawnick-Mmb7MZpHnFY@public.gmane.org> wrote:
> > Jean Delvare said the following:
> >> You apparently still have a hard time differentiating between devices
> >> and drivers. Loading modules at runtime != instantiating devices at
> >> runtime.
> >>
> > Well, till now I assumed that you can't instantiate a device who's
> > driver isn't loaded. But I am eager to learn.
> >
> >> (And both can be done, but if you can't define your needs, we won't go
> >> very far.)
> >
> > I thought I had explained very often. I'll do it once again:
> > We load drivers at runtime directly after boot which does automatic
> > instantiation. There will occur further devices on bus after some time
> > (hotplug) and therefore we need later on instantiation without reloading
> > drivers. We cannot define which devices will exactly be plugged in, it
> > might even be possible that on same bus address there occur different
> > devices (as an possible option and not at same time of course). So
> > device detection is highly appreciated.
> > Do you need more info?
> 
> Jean, haven't you told me multiple times that hotplug doesn't work for
> i2c? How can you tell when a new device has been added to an i2c bus?

I did say so, and I maintain this assertion. Warm-plug, on the other
hand, could be supported (but isn't yet.) By warm-plug, I mean: switch
off one "channel" of a multiplexer, add one chip to that segment, and
enable the channel again. Once we have multiplexing support, this
should be doable, if we implement the proper interface. But this really
is per-segment, not per-device.

Of course this can never be hot-plug like e.g. USB has it: you're not
going to get a signal when a new I2C device is connected. This has to
be a partly manual process, where you tell the system that a new I2C
segment is available, and that the usual rules (per-board instantiation
or device detection) have to be applied.

> There is already code in the kernel that will dynamically load the
> correct i2c device drivers based on the device tree. The i2c drivers
> don't need to be build into the kernel. Just put drivers for all of
> your devices onto the initrd.

-- 
Jean Delvare

Re: [Patch] MPC Adapter: read class attribute from device tree

[Jon Smirl]

On Tue, Apr 21, 2009 at 12:33 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> On Tue, 21 Apr 2009 12:00:02 -0400, Jon Smirl wrote:
>> On Tue, Apr 21, 2009 at 10:11 AM, Michael Lawnick <ml.lawnick-Mmb7MZpHnFY@public.gmane.org> wrote:
>> > Jean Delvare said the following:
>> >> You apparently still have a hard time differentiating between devices
>> >> and drivers. Loading modules at runtime != instantiating devices at
>> >> runtime.
>> >>
>> > Well, till now I assumed that you can't instantiate a device who's
>> > driver isn't loaded. But I am eager to learn.
>> >
>> >> (And both can be done, but if you can't define your needs, we won't go
>> >> very far.)
>> >
>> > I thought I had explained very often. I'll do it once again:
>> > We load drivers at runtime directly after boot which does automatic
>> > instantiation. There will occur further devices on bus after some time
>> > (hotplug) and therefore we need later on instantiation without reloading
>> > drivers. We cannot define which devices will exactly be plugged in, it
>> > might even be possible that on same bus address there occur different
>> > devices (as an possible option and not at same time of course). So
>> > device detection is highly appreciated.
>> > Do you need more info?
>>
>> Jean, haven't you told me multiple times that hotplug doesn't work for
>> i2c? How can you tell when a new device has been added to an i2c bus?
>
> I did say so, and I maintain this assertion. Warm-plug, on the other
> hand, could be supported (but isn't yet.) By warm-plug, I mean: switch
> off one "channel" of a multiplexer, add one chip to that segment, and
> enable the channel again. Once we have multiplexing support, this
> should be doable, if we implement the proper interface. But this really
> is per-segment, not per-device.
>
> Of course this can never be hot-plug like e.g. USB has it: you're not
> going to get a signal when a new I2C device is connected. This has to
> be a partly manual process, where you tell the system that a new I2C
> segment is available, and that the usual rules (per-board instantiation
> or device detection) have to be applied.

So a solution might lie down the path of having whatever catches the
hotplug event (pci, usb, ..) run code that modifies the device tree
and inserts the new i2c nodes. After the device tree has been modified
trigger a rescan of devices so that the i2c drivers get loaded.  I've
never tried doing that but it should be doable in theory.

Something has to trigger an interrupt (or the user types a command)
indicating that new hardware has been added. The driver of that device
can modify the device tree. The rescan of the device tree will cause
the i2c modules to automatically load. This shouldn't require any
changes to the current i2c sub-system.



>
>> There is already code in the kernel that will dynamically load the
>> correct i2c device drivers based on the device tree. The i2c drivers
>> don't need to be build into the kernel. Just put drivers for all of
>> your devices onto the initrd.
>
> --
> Jean Delvare
>



-- 
Jon Smirl
jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

Re: [Patch] MPC Adapter: read class attribute from device tree

[Michael Lawnick]

Jon Smirl said the following:
> So a solution might lie down the path of having whatever catches the
> hotplug event (pci, usb, ..) run code that modifies the device tree
> and inserts the new i2c nodes. After the device tree has been modified
> trigger a rescan of devices so that the i2c drivers get loaded.  I've
> never tried doing that but it should be doable in theory.
> 
> Something has to trigger an interrupt (or the user types a command)
> indicating that new hardware has been added. The driver of that device
> can modify the device tree. The rescan of the device tree will cause
> the i2c modules to automatically load. This shouldn't require any
> changes to the current i2c sub-system.

This is an solution, although it feels a bit more complicated than
necessary. My original plan is to check the bus in intervals of some
seconds via /dev/i2c for a specific device. When the device is detected
its module is loaded and the driver/device initialized. A simple user
space script, running in background.

-- 
Regards,
Michael

Re: [Patch] MPC Adapter: read class attribute from device tree

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 992 bytes --]


> Something has to trigger an interrupt (or the user types a command)
> indicating that new hardware has been added. The driver of that device
> can modify the device tree. The rescan of the device tree will cause
> the i2c modules to automatically load. This shouldn't require any
> changes to the current i2c sub-system.

As I understood it so far, the purpose of the devicetree is done as soon
it is presented to the linux-kernel and once parsed during boot.
Changing it at runtime and reacting to that looks like a bigger step to
me. Hints for that are that most fdt-functions are __init and I couldn't
quickly find an add_node-function...

Shouldn't really matter anyhow, should it? As soon as we are run-time,
there shouldn't be any difference between DTS and platform_devices. It
is just driver-model then...

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

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

Re: [Patch] MPC Adapter: read class attribute from device tree

[Jean Delvare]

On Tue, 21 Apr 2009 16:11:47 +0200, Michael Lawnick wrote:
> I took a look into the code of Rodolfo Giometti and it seemed to be in a
> way I would have done it, if I wouldn't have been too late. In my
> current working I assume this code as a basis.
> 
> Nevertheless my mailed patch has *nothing* to do with multiplexing.
> It works on the problem that i2c-mpc has 0 as default class and so there
> is no way to instantiate a new-style client that is not mentioned in DTS.

If you didn't want to handle warm-plug then that wouldn't be a problem,
because you would be able to put all the chip information in the DTS.
But you want warm-plug. And warm-plug needs multiplexing support. So
your problem is very much related to multiplexing.

Now I'll ignore you until I am done with the prerequisites to accept
Rodolfo's patches, and with Rodolfo's patches themselves. Discussing
your problems when there's nothing I can do about them at the time
being, is just a waste of everybody's time.

-- 
Jean Delvare

Re: [Patch] MPC Adapter: read class attribute from device tree

[Wolfgang Grandegger]

Michael Lawnick wrote:
> Wolfgang Grandegger said the following:
>> Hello,
>>
>> sorry for jumping in late. I just recently subscribed to this list.
>>
>> Michael Lawnick wrote:
>>> For MPC adapter there is no class assigned as it is done in other
>>> adapters. This way no new-style client will ever be instantiated. With
>>> this patch class assignment is read from device tree.
>>> Necessary entry:
>>> class = <1>; /* I2C_CLASS_HWMON (iic.h) */
>> Do we really need that? Probing is dangerous and not necessary. Does it
>> not work with a proper DTS entry? But maybe I have missed something?
> 
> Our current and our next system consists of two flavors of the same
> board with different devices. To minimize maintenance and testing we use
> a reduced kernel and load the needed modules at runtime. Furthermore we
> will have to handle hot-plugged I2C-devices. Whether this strategy is
> best could be discussed in another thread but is rather OT in this
> mailing list. Nevertheless loading modules at runtime is legal and
> generally supported by LINUX.
> 
> Defining all possible (I2C-)devices in DTS would give a mess. E.g. on
> one board there will be ~30 temperature sensors, on the other none.

The legacy device probing has it's limitations and I doubt that it will
be able to find 30 DTT devices properly. E.g. the legacy probing
algorithm is not able to distinguish between a LM75 and DS75. Let's
first try to find out why it does not work as expected with the DTS entry.

> As every DTS entry will force a sysFs subdirectory there would be a
> bunch of functionless directories - rather ugly.

Hopefully not. They will be created when the probing was successful, I
assume. I think it is OK to define multiple I2C nodes also for
non-existing devices in the DTS file if the I2C address is unique.

Wolfgang.

Re: [Patch] MPC Adapter: read class attribute from device tree

[Jean Delvare]

On Tue, 21 Apr 2009 12:35:49 +0200, Wolfgang Grandegger wrote:
> Michael Lawnick wrote:
> > Wolfgang Grandegger said the following:
> >> Do we really need that? Probing is dangerous and not necessary. Does it
> >> not work with a proper DTS entry? But maybe I have missed something?
> > 
> > Our current and our next system consists of two flavors of the same
> > board with different devices. To minimize maintenance and testing we use
> > a reduced kernel and load the needed modules at runtime. Furthermore we
> > will have to handle hot-plugged I2C-devices. Whether this strategy is
> > best could be discussed in another thread but is rather OT in this
> > mailing list. Nevertheless loading modules at runtime is legal and
> > generally supported by LINUX.
> > 
> > Defining all possible (I2C-)devices in DTS would give a mess. E.g. on
> > one board there will be ~30 temperature sensors, on the other none.
> 
> The legacy device probing has it's limitations and I doubt that it will
> be able to find 30 DTT devices properly. E.g. the legacy probing
> algorithm is not able to distinguish between a LM75 and DS75. Let's
> first try to find out why it does not work as expected with the DTS entry.
> 
> > As every DTS entry will force a sysFs subdirectory there would be a
> > bunch of functionless directories - rather ugly.
> 
> Hopefully not. They will be created when the probing was successful, I
> assume.

No, device creation is unconditional. It is assumed that the DTS file
author knew what he/she was doing.

There was a discussion about adding a flag to state the "may not be
present" nature of devices, which would result in a probe before
instantiating the device (on success), but it wasn't implemented yet. I
have no objection is this mechanism is deemed useful.

> I think it is OK to define multiple I2C nodes also for
> non-existing devices in the DTS file if the I2C address is unique.

-- 
Jean Delvare

Re: [Patch] MPC Adapter: read class attribute from device tree

[Wolfgang Grandegger]

Jean Delvare wrote:
> On Tue, 21 Apr 2009 12:35:49 +0200, Wolfgang Grandegger wrote:
>> Michael Lawnick wrote:
>>> Wolfgang Grandegger said the following:
>>>> Do we really need that? Probing is dangerous and not necessary. Does it
>>>> not work with a proper DTS entry? But maybe I have missed something?
>>> Our current and our next system consists of two flavors of the same
>>> board with different devices. To minimize maintenance and testing we use
>>> a reduced kernel and load the needed modules at runtime. Furthermore we
>>> will have to handle hot-plugged I2C-devices. Whether this strategy is
>>> best could be discussed in another thread but is rather OT in this
>>> mailing list. Nevertheless loading modules at runtime is legal and
>>> generally supported by LINUX.
>>>
>>> Defining all possible (I2C-)devices in DTS would give a mess. E.g. on
>>> one board there will be ~30 temperature sensors, on the other none.
>> The legacy device probing has it's limitations and I doubt that it will
>> be able to find 30 DTT devices properly. E.g. the legacy probing
>> algorithm is not able to distinguish between a LM75 and DS75. Let's
>> first try to find out why it does not work as expected with the DTS entry.
>>
>>> As every DTS entry will force a sysFs subdirectory there would be a
>>> bunch of functionless directories - rather ugly.
>> Hopefully not. They will be created when the probing was successful, I
>> assume.
> 
> No, device creation is unconditional. It is assumed that the DTS file
> author knew what he/she was doing.
> 
> There was a discussion about adding a flag to state the "may not be
> present" nature of devices, which would result in a probe before
> instantiating the device (on success), but it wasn't implemented yet. I
> have no objection is this mechanism is deemed useful.

Ah, I see. If the software can reliably find out if a device exists it
should probe for it. It would be useful to limit the amount of required
DTS files for the hardware variants. Often the RTC or other I2C devices
are optional. I added the Deviceterr-disccuss ML to the CC to reach the
the device-tree gurus.

Wolfgang.

Re: [Patch] MPC Adapter: read class attribute from device tree

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1460 bytes --]


> For MPC adapter there is no class assigned as it is done in other
> adapters.

Still, the trend is to use the class field less, not more often. Probing
causes more pain than gain. Most of us have been through that, try to
believe us.

> This way no new-style client will ever be instantiated.

Sorry, this seems wrong to me:

1) The client _instance_ does come from the dts, as Jean explained
before. Did you get the difference? You said yourself that you could see
the entries in sysfs. It seems just that it does not get connected to
the driver. This should be the place to fix the error.

2) When I showed you the output of modprobing the rtc-ds1307 driver, I
was using exactly this i2c-master driver (the board uses an MPC5200B).
Plus, I often use at24 as a module. Doesn't look like a problem in the
infrastructure to me.

> this patch class assignment is read from device tree.
> Necessary entry:
> class = <1>; /* I2C_CLASS_HWMON (iic.h) */

The device tree is OS independent. You are encoding something
linux-specific into it. This is regularly rejected. I am quite surprised
to see that there is such a thing in i2c-cpm. Lucky punch, I guess ;)
Should be removed IMHO.

Have you already posted the DTS snipplet for your I2C setup?

Regards,

   Wolfram

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

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

Re: [Patch] MPC Adapter: read class attribute from device tree

[Jean Delvare]

On Tue, 21 Apr 2009 08:42:27 +0200, Michael Lawnick wrote:
> For MPC adapter there is no class assigned as it is done in other
> adapters. This way no new-style client will ever be instantiated.

This statement is incorrect. How many times will I have to repeat
myself?

> With
> this patch class assignment is read from device tree.
> Necessary entry:
> class = <1>; /* I2C_CLASS_HWMON (iic.h) */

Hard-coding the value of Linux-specific flags in the openfirmware
device tree doesn't look too good to me.

> 
> Based on 2.6.29
> 
> Signed-off-by: Michael Lawnick <ml.lawnick-Mmb7MZpHnFY@public.gmane.org>
> Cc: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> Cc: Sang, Wolfram <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-mpc.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
> --- a/drivers/i2c/busses/i2c-mpc.c
> +++ b/drivers/i2c/busses/i2c-mpc.c
> @@ -318,6 +318,7 @@ static int __devinit fsl_i2c_probe(struct of_device
> *op, const struct of_device_
>  {
>  	int result = 0;
>  	struct mpc_i2c *i2c;
> +	int *of_val;
> 
>  	i2c = kzalloc(sizeof(*i2c), GFP_KERNEL);
>  	if (!i2c)
> @@ -354,6 +355,10 @@ static int __devinit fsl_i2c_probe(struct of_device
> *op, const struct of_device_
>  	dev_set_drvdata(&op->dev, i2c);
> 
>  	i2c->adap = mpc_ops;
> +	of_val = of_get_property(op->node, "class", NULL);
> +	if(of_val)

Incorrect coding style. Didn't you run scripts/checkpatch.pl on your
patch before submitting it?

> +		i2c->adap.class = *of_val;
> +
>  	i2c_set_adapdata(&i2c->adap, i2c);
>  	i2c->adap.dev.parent = &op->dev;
> 

Anyway: Wolfram and Wolfgang have explained to you how to do it right
and the above isn't that. Declaring the I2C devices in the device tree
works for everybody else, it has to work for you as well, if you do it
right.

-- 
Jean Delvare