Hi Jean,
I revised the driver and resubmit a new new for you. I listed the changes below
so that it can make your review a little easy. One thing I missed:
> It would also look better if the two blocks above had the same
> indentation, and if you stuck to capitals or not capitals for
> hexadecimal numbers.
applied, removed it, sorry I missed the capital issue.
On Wed, Sep 09, 2009 at 06:04:31PM +0200, Jean Delvare wrote:
> > +config CMI_I2C
>
> I2C_CMI
applied
>
> > + tristate "SMBus Control Method Interface"
> > + depends on X86 && ACPI
>
> Why depend on X86? As far as I know, IA64 systems can support ACPI too.
> And this dependency is already handled by CONFIG_ACPI so drivers
> shouldn't have to care.
applied
>
> > + help
> > + This driver supports the SMBus Control Method Interface. It needs
> > + BIOS declare ACPI control methods via SMBus Control Method Interface
> > + Spec.
>
> I suggest rewording the second sentence as follows: "It needs the BIOS
> to declare ACPI control methods as described in the SMBus Control
> Method Interface specification."
applied
>
> > +
> > + To compile this driver as a module, choose M here:
> > + the modules will be called cmi_i2c.
>
> module (no s)
applied
>
> > obj-$(CONFIG_SCx200_ACB) += scx200_acb.o
> > obj-$(CONFIG_SCx200_I2C) += scx200_i2c.o
> > +obj-$(CONFIG_CMI_I2C) += cmi_i2c.o
>
> The i2c-cmi driver should probably be listed first: if built into the
> kernel, it should be loaded before native drivers.
applied
>
> > +#include <linux/version.h>
>
> This header file should never be included by in-tree drivers.
applied
>
> > +#include <linux/delay.h>
>
> Your driver doesn't seem to delay or sleep, so not needed.
applied
>
> > +
> > +#define ACPI_SMB_HC_COMPONENT 0x00080000
> > +#define ACPI_SMB_HC_CLASS "smbus"
> > +#define ACPI_SMB_HC_DEVICE_NAME "smbus cmi"
> > +#define SMB_HC_DEVICE_NAME "SMBus CMI adapter"
>
> This last define is not used anywhere.
applied
>
> > +
> > +#define _COMPONENT ACPI_SMB_HC_COMPONENT
>
> Nor this one.
applied
>
> > +
> > +ACPI_MODULE_NAME("smbus_cmi");
> > +
> > +struct smbus_methods {
> > + char *mt_info;
> > + char *mt_sbr;
> > + char *mt_sbw;
>
> Shouldn't these be const char*?
applied
>
> > +};
> > +
> > +struct acpi_smbus_cmi {
> > + acpi_handle handle;
> > + struct i2c_adapter adapter;
> > + struct smbus_methods *methods;
> > +};
> > +
> > +static const struct smbus_methods smb_mtds = {
>
> I suggest that you never use "smb" as an abbreviation for "SMBus":
> "smb" stands for a number of different things in computer science, so
> it can easily get confusing. And "smbus" isn't much longer.
>
> Likewise, "mtds" as an abbreviation for "methods" doesn't save much
> space and somewhat hurts readability IMHO.
applied, smb -> smbus, mtds-> methods
>
> > + .mt_info = "_SBI",
> > + .mt_sbr = "_SBR",
> > + .mt_sbw = "_SBW",
> > +};
>
> What's the point of having a per-device structure for this if all
> devices end up using the same hard-coded structure?
applied, remove it from per-device structure
>
> > +
> > +static const struct acpi_device_id i2c_device_ids[] = {
>
> I would suggest a different name than i2c_device_ids, because it's
> almost similar to an existing type (i2c_device_id). What about
> "acpi_smbus_cmi_ids"?
applied
>
> > + {"SMBUS01", 0},
> > + {"", 0},
>
> Trailing comma isn't needed, as you have a list terminator.
applied
>
> > +static struct acpi_driver acpi_smb_cmi_driver = {
> > + .name = ACPI_SMB_HC_DEVICE_NAME,
> > + .class = ACPI_SMB_HC_CLASS,
> > + .ids = i2c_device_ids,
> > + .ops = {
> > + .add = acpi_smb_cmi_add,
> > + .remove = acpi_smb_cmi_remove,
> > + },
>
> Bad indentation for the last curly brace (should be aligned with ".ops".
applied
>
> > +};
>
> What about moving this driver definition later in the file, so that you
> no longer need to forward-declare the _add and _remove methods?
applied
>
> > +
> > +#define ACPI_SMB_STATUS_PEC 0x1F
> > +
> > +#define ACPI_SMB_PRTCL_WRITE 0x0
>
> 0x00 for consistency.
applied
>
> > +#define ACPI_SMB_PRTCL_READ 0x01
> > +#define ACPI_SMB_PRTCL_QUICK 0x02
> > +#define ACPI_SMB_PRTCL_BYTE 0x04
> > +#define ACPI_SMB_PRTCL_BYTE_DATA 0x06
> > +#define ACPI_SMB_PRTCL_WORD_DATA 0x08
> > +#define ACPI_SMB_PRTCL_BLOCK_DATA 0x0a
> > +#define ACPI_SMB_PRTCL_PROC_CALL 0x0c
> > +#define ACPI_SMB_PRTCL_BLOCK_PROC_CALL 0x0d
>
> Can't see this one in the CMI specification, and your driver doesn't
> use it anyway.
applied, removed it
>
> > +#define ACPI_SMB_PRTCL_PEC 0x80
>
> It would also look better if the two blocks above had the same
> indentation, and if you stuck to capitals or not capitals for
> hexadecimal numbers.
applied, removed it, sorry I missed the capital issue.
>
> > +
> > + result = obj->integer.value;
> > + switch (result) {
> > + case ACPI_SMB_STATUS_OK:
>
> This assumes that ACPI_SMB_STATUS_OK == 0, which is true but is only a
> coincidence.
applied, add debug message
>
> > + break;
> > + case ACPI_SMB_STATUS_BUSY:
> > + result = -EBUSY;
> > + goto out;
> > + case ACPI_SMB_STATUS_TIMEOUT:
> > + result = -ETIMEDOUT;
> > + goto out;
> > + case ACPI_SMB_STATUS_DNAK:
> > + result = -ENXIO;
> > + goto out;
> > + default:
> > + result = -EIO;
> > + goto out;
> > + }
>
> You might want to log error messages for unknown errors, and maybe
> debug messages for known errors too.
applied, add dev_dbg for debug usage
>
> > +
> > + if (read_write == I2C_SMBUS_WRITE)
>
> I think you also want to quit here on size == SMBUS_QUICK. What the CMI
> specification names "quick read" is really a write from a functional
> perspective.
applied
>
> > + goto out;
> > +
> > + obj = pkg->package.elements + 1;
> > + if (obj == NULL || obj->type != ACPI_TYPE_INTEGER) {
> > + ACPI_DEBUG_PRINT((ACPI_DB_WARN, "SMBus return package object \
> > + type error\n"));
>
> Not the right way to split a string.
applied
>
> > + result = -EIO;
> > + goto out;
> > + }
> > +
> > + len = obj->integer.value;
> > + obj = pkg->package.elements + 2;
> > + switch (size) {
> > + case I2C_SMBUS_BYTE:
> > + case I2C_SMBUS_BYTE_DATA:
> > + case I2C_SMBUS_WORD_DATA:
> > + if (obj == NULL || obj->type != ACPI_TYPE_INTEGER) {
> > + ACPI_DEBUG_PRINT((ACPI_DB_WARN, "SMBus return package \
> > + object type error\n"));
>
> Same here.
applied
>
> > + result = -EIO;
> > + goto out;
> > + }
> > + if (len == 2)
> > + data->word = obj->integer.value & 0xffff;
> > + else
> > + data->byte = obj->integer.value & 0xff;
>
> Masking is a no-op.
applied, removed them
>
> > + break;
> > + case I2C_SMBUS_BLOCK_DATA:
> > + if (obj == NULL || obj->type != ACPI_TYPE_BUFFER) {
> > + ACPI_DEBUG_PRINT((ACPI_DB_WARN, "SMBus return package \
> > + object type error\n"));
>
> Not the right way to split a string.
applied
>
> > + result = -EIO;
> > + goto out;
> > + }
> > + data->block[0] = len;
> > + if (data->block[0] == 0 || data->block[0] > I2C_SMBUS_BLOCK_MAX)
> > + return -EPROTO;
>
> You should check the value of len before copying it to data->block[0].
> data->block[0] is a u8 so you might get wrapping issues.
applied
>
> > + memcpy(data->block + 1, obj->buffer.pointer, len);
> > + break;
> > + }
> > +
> > +out:
> > + kfree(buffer.pointer);
> > + return result;
> > +}
> > +
> > +static u32 acpi_smb_cmi_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;
> > +}
>
> It doesn't look correct to hard-code this. As I read the CMI
> specification, each SMBus segment exposed may or may not support each
> type of SMBus transaction. Unfortunately there doesn't seem to be any
> way to know the exact subset of transactions that are supported. But at
> least you could use the presence or absence of methods _SBR and _SBW to
> determine whether reading, writing or both are supported.
>
> I hope this limitation won't cause too much trouble... some drivers
> really test for adapter functionality, and use different methods
> depending on the result.
applied, scan methods to determined its capability.
>
> > +
> > +static const struct i2c_algorithm acpi_smbus_cmi_algorithm = {
> > + .smbus_xfer = acpi_smb_cmi_access,
> > + .functionality = acpi_smb_cmi_func,
> > +};
> > +
> > +static int acpi_smb_cmi_add(struct acpi_device *device)
> > +{
> > + int status;
> > + struct acpi_smbus_cmi *smb_cmi;
> > + struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> > + union acpi_object *obj;
> > +
> > + if (!device)
> > + return -EINVAL;
>
> How could this ever happen?
applied, removed it
>
> > + if (obj->type != ACPI_TYPE_INTEGER) {
> > + ACPI_DEBUG_PRINT((ACPI_DB_WARN, "SMBus CMI Version object type \
> > + error\n"));
>
> This isn't how strings are split in C. Instead, do:
>
> ACPI_DEBUG_PRINT((ACPI_DB_WARN, "SMBus CMI Version object type "
> "error\n"));
applied
>
> > + } else
> > + ACPI_DEBUG_PRINT((ACPI_DB_WARN, "SMBus CMI Version %0x\n",
>
> How does %0x differ from %x? This might be displayed nicer anyway, as
> we know this byte is encoded in BCD.
applied
>
> > + (int)obj->integer.value));
> > + kfree(buffer.pointer);
>
> Maybe list some more information from the SMB_INFO object, in
> particular the version of SMBus that is supported, and the number of
> devices on the bus? Icing on the cake would be the complete list of
> devices, but this can be added later.
applied, printed SMB_INFO out in debug message.
>
> > +
> > + snprintf(smb_cmi->adapter.name, sizeof(smb_cmi->adapter.name),
> > + "SMBus CMI adapter");
>
> Isn't it possible for a given systems to have more than one SMBus
> adapter of this type? The CMI specification suggests so. The i2c_adapter
> name is supposed to be unique within a given system. We usually add the
> device address or any other unique identifier at the end of the name.
> As I read the CMI specification, we should be able to use _UID for this
> purpose?
applied
>
> > + smb_cmi->adapter.owner = THIS_MODULE;
> > + smb_cmi->adapter.algo = &acpi_smbus_cmi_algorithm;
> > + smb_cmi->adapter.algo_data = smb_cmi;
> > + smb_cmi->adapter.class = I2C_CLASS_HWMON | I2C_CLASS_SPD;
> > + smb_cmi->adapter.dev.parent = &device->dev;
> > +
> > + if (i2c_add_adapter(&smb_cmi->adapter)) {
> > + ACPI_DEBUG_PRINT((ACPI_DB_WARN,
> > + "SMBus CMI adapter: Failed to register adapter\n"));
> > + kfree(smb_cmi);
> > + return -EIO;
>
> Why not just "goto err"?
applied
>
> > + }
> > +
> > + printk(KERN_INFO PREFIX "%s [%s]\n",
> > + acpi_device_name(device), acpi_device_bid(device));
>
> That's a little rough IMHO.
applied, removed
>
> > +
> > + return AE_OK;
>
> I am surprised that you mix regular error codes with ACPI-specific ones.
applied
>
> > + if (!device)
> > + return -EINVAL;
>
> I fail to see how this would be possible.
applied
>
> > +
> > + smbus_cmi = acpi_driver_data(device);
> > +
> > + i2c_del_adapter(&smbus_cmi->adapter);
> > + kfree(smbus_cmi);
>
> Care to reset driver data to NULL?
applied
>
> > +
> > + return AE_OK;
>
> Here again, I am surprised by the mix of regular error codes and ACPI
> ones.
>
> > +}
> > +
> > +static int __init acpi_smb_cmi_init(void)
> > +{
> > + int result;
> > +
> > + result = acpi_bus_register_driver(&acpi_smb_cmi_driver);
> > + if (result < 0)
> > + return -ENODEV;
>
> Why don't you simply return the error code? Would be simpler and more
> informative too.
applied
>
> > +MODULE_AUTHOR("Crane Cai");
>
> No e-mail address?
applied
>
> And lastly a general comment: is it OK that you always use
> ACPI_DEBUG_PRINT((ACPI_DB_WARN for all kernel messages, be they errors,
> warning or informational?
applied, ACPI_DEBUG_PRINT -> ACPI_ERROR etc.
>
> --
> Jean Delvare
--
Best Regards,
- Crane
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
