Re: [patch] i2c: announce SMBus host controllers

Re: [patch] i2c: announce SMBus host controllers

[Jean Delvare]

Hi Bjorn,

Le 21/12/2007, Bjorn Helgaas a écrit:
>Would you consider adding a patch like this to your i2c tree?
>Unfortunately, I don't think I have hardware for any of these
>devices, so all I have done is compiled this.
>(...)
>i2c: announce SMBus host controllers
>
>Note where we find SMBus host controllers and what resources they use.

I like the idea. The information printed is somewhat redundant with
/proc/ioports but sometimes it's useful to see the information in dmesg
directly, and having a common string to grep for should be convenient as
well. Just a couple comments:

* i2c-i810, i2c-prosavage, i2c-savage4 and i2c-voodoo3 are NOT SMBus host
controller drivers. They do I2C/DDC over GPIO for graphics adapters. I
don't think that you really need to know about the resources they use,
they are not candidates for conflicts with ACPI / BIOS / whatever. And 3
out of 4 are deprecated, so all in all they are probably just not worth
modifying.

* I think that you should make sure that the device has been successfully
registered before you print the message, otherwise it might be
confusing. I only looked at i2c-pasemi and scx200_acb and for both
drivers you print the message at a point where a failure could still
happen. Please double-check for all drivers, adjust as needed and
resubmit.

I can test the i2c-i801 driver now and i2c-viapro and i2c-piix4 later.

--
Jean Delvare

Re: [patch] i2c: announce SMBus host controllers

[Bjorn Helgaas]

On Saturday 22 December 2007 9:10:44 am Jean Delvare wrote:
> Hi Bjorn,
>
> Le 21/12/2007, Bjorn Helgaas a écrit:
> >Would you consider adding a patch like this to your i2c tree?
> >Unfortunately, I don't think I have hardware for any of these
> >devices, so all I have done is compiled this.
> >(...)
> >i2c: announce SMBus host controllers
> >
> >Note where we find SMBus host controllers and what resources they use.
>
> I like the idea. The information printed is somewhat redundant with
> /proc/ioports but sometimes it's useful to see the information in dmesg
> directly, and having a common string to grep for should be convenient as
> well. Just a couple comments:
>
> * i2c-i810, i2c-prosavage, i2c-savage4 and i2c-voodoo3 are NOT SMBus host
> controller drivers. They do I2C/DDC over GPIO for graphics adapters. I
> don't think that you really need to know about the resources they use,
> they are not candidates for conflicts with ACPI / BIOS / whatever. And 3
> out of 4 are deprecated, so all in all they are probably just not worth
> modifying.
>
> * I think that you should make sure that the device has been successfully
> registered before you print the message, otherwise it might be
> confusing. I only looked at i2c-pasemi and scx200_acb and for both
> drivers you print the message at a point where a failure could still
> happen. Please double-check for all drivers, adjust as needed and
> resubmit.
>
> I can test the i2c-i801 driver now and i2c-viapro and i2c-piix4 later.

Thanks for the comments!  My work machine is powered off for the
holidays, so I'll rework the patch after the new year.

Bjorn

Re: [patch] i2c: announce SMBus host controllers

[Bjorn Helgaas]

On Saturday 22 December 2007 09:10:44 am Jean Delvare wrote:
> Le 21/12/2007, Bjorn Helgaas a écrit:
> >Would you consider adding a patch like this to your i2c tree?
> >Unfortunately, I don't think I have hardware for any of these
> >devices, so all I have done is compiled this.
> >(...)
> >i2c: announce SMBus host controllers
> >
> >Note where we find SMBus host controllers and what resources they use.
> ...

> * i2c-i810, i2c-prosavage, i2c-savage4 and i2c-voodoo3 are NOT SMBus host
> controller drivers. They do I2C/DDC over GPIO for graphics adapters. I
> don't think that you really need to know about the resources they use,
> they are not candidates for conflicts with ACPI / BIOS / whatever. And 3
> out of 4 are deprecated, so all in all they are probably just not worth
> modifying.

I removed the patches for these drivers.

> * I think that you should make sure that the device has been successfully
> registered before you print the message, otherwise it might be
> confusing. I only looked at i2c-pasemi and scx200_acb and for both
> drivers you print the message at a point where a failure could still
> happen. Please double-check for all drivers, adjust as needed and
> resubmit.

Take a look at i2c-ali1535.c.  I put the new printk in ali1535_setup(),
which does PCI initialization, even though a failure could happen later
in i2c_add_adapter().

I picked that because putting the new printk in ali1535_probe() would
mean:
  - the printk would be far removed from the request_region(), so it's
    a little harder to be sure the "ali1535_smba + ALI1535_SMB_IOSIZE - 1"
    expression matches,
  - I wouldn't be able print the SMBREV value, and
  - I'd have to add a temporary to check the return from
    i2c_add_adapter() instead of just returning it directly,

I'd still be glad to change them if you prefer.  I just want to do
them all the same (either all before or all after i2c_add_adapter()).
Let me know your preference.

Bjorn

Re: [patch] i2c: announce SMBus host controllers

[Jean Delvare]

Hi Bjorn,

On Wed, 2 Jan 2008 16:35:04 -0700, Bjorn Helgaas wrote:
> On Saturday 22 December 2007 09:10:44 am Jean Delvare wrote:
> > * I think that you should make sure that the device has been successfully
> > registered before you print the message, otherwise it might be
> > confusing. I only looked at i2c-pasemi and scx200_acb and for both
> > drivers you print the message at a point where a failure could still
> > happen. Please double-check for all drivers, adjust as needed and
> > resubmit.
> 
> Take a look at i2c-ali1535.c.  I put the new printk in ali1535_setup(),
> which does PCI initialization, even though a failure could happen later
> in i2c_add_adapter().
> 
> I picked that because putting the new printk in ali1535_probe() would
> mean:
>   - the printk would be far removed from the request_region(), so it's
>     a little harder to be sure the "ali1535_smba + ALI1535_SMB_IOSIZE - 1"
>     expression matches,
>   - I wouldn't be able print the SMBREV value, and
>   - I'd have to add a temporary to check the return from
>     i2c_add_adapter() instead of just returning it directly,
> 
> I'd still be glad to change them if you prefer.  I just want to do
> them all the same (either all before or all after i2c_add_adapter()).
> Let me know your preference.

If moving the message makes the code mode complex, then I agree that
you shouldn't do that. After all, i2c_add_adapter() is very unlikely to
fail, and if it does, I certainly hope that it'll clearly show in the
kernel log so that no confusion is possible.

So just do what you think is best and I'll take that.

Thanks,
-- 
Jean Delvare

Re: [patch] i2c: announce SMBus host controllers

[Jean Delvare]

Oh BTW your patch yields the following warning on x86_64:

drivers/i2c/busses/i2c-i801.c: In function "i801_probe":
drivers/i2c/busses/i2c-i801.c:669: warning: format "%lx" expects type "long unsigned int", but argument 5 has type "long long unsigned int"

Please fix in the next iteration of your patch.

-- 
Jean Delvare