Hi Benjamin, On Tue, 23 Jun 2015 14:58:19 -0400, Benjamin Tissoires wrote: > The i801 chip can handle the Host Notify feature since ICH 3 as mentioned > in http://www.intel.com/content/dam/doc/datasheet/82801ca-io-controller-hub-3-datasheet.pdf > > Implement .toggle_smbus_host_notify() and rely on .alert() to notify > the actual client that it has a notification. > > With a T440s and a Synaptics touchpad that implements Host Notify, the > payload data is always 0x0000, so I am not sure if the device actually > sends the payload or if there is a problem regarding the implementation. > > Part of this code is inspired by i2c-smbus.c. > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx> An explanation on why a fifo is needed would be good to add, as I can't see any obvious reason. No time for a full review but just a few random comments. > --- > drivers/i2c/busses/i2c-i801.c | 223 +++++++++++++++++++++++++++++++++--------- > drivers/i2c/i2c-core.c | 34 +++++++ > drivers/i2c/i2c-smbus.c | 17 +--- > include/linux/i2c.h | 3 + > 4 files changed, 217 insertions(+), 60 deletions(-) You really shouldn't mix core changes with driver specific changes. Anyone should be able to backport the core changes alone, and then add support to a different bus driver, without drawing in the i2c-i801 specific changes (that may not be needed and may not even apply.) > diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c > index 5ecbb3f..22a3218 100644 > --- a/drivers/i2c/busses/i2c-i801.c > +++ b/drivers/i2c/busses/i2c-i801.c > (...) > @@ -221,6 +233,17 @@ struct i801_priv { > const struct i801_mux_config *mux_drvdata; > struct platform_device *mux_pdev; > #endif > + > + bool host_notify_requested; > + struct work_struct host_notify; > + struct kfifo host_notify_fifo; > +}; > + > +#define SMBHSTNTFY_SIZE 8 > + > +struct smbus_host_notify_data { > + u8 addr; > + u16 payload; > }; This structure seems generic to the protocol and not specific to the Intel 82801 implementation, so it should go to <linux/i2c-smbus.h>. > #define FEATURE_SMBUS_PEC (1 << 0) > (...) > @@ -801,12 +894,35 @@ static u32 i801_func(struct i2c_adapter *adapter) > I2C_FUNC_SMBUS_BLOCK_DATA | I2C_FUNC_SMBUS_WRITE_I2C_BLOCK | > ((priv->features & FEATURE_SMBUS_PEC) ? I2C_FUNC_SMBUS_PEC : 0) | > ((priv->features & FEATURE_I2C_BLOCK_READ) ? > - I2C_FUNC_SMBUS_READ_I2C_BLOCK : 0); > + I2C_FUNC_SMBUS_READ_I2C_BLOCK : 0) | > + ((priv->features & FEATURE_HOST_NOTIFY) ? > + I2C_FUNC_SMBUS_HOST_NOTIFY : 0); > +} > + > +static int i801_toggle_host_ntfy(struct i2c_adapter *adapter, bool state) Probably no longer relevant, but please spell out "notify" completely in function and variable names (NTFY in register defines is OK.) > +{ > + struct i801_priv *priv = i2c_get_adapdata(adapter); > + > + if (!(priv->features & FEATURE_HOST_NOTIFY)) > + return -EOPNOTSUPP; > + > + if (state) { > + outb_p(SMBSLVCMD_HST_NTFY_INTREN, SMBSLVCMD(priv)); > + /* clear Host Notify bit to allow a new notification */ > + outb_p(SMBSLVSTS_HST_NTFY_STS, SMBSLVSTS(priv)); > + } else { > + outb_p(0, SMBSLVCMD(priv)); > + } > + > + priv->host_notify_requested = state; > + > + return 0; > } > > static const struct i2c_algorithm smbus_algorithm = { > - .smbus_xfer = i801_access, > - .functionality = i801_func, > + .smbus_xfer = i801_access, > + .functionality = i801_func, > + .toggle_smbus_host_notify = i801_toggle_host_ntfy, > }; > > static const struct pci_device_id i801_ids[] = { > @@ -1166,6 +1282,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) > priv->features |= FEATURE_BLOCK_BUFFER; > /* fall through */ > case PCI_DEVICE_ID_INTEL_82801CA_3: > + priv->features |= FEATURE_HOST_NOTIFY; Please add a /* fall through */ comment for consistency and clarity. > case PCI_DEVICE_ID_INTEL_82801BA_2: > case PCI_DEVICE_ID_INTEL_82801AB_3: > case PCI_DEVICE_ID_INTEL_82801AA_3: > @@ -1250,6 +1367,15 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) > } > } > > + INIT_WORK(&priv->host_notify, i801_host_notify); > + if (kfifo_alloc(&priv->host_notify_fifo, > + SMBHSTNTFY_SIZE * sizeof(struct smbus_host_notify_data), > + GFP_KERNEL)) { > + dev_err(&dev->dev, > + "%s:failed allocating host_notify_fifo\n", __func__); Adding the function name does not add value. Other messages in this driver do not include it, so for consistency do not either. A leading capital would be good too. Also kfifo_alloc() returns an actual error code, which you should save, include in the error message and return to the caller instead of hard-coding -ENOMEM. I think priv->host_notify_fifo is leaked if i801_probe() fails later on. > + return -ENOMEM; > + } > + > if (priv->features & FEATURE_IRQ) { > init_waitqueue_head(&priv->waitq); > > @@ -1292,6 +1418,9 @@ static void i801_remove(struct pci_dev *dev) > { > struct i801_priv *priv = pci_get_drvdata(dev); > > + cancel_work_sync(&priv->host_notify); > + kfifo_free(&priv->host_notify_fifo); > + > i801_del_mux(priv); > i2c_del_adapter(&priv->adapter); > pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg); > @@ -1315,8 +1444,12 @@ static int i801_suspend(struct pci_dev *dev, pm_message_t mesg) > > static int i801_resume(struct pci_dev *dev) > { > + struct i801_priv *priv = pci_get_drvdata(dev); > + > pci_set_power_state(dev, PCI_D0); > pci_restore_state(dev); > + if (priv->host_notify_requested) > + i801_toggle_host_ntfy(&priv->adapter, true); > return 0; > } > #else > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c > index eaaf5b0..87904ec 100644 > --- a/drivers/i2c/i2c-core.c > +++ b/drivers/i2c/i2c-core.c > @@ -2145,6 +2145,40 @@ int i2c_master_send(const struct i2c_client *client, const char *buf, int count) > EXPORT_SYMBOL(i2c_master_send); > > /** > + * i2c_alert - call an alert for the given i2c_client. > + * @client: Handle to slave device > + * @data: Payload data that will be sent to the slave Actually this is data that was received from the "slave" device (temporarily acting as a master), if I understand how Host Notify works. > + * > + * Returns negative errno, or 0. > + */ > +int i2c_alert(struct i2c_client *client, unsigned int data) > +{ > + struct i2c_driver *driver; > + > + if (!client) > + return -EINVAL; Do you actually need to check for that? It seems redundant with the check in smbus_do_alert(). > + > + /* > + * Drivers should either disable alerts, or provide at least > + * a minimal handler. Lock so the driver won't change. > + */ > + device_lock(&client->adapter->dev); > + if (client->dev.driver) { > + driver = to_i2c_driver(client->dev.driver); > + if (driver->alert) > + driver->alert(client, data); So you use the same driver callback for SMBus Alert and SMBus Host Notify. This makes some sense, but if a given driver supports both, how does it know which event happened? The data is completely different and most probably the action required from the driver as well. > + else > + dev_warn(&client->dev, "no driver alert()!\n"); > + } else > + dev_dbg(&client->dev, "alert with no driver\n"); > + device_unlock(&client->adapter->dev); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(i2c_alert); > + > + Please avoid duplicate blank lines. > +/** > * i2c_master_recv - issue a single I2C message in master receive mode > * @client: Handle to slave device > * @buf: Where to store data read from slave > diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c > index 9ebf9cb..26439a8 100644 > --- a/drivers/i2c/i2c-smbus.c > +++ b/drivers/i2c/i2c-smbus.c > @@ -41,27 +41,14 @@ static int smbus_do_alert(struct device *dev, void *addrp) > { > struct i2c_client *client = i2c_verify_client(dev); > struct alert_data *data = addrp; > - struct i2c_driver *driver; > > if (!client || client->addr != data->addr) > return 0; > if (client->flags & I2C_CLIENT_TEN) > return 0; > > - /* > - * Drivers should either disable alerts, or provide at least > - * a minimal handler. Lock so the driver won't change. > - */ > - device_lock(dev); > - if (client->dev.driver) { > - driver = to_i2c_driver(client->dev.driver); > - if (driver->alert) > - driver->alert(client, data->flag); > - else > - dev_warn(&client->dev, "no driver alert()!\n"); > - } else > - dev_dbg(&client->dev, "alert with no driver\n"); > - device_unlock(dev); > + if (i2c_alert(client, data->flag)) > + return 0; > > /* Stop iterating after we find the device */ > return -EBUSY; > diff --git a/include/linux/i2c.h b/include/linux/i2c.h > index 7ffc970..fbca48a 100644 > --- a/include/linux/i2c.h > +++ b/include/linux/i2c.h > @@ -72,6 +72,9 @@ extern int i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, > extern int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, > int num); > > +/* call an alert. */ What a vague comment :( > +extern int i2c_alert(struct i2c_client *client, unsigned int data); > + > /* This is the very generalized SMBus access routine. You probably do not > want to use this, though; one of the functions below may be much easier, > and probably just as fast. -- Jean Delvare SUSE L3 Support -- 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
