Hi Adam,
On Sun, 13 Mar 2022 17:40:20 -0500, Adam Honse wrote:
> This patch introduces a new driver for the SMBus adapter that is built in
> to most Nuvoton Super-IO chips. This SMBus adapter is used for RGB
> lighting control on some ASUS motherboards with Intel chipsets.
>
> The interface's register description is available in the supported
> devices' datasheets. Operation of this interface has been verified
> with OpenRGB on an ASUS PRIME Z270-A motherboard.
>
> Signed-off-by: Adam Honse <calcprogrammer1@xxxxxxxxx>
See my review inline below.
> ---
> drivers/i2c/busses/Kconfig | 9 +
> drivers/i2c/busses/Makefile | 1 +
> drivers/i2c/busses/i2c-nct6775.c | 608 +++++++++++++++++++++++++++++++
> 3 files changed, 618 insertions(+)
> create mode 100644 drivers/i2c/busses/i2c-nct6775.c
>
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 8a6c6ee28556..cf4c5e92738b 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -219,6 +219,15 @@ config I2C_CHT_WC
> combined with a FUSB302 Type-C port-controller as such it is advised
> to also select CONFIG_TYPEC_FUSB302=m.
>
> +config I2C_NCT6775
> + tristate "Nuvoton NCT6775 and compatible SMBus controller"
> + help
> + If you say yes to this option, support will be included for the
> + Nuvoton NCT6775 and compatible SMBus controllers.
> +
> + This driver can also be built as a module. If so, the module
> + will be called i2c-nct6775.
> +
> config I2C_NFORCE2
> tristate "Nvidia nForce2, nForce3 and nForce4"
> depends on PCI
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 1d00dce77098..450cde39c7d5 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -17,6 +17,7 @@ obj-$(CONFIG_I2C_CHT_WC) += i2c-cht-wc.o
> obj-$(CONFIG_I2C_I801) += i2c-i801.o
> obj-$(CONFIG_I2C_ISCH) += i2c-isch.o
> obj-$(CONFIG_I2C_ISMT) += i2c-ismt.o
> +obj-$(CONFIG_I2C_NCT6775) += i2c-nct6775.o
> obj-$(CONFIG_I2C_NFORCE2) += i2c-nforce2.o
> obj-$(CONFIG_I2C_NFORCE2_S4985) += i2c-nforce2-s4985.o
> obj-$(CONFIG_I2C_NVIDIA_GPU) += i2c-nvidia-gpu.o
> diff --git a/drivers/i2c/busses/i2c-nct6775.c b/drivers/i2c/busses/i2c-nct6775.c
> new file mode 100644
> index 000000000000..fdffe30f595b
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-nct6775.c
> @@ -0,0 +1,608 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * i2c-nct6775 - Driver for the SMBus master functionality of
> + * Nuvoton NCT677x Super-I/O chips
> + *
> + * Copyright (C) 2019 Adam Honse <calcprogrammer1@xxxxxxxxx>
> + *
> + * Derived from nct6775 hwmon driver
> + * Copyright (C) 2012 Guenter Roeck <linux@xxxxxxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * 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.
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/jiffies.h>
Not needed?
> +#include <linux/platform_device.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/hwmon-vid.h>
You don't need these 3 hwmon header files.
> +#include <linux/err.h>
> +#include <linux/mutex.h>
Also not needed.
> +#include <linux/delay.h>
> +#include <linux/ioport.h>
> +#include <linux/i2c.h>
> +#include <linux/acpi.h>
> +#include <linux/bitops.h>
Not needed.
> +#include <linux/dmi.h>
Not needed.
> +#include <linux/io.h>
> +#include <linux/nospec.h>
Why?
> +
> +#define DRVNAME "i2c-nct6775"
> +
> +/* Nuvoton SMBus address offsets */
> +#define SMBHSTDAT (0 + nuvoton_nct6793d_smba)
> +#define SMBBLKSZ (1 + nuvoton_nct6793d_smba)
> +#define SMBHSTCMD (2 + nuvoton_nct6793d_smba)
> +#define SMBHSTIDX (3 + nuvoton_nct6793d_smba)
> +#define SMBHSTCTL (4 + nuvoton_nct6793d_smba)
> +#define SMBHSTADD (5 + nuvoton_nct6793d_smba)
> +#define SMBHSTERR (9 + nuvoton_nct6793d_smba)
> +#define SMBHSTSTS (0xE + nuvoton_nct6793d_smba)
For consistency, you should be using hexadecimal for all the defines. I
also believe that (nuvoton_nct6793d_smba + 0) would make more sense
than (0 + nuvoton_nct6793d_smba), even though the result is the same.
> +
> +/* Command register */
> +#define NCT6793D_READ_BYTE 0
It is pretty confusing to prefix these constants with NCT6793D when the
driver is named i2c-nct6775.
> +#define NCT6793D_READ_WORD 1
> +#define NCT6793D_READ_BLOCK 2
> +#define NCT6793D_BLOCK_WRITE_READ_PROC_CALL 3
NCT6793D_BLOCK_PROC_CALL would be enough.
> +#define NCT6793D_PROC_CALL 4
> +#define NCT6793D_WRITE_BYTE 8
> +#define NCT6793D_WRITE_WORD 9
> +#define NCT6793D_WRITE_BLOCK 10
> +
> +/* Control register */
> +#define NCT6793D_MANUAL_START 128
> +#define NCT6793D_SOFT_RESET 64
> +
> +/* Error register */
> +#define NCT6793D_NO_ACK 32
> +
> +/* Status register */
> +#define NCT6793D_FIFO_EMPTY 1
> +#define NCT6793D_FIFO_FULL 2
> +#define NCT6793D_MANUAL_ACTIVE 4
The 6 values above are clearly bit masks, so you should use BIT()
instead of decimal value for clarity.
> +
> +#define NCT6775_LD_SMBUS 0x0B
> +
> +/* Other settings */
> +#define MAX_RETRIES 400
> +
> +enum kinds { nct6106, nct6775, nct6776, nct6779, nct6791, nct6792, nct6793,
> + nct6795, nct6796, nct6798 };
> +
> +struct nct6775_sio_data {
> + int sioreg;
> + enum kinds kind;
> +};
> +
> +/* used to set data->name = nct6775_device_names[data->sio_kind] */
> +static const char * const nct6775_device_names[] = {
> + "nct6106",
> + "nct6775",
> + "nct6776",
> + "nct6779",
> + "nct6791",
> + "nct6792",
> + "nct6793",
> + "nct6795",
> + "nct6796",
> + "nct6798",
> +};
> +
> +static const char * const nct6775_sio_names[] __initconst = {
> + "NCT6106D",
> + "NCT6775F",
> + "NCT6776D/F",
> + "NCT6779D",
> + "NCT6791D",
> + "NCT6792D",
> + "NCT6793D",
> + "NCT6795D",
> + "NCT6796D",
> + "NCT6798D",
> +};
> +
> +#define SIO_REG_LDSEL 0x07 /* Logical device select */
> +#define SIO_REG_DEVID 0x20 /* Device ID (2 bytes) */
> +#define SIO_REG_SMBA 0x62 /* SMBus base address register */
> +
> +#define SIO_NCT6106_ID 0xc450
> +#define SIO_NCT6775_ID 0xb470
> +#define SIO_NCT6776_ID 0xc330
> +#define SIO_NCT6779_ID 0xc560
> +#define SIO_NCT6791_ID 0xc800
> +#define SIO_NCT6792_ID 0xc910
> +#define SIO_NCT6793_ID 0xd120
> +#define SIO_NCT6795_ID 0xd350
> +#define SIO_NCT6796_ID 0xd420
> +#define SIO_NCT6798_ID 0xd428
> +#define SIO_ID_MASK 0xFFF0
> +
> +static inline void
> +superio_outb(int ioreg, int reg, int val)
> +{
> + outb(reg, ioreg);
> + outb(val, ioreg + 1);
> +}
> +
> +static inline int
> +superio_inb(int ioreg, int reg)
> +{
> + outb(reg, ioreg);
> + return inb(ioreg + 1);
> +}
> +
> +static inline void
> +superio_select(int ioreg, int ld)
> +{
> + outb(SIO_REG_LDSEL, ioreg);
> + outb(ld, ioreg + 1);
> +}
> +
> +static inline int
> +superio_enter(int ioreg)
> +{
> + /*
> + * Try to reserve <ioreg> and <ioreg + 1> for exclusive access.
> + */
> + if (!request_muxed_region(ioreg, 2, DRVNAME))
> + return -EBUSY;
> +
> + outb(0x87, ioreg);
> + outb(0x87, ioreg);
> +
> + return 0;
> +}
> +
> +static inline void
> +superio_exit(int ioreg)
> +{
> + outb(0xaa, ioreg);
> + outb(0x02, ioreg);
> + outb(0x02, ioreg + 1);
> + release_region(ioreg, 2);
> +}
> +
> +/*
> + * ISA constants
> + */
> +
> +#define IOREGION_ALIGNMENT (~7)
> +#define IOREGION_LENGTH 2
> +#define ADDR_REG_OFFSET 0
> +#define DATA_REG_OFFSET 1
The offset defines are never used.
> +
> +#define NCT6775_REG_BANK 0x4E
> +#define NCT6775_REG_CONFIG 0x40
You don't use these anywhere either.
> +
> +static struct i2c_adapter *nct6775_adapter;
You can't possibly have a driver which claims to support 2 platform
devices but have a single static pointer to store the SMBus adapter's
data. The adapter's data should be stored as per-platform device data
instead.
> +
> +struct i2c_nct6775_adapdata {
> + unsigned short smba;
> +};
> +
> +/* Return negative errno on error. */
> +static s32 nct6775_access(struct i2c_adapter *adap, u16 addr,
> + unsigned short flags, char read_write,
> + u8 command, int size, union i2c_smbus_data *data)
> +{
> + struct i2c_nct6775_adapdata *adapdata = i2c_get_adapdata(adap);
> + unsigned short nuvoton_nct6793d_smba = adapdata->smba;
> + int i, len, cnt;
> + union i2c_smbus_data tmp_data;
"tmp" is never a good variable name. All local variables are temporary
by nature. I don't understand why you need this variable anyway, you
already have *data.
> + int timeout = 0;
> +
> + tmp_data.word = 0;
> + cnt = 0;
> + len = 0;
Do you actually need to initialize these variables? Initializing to 0
"just in case" is bad practice as it prevents the compiler from warning
you if you end up using an uninitialized variable.
> +
> + outb_p(NCT6793D_SOFT_RESET, SMBHSTCTL);
> +
> + switch (size) {
> + case I2C_SMBUS_QUICK:
Are you sure this is actually supported? There's no define for this
command.
> + outb_p((addr << 1) | read_write,
> + SMBHSTADD);
Fits on one line. Same comment several times below.
> + break;
> + case I2C_SMBUS_BYTE_DATA:
> + tmp_data.byte = data->byte;
Please use the fallthrough pseudo keyword to make it clear that the
lack of break is intentional.
> + case I2C_SMBUS_BYTE:
> + outb_p((addr << 1) | read_write,
> + SMBHSTADD);
> + outb_p(command, SMBHSTIDX);
> + if (read_write == I2C_SMBUS_WRITE) {
> + outb_p(tmp_data.byte, SMBHSTDAT);
> + outb_p(NCT6793D_WRITE_BYTE, SMBHSTCMD);
> + } else {
> + outb_p(NCT6793D_READ_BYTE, SMBHSTCMD);
> + }
> + break;
This look highly suspicious. You can't possibly support two different
SMBus transaction types (Receive Byte and Read Byte) with the same
command value (NCT6793D_READ_BYTE). Same for Send Byte vs Write Byte.
> + case I2C_SMBUS_WORD_DATA:
> + outb_p((addr << 1) | read_write,
> + SMBHSTADD);
> + outb_p(command, SMBHSTIDX);
> + if (read_write == I2C_SMBUS_WRITE) {
> + outb_p(data->word & 0xff, SMBHSTDAT);
> + outb_p((data->word & 0xff00) >> 8, SMBHSTDAT);
> + outb_p(NCT6793D_WRITE_WORD, SMBHSTCMD);
> + } else {
> + outb_p(NCT6793D_READ_WORD, SMBHSTCMD);
> + }
> + break;
> + case I2C_SMBUS_BLOCK_DATA:
> + outb_p((addr << 1) | read_write,
> + SMBHSTADD);
> + outb_p(command, SMBHSTIDX);
> + if (read_write == I2C_SMBUS_WRITE) {
> + len = data->block[0];
> + if (len == 0 || len > I2C_SMBUS_BLOCK_MAX)
> + return -EINVAL;
> + outb_p(len, SMBBLKSZ);
> +
> + cnt = 1;
> + if (len >= 4) {
> + for (i = cnt; i <= 4; i++)
> + outb_p(data->block[i], SMBHSTDAT);
> +
> + len -= 4;
> + cnt += 4;
> + } else {
> + for (i = cnt; i <= len; i++)
> + outb_p(data->block[i], SMBHSTDAT);
> +
> + len = 0;
> + }
> +
> + outb_p(NCT6793D_WRITE_BLOCK, SMBHSTCMD);
> + } else {
> + return -EOPNOTSUPP;
> + }
> + break;
> + default:
> + dev_warn(&adap->dev, "Unsupported transaction %d\n", size);
> + return -EOPNOTSUPP;
> + }
> +
> + outb_p(NCT6793D_MANUAL_START, SMBHSTCTL);
> +
> + while ((size == I2C_SMBUS_BLOCK_DATA) && (len > 0)) {
More parentheses than needed.
> + if (read_write == I2C_SMBUS_WRITE) {
> + timeout = 0;
> + while ((inb_p(SMBHSTSTS) & NCT6793D_FIFO_EMPTY) == 0) {
> + if (timeout > MAX_RETRIES)
> + return -ETIMEDOUT;
> +
> + usleep_range(250, 500);
> + timeout++;
> + }
> +
> + //Load more bytes into FIFO
Please use /* */ for comments.
> + if (len >= 4) {
> + for (i = cnt; i <= (cnt + 4); i++)
> + outb_p(data->block[i], SMBHSTDAT);
> +
> + len -= 4;
> + cnt += 4;
> + } else {
> + for (i = cnt; i <= (cnt + len); i++)
> + outb_p(data->block[i], SMBHSTDAT);
> +
> + len = 0;
> + }
> + } else {
This can't happen.
> + return -EOPNOTSUPP;
> + }
> +
> + }
> +
> + //wait for manual mode to complete
> + timeout = 0;
> + while ((inb_p(SMBHSTSTS) & NCT6793D_MANUAL_ACTIVE) != 0) {
> + if (timeout > MAX_RETRIES)
> + return -ETIMEDOUT;
> +
> + usleep_range(250, 500);
> + timeout++;
> + }
> +
> + if ((inb_p(SMBHSTERR) & NCT6793D_NO_ACK) != 0)
> + return -ENXIO;
> +
> + else if ((read_write == I2C_SMBUS_WRITE) || (size == I2C_SMBUS_QUICK))
No blank line before "else". Or just remove the "else" which isn't
needed. Too many parentheses.
> + return 0;
> +
> + switch (size) {
> + case I2C_SMBUS_QUICK:
Never true.
> + case I2C_SMBUS_BYTE_DATA:
> + data->byte = inb_p(SMBHSTDAT);
> + break;
> + case I2C_SMBUS_WORD_DATA:
> + data->word = inb_p(SMBHSTDAT) + (inb_p(SMBHSTDAT) << 8);
| is preferred here.
> + break;
> + }
> + return 0;
> +}
> +
> +static u32 nct6775_func(struct i2c_adapter *adapter)
> +{
> + return I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE |
> + I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA |
> + I2C_FUNC_SMBUS_BLOCK_DATA;
> +}
> +
> +static const struct i2c_algorithm smbus_algorithm = {
> + .smbus_xfer = nct6775_access,
> + .functionality = nct6775_func,
> +};
> +
> +static int nct6775_add_adapter(unsigned short smba, const char *name, struct i2c_adapter **padap)
IMHO it would make more sense to have this function return the struct
i2c_adapter * (and use ERR_PTR to return errors).
> +{
> + struct i2c_adapter *adap;
> + struct i2c_nct6775_adapdata *adapdata;
> + int retval;
> +
> + adap = kzalloc(sizeof(*adap), GFP_KERNEL);
> + if (adap == NULL)
> + return -ENOMEM;
> +
> + adap->owner = THIS_MODULE;
> + adap->class = I2C_CLASS_HWMON | I2C_CLASS_SPD;
Doesn't seem appropriate if the only known usage of this SMBus
controller is RGB light control.
> + adap->algo = &smbus_algorithm;
> +
> + adapdata = kzalloc(sizeof(*adapdata), GFP_KERNEL);
> + if (adapdata == NULL) {
> + kfree(adap);
> + return -ENOMEM;
> + }
> +
> + adapdata->smba = smba;
> +
> + snprintf(adap->name, sizeof(adap->name),
> + "SMBus NCT67xx adapter%s at %04x", name, smba);
Name is already an empty string, so this could be simplified.
Alternatively, set name to the actual device name (available in
nct6775_sio_names) and use that instead of NCT67xx.
> +
> + i2c_set_adapdata(adap, adapdata);
> +
> + retval = i2c_add_adapter(adap);
> + if (retval) {
> + kfree(adapdata);
> + kfree(adap);
> + return retval;
> + }
> +
> + *padap = adap;
> + return 0;
> +}
> +
> +static void nct6775_remove_adapter(struct i2c_adapter *adap)
> +{
> + struct i2c_nct6775_adapdata *adapdata = i2c_get_adapdata(adap);
> +
> + if (adapdata->smba) {
Always true, as far as I can see.
> + i2c_del_adapter(adap);
> + kfree(adapdata);
> + kfree(adap);
> + }
> +}
> +
> +/*
> + * when Super-I/O functions move to a separate file, the Super-I/O
> + * bus will manage the lifetime of the device and this module will only keep
> + * track of the nct6775 driver. But since we use platform_device_alloc(), we
> + * must keep track of the device
Leading capital and trailing dot would be appreciated.
> + */
> +static struct platform_device *pdev[2];
> +
> +static int nct6775_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct nct6775_sio_data *sio_data = dev_get_platdata(dev);
> + struct resource *res;
> +
> + res = platform_get_resource(pdev, IORESOURCE_IO, 0);
> + if (!devm_request_region(&pdev->dev, res->start, IOREGION_LENGTH,
> + DRVNAME))
> + return -EBUSY;
> +
> + switch (sio_data->kind) {
> + case nct6791:
> + case nct6792:
> + case nct6793:
> + case nct6795:
> + case nct6796:
> + case nct6798:
> + nct6775_add_adapter(res->start, "", &nct6775_adapter);
> + break;
I'm confused. The driver is named i2c-nct6775 but the NCT6775 isn't
supported? I understand the idea of matching the name of the hwmon
driver you used as a based, but it only makes sense if the same devices
are supported.
All references to unsupported devices should be removed from this
driver, there's no reason to make the driver larger.
> + default:
> + return -ENODEV;
> + }
> +
> + return 0;
> +}
> +
> +static struct platform_driver i2c_nct6775_driver = {
> + .driver = {
> + .name = DRVNAME,
> + },
> + .probe = nct6775_probe,
> +};
> +
> +static void __exit i2c_nct6775_exit(void)
> +{
> + int i;
> +
> + if (nct6775_adapter)
> + nct6775_remove_adapter(nct6775_adapter);
That's a weird construct. Unloading the module should remove the
platform devices, and the remove callback of the platform device driver
should remove the adapter.
> +
> + for (i = 0; i < ARRAY_SIZE(pdev); i++) {
> + if (pdev[i])
> + platform_device_unregister(pdev[i]);
> + }
> + platform_driver_unregister(&i2c_nct6775_driver);
> +}
> +
> +/* nct6775_find() looks for a '627 in the Super-I/O config space */
This reference to the Winbond W83627HF chipset is way outdated.
> +static int __init nct6775_find(int sioaddr, struct nct6775_sio_data *sio_data)
> +{
> + u16 val;
> + int err;
> + int addr;
> +
> + err = superio_enter(sioaddr);
> + if (err)
> + return err;
> +
> + val = (superio_inb(sioaddr, SIO_REG_DEVID) << 8) |
> + superio_inb(sioaddr, SIO_REG_DEVID + 1);
> +
> + switch (val & SIO_ID_MASK) {
> + case SIO_NCT6106_ID:
> + sio_data->kind = nct6106;
> + break;
> + case SIO_NCT6775_ID:
> + sio_data->kind = nct6775;
> + break;
> + case SIO_NCT6776_ID:
> + sio_data->kind = nct6776;
> + break;
> + case SIO_NCT6779_ID:
> + sio_data->kind = nct6779;
> + break;
> + case SIO_NCT6791_ID:
> + sio_data->kind = nct6791;
> + break;
> + case SIO_NCT6792_ID:
> + sio_data->kind = nct6792;
> + break;
> + case SIO_NCT6793_ID:
> + sio_data->kind = nct6793;
> + break;
> + case SIO_NCT6795_ID:
> + sio_data->kind = nct6795;
> + break;
> + case SIO_NCT6796_ID:
> + sio_data->kind = nct6796;
> + break;
> + case SIO_NCT6798_ID:
> + sio_data->kind = nct6798;
> + break;
> + default:
> + if (val != 0xffff)
> + pr_debug("unsupported chip ID: 0x%04x\n", val);
> + superio_exit(sioaddr);
> + return -ENODEV;
> + }
> +
> + /* We have a known chip, find the SMBus I/O address */
> + superio_select(sioaddr, NCT6775_LD_SMBUS);
> + val = (superio_inb(sioaddr, SIO_REG_SMBA) << 8)
> + | superio_inb(sioaddr, SIO_REG_SMBA + 1);
> + addr = val & IOREGION_ALIGNMENT;
> + if (addr == 0) {
> + pr_err("Refusing to enable a Super-I/O device with a base I/O port 0\n");
> + superio_exit(sioaddr);
> + return -ENODEV;
> + }
> +
> + superio_exit(sioaddr);
> + pr_info("Found %s or compatible chip at %#x:%#x\n",
> + nct6775_sio_names[sio_data->kind], sioaddr, addr);
> + sio_data->sioreg = sioaddr;
> +
> + return addr;
> +}
> +
> +static int __init i2c_nct6775_init(void)
> +{
> + int i, err;
> + bool found = false;
> + int address;
> + struct resource res;
> + struct nct6775_sio_data sio_data;
> + int sioaddr[2] = { 0x2e, 0x4e };
> +
> + err = platform_driver_register(&i2c_nct6775_driver);
> + if (err)
> + return err;
> +
> + /*
> + * initialize sio_data->kind and sio_data->sioreg.
> + *
> + * when Super-I/O functions move to a separate file, the Super-I/O
> + * driver will probe 0x2e and 0x4e and auto-detect the presence of a
> + * nct6775 hardware monitor, and call probe()
> + */
> + for (i = 0; i < ARRAY_SIZE(pdev); i++) {
> + address = nct6775_find(sioaddr[i], &sio_data);
> + if (address <= 0)
> + continue;
> +
> + found = true;
> +
> + pdev[i] = platform_device_alloc(DRVNAME, address);
> + if (!pdev[i]) {
> + err = -ENOMEM;
> + goto exit_device_unregister;
> + }
> +
> + err = platform_device_add_data(pdev[i], &sio_data,
> + sizeof(struct nct6775_sio_data));
> + if (err)
> + goto exit_device_put;
> +
> + memset(&res, 0, sizeof(res));
> + res.name = DRVNAME;
> + res.start = address;
> + res.end = address + IOREGION_LENGTH - 1;
> + res.flags = IORESOURCE_IO;
> +
> + err = acpi_check_resource_conflict(&res);
> + if (err) {
> + platform_device_put(pdev[i]);
> + pdev[i] = NULL;
> + continue;
> + }
> +
> + err = platform_device_add_resources(pdev[i], &res, 1);
> + if (err)
> + goto exit_device_put;
> +
> + /* platform_device_add calls probe() */
> + err = platform_device_add(pdev[i]);
> + if (err)
> + goto exit_device_put;
> + }
> + if (!found) {
> + err = -ENODEV;
> + goto exit_unregister;
> + }
> +
> + return 0;
> +
> +exit_device_put:
> + platform_device_put(pdev[i]);
> +exit_device_unregister:
> + while (--i >= 0) {
> + if (pdev[i])
> + platform_device_unregister(pdev[i]);
> + }
> +exit_unregister:
> + platform_driver_unregister(&i2c_nct6775_driver);
> + return err;
> +}
> +
> +MODULE_AUTHOR("Adam Honse <calcprogrammer1@xxxxxxxxx>");
> +MODULE_DESCRIPTION("SMBus driver for NCT6775F and compatible chips");
> +MODULE_LICENSE("GPL");
> +
> +module_init(i2c_nct6775_init);
> +module_exit(i2c_nct6775_exit);
--
Jean Delvare
SUSE L3 Support
