Re: [PATCH 1/3] mmc: tmio: Add initial setting of interrupt mask register
On Thu, Sep 20, 2018 at 12:02:27AM +0200, Niklas Söderlund wrote: > Hi Wolfram, > > On 2018-09-16 16:48:07 +0200, Wolfram Sang wrote: > > > > > > > > So, we should at least protect this with TMIO_MMC_MIN_RCAR2, I'd > > > > > > think. > > > > > > Maybe we should even move it to renesas_sdhi_core.c, but I am not > > > > > > too > > > > > > sure about that. > > > > > > > > ... this. > > > > > > > > > > I will look into this and send a new version :-) > > > > Did you send V2 of this? I can't find it. > > > > No I have not yet posted a v2 of this series. Perhaps I should break > this patch out and send separate? I have handled the comments for this > but patch 3/3 in this series still requires more work, what do you > think? Yes, please. signature.asc Description: PGP signature
Re: [PATCH 4/5] pinctrl: sh-pfc: r8a77470: Add SDHI2 pin groups
Hi Fabrizio,
CC wolfram
On Wed, Sep 19, 2018 at 12:19 PM Fabrizio Castro
wrote:
> Although this patch is pretty much standard, I would like to start a
> discussion as while testing SDHI2 (which goes on the uSD connector on the
> bottom side of the iwg23s) I have come across an issue. The POC Control
> Register (IOCTRL6) of the RZ/G1C is structured in a completely different way
> from the other members of the RZ/G1 family, only one bit is used to control
> the interface, as opposed to the usual one bit per pin layout.
>
> There are 3 possible ways to fix this:
> 1) keep the clk pin of the interface in a pin group on its own in the PFC
> driver (which means I would need to drop this patch or rework the pin groups
> with an additional patch), specify SH_PFC_PIN_CFG_IO_VOLTAGE for the clock
> line alone, keep the clk pin in a device tree node on its own in the board
> specific device tree and specify power-source only within the device tree
> node containing the clk line. The SD card device tree node in the board
> specific device tree would look like the following:
> ...
> pinctrl-0 = <&sdhi2_pins>, <&sdhi2_pins_clk>;
> pinctrl-1 = <&sdhi2_pins>, <&sdhi2_pins_clk_uhs>;
> pinctrl-names = "default", "state_uhs";
>
That matches the datasheet, which says the bit is for the CLK line,
but that can't
be true, as the voltage selection should affect other lines, too.
> 2) Specify SH_PFC_PIN_CFG_IO_VOLTAGE for every line that belongs to the
> interface, keep the SD card pin groups as specified by this patch, map all of
> the pins to the same bit in the POC register (as per pin_to_pocctrl is
> concerned), and the board specific device tree definitions would look like
> every other RZ/G1 or R-Car Gen2 boards that support SDR*
> The only downside would be that the kernel would read-modify-write the POC
> Control Register with the same value for every line in the interface.
This looks the most sensible solution to me: just map in your
.pin_to_pocctrl() method
all pins of the interface to the single bit.
> 3) specify SH_PFC_PIN_CFG_IO_VOLTAGE for the clock line alone, come up with
> another macro for the other lines, keep the pin groups as specified by this
> patch, modify the logic of sh_pfc_pinconf_set and sh_pfc_pinconf_validate,
> and the board specific device tree would look like any other RZ/G1 or R-Car
> Gen2 board that supports SDR*
This looks overly complex.
So .pin_to_pocctrl() would need to return "ignore" for the other pins,
which can work easily for the "set" case, but not for the "get" case.
> I am not particularly enthusiastic about option 3), but option 1) and option
> 2) seem equally sound to me.
>
> What do you think about this?
I'd go for option 2.
Wolfram: what do you think?
Thanks!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH 1/5] pinctrl: sh-pfc: r8a77470: Add I2C4 pin groups
Hi Fabrizio,
On Wed, Sep 19, 2018 at 11:50 AM Fabrizio Castro
wrote:
> > Subject: Re: [PATCH 1/5] pinctrl: sh-pfc: r8a77470: Add I2C4 pin groups
> > On Tue, Sep 18, 2018 at 3:48 PM Fabrizio Castro
> > wrote:
> > > Add I2C4 pin groups and function to the R8A77470 SoC.
> >
> > Thanks for your patch!
> >
> > Any specific reason you added I2C4 only, and not the other I2C instances?
> > Usually we add all of them in one run.
>
> The iwg23s is a very small Raspberry Pi like SBC, there isn't much on it,
> therefore we can't test all of the interfaces we would like to test.
> The plan is to start supporting what we can easily test/access, for
> everything else we would like to wait and see, maybe at a later stage?
If your target is just the iwg23s, this is indeed fine.
However, you may have customers who want to use this SoC in their own products,
and thus aren't limited to the pins available on iwg23s.
Including this support upstream makes their life easier. While not everything
can be tested, it will at least have received some review.
Anyway, I'll queue this series in sh-pfc-for-v4.20.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
[RFC PATCH 1/4] i2c: core: remove outdated DEBUG output
The usefulness of this debug output is questionable. It covers only
direct i2c_transfer calls and no SMBUS calls, neither direct nor
emulated ones. And the latter one is largely used in the kernel, so a
lot of stuff is missed. Also, we have a proper tracing mechanism for all
these kinds of transfers in place for years now. Remove this old one.
Signed-off-by: Wolfram Sang
---
drivers/i2c/i2c-core-base.c | 10 --
1 file changed, 10 deletions(-)
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 9ee9a15e7134..c2b352c46fae 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1940,16 +1940,6 @@ int i2c_transfer(struct i2c_adapter *adap, struct
i2c_msg *msgs, int num)
*/
if (adap->algo->master_xfer) {
-#ifdef DEBUG
- for (ret = 0; ret < num; ret++) {
- dev_dbg(&adap->dev,
- "master_xfer[%d] %c, addr=0x%02x, len=%d%s\n",
- ret, (msgs[ret].flags & I2C_M_RD) ? 'R' : 'W',
- msgs[ret].addr, msgs[ret].len,
- (msgs[ret].flags & I2C_M_RECV_LEN) ? "+" : "");
- }
-#endif
-
if (in_atomic() || irqs_disabled()) {
ret = i2c_trylock_bus(adap, I2C_LOCK_SEGMENT);
if (!ret)
--
2.18.0
[RFC PATCH 2/4] i2c: core: remove level of indentation in i2c_transfer
Using the common kernel pattern to bail out at the beginning if some
conditions are not met, we can save a level of indentation. No
functional change.
Signed-off-by: Wolfram Sang
---
drivers/i2c/i2c-core-base.c | 33 -
1 file changed, 16 insertions(+), 17 deletions(-)
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index c2b352c46fae..799776c6d421 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1922,6 +1922,11 @@ int i2c_transfer(struct i2c_adapter *adap, struct
i2c_msg *msgs, int num)
{
int ret;
+ if (!adap->algo->master_xfer) {
+ dev_dbg(&adap->dev, "I2C level transfers not supported\n");
+ return -EOPNOTSUPP;
+ }
+
/* REVISIT the fault reporting model here is weak:
*
* - When we get an error after receiving N bytes from a slave,
@@ -1938,25 +1943,19 @@ int i2c_transfer(struct i2c_adapter *adap, struct
i2c_msg *msgs, int num)
*one (discarding status on the second message) or errno
*(discarding status on the first one).
*/
-
- if (adap->algo->master_xfer) {
- if (in_atomic() || irqs_disabled()) {
- ret = i2c_trylock_bus(adap, I2C_LOCK_SEGMENT);
- if (!ret)
- /* I2C activity is ongoing. */
- return -EAGAIN;
- } else {
- i2c_lock_bus(adap, I2C_LOCK_SEGMENT);
- }
-
- ret = __i2c_transfer(adap, msgs, num);
- i2c_unlock_bus(adap, I2C_LOCK_SEGMENT);
-
- return ret;
+ if (in_atomic() || irqs_disabled()) {
+ ret = i2c_trylock_bus(adap, I2C_LOCK_SEGMENT);
+ if (!ret)
+ /* I2C activity is ongoing. */
+ return -EAGAIN;
} else {
- dev_dbg(&adap->dev, "I2C level transfers not supported\n");
- return -EOPNOTSUPP;
+ i2c_lock_bus(adap, I2C_LOCK_SEGMENT);
}
+
+ ret = __i2c_transfer(adap, msgs, num);
+ i2c_unlock_bus(adap, I2C_LOCK_SEGMENT);
+
+ return ret;
}
EXPORT_SYMBOL(i2c_transfer);
--
2.18.0
[RFC PATCH 3/4] i2c: core: use I2C locking behaviour also for SMBUS
If I2C transfers are executed in atomic contexts, trylock is used
instead of lock. This behaviour was missing for SMBUS, although a lot of
transfers are of SMBUS type, either emulated or direct. So, factor out
the locking routine into a helper and use it for I2C and SMBUS.
Signed-off-by: Wolfram Sang
---
drivers/i2c/i2c-core-base.c | 11 +++
drivers/i2c/i2c-core-smbus.c | 7 ++-
drivers/i2c/i2c-core.h | 12
3 files changed, 21 insertions(+), 9 deletions(-)
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 799776c6d421..904b4d2ebefa 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1943,14 +1943,9 @@ int i2c_transfer(struct i2c_adapter *adap, struct
i2c_msg *msgs, int num)
*one (discarding status on the second message) or errno
*(discarding status on the first one).
*/
- if (in_atomic() || irqs_disabled()) {
- ret = i2c_trylock_bus(adap, I2C_LOCK_SEGMENT);
- if (!ret)
- /* I2C activity is ongoing. */
- return -EAGAIN;
- } else {
- i2c_lock_bus(adap, I2C_LOCK_SEGMENT);
- }
+ ret = __i2c_lock_bus_helper(adap);
+ if (ret)
+ return ret;
ret = __i2c_transfer(adap, msgs, num);
i2c_unlock_bus(adap, I2C_LOCK_SEGMENT);
diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
index 9cd66cabb84f..dbb46edb8e02 100644
--- a/drivers/i2c/i2c-core-smbus.c
+++ b/drivers/i2c/i2c-core-smbus.c
@@ -20,6 +20,8 @@
#include
#include
+#include "i2c-core.h"
+
#define CREATE_TRACE_POINTS
#include
@@ -530,7 +532,10 @@ s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr,
{
s32 res;
- i2c_lock_bus(adapter, I2C_LOCK_SEGMENT);
+ res = __i2c_lock_bus_helper(adapter);
+ if (res)
+ return res;
+
res = __i2c_smbus_xfer(adapter, addr, flags, read_write,
command, protocol, data);
i2c_unlock_bus(adapter, I2C_LOCK_SEGMENT);
diff --git a/drivers/i2c/i2c-core.h b/drivers/i2c/i2c-core.h
index 37576f50fe20..6e98aa811980 100644
--- a/drivers/i2c/i2c-core.h
+++ b/drivers/i2c/i2c-core.h
@@ -29,6 +29,18 @@ extern int __i2c_first_dynamic_bus_num;
int i2c_check_7bit_addr_validity_strict(unsigned short addr);
+static inline int __i2c_lock_bus_helper(struct i2c_adapter *adap)
+{
+ int ret = 0;
+
+ if (in_atomic() || irqs_disabled())
+ ret = i2c_trylock_bus(adap, I2C_LOCK_SEGMENT) ? 0 : -EAGAIN;
+ else
+ i2c_lock_bus(adap, I2C_LOCK_SEGMENT);
+
+ return ret;
+}
+
#ifdef CONFIG_ACPI
const struct acpi_device_id *
i2c_acpi_match_device(const struct acpi_device_id *matches,
--
2.18.0
[RFC PATCH 0/4] i2c: core: introduce master_xfer_irqless
To keep the discussion about master_xfer_irqless going, I post here a draft how I envision the changes to the I2C core. They are only build tested. I am unsure if I can test them on hardware before next week, so I'll send them around as RFC already, so people can get an idea and comment. Maybe Stefan has some bandwidth to test his imx driver implementation on top of this? A branch can be found here: git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/i2c/xfer_irqless Happy hacking and thanks for all the input to this, Wolfram Wolfram Sang (4): i2c: core: remove outdated DEBUG output i2c: core: remove level of indentation in i2c_transfer i2c: core: use I2C locking behaviour also for SMBUS i2c: core: introduce master_xfer_irqless callback drivers/i2c/i2c-core-base.c | 44 +--- drivers/i2c/i2c-core-smbus.c | 7 +- drivers/i2c/i2c-core.h | 12 ++ include/linux/i2c.h | 10 +--- 4 files changed, 41 insertions(+), 32 deletions(-) -- 2.18.0
[RFC PATCH 4/4] i2c: core: introduce master_xfer_irqless callback
We had the request to access devices very late when interrupts are not
available anymore multiple times now. Mostly to prepare shutdown or
reboot. Allow adapters to specify a specific callback for this case.
Note that we fall back to the generic master_xfer callback if this new
irqless one is not present. This is intentional to preserve the previous
behaviour and avoid regressions. Because there are drivers not using
interrupts or because it might have worked "accidently" before.
Signed-off-by: Wolfram Sang
---
drivers/i2c/i2c-core-base.c | 6 +-
include/linux/i2c.h | 10 +++---
2 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 904b4d2ebefa..f827446c3089 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1887,7 +1887,11 @@ int __i2c_transfer(struct i2c_adapter *adap, struct
i2c_msg *msgs, int num)
/* Retry automatically on arbitration loss */
orig_jiffies = jiffies;
for (ret = 0, try = 0; try <= adap->retries; try++) {
- ret = adap->algo->master_xfer(adap, msgs, num);
+ if ((in_atomic() || irqs_disabled()) &&
adap->algo->master_xfer_irqless)
+ ret = adap->algo->master_xfer_irqless(adap, msgs, num);
+ else
+ ret = adap->algo->master_xfer(adap, msgs, num);
+
if (ret != -EAGAIN)
break;
if (time_after(jiffies, orig_jiffies + adap->timeout))
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 65b4eaed1d96..11e615123bd0 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -498,6 +498,8 @@ i2c_register_board_info(int busnum, struct i2c_board_info
const *info,
* @master_xfer: Issue a set of i2c transactions to the given I2C adapter
* defined by the msgs array, with num messages available to transfer via
* the adapter specified by adap.
+ * @master_xfer_irqless: same as master_xfer. Yet, not using any interrupts
+ * so e.g. PMICs can be accessed very late before shutdown
* @smbus_xfer: Issue smbus transactions to the given I2C adapter. If this
* is not present, then the bus layer will try and convert the SMBus calls
* into I2C transfers instead.
@@ -511,9 +513,9 @@ i2c_register_board_info(int busnum, struct i2c_board_info
const *info,
* be addressed using the same bus algorithms - i.e. bit-banging or the PCF8584
* to name two of the most common.
*
- * The return codes from the @master_xfer field should indicate the type of
- * error code that occurred during the transfer, as documented in the kernel
- * Documentation file Documentation/i2c/fault-codes.
+ * The return codes from the @master_xfer{_irqless} field should indicate the
+ * type of error code that occurred during the transfer, as documented in the
+ * Kernel Documentation file Documentation/i2c/fault-codes.
*/
struct i2c_algorithm {
/* If an adapter algorithm can't do I2C-level access, set master_xfer
@@ -524,6 +526,8 @@ struct i2c_algorithm {
processed, or a negative value on error */
int (*master_xfer)(struct i2c_adapter *adap, struct i2c_msg *msgs,
int num);
+ int (*master_xfer_irqless)(struct i2c_adapter *adap,
+ struct i2c_msg *msgs, int num);
int (*smbus_xfer) (struct i2c_adapter *adap, u16 addr,
unsigned short flags, char read_write,
u8 command, int size, union i2c_smbus_data *data);
--
2.18.0
Re: [RFC PATCH 1/4] i2c: core: remove outdated DEBUG output
On 2018-09-20 18:14, Wolfram Sang wrote:
> The usefulness of this debug output is questionable. It covers only
> direct i2c_transfer calls and no SMBUS calls, neither direct nor
> emulated ones. And the latter one is largely used in the kernel, so a
> lot of stuff is missed. Also, we have a proper tracing mechanism for all
> these kinds of transfers in place for years now. Remove this old one.
>
> Signed-off-by: Wolfram Sang
This old one fires before locking and even if locking fails for the
atomic/irq case. You might want to mention that in the commit message?
But I certainly agree with the usefulness statement...
Acked-by: Peter Rosin
> ---
> drivers/i2c/i2c-core-base.c | 10 --
> 1 file changed, 10 deletions(-)
>
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 9ee9a15e7134..c2b352c46fae 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -1940,16 +1940,6 @@ int i2c_transfer(struct i2c_adapter *adap, struct
> i2c_msg *msgs, int num)
>*/
>
> if (adap->algo->master_xfer) {
> -#ifdef DEBUG
> - for (ret = 0; ret < num; ret++) {
> - dev_dbg(&adap->dev,
> - "master_xfer[%d] %c, addr=0x%02x, len=%d%s\n",
> - ret, (msgs[ret].flags & I2C_M_RD) ? 'R' : 'W',
> - msgs[ret].addr, msgs[ret].len,
> - (msgs[ret].flags & I2C_M_RECV_LEN) ? "+" : "");
> - }
> -#endif
> -
> if (in_atomic() || irqs_disabled()) {
> ret = i2c_trylock_bus(adap, I2C_LOCK_SEGMENT);
> if (!ret)
>
Re: [RFC PATCH 2/4] i2c: core: remove level of indentation in i2c_transfer
On 2018-09-20 18:14, Wolfram Sang wrote: > Using the common kernel pattern to bail out at the beginning if some > conditions are not met, we can save a level of indentation. No > functional change. > > Signed-off-by: Wolfram Sang Yes please! Acked-by: Peter Rosin Cheers, Peter
Re: [RFC PATCH 3/4] i2c: core: use I2C locking behaviour also for SMBUS
On 2018-09-20 18:14, Wolfram Sang wrote:
> If I2C transfers are executed in atomic contexts, trylock is used
> instead of lock. This behaviour was missing for SMBUS, although a lot of
> transfers are of SMBUS type, either emulated or direct. So, factor out
> the locking routine into a helper and use it for I2C and SMBUS.
>
> Signed-off-by: Wolfram Sang
Is it ok with static analyzers to "hide" the locking in helpers like
this? Will it not be harder for them to "see" what's going on? But I
don't think we have any annotations anyway, so...
Acked-by: Peter Rosin
> ---
> drivers/i2c/i2c-core-base.c | 11 +++
> drivers/i2c/i2c-core-smbus.c | 7 ++-
> drivers/i2c/i2c-core.h | 12
> 3 files changed, 21 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 799776c6d421..904b4d2ebefa 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -1943,14 +1943,9 @@ int i2c_transfer(struct i2c_adapter *adap, struct
> i2c_msg *msgs, int num)
>*one (discarding status on the second message) or errno
>*(discarding status on the first one).
>*/
> - if (in_atomic() || irqs_disabled()) {
> - ret = i2c_trylock_bus(adap, I2C_LOCK_SEGMENT);
> - if (!ret)
> - /* I2C activity is ongoing. */
> - return -EAGAIN;
> - } else {
> - i2c_lock_bus(adap, I2C_LOCK_SEGMENT);
> - }
> + ret = __i2c_lock_bus_helper(adap);
> + if (ret)
> + return ret;
>
> ret = __i2c_transfer(adap, msgs, num);
> i2c_unlock_bus(adap, I2C_LOCK_SEGMENT);
> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
> index 9cd66cabb84f..dbb46edb8e02 100644
> --- a/drivers/i2c/i2c-core-smbus.c
> +++ b/drivers/i2c/i2c-core-smbus.c
> @@ -20,6 +20,8 @@
> #include
> #include
>
> +#include "i2c-core.h"
> +
> #define CREATE_TRACE_POINTS
> #include
>
> @@ -530,7 +532,10 @@ s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr,
> {
> s32 res;
>
> - i2c_lock_bus(adapter, I2C_LOCK_SEGMENT);
> + res = __i2c_lock_bus_helper(adapter);
> + if (res)
> + return res;
> +
> res = __i2c_smbus_xfer(adapter, addr, flags, read_write,
> command, protocol, data);
> i2c_unlock_bus(adapter, I2C_LOCK_SEGMENT);
> diff --git a/drivers/i2c/i2c-core.h b/drivers/i2c/i2c-core.h
> index 37576f50fe20..6e98aa811980 100644
> --- a/drivers/i2c/i2c-core.h
> +++ b/drivers/i2c/i2c-core.h
> @@ -29,6 +29,18 @@ extern int __i2c_first_dynamic_bus_num;
>
> int i2c_check_7bit_addr_validity_strict(unsigned short addr);
>
> +static inline int __i2c_lock_bus_helper(struct i2c_adapter *adap)
> +{
> + int ret = 0;
> +
> + if (in_atomic() || irqs_disabled())
> + ret = i2c_trylock_bus(adap, I2C_LOCK_SEGMENT) ? 0 : -EAGAIN;
> + else
> + i2c_lock_bus(adap, I2C_LOCK_SEGMENT);
> +
> + return ret;
> +}
> +
> #ifdef CONFIG_ACPI
> const struct acpi_device_id *
> i2c_acpi_match_device(const struct acpi_device_id *matches,
>
Re: [RFC PATCH 4/4] i2c: core: introduce master_xfer_irqless callback
On 2018-09-20 18:14, Wolfram Sang wrote:
> We had the request to access devices very late when interrupts are not
> available anymore multiple times now. Mostly to prepare shutdown or
The first sentence is a bit backwards, I'd rephrase like so:
Multiple times now we've had the request to access devices very late, when
interrupts are no longer available.
> reboot. Allow adapters to specify a specific callback for this case.
> Note that we fall back to the generic master_xfer callback if this new
> irqless one is not present. This is intentional to preserve the previous
> behaviour and avoid regressions. Because there are drivers not using
> interrupts or because it might have worked "accidently" before.
accidentally
>
> Signed-off-by: Wolfram Sang
> ---
> drivers/i2c/i2c-core-base.c | 6 +-
> include/linux/i2c.h | 10 +++---
> 2 files changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 904b4d2ebefa..f827446c3089 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -1887,7 +1887,11 @@ int __i2c_transfer(struct i2c_adapter *adap, struct
> i2c_msg *msgs, int num)
> /* Retry automatically on arbitration loss */
> orig_jiffies = jiffies;
> for (ret = 0, try = 0; try <= adap->retries; try++) {
> - ret = adap->algo->master_xfer(adap, msgs, num);
> + if ((in_atomic() || irqs_disabled()) &&
> adap->algo->master_xfer_irqless)
> + ret = adap->algo->master_xfer_irqless(adap, msgs, num);
> + else
> + ret = adap->algo->master_xfer(adap, msgs, num);
> +
> if (ret != -EAGAIN)
> break;
> if (time_after(jiffies, orig_jiffies + adap->timeout))
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 65b4eaed1d96..11e615123bd0 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -498,6 +498,8 @@ i2c_register_board_info(int busnum, struct i2c_board_info
> const *info,
> * @master_xfer: Issue a set of i2c transactions to the given I2C adapter
> * defined by the msgs array, with num messages available to transfer via
> * the adapter specified by adap.
> + * @master_xfer_irqless: same as master_xfer. Yet, not using any interrupts
"Same" (with capital 'S') to match the other entries. Also, should it
not be @master_xfer to help the tools do the right thing?
> + * so e.g. PMICs can be accessed very late before shutdown
Trailing period.
I'm fine with this change, but should it not wait until there is a user?
(I think there is one in the wings, so that's a very weak objection...)
Acked-by: Peter Rosin
> * @smbus_xfer: Issue smbus transactions to the given I2C adapter. If this
> * is not present, then the bus layer will try and convert the SMBus calls
> * into I2C transfers instead.
> @@ -511,9 +513,9 @@ i2c_register_board_info(int busnum, struct i2c_board_info
> const *info,
> * be addressed using the same bus algorithms - i.e. bit-banging or the
> PCF8584
> * to name two of the most common.
> *
> - * The return codes from the @master_xfer field should indicate the type of
> - * error code that occurred during the transfer, as documented in the kernel
> - * Documentation file Documentation/i2c/fault-codes.
> + * The return codes from the @master_xfer{_irqless} field should indicate the
> + * type of error code that occurred during the transfer, as documented in the
> + * Kernel Documentation file Documentation/i2c/fault-codes.
> */
> struct i2c_algorithm {
> /* If an adapter algorithm can't do I2C-level access, set master_xfer
> @@ -524,6 +526,8 @@ struct i2c_algorithm {
> processed, or a negative value on error */
> int (*master_xfer)(struct i2c_adapter *adap, struct i2c_msg *msgs,
> int num);
> + int (*master_xfer_irqless)(struct i2c_adapter *adap,
> +struct i2c_msg *msgs, int num);
> int (*smbus_xfer) (struct i2c_adapter *adap, u16 addr,
> unsigned short flags, char read_write,
> u8 command, int size, union i2c_smbus_data *data);
>
Re: [RFC PATCH 0/4] i2c: core: introduce master_xfer_irqless
* Wolfram Sang [180920 09:19]: > To keep the discussion about master_xfer_irqless going, I post here a > draft how I envision the changes to the I2C core. They are only build > tested. I am unsure if I can test them on hardware before next week, so > I'll send them around as RFC already, so people can get an idea and > comment. Maybe Stefan has some bandwidth to test his imx driver > implementation on top of this? > > A branch can be found here: > > git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git > renesas/i2c/xfer_irqless > > Happy hacking and thanks for all the input to this, Thanks looks nice to me: Acked-by: Tony Lindgren
Re: [RFC PATCH 2/4] i2c: core: remove level of indentation in i2c_transfer
On Thu, Sep 20, 2018 at 07:26:21PM +0200, Peter Rosin wrote: > On 2018-09-20 18:14, Wolfram Sang wrote: > > Using the common kernel pattern to bail out at the beginning if some > > conditions are not met, we can save a level of indentation. No > > functional change. > > > > Signed-off-by: Wolfram Sang > > Yes please! :) signature.asc Description: PGP signature
Re: [RFC PATCH 3/4] i2c: core: use I2C locking behaviour also for SMBUS
On Thu, Sep 20, 2018 at 07:31:19PM +0200, Peter Rosin wrote: > On 2018-09-20 18:14, Wolfram Sang wrote: > > If I2C transfers are executed in atomic contexts, trylock is used > > instead of lock. This behaviour was missing for SMBUS, although a lot of > > transfers are of SMBUS type, either emulated or direct. So, factor out > > the locking routine into a helper and use it for I2C and SMBUS. > > > > Signed-off-by: Wolfram Sang > > Is it ok with static analyzers to "hide" the locking in helpers like > this? Will it not be harder for them to "see" what's going on? But I > don't think we have any annotations anyway, so... Yes, you are right. Yet, I prefer this to open coding the same twice and have the problem to keep them in sync. signature.asc Description: PGP signature
Re: [RFC PATCH 4/4] i2c: core: introduce master_xfer_irqless callback
On Thu, Sep 20, 2018 at 07:41:05PM +0200, Peter Rosin wrote: > On 2018-09-20 18:14, Wolfram Sang wrote: > > We had the request to access devices very late when interrupts are not > > available anymore multiple times now. Mostly to prepare shutdown or > > The first sentence is a bit backwards, I'd rephrase like so: > > Multiple times now we've had the request to access devices very late, when > interrupts are no longer available. Ok. Don't see much difference, but I don't mind. > > reboot. Allow adapters to specify a specific callback for this case. > > Note that we fall back to the generic master_xfer callback if this new > > irqless one is not present. This is intentional to preserve the previous > > behaviour and avoid regressions. Because there are drivers not using > > interrupts or because it might have worked "accidently" before. > > accidentally Thanks. > > @@ -498,6 +498,8 @@ i2c_register_board_info(int busnum, struct > > i2c_board_info const *info, > > * @master_xfer: Issue a set of i2c transactions to the given I2C adapter > > * defined by the msgs array, with num messages available to transfer via > > * the adapter specified by adap. > > + * @master_xfer_irqless: same as master_xfer. Yet, not using any interrupts > > "Same" (with capital 'S') to match the other entries. Also, should it > not be @master_xfer to help the tools do the right thing? I'll check. You are probably right. > > + * so e.g. PMICs can be accessed very late before shutdown > > Trailing period. > > I'm fine with this change, but should it not wait until there is a user? > (I think there is one in the wings, so that's a very weak objection...) As I mentioned in the cover-letter, this series is RFC because it is mainly meant as assistance for Stefan, so he could base his imx patches on top of it. Or the TI folks for their omap driver. I somewhen need to implement irqless transfers for the i2c-sh_mobile driver. But this may take a while, so I hope the others are first. And yes, I won't apply this series without a user and proper testing. Thanks for the review, Peter! signature.asc Description: PGP signature
Re: [RFC PATCH 0/4] i2c: core: introduce master_xfer_irqless
> Thanks looks nice to me: > > Acked-by: Tony Lindgren Thanks, Tony! signature.asc Description: PGP signature
Re: [RFC PATCH 0/4] i2c: core: introduce master_xfer_irqless
On Thu, Sep 20, 2018 at 06:14:19PM +0200, Wolfram Sang wrote: > To keep the discussion about master_xfer_irqless going, I post here a > draft how I envision the changes to the I2C core. They are only build > tested. I am unsure if I can test them on hardware before next week, so > I'll send them around as RFC already, so people can get an idea and > comment. Maybe Stefan has some bandwidth to test his imx driver > implementation on top of this? > > A branch can be found here: > > git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git > renesas/i2c/xfer_irqless What I forgot to mention: At some point, I'd like an adiitional check if the device is flagged to have very late I2C transfers and trigger a fat warning if not. To keep bugs visible. I have ideas but not perfect yet, so I'll leave this as the next step. signature.asc Description: PGP signature
