Re: [PATCH 2/2] i2c: i801: add support of Host Notify

Jean Delvare
2015-07-07T18:11:25Z

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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

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