| Message ID | 20180615101506.8012-2-peda@axentia.se (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> +static inline void > +i2c_lock_segment(struct i2c_adapter *adapter) > +{ > + i2c_lock_bus(adapter, I2C_LOCK_SEGMENT); > +} > + > +static inline int > +i2c_trylock_segment(struct i2c_adapter *adapter) > +{ > + return i2c_trylock_bus(adapter, I2C_LOCK_SEGMENT); > +} > + > +static inline void > +i2c_unlock_segment(struct i2c_adapter *adapter) > +{ > + i2c_unlock_bus(adapter, I2C_LOCK_SEGMENT); > +} I wonder if i2c_lock_segment() and i2c_lock_root_adapter() are really more readable and convenient than i2c_lock_bus() with the flag. I think the flags have speaking names, too. Is that an idea to remove these functions altogether and start using i2c_lock_bus()?
On 2018-06-18 13:05, Wolfram Sang wrote: > >> +static inline void >> +i2c_lock_segment(struct i2c_adapter *adapter) >> +{ >> + i2c_lock_bus(adapter, I2C_LOCK_SEGMENT); >> +} >> + >> +static inline int >> +i2c_trylock_segment(struct i2c_adapter *adapter) >> +{ >> + return i2c_trylock_bus(adapter, I2C_LOCK_SEGMENT); >> +} >> + >> +static inline void >> +i2c_unlock_segment(struct i2c_adapter *adapter) >> +{ >> + i2c_unlock_bus(adapter, I2C_LOCK_SEGMENT); >> +} > > I wonder if i2c_lock_segment() and i2c_lock_root_adapter() are really > more readable and convenient than i2c_lock_bus() with the flag. I think > the flags have speaking names, too. > > Is that an idea to remove these functions altogether and start using > i2c_lock_bus()? That would be fine with me. I don't have a strong opinion and agree that both are readable enough... It would make for a reduction of the number of lines so that's nice, but the macro in drivers/i2c/busses/i2c-gpio.c (patch 11) would not fit in the current \-width (or whatever you'd call that line of backslashes to the right in a multi-line macro). Does anyone have a strong opinion? Cheers, Peter
> > I wonder if i2c_lock_segment() and i2c_lock_root_adapter() are really > > more readable and convenient than i2c_lock_bus() with the flag. I think > > the flags have speaking names, too. > > > > Is that an idea to remove these functions altogether and start using > > i2c_lock_bus()? > > That would be fine with me. I don't have a strong opinion and agree that > both are readable enough... > > It would make for a reduction of the number of lines so that's nice, but > the macro in drivers/i2c/busses/i2c-gpio.c (patch 11) would not fit in > the current \-width (or whatever you'd call that line of backslashes to > the right in a multi-line macro). > > Does anyone have a strong opinion? I have a strong opinion on making i2c.h less bloated. And yes, less number of lines is nice, too. I think that surely pays off the whitespace exception. Thanks!
On 2018-06-18 13:54, Wolfram Sang wrote: > >>> I wonder if i2c_lock_segment() and i2c_lock_root_adapter() are really >>> more readable and convenient than i2c_lock_bus() with the flag. I think >>> the flags have speaking names, too. >>> >>> Is that an idea to remove these functions altogether and start using >>> i2c_lock_bus()? >> >> That would be fine with me. I don't have a strong opinion and agree that >> both are readable enough... >> >> It would make for a reduction of the number of lines so that's nice, but >> the macro in drivers/i2c/busses/i2c-gpio.c (patch 11) would not fit in >> the current \-width (or whatever you'd call that line of backslashes to >> the right in a multi-line macro). >> >> Does anyone have a strong opinion? > > I have a strong opinion on making i2c.h less bloated. And yes, less > number of lines is nice, too. I think that surely pays off the > whitespace exception. Ok, I have rebased onto v4.18-rc1, killed the i2c-tegra hunk and converted i2c_lock_root(foo) over to i2c_lock_bus(foo, I2C_LOCK_ROOT_ADAPTER) and i2c_lock_segment(foo) over to i2c_lock_bus(foo, I2C_LOCK_SEGMENT). And I of course killed a bunch of locking helpers in i2c.h. I doing build tests now, will post a v2 in the morning. Cheers, Peter
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index 1ba40bb2b966..3eb09dc20573 100644 --- a/drivers/i2c/i2c-core-base.c +++ b/drivers/i2c/i2c-core-base.c @@ -1932,16 +1932,16 @@ int i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) #endif if (in_atomic() || irqs_disabled()) { - ret = i2c_trylock_bus(adap, I2C_LOCK_SEGMENT); + ret = i2c_trylock_segment(adap); if (!ret) /* I2C activity is ongoing. */ return -EAGAIN; } else { - i2c_lock_bus(adap, I2C_LOCK_SEGMENT); + i2c_lock_segment(adap); } ret = __i2c_transfer(adap, msgs, num); - i2c_unlock_bus(adap, I2C_LOCK_SEGMENT); + i2c_unlock_segment(adap); return ret; } else { diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c index b5aec33002c3..8a820fdef3e0 100644 --- a/drivers/i2c/i2c-core-smbus.c +++ b/drivers/i2c/i2c-core-smbus.c @@ -537,7 +537,7 @@ s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr, unsigned short flags, flags &= I2C_M_TEN | I2C_CLIENT_PEC | I2C_CLIENT_SCCB; if (adapter->algo->smbus_xfer) { - i2c_lock_bus(adapter, I2C_LOCK_SEGMENT); + i2c_lock_segment(adapter); /* Retry automatically on arbitration loss */ orig_jiffies = jiffies; @@ -551,7 +551,7 @@ s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr, unsigned short flags, orig_jiffies + adapter->timeout)) break; } - i2c_unlock_bus(adapter, I2C_LOCK_SEGMENT); + i2c_unlock_segment(adapter); if (res != -EOPNOTSUPP || !adapter->algo->master_xfer) goto trace; diff --git a/include/linux/i2c.h b/include/linux/i2c.h index 44ad14e016b5..c9080d49e988 100644 --- a/include/linux/i2c.h +++ b/include/linux/i2c.h @@ -768,6 +768,24 @@ i2c_unlock_adapter(struct i2c_adapter *adapter) i2c_unlock_bus(adapter, I2C_LOCK_ROOT_ADAPTER); } +static inline void +i2c_lock_segment(struct i2c_adapter *adapter) +{ + i2c_lock_bus(adapter, I2C_LOCK_SEGMENT); +} + +static inline int +i2c_trylock_segment(struct i2c_adapter *adapter) +{ + return i2c_trylock_bus(adapter, I2C_LOCK_SEGMENT); +} + +static inline void +i2c_unlock_segment(struct i2c_adapter *adapter) +{ + i2c_unlock_bus(adapter, I2C_LOCK_SEGMENT); +} + /*flags for the client struct: */ #define I2C_CLIENT_PEC 0x04 /* Use Packet Error Checking */ #define I2C_CLIENT_TEN 0x10 /* we have a ten bit chip address */
This is what almost all drivers want to do. By only advertising i2c_lock_adapter, they are tricked into locking the root adapter which is too big of a hammer in most cases. While at it, convert all open-coded locking of the I2C segment. Signed-off-by: Peter Rosin <peda@axentia.se> --- drivers/i2c/i2c-core-base.c | 6 +++--- drivers/i2c/i2c-core-smbus.c | 4 ++-- include/linux/i2c.h | 18 ++++++++++++++++++ 3 files changed, 23 insertions(+), 5 deletions(-)