On Jul 07 2015 or thereabouts, Jean Delvare wrote:
> 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 <[email protected]>
>
> An explanation on why a fifo is needed would be good to add, as I can't
> see any obvious reason.
I inserted the kfifo to be able to read the payload and address in the
same thread that the one handling the IRQ. But a few tests shows that I
can simply trigger the work in the interrupt, and read the data and
clear the Host Notification bit in the work thread directly. So consider
I will remove this in the v2.
>
> 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.)
OK, I will split the series for each driver/feature.
>
> > 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>.
fair enough
>
> > #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.)
Still relevant actually :)
I still need to enable the feature in the driver and on resume. So I
will amend in the v2.
>
> > +{
> > + 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.
Oops, my mistake.
>
> > 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.
OK. But given that the fifo disappeared (hopefully), this is no longer
relevant. I also made sure the work queue is not triggered before any
return path in order not having a out_err label.
>
> > + 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.
You are right.
>
> > + *
> > + * 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().
Looks like i801_do_host_notify() checks for that too. I will drop it.
>
> > +
> > + /*
> > + * 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.
Yeah, this gets messy. I re-used the .alert() callback because of the
documentation: "Alert callback, for example for the SMBus alert protocol".
It would seem that the alert is generic and could be re-used. But OTOH,
it is not prepared to receive anything else than a SMBus Alert.
Given that I had a toggle_host_notify() call, I figured that this was
not a problem unless you write a driver which implements both (I can not
find a sane use case for this though).
But now that this call has disappeared, we would need a way to
differentiate the too of them.
I can see two solutions out of my head right now:
- add a "protocol" parameter (with an enum) to .alert()
- add a new callback .host_notify() in struct i2c_driver.
I think I like the second option more given that it will allow to not
touch the current code in i2c_smbus.
>
> > + 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.
>
Sorry :(
> > +/**
> > * 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 :(
sorry again :(
>
> > +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.
>
Cheers,
Benjamin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
