SFC driver implements its own I2C support

SFC driver implements its own I2C support

[Jean Delvare]

Hi Ben, hi Jeff,

My attention was brought to the drivers/net/sfc/i2c-direct.c file which
appeared recently in the Linux kernel. Apparently this is a custom
implementation of I2C support, including software bit-banging. All this
is already available in the Linux kernel in:
  drivers/i2c/i2c-core.c
  drivers/i2c/algos/i2c-algo-bit.c
So it really doesn't make sense to duplicate the code in the sfc
network driver.

Can you please get rid of this custom I2C implementation and use the
standard one? It should be really easy, all you have to do is include
<linux/i2c.h> and <linux/i2c-algo-bit.h>, pack your SDA and SCL get/set
functions in a struct i2c_algo_bit_data, and call i2c_bit_add_bus() on
it. See drivers/video/intelfb/intelfb_i2c.c (or any framebuffer driver
with I2C/DDC support) for an example.

Using the standard I2C implementation will give you access to all the
i2c debugging tools, user-space access to the I2C bus if needed, and
compatibility with all the Linux i2c device drivers, including hardware
monitoring and GPIO drivers which apparently the SFC hardware uses.

Thanks,
-- 
Jean Delvare

Re: SFC driver implements its own I2C support

[Ben Hutchings]

Jean Delvare wrote:
> Hi Ben, hi Jeff,

Please use the address linux-net-drivers@solarflare.com, as listed in
MAINTAINERS, which goes to everyone working on the sfc driver here.

> My attention was brought to the drivers/net/sfc/i2c-direct.c file which
> appeared recently in the Linux kernel. Apparently this is a custom
> implementation of I2C support, including software bit-banging. All this
> is already available in the Linux kernel in:
>   drivers/i2c/i2c-core.c
>   drivers/i2c/algos/i2c-algo-bit.c
> So it really doesn't make sense to duplicate the code in the sfc
> network driver.
>
> Can you please get rid of this custom I2C implementation and use the
> standard one?

We intend to do so, but may not be able to do it before 2.6.26.

> It should be really easy, all you have to do is include
> <linux/i2c.h> and <linux/i2c-algo-bit.h>, pack your SDA and SCL get/set
> functions in a struct i2c_algo_bit_data, and call i2c_bit_add_bus() on
> it. See drivers/video/intelfb/intelfb_i2c.c (or any framebuffer driver
> with I2C/DDC support) for an example.

I have made a start on this, and it looked like it would be a bit more
difficult than that.  But then perhaps I was doing more work than I
really needed to.

Also, see comments from the original author of our I2C code in
<http://thread.gmane.org/gmane.linux.kernel.commits.head/145999/focus=92866>.

> Using the standard I2C implementation will give you access to all the
> i2c debugging tools, user-space access to the I2C bus if needed, and
> compatibility with all the Linux i2c device drivers, including hardware
> monitoring and GPIO drivers which apparently the SFC hardware uses.

Right.  But the most immediate requirement is that we be able to
configure I2C devices from the sfc driver.  Most of our reference
designs require that as part of their initialisation.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.

Re: SFC driver implements its own I2C support

[Jean Delvare]

Hi Ben,

On Wed, 14 May 2008 22:58:20 +0100, Ben Hutchings wrote:
> Jean Delvare wrote:
> > Hi Ben, hi Jeff,
> 
> Please use the address linux-net-drivers@solarflare.com, as listed in
> MAINTAINERS, which goes to everyone working on the sfc driver here.

OK, will do.

> > My attention was brought to the drivers/net/sfc/i2c-direct.c file which
> > appeared recently in the Linux kernel. Apparently this is a custom
> > implementation of I2C support, including software bit-banging. All this
> > is already available in the Linux kernel in:
> >   drivers/i2c/i2c-core.c
> >   drivers/i2c/algos/i2c-algo-bit.c
> > So it really doesn't make sense to duplicate the code in the sfc
> > network driver.
> >
> > Can you please get rid of this custom I2C implementation and use the
> > standard one?
> 
> We intend to do so, but may not be able to do it before 2.6.26.

OK, thanks. I understand that it's a bit late for 2.6.26, but you
should work on it now so that it's ready for the 2.6.27 merge window.

