Re: [PATCH 1/2] i2c: add SMBus Host Notify support

[Benjamin Tissoires]

On Jul 07 2015 or thereabouts, Jean Delvare wrote:
> On Tue, 7 Jul 2015 10:23:33 -0400, Benjamin Tissoires wrote:
> > Hi Jean,
> > 
> > On Jun 29 2015 or thereabouts, Jean Delvare wrote:
> > > Hi Benjamin,
> > > 
> > > On Tue, 23 Jun 2015 14:58:18 -0400, Benjamin Tissoires wrote:
> > > > SMBus Host Notify allows a slave device to act as a master on a bus to
> > > > notify the host of an interrupt. The functionality is directly implemented
> > > > in the firmware so we just export a toggle function and rely on the
> > > > adapter to actually support this.
> > > 
> > > Why does it need to be toggled? I mean, if the controller supports it,
> > > why don't we just enable the feature all the time?
> > 
> > That is definitively a solution. I however had once a problem with
> > suspend/resume where i2c_i801 was not able to reset the Host Notify
> > feature, but that was maybe a side effect of unloading/reloading the
> > module tons of times during the session.
> > 
> > I will work on that and submit a v2 this week.
> > 
> > I still wonder if you think we should expose a KConfig parameter for
> > this or not. I would prefer not to (because it is handled in hardware
> > and people might screw up their input drivers), but I'd prefer have your
> > opinion on this first.
> 
> What would the Kconfig option do exactly? As a general rule, the fewer
> Kconfig options the better. We already have I2C_SMBUS for the SMBus
> protocol specifics and Host Notify is one of these.

Yeah, that was a dumb suggestion. i2c_i801 is a SMBus driver, so we
should be able to enable Host Notify unconditionally.

> 
> > > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx>
> > > > ---
> > > >  Documentation/i2c/smbus-protocol |  4 ++++
> > > >  drivers/i2c/i2c-core.c           | 26 ++++++++++++++++++++++++++
> > > >  include/linux/i2c.h              |  8 ++++++++
> > > >  include/uapi/linux/i2c.h         |  1 +
> > > >  4 files changed, 39 insertions(+)
> > > > 
> > > > diff --git a/Documentation/i2c/smbus-protocol b/Documentation/i2c/smbus-protocol
> > > > index 6012b12..af4e4b9 100644
> > > > --- a/Documentation/i2c/smbus-protocol
> > > > +++ b/Documentation/i2c/smbus-protocol
> > > > @@ -199,6 +199,10 @@ alerting device's address.
> > > >  
> > > >  [S] [HostAddr] [Wr] A [DevAddr] A [DataLow] A [DataHigh] A [P]
> > > >  
> > > > +I2C drivers for devices which can trigger SMBus Host Notify should call
> > > > +i2c_smbus_toggle_host_notify() to enable SMBUS Host Notify on the adapter
> > > > +and implement the optional alert() callback.
> > > > +
> > > >  
> > > >  Packet Error Checking (PEC)
> > > >  ===========================
> > > > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> > > > index 987c124..eaaf5b0 100644
> > > > --- a/drivers/i2c/i2c-core.c
> > > > +++ b/drivers/i2c/i2c-core.c
> > > > @@ -2959,6 +2959,32 @@ int i2c_slave_unregister(struct i2c_client *client)
> > > >  EXPORT_SYMBOL_GPL(i2c_slave_unregister);
> > > >  #endif
> > > >  
> > > > +int i2c_smbus_toggle_host_notify(struct i2c_client *client, bool state)
> > > > +{
> > > > +	int ret;
> > > > +
> > > > +	if (!client)
> > > > +		return -EINVAL;
> > > > +
> > > > +	if (!(client->flags & I2C_CLIENT_TEN)) {
> > > > +		/* Enforce stricter address checking */
> > > > +		ret = i2c_check_addr_validity(client->addr);
> > > > +		if (ret)
> > > > +			return ret;
> > > > +	}
> > > > +
> > > > +	if (!client->adapter->algo->toggle_smbus_host_notify)
> > > > +		return -EOPNOTSUPP;
> > > > +
> > > > +	i2c_lock_adapter(client->adapter);
> > > > +	ret = client->adapter->algo->toggle_smbus_host_notify(client->adapter,
> > > > +							      state);
> > > > +	i2c_unlock_adapter(client->adapter);
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(i2c_smbus_toggle_host_notify);
> > > > +
> > > 
> > > As this is an SMBus feature, can't it go to drivers/i2c/i2c-smbus.c?
> > 
> > I asked myself this question, and I figured it was not possible because
> > i2c-smbus consists in the smbus_alert i2c driver.
> > 
> > After re-reading the code, I see now that it could definitively goes
> > there too. However, if we choose not to expose the toggle to the
> > drivers, then there will be no need to make deep changes in i2c-smbus
> > :).
> 
> The smbus_alert i2c driver was just the most simple way to implement
> the SMBus Alert feature. It's not an actual device driver, nothing to
> be afraid of.

Hehe. OK.

> 
> > (...)
> > Thanks for the review!
> 
> You're welcome. I wish I could do more but -ENOTIME :/

No worries. That's already a lot. And from my side, I was quite silent
because I just spent days of vacation without checking mails :)

Cheers,
Benjamin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/