Re: [PATCH 2/2] i2c, i2c_imc: Add DIMM bus code
On Sat, Mar 7, 2015 at 8:03 AM, Guenter Roeck wrote: > On 03/06/2015 06:50 PM, Andy Lutomirski wrote: >> >> Add i2c_scan_dimm_bus to declare that a particular i2c_adapter >> contains DIMMs. This will probe (and autoload modules!) for useful >> SMBUS devices that live on DIMMs. i2c_imc calls it. >> >> As more SMBUS-addressable DIMM components become supported, this >> code can be extended to probe for them. >> >> Signed-off-by: Andy Lutomirski > > > Similar to the other patch, I would suggest to run it through checkpatch. > Done for both patches. > >> --- >> drivers/i2c/busses/Kconfig| 4 ++ >> drivers/i2c/busses/Makefile | 4 ++ >> drivers/i2c/busses/dimm-bus.c | 97 >> +++ >> drivers/i2c/busses/i2c-imc.c | 3 ++ >> include/linux/i2c/dimm-bus.h | 24 +++ >> 5 files changed, 132 insertions(+) >> create mode 100644 drivers/i2c/busses/dimm-bus.c >> create mode 100644 include/linux/i2c/dimm-bus.h >> >> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig >> index d6b9ce164fbf..2ea6648492eb 100644 >> --- a/drivers/i2c/busses/Kconfig >> +++ b/drivers/i2c/busses/Kconfig >> @@ -149,6 +149,10 @@ config I2C_ISMT >> This driver can also be built as a module. If so, the module >> will be >> called i2c-ismt. >> >> +config I2C_DIMM_BUS >> + tristate >> + default n >> + >> config I2C_IMC >> tristate "Intel iMC (LGA 2011) SMBus Controller" >> depends on PCI && X86 >> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile >> index 4287c891e782..a01bdcc0e239 100644 >> --- a/drivers/i2c/busses/Makefile >> +++ b/drivers/i2c/busses/Makefile >> @@ -25,6 +25,10 @@ obj-$(CONFIG_I2C_SIS96X) += i2c-sis96x.o >> obj-$(CONFIG_I2C_VIA) += i2c-via.o >> obj-$(CONFIG_I2C_VIAPRO) += i2c-viapro.o >> >> +# DIMM busses >> +obj-$(CONFIG_I2C_DIMM_BUS) += dimm-bus.o >> +obj-$(CONFIG_I2C_IMC) += i2c-imc.o >> + >> # Mac SMBus host controller drivers >> obj-$(CONFIG_I2C_HYDRA) += i2c-hydra.o >> obj-$(CONFIG_I2C_POWERMAC)+= i2c-powermac.o >> diff --git a/drivers/i2c/busses/dimm-bus.c b/drivers/i2c/busses/dimm-bus.c >> new file mode 100644 >> index ..096842811199 >> --- /dev/null >> +++ b/drivers/i2c/busses/dimm-bus.c >> @@ -0,0 +1,97 @@ >> +/* >> + * Copyright (c) 2013 Andrew Lutomirski > > > 2013 ? > Fixed. It's been a long time since I wrote most of this code... > >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 >> + * as published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + * >> + * You should have received a copy of the GNU General Public License >> + * along with this program; if not, write to the Free Software >> + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA. >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> + >> +static bool probe_addr(struct i2c_adapter *adapter, int addr) >> +{ >> + /* >> +* So far, all known devices that live on DIMMs can be safely >> +* and reliably detected by trying to read a byte at address >> +* zero. (The exception is the SPD write protection control, >> +* which can't be probed and requires special hardware and/or >> +* quick writes to access, and has no driver.) >> +*/ > > > That leads to the question if the controller supports quick commands, > and if it does, if would make sense to add support for it, > just for the purpose of reducing risk here. I don't think it supports them, and even if it did, I don't know how to test them since I don't have an obvious i2c slave device to poke at with them. I have a friend with logic analyzers, though... > > >> + union i2c_smbus_data dummy; >> + return i2c_smbus_xfer(adapter, addr, 0, I2C_SMBUS_READ, 0, >> + I2C_SMBUS_BYTE_DATA, ) >= 0; >> +} >> + >> +/** >> + * i2c_scan_dimm_bus() - Scans an SMBUS segment known to contain DIMMs >> + * @adapter: The SMBUS adapter to scan >> + * >> + * This function tells the DIMM-bus code that the adapter is known to >> + * contain DIMMs. i2c_scan_dimm_bus will probe for devices known to >> + * live on DIMMs. >> + * >> + * Do NOT call this function on general-purpose system SMBUS segments >> + * unless you know that the only things on the bus are DIMMs. >> + * Otherwise is it very likely to mis-identify other things on the >> + * bus. >> + * >> + * Callers are advised not to set adapter->class = I2C_CLASS_SPD. > > > Why not ? > It could make the legacy eeprom driver try to bind to the bus if it's loaded. I improved
Re: [PATCH 2/2] i2c, i2c_imc: Add DIMM bus code
On Sat, Mar 7, 2015 at 8:03 AM, Guenter Roeck li...@roeck-us.net wrote: On 03/06/2015 06:50 PM, Andy Lutomirski wrote: Add i2c_scan_dimm_bus to declare that a particular i2c_adapter contains DIMMs. This will probe (and autoload modules!) for useful SMBUS devices that live on DIMMs. i2c_imc calls it. As more SMBUS-addressable DIMM components become supported, this code can be extended to probe for them. Signed-off-by: Andy Lutomirski l...@amacapital.net Similar to the other patch, I would suggest to run it through checkpatch. Done for both patches. --- drivers/i2c/busses/Kconfig| 4 ++ drivers/i2c/busses/Makefile | 4 ++ drivers/i2c/busses/dimm-bus.c | 97 +++ drivers/i2c/busses/i2c-imc.c | 3 ++ include/linux/i2c/dimm-bus.h | 24 +++ 5 files changed, 132 insertions(+) create mode 100644 drivers/i2c/busses/dimm-bus.c create mode 100644 include/linux/i2c/dimm-bus.h diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index d6b9ce164fbf..2ea6648492eb 100644 --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -149,6 +149,10 @@ config I2C_ISMT This driver can also be built as a module. If so, the module will be called i2c-ismt. +config I2C_DIMM_BUS + tristate + default n + config I2C_IMC tristate Intel iMC (LGA 2011) SMBus Controller depends on PCI X86 diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile index 4287c891e782..a01bdcc0e239 100644 --- a/drivers/i2c/busses/Makefile +++ b/drivers/i2c/busses/Makefile @@ -25,6 +25,10 @@ obj-$(CONFIG_I2C_SIS96X) += i2c-sis96x.o obj-$(CONFIG_I2C_VIA) += i2c-via.o obj-$(CONFIG_I2C_VIAPRO) += i2c-viapro.o +# DIMM busses +obj-$(CONFIG_I2C_DIMM_BUS) += dimm-bus.o +obj-$(CONFIG_I2C_IMC) += i2c-imc.o + # Mac SMBus host controller drivers obj-$(CONFIG_I2C_HYDRA) += i2c-hydra.o obj-$(CONFIG_I2C_POWERMAC)+= i2c-powermac.o diff --git a/drivers/i2c/busses/dimm-bus.c b/drivers/i2c/busses/dimm-bus.c new file mode 100644 index ..096842811199 --- /dev/null +++ b/drivers/i2c/busses/dimm-bus.c @@ -0,0 +1,97 @@ +/* + * Copyright (c) 2013 Andrew Lutomirski l...@amacapital.net 2013 ? Fixed. It's been a long time since I wrote most of this code... + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA. + */ + +#include linux/i2c.h +#include linux/bug.h +#include linux/module.h +#include linux/i2c/dimm-bus.h + +static bool probe_addr(struct i2c_adapter *adapter, int addr) +{ + /* +* So far, all known devices that live on DIMMs can be safely +* and reliably detected by trying to read a byte at address +* zero. (The exception is the SPD write protection control, +* which can't be probed and requires special hardware and/or +* quick writes to access, and has no driver.) +*/ That leads to the question if the controller supports quick commands, and if it does, if would make sense to add support for it, just for the purpose of reducing risk here. I don't think it supports them, and even if it did, I don't know how to test them since I don't have an obvious i2c slave device to poke at with them. I have a friend with logic analyzers, though... + union i2c_smbus_data dummy; + return i2c_smbus_xfer(adapter, addr, 0, I2C_SMBUS_READ, 0, + I2C_SMBUS_BYTE_DATA, dummy) = 0; +} + +/** + * i2c_scan_dimm_bus() - Scans an SMBUS segment known to contain DIMMs + * @adapter: The SMBUS adapter to scan + * + * This function tells the DIMM-bus code that the adapter is known to + * contain DIMMs. i2c_scan_dimm_bus will probe for devices known to + * live on DIMMs. + * + * Do NOT call this function on general-purpose system SMBUS segments + * unless you know that the only things on the bus are DIMMs. + * Otherwise is it very likely to mis-identify other things on the + * bus. + * + * Callers are advised not to set adapter-class = I2C_CLASS_SPD. Why not ? It could make the legacy eeprom driver try to bind to the bus if it's loaded. I improved the comment. + */ +void i2c_scan_dimm_bus(struct i2c_adapter *adapter) +{ + struct i2c_board_info info = {}; +
Re: [PATCH 2/2] i2c, i2c_imc: Add DIMM bus code
On 03/06/2015 06:50 PM, Andy Lutomirski wrote: Add i2c_scan_dimm_bus to declare that a particular i2c_adapter contains DIMMs. This will probe (and autoload modules!) for useful SMBUS devices that live on DIMMs. i2c_imc calls it. As more SMBUS-addressable DIMM components become supported, this code can be extended to probe for them. Signed-off-by: Andy Lutomirski Similar to the other patch, I would suggest to run it through checkpatch. --- drivers/i2c/busses/Kconfig| 4 ++ drivers/i2c/busses/Makefile | 4 ++ drivers/i2c/busses/dimm-bus.c | 97 +++ drivers/i2c/busses/i2c-imc.c | 3 ++ include/linux/i2c/dimm-bus.h | 24 +++ 5 files changed, 132 insertions(+) create mode 100644 drivers/i2c/busses/dimm-bus.c create mode 100644 include/linux/i2c/dimm-bus.h diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index d6b9ce164fbf..2ea6648492eb 100644 --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -149,6 +149,10 @@ config I2C_ISMT This driver can also be built as a module. If so, the module will be called i2c-ismt. +config I2C_DIMM_BUS + tristate + default n + config I2C_IMC tristate "Intel iMC (LGA 2011) SMBus Controller" depends on PCI && X86 diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile index 4287c891e782..a01bdcc0e239 100644 --- a/drivers/i2c/busses/Makefile +++ b/drivers/i2c/busses/Makefile @@ -25,6 +25,10 @@ obj-$(CONFIG_I2C_SIS96X) += i2c-sis96x.o obj-$(CONFIG_I2C_VIA) += i2c-via.o obj-$(CONFIG_I2C_VIAPRO) += i2c-viapro.o +# DIMM busses +obj-$(CONFIG_I2C_DIMM_BUS) += dimm-bus.o +obj-$(CONFIG_I2C_IMC) += i2c-imc.o + # Mac SMBus host controller drivers obj-$(CONFIG_I2C_HYDRA) += i2c-hydra.o obj-$(CONFIG_I2C_POWERMAC)+= i2c-powermac.o diff --git a/drivers/i2c/busses/dimm-bus.c b/drivers/i2c/busses/dimm-bus.c new file mode 100644 index ..096842811199 --- /dev/null +++ b/drivers/i2c/busses/dimm-bus.c @@ -0,0 +1,97 @@ +/* + * Copyright (c) 2013 Andrew Lutomirski 2013 ? + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA. + */ + +#include +#include +#include +#include + +static bool probe_addr(struct i2c_adapter *adapter, int addr) +{ + /* +* So far, all known devices that live on DIMMs can be safely +* and reliably detected by trying to read a byte at address +* zero. (The exception is the SPD write protection control, +* which can't be probed and requires special hardware and/or +* quick writes to access, and has no driver.) +*/ That leads to the question if the controller supports quick commands, and if it does, if would make sense to add support for it, just for the purpose of reducing risk here. + union i2c_smbus_data dummy; + return i2c_smbus_xfer(adapter, addr, 0, I2C_SMBUS_READ, 0, + I2C_SMBUS_BYTE_DATA, ) >= 0; +} + +/** + * i2c_scan_dimm_bus() - Scans an SMBUS segment known to contain DIMMs + * @adapter: The SMBUS adapter to scan + * + * This function tells the DIMM-bus code that the adapter is known to + * contain DIMMs. i2c_scan_dimm_bus will probe for devices known to + * live on DIMMs. + * + * Do NOT call this function on general-purpose system SMBUS segments + * unless you know that the only things on the bus are DIMMs. + * Otherwise is it very likely to mis-identify other things on the + * bus. + * + * Callers are advised not to set adapter->class = I2C_CLASS_SPD. Why not ? + */ +void i2c_scan_dimm_bus(struct i2c_adapter *adapter) +{ + struct i2c_board_info info = {}; + int slot; + + /* +* We probe with "read byte data". If any DIMM SMBUS driver can't +* support that access type, this function should be updated. +*/ + if (WARN_ON(!i2c_check_functionality(adapter, + I2C_FUNC_SMBUS_READ_BYTE_DATA))) + return; + + /* +* Addresses on DIMMs use the three low bits to identify the slot +* and the four high bits to identify the device type. Known +* devices are: +* +* - 0x50 - 0x57: SPD (Serial Presence Detect) EEPROM +* - 0x30 - 0x37: SPD WP control -- not worth trying to probe Not worth,
Re: [PATCH 2/2] i2c, i2c_imc: Add DIMM bus code
Just a license nit, I'm afraid. Andy Lutomirski schreef op vr 06-03-2015 om 18:50 [-0800]: > --- /dev/null > +++ b/drivers/i2c/busses/dimm-bus.c > @@ -0,0 +1,97 @@ > +/* > + * Copyright (c) 2013 Andrew Lutomirski > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 > + * as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, write to the Free Software > + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA. > + */ This states the license is GPL v2. > +MODULE_LICENSE("GPL"); So you probably want MODULE_LICENSE("GPL v2"); here too. Paul Bolle -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] i2c, i2c_imc: Add DIMM bus code
On 03/06/2015 06:50 PM, Andy Lutomirski wrote: Add i2c_scan_dimm_bus to declare that a particular i2c_adapter contains DIMMs. This will probe (and autoload modules!) for useful SMBUS devices that live on DIMMs. i2c_imc calls it. As more SMBUS-addressable DIMM components become supported, this code can be extended to probe for them. Signed-off-by: Andy Lutomirski l...@amacapital.net Similar to the other patch, I would suggest to run it through checkpatch. --- drivers/i2c/busses/Kconfig| 4 ++ drivers/i2c/busses/Makefile | 4 ++ drivers/i2c/busses/dimm-bus.c | 97 +++ drivers/i2c/busses/i2c-imc.c | 3 ++ include/linux/i2c/dimm-bus.h | 24 +++ 5 files changed, 132 insertions(+) create mode 100644 drivers/i2c/busses/dimm-bus.c create mode 100644 include/linux/i2c/dimm-bus.h diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index d6b9ce164fbf..2ea6648492eb 100644 --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -149,6 +149,10 @@ config I2C_ISMT This driver can also be built as a module. If so, the module will be called i2c-ismt. +config I2C_DIMM_BUS + tristate + default n + config I2C_IMC tristate Intel iMC (LGA 2011) SMBus Controller depends on PCI X86 diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile index 4287c891e782..a01bdcc0e239 100644 --- a/drivers/i2c/busses/Makefile +++ b/drivers/i2c/busses/Makefile @@ -25,6 +25,10 @@ obj-$(CONFIG_I2C_SIS96X) += i2c-sis96x.o obj-$(CONFIG_I2C_VIA) += i2c-via.o obj-$(CONFIG_I2C_VIAPRO) += i2c-viapro.o +# DIMM busses +obj-$(CONFIG_I2C_DIMM_BUS) += dimm-bus.o +obj-$(CONFIG_I2C_IMC) += i2c-imc.o + # Mac SMBus host controller drivers obj-$(CONFIG_I2C_HYDRA) += i2c-hydra.o obj-$(CONFIG_I2C_POWERMAC)+= i2c-powermac.o diff --git a/drivers/i2c/busses/dimm-bus.c b/drivers/i2c/busses/dimm-bus.c new file mode 100644 index ..096842811199 --- /dev/null +++ b/drivers/i2c/busses/dimm-bus.c @@ -0,0 +1,97 @@ +/* + * Copyright (c) 2013 Andrew Lutomirski l...@amacapital.net 2013 ? + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA. + */ + +#include linux/i2c.h +#include linux/bug.h +#include linux/module.h +#include linux/i2c/dimm-bus.h + +static bool probe_addr(struct i2c_adapter *adapter, int addr) +{ + /* +* So far, all known devices that live on DIMMs can be safely +* and reliably detected by trying to read a byte at address +* zero. (The exception is the SPD write protection control, +* which can't be probed and requires special hardware and/or +* quick writes to access, and has no driver.) +*/ That leads to the question if the controller supports quick commands, and if it does, if would make sense to add support for it, just for the purpose of reducing risk here. + union i2c_smbus_data dummy; + return i2c_smbus_xfer(adapter, addr, 0, I2C_SMBUS_READ, 0, + I2C_SMBUS_BYTE_DATA, dummy) = 0; +} + +/** + * i2c_scan_dimm_bus() - Scans an SMBUS segment known to contain DIMMs + * @adapter: The SMBUS adapter to scan + * + * This function tells the DIMM-bus code that the adapter is known to + * contain DIMMs. i2c_scan_dimm_bus will probe for devices known to + * live on DIMMs. + * + * Do NOT call this function on general-purpose system SMBUS segments + * unless you know that the only things on the bus are DIMMs. + * Otherwise is it very likely to mis-identify other things on the + * bus. + * + * Callers are advised not to set adapter-class = I2C_CLASS_SPD. Why not ? + */ +void i2c_scan_dimm_bus(struct i2c_adapter *adapter) +{ + struct i2c_board_info info = {}; + int slot; + + /* +* We probe with read byte data. If any DIMM SMBUS driver can't +* support that access type, this function should be updated. +*/ + if (WARN_ON(!i2c_check_functionality(adapter, + I2C_FUNC_SMBUS_READ_BYTE_DATA))) + return; + + /* +* Addresses on DIMMs use the three low bits to identify the slot +* and the four high bits to identify the device type. Known +* devices are: +* +* - 0x50 - 0x57: SPD (Serial Presence Detect)
Re: [PATCH 2/2] i2c, i2c_imc: Add DIMM bus code
Just a license nit, I'm afraid. Andy Lutomirski schreef op vr 06-03-2015 om 18:50 [-0800]: --- /dev/null +++ b/drivers/i2c/busses/dimm-bus.c @@ -0,0 +1,97 @@ +/* + * Copyright (c) 2013 Andrew Lutomirski l...@amacapital.net + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA. + */ This states the license is GPL v2. +MODULE_LICENSE(GPL); So you probably want MODULE_LICENSE(GPL v2); here too. Paul Bolle -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/