tech-kern archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

Re: Yet another dirct config proposal for i2c busses

I like this approach. I'll try to find some time in the next few days to build and run my amd64 boxes with your patches; at the very least, that should verify that indirect config still works.

BTW, there are several other i2c drivers that do the address-and-mask hack. :) Eventually, they should all be modified in the same way as your spdmem.c changes...

On Wed, 3 Feb 2010, Martin Husemann wrote:


we still have to solve the problem of "scanning" i2c busses, especially
on machines where no scan is needed since the firmware happily tells
us everything we might want to know.

In the past (as far as I remember) two proposal where presented and
both shot down. Now I have a machine where I realy needed the i2c
based fan controll to make the noise bearble - so I put on my asbestos
suit and wrote another implementation, which I'd like to present here.

The previous proposals:

- use the OpenBSD way: an optional "scan" callback provided by the
  i2c controller driver. Downsides: needs changes ~every i2c controller
  driver in-tree
- the macppc way: see macppc/dev/kiic* - basically a slightly different
  bus, needs frontend/backend split of i2c device drivers and a lot
  of additional frontends to be written

I may misrember details and critics raised against one of those.

Goals I tried to achieve:

- Allow both direct and indirect config at the i2c bus layer,
  depending on availability of firmware provided locators
- Allow unmodified i2c device drivers to continue working
- Keep MD changes as simple and small as possible
- No changes to MI i2c bus controllers
- Allow MD i2c bus controllers to easily override the generic
  behaviour (i.e.: provide additional locators or modify firmware
  provided ones)

Seems like it worked out, and the changes are pretty small in the end.

Quick overview how it works:

- If we are doing direct config, MD code (via generic support routines,
  or by overriding those) adds a prop_array to the device properties
  of the i2c bus controller (the parent(!) of the i2c bus). This array
  contains a dictionary for each i2c device on the bus. Entries in
  this dictionary are:
    "name" -> string, device name
    "address" -> uint32, i2c address
    "size" -> uint32 (optional)
    "compatibility" -> a list of names, i.e. the chip used, used for
       matching a hardware driver (think: alternative "name" props)

- When the i2c bus attaches, it queries the device properties of it's
  parent device and checks the "i2c-child-devices" property (the array
  described above), and if it is present, iterates over the array
  creating i2c_attach_arg from it. To allow direct config matches,
  the i2c_attach_args structure has been extended.

  If the device property is not available, indirect config is done.

- An i2c device driver for a proper device will need no changes, but
  for i.e. write-only devices matching based on strings can be added.
  A generic helper function to match the "compatible" string list against
  a driver specific list is provided.

A few more details:

Let's start with setting up the device property. In the attached sparc64 MD
code the setup is done inside device_register() whenever a "iic" device
attaches and there is not yet a "i2c-child-devices" property at the parent.
This check is needed to allow MD i2c controller drivers to override
the generic behaviour. For OpenFirmware based machines, a convenience
funtction "of_enter_i2c_devs()" to do the device property setup is provided.

Next step: the i2c bus attaches and checks for the device property. This is
all done in the i2c bus code, no i2c controller driver needs modifications.

Last part of the puzzle: the i2c device drivers can check for the (new)
ia_name pointer in the i2c_attach_args structure to find out if direct
config is available. For example the spdmem driver does a (nasty/stupid?)
check for certain address values - which does not make any sense in the
direct config case:

@@ -164,8 +165,17 @@ spdmem_match(device_t parent, cfdata_t m
       int spd_len, spd_crc_cover;
       uint16_t crc_calc, crc_spd;

-       if ((ia->ia_addr & SPDMEM_ADDRMASK) != SPDMEM_ADDR)
-               return 0;
+       if (ia->ia_name) {
+               /* add other names as we find more firmware variations */
+               if (strcmp(ia->ia_name, "dimm-spd"))
+                       return 0;
+       }
+       /* only do this lame test when not using direct config */
+       if (ia->ia_name == NULL) {
+               if ((ia->ia_addr & SPDMEM_ADDRMASK) != SPDMEM_ADDR)
+                       return 0;
+       }

       sc.sc_tag = ia->ia_tag;
       sc.sc_addr = ia->ia_addr;

If the firmware name is not a good indicator of the driver to use, the
"compatible" list can be used, via the generic iic_compat_match()

I have tested this on a few sparc64 machines, it works for me.
I won't mind if we decide to not used this but go with one of the older
proposals instead - but we need to move on.



|   Paul Goyette   | PGP DSS Key fingerprint: |  E-mail addresses:      |
| Customer Service | FA29 0E3B 35AF E8AE 6651 |  paul at   |
| Network Engineer | 0786 F758 55DE 53BA 7731 | pgoyette at |
| Kernel Developer |                          | pgoyette at  |

Home | Main Index | Thread Index | Old Index