> > It should be really easy, all you have to do is include
> > <linux/i2c.h> and <linux/i2c-algo-bit.h>, pack your SDA and SCL get/set
> > functions in a struct i2c_algo_bit_data, and call i2c_bit_add_bus() on
> > it. See drivers/video/intelfb/intelfb_i2c.c (or any framebuffer driver
> > with I2C/DDC support) for an example.
> 
> I have made a start on this, and it looked like it would be a bit more
> difficult than that.  But then perhaps I was doing more work than I
> really needed to.

If you come to the conclusion that either i2c-core or i2c-algo-bit
misses something for you to be able (and happy) to use the standard i2c
subsystem, please let me know. I'd rather add the missing pieces there
for everybody to benefit than add yet another reimplementation of i2c.
There are way too many already :(

If you need help with the conversion, just ask me or on the i2c list.

> Also, see comments from the original author of our I2C code in
> <http://thread.gmane.org/gmane.linux.kernel.commits.head/145999/focus=92866>.

Quoting this post:

> Last time I checked (i.e. when I originally wrote this bit of the code), 
> the kernel's own i2c layer didn't provide any clean way for kernel code 
> (rather than user code) to access i2c devices.

I am not sure what Michael was referring to exactly, but access to i2c
devices from the kernel has been supported pretty much forever. Maybe
he really meant access to hardware monitoring devices? For these indeed
there is a standard user-space interface (through sysfs) but no
standard in-kernel access; mainly because there has never been a clear
need for one. Again, if you need something, please discuss it on the
relevant mailing lists and we'll find a way for you to use the standard
subsystems rather than reimplementing them for your own use.

> > Using the standard I2C implementation will give you access to all the
> > i2c debugging tools, user-space access to the I2C bus if needed, and
> > compatibility with all the Linux i2c device drivers, including hardware
> > monitoring and GPIO drivers which apparently the SFC hardware uses.
> 
> Right.  But the most immediate requirement is that we be able to
> configure I2C devices from the sfc driver.  Most of our reference
> designs require that as part of their initialisation.

The changes that went into the i2c subsystem since kernel 2.6.22 should
make it possible and relatively easy. You can instantiate the I2C
devices you know are present on each adapter, and access them directly
for specific configuration as needed. For hardware monitoring devices,
you will have to add support for explicit device creation (aka
new-style i2c driver), only a couple of hardware monitoring drivers have
been converted at the moment (in particular the lm87 driver has not.)
Again I can help with that if needed.

Thanks,
-- 
Jean Delvare

Re: SFC driver implements its own I2C support

[Michael Brown]

On Thu, 15 May 2008, Jean Delvare wrote:
> > Last time I checked (i.e. when I originally wrote this bit of the 
> > code), the kernel's own i2c layer didn't provide any clean way for 
> > kernel code (rather than user code) to access i2c devices.
> 
> I am not sure what Michael was referring to exactly, but access to i2c 
> devices from the kernel has been supported pretty much forever. Maybe he 
> really meant access to hardware monitoring devices? For these indeed 
> there is a standard user-space interface (through sysfs) but no standard 
> in-kernel access; mainly because there has never been a clear need for 
> one. Again, if you need something, please discuss it on the relevant 
> mailing lists and we'll find a way for you to use the standard 
> subsystems rather than reimplementing them for your own use.

>From memory (and this may be inaccurate), it looked as though the i2c 
subsystem code for EEPROM access was contained within 
drivers/i2c/chips/eeprom.c, and that this code provided an interface for 
userspace to access the EEPROM contents, but no interface for kernel code 
to do so.

I think there may also have been issues with the fact that the i2c system 
allows for failures on the shutdown path (e.g. i2c_detach_client() can 
return a failure), which becomes awkward to handle when you are in the 
middle of a shutdown path that is not allowed to fail (e.g. a
pci_driver->remove method).

Michael

Re: SFC driver implements its own I2C support

[Jean Delvare]

On Thu, 15 May 2008 13:04:50 +0100 (BST), Michael Brown wrote:
> On Thu, 15 May 2008, Jean Delvare wrote:
> > > Last time I checked (i.e. when I originally wrote this bit of the 
> > > code), the kernel's own i2c layer didn't provide any clean way for 
> > > kernel code (rather than user code) to access i2c devices.
> > 
> > I am not sure what Michael was referring to exactly, but access to i2c 
> > devices from the kernel has been supported pretty much forever. Maybe he 
> > really meant access to hardware monitoring devices? For these indeed 
> > there is a standard user-space interface (through sysfs) but no standard 
> > in-kernel access; mainly because there has never been a clear need for 
> > one. Again, if you need something, please discuss it on the relevant 
> > mailing lists and we'll find a way for you to use the standard 
> > subsystems rather than reimplementing them for your own use.
> 
> From memory (and this may be inaccurate), it looked as though the i2c 
> subsystem code for EEPROM access was contained within 
> drivers/i2c/chips/eeprom.c, and that this code provided an interface for 
> userspace to access the EEPROM contents, but no interface for kernel code 
> to do so.

This is correct and is still the case. This eeprom driver is a simple
way to export small EEPROM data to user-space in read-only mode. It
should not be considered as a real driver for EEPROM access. For that,
please see the upcoming at24c driver:
  http://lists.lm-sensors.org/pipermail/i2c/2008-April/003307.html
Apparently there are a lot of people interested in this so I will have
to find some time to finally review and merge the driver if it is ready
to go. Help with testing and review is welcome.

> I think there may also have been issues with the fact that the i2c system 
> allows for failures on the shutdown path (e.g. i2c_detach_client() can 
> return a failure), which becomes awkward to handle when you are in the 
> middle of a shutdown path that is not allowed to fail (e.g. a
> pci_driver->remove method).

This part of the code has been reworked completely. That being said, I
see that the i2c_driver remove method returns an int not void, so it
can still fail. There doesn't seem to be a common behavior in this
respect amongst subsystems, some allow the remove method to fail and
others don't. If you think that the i2c subsystem should not let remove
methods fail, we can discuss this. But most likely, the error code is
purely informative so it doesn't really matter if we return it or void.

-- 
Jean Delvare

Re: [i2c] SFC driver implements its own I2C support

[Wolfram Sang]

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

On Thu, May 15, 2008 at 02:39:54PM +0200, Jean Delvare wrote:
> Apparently there are a lot of people interested in this so I will have
> to find some time to finally review and merge the driver if it is ready
> to go. Help with testing and review is welcome.
Will send another version today (updated and ported to 2.6.26).
Promised!

   Wolfram

-- 
  Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de
 Pengutronix - Linux Solutions for Science and Industry

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

Re: SFC driver implements its own I2C support

[Michael Brown]

On Thu, 15 May 2008, Jean Delvare wrote:
> > I think there may also have been issues with the fact that the i2c 
> > system allows for failures on the shutdown path (e.g. 
> > i2c_detach_client() can return a failure), which becomes awkward to 
> > handle when you are in the middle of a shutdown path that is not 
> > allowed to fail (e.g. a pci_driver->remove method).
> 
> This part of the code has been reworked completely. That being said, I 
> see that the i2c_driver remove method returns an int not void, so it can 
> still fail. There doesn't seem to be a common behavior in this respect 
> amongst subsystems, some allow the remove method to fail and others 
> don't. If you think that the i2c subsystem should not let remove methods 
> fail, we can discuss this. But most likely, the error code is purely 
> informative so it doesn't really matter if we return it or void.

It does matter if the i2c remove failure indicates that data structures 
are still left linked in to various lists etc.

Michael

Re: [i2c] SFC driver implements its own I2C support

[Riku Voipio]

Ben Hutchings wrote:
>> Using the standard I2C implementation will give you access to all the
>> i2c debugging tools, user-space access to the I2C bus if needed, and
>> compatibility with all the Linux i2c device drivers, including hardware
>> monitoring and GPIO drivers which apparently the SFC hardware uses.
>>     
>
> Right.  But the most immediate requirement is that we be able to
> configure I2C devices from the sfc driver.  Most of our reference
> designs require that as part of their initialisation.

That's how many DVB adapter drivers already work. Although
I'm not sure which one would be cleanest and simplest example.