Re: [PATCH -next v3 2/2] media: ov772x: use SCCB helpers

[Wolfram Sang]

> > I think it would be even better to introduce a SSCB regmap layer
> > and use that.
> 
> I'm fine with introducing a SCCB regmap layer.

I am fine with this approach, too.

> But do we need to provide both a SCCB regmap and these SCCB helpers?

I don't know much about the OV sensor drivers. I'd think they would
benefit from regmap, so a-kind-of enforced conversion to regmap doesn't
sound too bad to me.

> If we have a regmap_init_sccb(), I feel these three SCCB helper functions
> don't need to be exported anymore.

Might be true. But other media guys are probably in a better position to
comment on this?

Note that I found SCCB devices with 16 bit regs: ov2659 & ov 2685. I
think we can cover those with SMBus word accesses. regmap is probably
also the cleanest way to hide this detail?



signature.asc
Description: PGP signature

Re: [PATCH -next v3 1/2] i2c: add SCCB helpers

[Wolfram Sang]

> even when not used. I think it will also cause the compiler to emit warnings 
> for unused functions. I don't think that's a good idea.

Where is my brown paper bag? :/

> > But if you insist on drivers/i2c/i2c-sccb.c, then it should be a
> > seperate module, I'd think?
> 
> Given how small the functions are, I wouldn't request that, as it would 
> introduce another Kconfig symbol, but I'm not opposed to such a new module 
> either.

OK, let's keep it simple for now and introduce
drivers/i2c/i2c-core-sccb.c and link it into the core module. It can
still be factored out later if the need arises.



signature.asc
Description: PGP signature

Re: [PATCH -next v3 1/2] i2c: add SCCB helpers

[Wolfram Sang]

> > +static inline int sccb_read_byte(struct i2c_client *client, u8 addr)
> > +{
> > +   int ret;
> > +   union i2c_smbus_data data;
> > +
> > +   i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
> > +
> > +   ret = __i2c_smbus_xfer(client->adapter, client->addr, client->flags,
> > +   I2C_SMBUS_WRITE, addr, I2C_SMBUS_BYTE, NULL);
> > +   if (ret < 0)
> > +   goto out;
> > +
> > +   ret = __i2c_smbus_xfer(client->adapter, client->addr, client->flags,
> > +   I2C_SMBUS_READ, 0, I2C_SMBUS_BYTE, );
> > +out:
> > +   i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
> > +
> > +   return ret < 0 ? ret : data.byte;
> > +}
> 
> I think I mentioned in a previous review of this patch that the function is 
> too big to be a static inline. It should instead be moved to a .c file.

Can be argued. I assume just removing the 'inline' won't do it for you?
I'd be fine with that, there are not many SCCB useres out there...

But if you insist on drivers/i2c/i2c-sccb.c, then it should be a
seperate module, I'd think?



signature.asc
Description: PGP signature

Re: [PATCH -next v3 2/2] media: ov772x: use SCCB helpers

[Wolfram Sang]

>  static int ov772x_read(struct i2c_client *client, u8 addr)
>  {
> - int ret;
> - u8 val;
> -
> - ret = i2c_master_send(client, , 1);
> - if (ret < 0)
> - return ret;
> - ret = i2c_master_recv(client, , 1);
> - if (ret < 0)
> - return ret;
> -
> - return val;
> + return sccb_read_byte(client, addr);
>  }
>  
>  static inline int ov772x_write(struct i2c_client *client, u8 addr, u8 value)
>  {
> - return i2c_smbus_write_byte_data(client, addr, value);
> + return sccb_write_byte(client, addr, value);
>  }

Minor nit: I'd rather drop these two functions and use the
sccb-accessors directly.

However, I really like how this looks here: It is totally clear we are
doing SCCB and hide away all the details.



signature.asc
Description: PGP signature

Re: [PATCH -next v3 1/2] i2c: add SCCB helpers

[Wolfram Sang]

Hi,

yes! From a first look this nails it for me. Thanks for doing it.

Minor comments follow...

> +#if 0
> + /*
> +  * sccb_xfer not needed yet, since there is no driver support currently.
> +  * Just showing how it should be done if we ever need it.
> +  */
> + if (adap->algo->sccb_xfer)
> + return true;
> +#endif

I think this should be a more generic comment in the header, like

/*
 * If we ever want support for hardware doing SCCB natively, we will
 * introduce a sccb_xfer() callback to struct i2c_algorithm and check
 * for it here.
 */

> +/**
> + * sccb_read_byte - Read data from SCCB slave device
> + * @client: Handle to slave device
> + * @addr: Register to be read from

I think 'addr' is too easy to confuse with client->addr. SMBus uses
'command' but I am also fine with 'reg' because we will deal with
registers.

> + * This executes the 2-phase write transmission cycle that is followed by a
> + * 2-phase read transmission cycle, returning negative errno else a data byte
> + * received from the device.
> + */

Nice docs!

> +/**
> + * sccb_write_byte - Write data to SCCB slave device
> + * @client: Handle to slave device
> + * @addr: Register to write to

'reg'?

Kind regards,

   Wolfram



signature.asc
Description: PGP signature

Re: [RFC PATCH v2] media: i2c: add SCCB helpers

[Wolfram Sang]

> > I'd prefer this file to be in the i2c realm. Maybe
> > 'include/linux/i2c-sccb.h" or something. I will come back to this.
> 
> And while at it, I think we also need a .c file, the functions (and 
> especially 
> sccb_read_byte()) should not be static inline.

Before we discuss this, we should make sure that the read-function is
complete and then we can decide further. I found some old notes based on
our previous discussions about SCCB and refactoring its access. You
mentioned there is HW supporting SCCB natively. Also, we found a device
where an SCCB device is connected to an SMBUS controller (yuck!). So,
given that, I'd think the read routine should look like this (in pseudo
code):


bool is_sccb_available(adap)
{
u32 needed_funcs = I2C_FUNC_SMBUS_BYTE | I2C_FUNC_SMBUS_WRITE_BYTE_DATA;

/*
 * sccb_xfer not needed yet, since there is no driver support currently.
 * Just showing how it should be done if we ever need it.
 */
if (adap->algo->sccb_xfer)
return true;

if (i2c_get_functionality(adap) & needed_funcs == needed_funcs)
return true;

return false;
}

sccb_get_byte()
{
if (adap->algo->sccb_xfer)
adap->algo->sccb_xfer(...)

/*
 * Prereq: __i2c_smbus_xfer needs to be created!
 */
i2c_lock_adapter(SEGMENT);
__i2c_smbus_xfer(..., SEND_BYTE, ...)
__i2c_smbus_xfer(..., GET_BYTE, ...)
i2c_unlock_adapter(SEGMENT)
}

sccb_write_byte()
{
return i2c_smbus_write_byte_data(...)
}

If I haven't overlooked something, this should make SCCB possible on I2C
controllers and SMBus controllers. We honor the requirement of not
having repeated start anywhere. As such, we might get even rid of the
I2C_M_STOP flag (for in-kernel users, but we sadly export it to
userspace).

About the IGNORE_NAK flag, I think this still should work where
supported as it is passed along indirectly with I2C_CLIENT_SCCB.
However, this flag handling with SCCB is really a mess and needs an
overhaul. This can be a second step, however. Most devices don't really
need it, do they?

Any further comments? Anything I missed?

Kind regards,

   Wolfram



signature.asc
Description: PGP signature

Re: [RFC PATCH v2] media: i2c: add SCCB helpers

[Wolfram Sang]

> > It sounds tempting, yet I am concerned about regressions. From that
> > point of view, it is safer to introduce i2c_lock_segment() and convert the
> > users which would benefit from that. How many drivers would be affected?
> 
> Right, there is also the aspect that changing a function like this
> might surprise people. Maybe i2c_lock_adapter should be killed and
> all callers changed to one of i2c_lock_segment and i2c_lock_root?

Yes, I like this one. It makes the change very clear to people.

> It's not that much churn...

OK, convinced. Are you willing/able to take on this? We are close to rc1
which would be a very good timing because a) linux-next should be almost
empty and b) we have nearly one cycle for linux-next and one cycle for
the rc-phase to get as much testing as possible. But also, there is no
actual need to rush...

This means for the original SCCB patch, it is independent of our
thoughts here. If SCCB gets in first, we will just convert it, too.

Thanks Peter!



signature.asc
Description: PGP signature

Re: [RFC PATCH v2] media: i2c: add SCCB helpers

[Wolfram Sang]

> So, maybe the easier thing to do is change i2c_lock_adapter to only
> lock the segment, and then have the callers beneath drivers/i2c/
> (plus the above mlx90614 driver) that really want to lock the root
> adapter instead of the segment adapter call a new function named
> i2c_lock_root (or something like that). Admittedly, that will be
> a few more trivial changes, but all but one will be under the I2C
> umbrella and thus require less interaction.
> 
> Wolfram, what do you think?

It sounds tempting, yet I am concerned about regressions. From that
point of view, it is safer to introduce i2c_lock_segment() and convert the
users which would benefit from that. How many drivers would be affected?



signature.asc
Description: PGP signature

Re: [RFC PATCH v2] media: i2c: add SCCB helpers

[Wolfram Sang]

On Wed, Jun 13, 2018 at 12:34:46AM +0900, Akinobu Mita wrote:
> (This is 2nd version of SCCB helpers patch.  After 1st version was
> submitted, I sent alternative patch titled "i2c: add I2C_M_FORCE_STOP".
> But it wasn't accepted because it makes the I2C core code unreadable.
> I couldn't find out a way to untangle it, so I returned to the original
> approach.)
> 
> This adds Serial Camera Control Bus (SCCB) helper functions (sccb_read_byte
> and sccb_write_byte) that are intended to be used by some of Omnivision
> sensor drivers.
> 
> The ov772x driver is going to use these functions in order to make it work
> with most i2c controllers.
> 
> As the ov772x device doesn't support repeated starts, this driver currently
> requires I2C_FUNC_PROTOCOL_MANGLING that is not supported by many i2c
> controller drivers.
> 
> With the sccb_read_byte() that issues two separated requests in order to
> avoid repeated start, the driver doesn't require I2C_FUNC_PROTOCOL_MANGLING.

From a first glance, this looks like my preferred solution so far.
Thanks for doing it! Let me sleep a bit over it for a thorough review...

> --- /dev/null
> +++ b/drivers/media/i2c/sccb.h

I'd prefer this file to be in the i2c realm. Maybe
'include/linux/i2c-sccb.h" or something. I will come back to this.



signature.asc
Description: PGP signature

Re: [PATCH v5 03/14] media: ov772x: allow i2c controllers without I2C_FUNC_PROTOCOL_MANGLING

[Wolfram Sang]

> It is a very bad idea to replace an i2c xfer by a pair of i2c
> send()/recv(), as, if are there any other device at the bus managed
> by an independent driver, you may end by mangling i2c transfers and
> eventually cause device malfunctions.

For I2C, this is true and a very important detail. Yet, we are talking
not I2C but SCCB here and SCCB demands a STOP between messages. So,
technically, to avoid what you describe one shouldn't mix I2C and SCCB
devices. I am quite aware the reality is very different, but still...

My preference would be to stop acting as SCCB was I2C but give it its
own set of functions so it becomes clear for everyone what protocol is
used for what device.

> So, IMO, the best is to push the patch you proposed that adds a
> new I2C flag:
> 
>   https://patchwork.linuxtv.org/patch/49396/

Sorry, but I don't like it. This makes the I2C core code very
unreadable. This is why I think SCCB should be exported to its own
realm. Which may live in i2c-core-sccb.c, no need for a seperate
subsystem.



signature.asc
Description: PGP signature

[PATCH 3/7] media: cx231xx: don't check number of messages in the driver

[Wolfram Sang]

Since commit 1eace8344c02 ("i2c: add param sanity check to
i2c_transfer()"), the I2C core does this check now. We can remove it
from drivers.

Signed-off-by: Wolfram Sang <w...@the-dreams.de>
---

Only build tested.

 drivers/media/usb/cx231xx/cx231xx-i2c.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/media/usb/cx231xx/cx231xx-i2c.c 
b/drivers/media/usb/cx231xx/cx231xx-i2c.c
index 6e1bef2a45bb..15a91169e749 100644
--- a/drivers/media/usb/cx231xx/cx231xx-i2c.c
+++ b/drivers/media/usb/cx231xx/cx231xx-i2c.c
@@ -376,8 +376,6 @@ static int cx231xx_i2c_xfer(struct i2c_adapter *i2c_adap,
struct cx231xx *dev = bus->dev;
int addr, rc, i, byte;
 
-   if (num <= 0)
-   return 0;
mutex_lock(>i2c_lock);
for (i = 0; i < num; i++) {
 
-- 
2.11.0

[PATCH 1/7] media: netup_unidvb: don't check number of messages in the driver

[Wolfram Sang]

Since commit 1eace8344c02 ("i2c: add param sanity check to
i2c_transfer()"), the I2C core does this check now. We can remove it
from drivers.

Signed-off-by: Wolfram Sang <w...@the-dreams.de>
---

Only build tested.

 drivers/media/pci/netup_unidvb/netup_unidvb_i2c.c | 5 -
 1 file changed, 5 deletions(-)

diff --git a/drivers/media/pci/netup_unidvb/netup_unidvb_i2c.c 
b/drivers/media/pci/netup_unidvb/netup_unidvb_i2c.c
index b13e319d24b7..5f1613aec93c 100644
--- a/drivers/media/pci/netup_unidvb/netup_unidvb_i2c.c
+++ b/drivers/media/pci/netup_unidvb/netup_unidvb_i2c.c
@@ -214,11 +214,6 @@ static int netup_i2c_xfer(struct i2c_adapter *adap,
struct netup_i2c *i2c = i2c_get_adapdata(adap);
u16 reg;
 
-   if (num <= 0) {
-   dev_dbg(i2c->adap.dev.parent,
-   "%s(): num == %d\n", __func__, num);
-   return -EINVAL;
-   }
spin_lock_irqsave(>lock, flags);
if (i2c->state != STATE_DONE) {
dev_dbg(i2c->adap.dev.parent,
-- 
2.11.0

[PATCH 4/7] media: em28xx: don't check number of messages in the driver

[Wolfram Sang]

Since commit 1eace8344c02 ("i2c: add param sanity check to
i2c_transfer()"), the I2C core does this check now. We can remove it
from drivers.

Signed-off-by: Wolfram Sang <w...@the-dreams.de>
---

Only build tested.

 drivers/media/usb/em28xx/em28xx-i2c.c | 4 
 1 file changed, 4 deletions(-)

diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c 
b/drivers/media/usb/em28xx/em28xx-i2c.c
index 6458682bc6e2..e19d6342e0d0 100644
--- a/drivers/media/usb/em28xx/em28xx-i2c.c
+++ b/drivers/media/usb/em28xx/em28xx-i2c.c
@@ -559,10 +559,6 @@ static int em28xx_i2c_xfer(struct i2c_adapter *i2c_adap,
dev->cur_i2c_bus = bus;
}
 
-   if (num <= 0) {
-   rt_mutex_unlock(>i2c_bus_lock);
-   return 0;
-   }
for (i = 0; i < num; i++) {
addr = msgs[i].addr << 1;
if (!msgs[i].len) {
-- 
2.11.0

[PATCH 2/7] media: si4713: don't check number of messages in the driver

[Wolfram Sang]

Since commit 1eace8344c02 ("i2c: add param sanity check to
i2c_transfer()"), the I2C core does this check now. We can remove it
from drivers.

Signed-off-by: Wolfram Sang <w...@the-dreams.de>
---

Only build tested.

 drivers/media/radio/si4713/radio-usb-si4713.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/media/radio/si4713/radio-usb-si4713.c 
b/drivers/media/radio/si4713/radio-usb-si4713.c
index 05c66701a899..1ebbf0217142 100644
--- a/drivers/media/radio/si4713/radio-usb-si4713.c
+++ b/drivers/media/radio/si4713/radio-usb-si4713.c
@@ -370,9 +370,6 @@ static int si4713_transfer(struct i2c_adapter *i2c_adapter,
int retval = -EINVAL;
int i;
 
-   if (num <= 0)
-   return 0;
-
for (i = 0; i < num; i++) {
if (msgs[i].flags & I2C_M_RD)
retval = si4713_i2c_read(radio, msgs[i].buf, 
msgs[i].len);
-- 
2.11.0

[PATCH 0/7] don't check number of I2C messages in drivers

[Wolfram Sang]

The core does it now, we can simplify drivers.

Based on v4.17-rc5. buildbot is happy. I'd suggest the media tree.

Thanks,

   Wolfram

Wolfram Sang (7):
  media: netup_unidvb: don't check number of messages in the driver
  media: si4713: don't check number of messages in the driver
  media: cx231xx: don't check number of messages in the driver
  media: em28xx: don't check number of messages in the driver
  media: hdpvr: don't check number of messages in the driver
  media: tm6000: don't check number of messages in the driver
  media: dvb-usb: don't check number of messages in the driver

 drivers/media/pci/netup_unidvb/netup_unidvb_i2c.c | 5 -
 drivers/media/radio/si4713/radio-usb-si4713.c | 3 ---
 drivers/media/usb/cx231xx/cx231xx-i2c.c   | 2 --
 drivers/media/usb/dvb-usb/m920x.c | 3 ---
 drivers/media/usb/em28xx/em28xx-i2c.c | 4 
 drivers/media/usb/hdpvr/hdpvr-i2c.c   | 3 ---
 drivers/media/usb/tm6000/tm6000-i2c.c | 2 --
 7 files changed, 22 deletions(-)

-- 
2.11.0

[PATCH 5/7] media: hdpvr: don't check number of messages in the driver

[Wolfram Sang]

Since commit 1eace8344c02 ("i2c: add param sanity check to
i2c_transfer()"), the I2C core does this check now. We can remove it
from drivers.

Signed-off-by: Wolfram Sang <w...@the-dreams.de>
---

Only build tested.

 drivers/media/usb/hdpvr/hdpvr-i2c.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/media/usb/hdpvr/hdpvr-i2c.c 
b/drivers/media/usb/hdpvr/hdpvr-i2c.c
index 4720d79b0282..c97dcf981b3f 100644
--- a/drivers/media/usb/hdpvr/hdpvr-i2c.c
+++ b/drivers/media/usb/hdpvr/hdpvr-i2c.c
@@ -117,9 +117,6 @@ static int hdpvr_transfer(struct i2c_adapter *i2c_adapter, 
struct i2c_msg *msgs,
struct hdpvr_device *dev = i2c_get_adapdata(i2c_adapter);
int retval = 0, addr;
 
-   if (num <= 0)
-   return 0;
-
mutex_lock(>i2c_mutex);
 
addr = msgs[0].addr << 1;
-- 
2.11.0

[PATCH 7/7] media: dvb-usb: don't check number of messages in the driver

[Wolfram Sang]

Since commit 1eace8344c02 ("i2c: add param sanity check to
i2c_transfer()"), the I2C core does this check now. We can remove it
from drivers.

Signed-off-by: Wolfram Sang <w...@the-dreams.de>
---

Only build tested.

 drivers/media/usb/dvb-usb/m920x.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/media/usb/dvb-usb/m920x.c 
b/drivers/media/usb/dvb-usb/m920x.c
index 32081c2ce0da..a6ab6688cbb3 100644
--- a/drivers/media/usb/dvb-usb/m920x.c
+++ b/drivers/media/usb/dvb-usb/m920x.c
@@ -255,9 +255,6 @@ static int m920x_i2c_xfer(struct i2c_adapter *adap, struct 
i2c_msg msg[], int nu
int i, j;
int ret = 0;
 
-   if (!num)
-   return -EINVAL;
-
if (mutex_lock_interruptible(>i2c_mutex) < 0)
return -EAGAIN;
 
-- 
2.11.0

[PATCH 6/7] media: tm6000: don't check number of messages in the driver

[Wolfram Sang]

Since commit 1eace8344c02 ("i2c: add param sanity check to
i2c_transfer()"), the I2C core does this check now. We can remove it
from drivers.

Signed-off-by: Wolfram Sang <w...@the-dreams.de>
---

Only build tested.

 drivers/media/usb/tm6000/tm6000-i2c.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/media/usb/tm6000/tm6000-i2c.c 
b/drivers/media/usb/tm6000/tm6000-i2c.c
index 659b63febf85..ccd1adf862b1 100644
--- a/drivers/media/usb/tm6000/tm6000-i2c.c
+++ b/drivers/media/usb/tm6000/tm6000-i2c.c
@@ -145,8 +145,6 @@ static int tm6000_i2c_xfer(struct i2c_adapter *i2c_adap,
struct tm6000_core *dev = i2c_adap->algo_data;
int addr, rc, i, byte;
 
-   if (num <= 0)
-   return 0;
for (i = 0; i < num; i++) {
addr = (msgs[i].addr << 1) & 0xff;
i2c_dprintk(2, "%s %s addr=0x%x len=%d:",
-- 
2.11.0

Re: [PATCH 0/7] i2c: clean up include/linux/i2c-*

[Wolfram Sang]

On Thu, Apr 19, 2018 at 10:00:06PM +0200, Wolfram Sang wrote:
> Move all plain platform_data includes to the platform_data-dir
> (except for i2c-pnx which can be moved into the driver itself).
> 
> My preference is to take these patches via the i2c tree. I can provide an
> immutable branch if needed. But we can also discuss those going in via
> arch-trees if dependencies are against us.

All applied to for-next!

The immutable branch is here:

git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git 
i2c/platform_data-immutable

Thanks,

   Wolfram



signature.asc
Description: PGP signature

Re: [PATCH 1/7] i2c: i2c-gpio: move header to platform_data

[Wolfram Sang]

>  arch/arm/mach-ks8695/board-acs5k.c   | 2 +-
>  arch/arm/mach-sa1100/simpad.c| 2 +-
>  arch/mips/alchemy/board-gpr.c| 2 +-

Those still need acks...

> diff --git a/arch/arm/mach-ks8695/board-acs5k.c 
> b/arch/arm/mach-ks8695/board-acs5k.c
> index 937eb1d47e7b..ef835d82cdb9 100644
> --- a/arch/arm/mach-ks8695/board-acs5k.c
> +++ b/arch/arm/mach-ks8695/board-acs5k.c
> @@ -19,7 +19,7 @@
>  #include 
>  #include 
>  #include 
> -#include 
> +#include 
>  #include 
>  
>  #include 

...

> diff --git a/arch/arm/mach-sa1100/simpad.c b/arch/arm/mach-sa1100/simpad.c
> index ace010479eb6..49a61e6f3c5f 100644
> --- a/arch/arm/mach-sa1100/simpad.c
> +++ b/arch/arm/mach-sa1100/simpad.c
> @@ -37,7 +37,7 @@
>  #include 
>  #include 
>  #include 
> -#include 
> +#include 
>  
>  #include "generic.h"
>  
> diff --git a/arch/mips/alchemy/board-gpr.c b/arch/mips/alchemy/board-gpr.c
> index 4e79dbd54a33..fa75d75b5ba9 100644
> --- a/arch/mips/alchemy/board-gpr.c
> +++ b/arch/mips/alchemy/board-gpr.c
> @@ -29,7 +29,7 @@
>  #include 
>  #include 
>  #include 
> -#include 
> +#include 
>  #include 
>  #include 
>  #include 

... and this was the shortened diff for those.

Greg, Russell, Ralf, James? Is it okay if I take this via my tree?

Thanks,

   Wolfram



signature.asc
Description: PGP signature

Re: [PATCH] v4l: vsp1: Fix vsp1_regs.h license header

[Wolfram Sang]

On Sat, Apr 28, 2018 at 12:46:47AM +0300, Laurent Pinchart wrote:
> All source files of the vsp1 driver are licensed under the GPLv2+ except
> for vsp1_regs.h which is licensed under GPLv2. This is caused by a bad
> copy that dates back from the initial version of the driver. Fix
> it.
> 
> Cc: Nobuhiro Iwamatsu <nobuhiro.iwamatsu...@renesas.com>
> Cc: Kieran Bingham <kieran.bingham+rene...@ideasonboard.com>
> Cc: Sergei Shtylyov <sergei.shtyl...@cogentembedded.com>
> Cc: Niklas Söderlund <niklas.soderlund+rene...@ragnatech.se>
> Cc: Wolfram Sang <wsa+rene...@sang-engineering.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+rene...@ideasonboard.com>

Acked-by: Wolfram Sang <wsa+rene...@sang-engineering.com>



signature.asc
Description: PGP signature

Re: [PATCH v3 02/11] media: ov772x: allow i2c controllers without I2C_FUNC_PROTOCOL_MANGLING

[Wolfram Sang]

> > Ah, didn't notice that so far. Can't find it in drivers/i2c/busses.
> > Where are those?
> 
> IIRC the OMAP I2C adapter supports SCCB natively. I'm not sure the driver 
> implements that though.

It doesn't currently. And seeing how long it sits in HW without a driver
for it, I don't have much expectations.

> > > we need to forward native SCCB calls all the way down the stack in that
> > > case.
> > 
> > And how is it done currently?
> 
> Currently we go down to .master_xfer(), and adapters can then decide to use 
> the hardware SCCB support. Again, it might not be implemented :-)

To sum it up: hardware-driven SCCB support is a very rare exception not
implemented anywhere in all those years. From a pragmatic point of view,
I'd say: we should be open for it, but we don't need to design around
it.

> I'm fine with SCCB helpers. Please note, however, that SCCB differs from 
> SMBus 
> in two ways: NACKs shall be ignored by the master (even though most SCCB 
> devices generate an ack, so we could likely ignore this), and write-read 
> sequences shouldn't use a repeated start. Apart from that register reads and 

Especially the latter is a huge difference to SMBus, and so I think it
will be much safer to not abuse SMBus calls for SCCB.

> register writes are identical to SMBus, which prompted the reuse (or abuse) 
> of 
> the SMBus API. If we end up implementing SCCB helpers, they will likely look 
> very, very similar to the SMBus implementation, including the SMBus emulated 
> transfer helper.

I don't think so. SCCB has much less transaction types than SMBus. Also, the
fallback as sketched in this patch (one master write, then a master
read) would be impossible on SMBus.

I have an idea in my mind. Maybe it is better to implement an RFC first,
so we can talk over code (even if only in prototype stage). I already
found this in ov7670.c, so I am proven wrong on this one already:

 472  * Note that there are two versions of these.  On the XO 1, the
 473  * i2c controller only does SMBUS, so that's what we use.  The
 474  * ov7670 is not really an SMBUS device, though, so the communication
 475  * is not always entirely reliable.

Sigh...



signature.asc
Description: PGP signature

Re: [PATCH v3 02/11] media: ov772x: allow i2c controllers without I2C_FUNC_PROTOCOL_MANGLING

[Wolfram Sang]

> SCCB helpers would work too. It would be easy to just move the functions 
> defined in this patch to helpers and be done with it. However, there are I2C 
> adapters that have native SCCB support, so to take advantage of that feature 

Ah, didn't notice that so far. Can't find it in drivers/i2c/busses.
Where are those?

> we need to forward native SCCB calls all the way down the stack in that case. 

And how is it done currently?

> That's why I thought an implementation in the I2C subsystem would be better. 
> Furthermore, as SCCB is really a slightly mangled version of I2C, I think the 
> I2C subsystem would be a natural location for the implementation. It 
> shouldn't 

Can be argued. But it can also be argues that it sits on top of I2C and
doesn't need to live in i2c-folders itself (like PMBus). The
implementation given in this patch looks a bit like the latter. However,
this is not the main question currently.

> be too much work, it's just a matter of deciding what the call stacks should 
> be for the native vs. emulated cases.

I don't like it. We shouldn't use SMBus calls for SCCB because SMBus
will very likely never support it. Or do you know of such a case? I
think I really want sccb helpers. So, people immediately see that SCCB
is used and not SMBus or I2C. And there, we can handle native support
vs. I2C-SCCB-emulation. And maybe SMBus-SCCB emulation but I doubt we
will ever need it.



signature.asc
Description: PGP signature

Re: [PATCH v3 02/11] media: ov772x: allow i2c controllers without I2C_FUNC_PROTOCOL_MANGLING

[Wolfram Sang]

> How about i2c_smbus_xfer_emulated() ? The tricky part will be to handle the 
> I2C adapters that implement .smbus_xfer(), as those won't go through 
> i2c_smbus_xfer_emulated(). i2c_smbus_xfer_emulated() relies on 
> i2c_transfer(), 
> which itself relies on the I2C adapter's .master_xfer() operation. We're thus 
> only concerned about the drivers that implement both .smbus_xfer() and 
> master_xfer(), and there's only 4 of them (i2c-opal, i2c-pasemi, i2c-powermac 
> and i2c-zx2967). Maybe the simplest solution would be to force the emulation 
> path if I2C_CLIENT_SCCB && !I2C_FUNC_PROTOCOL_MANGLING && ->master_xfer != 
> NULL ?
> 
> Wolfram, what do you think ?

I think it is a mess :)

Further: I don't think we will ever see an SMBus controller which allows
mangling. SMBus is way more precisely defined than I2C, so HW can then
do much more things automatically. They will always do a REP_START, so I
don't think you can connect SCCB devices to SMBus.

As a result, we shouldn't do SMBus calls for SCCB. Maybe we should
introduce sccb_byte_read? SCCB didn't specify much more than byte read
IIRC, or? The implementation here with two seperate messages makes much
sense to me then.

I could argue that the sccb_* helpers should live in drivers/media since
it is probably only Omnivision trying to work around I2C licensing here?

But if it is not too heavy, maybe we could take it into i2c as well.

Makes sense or did I miss something?



signature.asc
Description: PGP signature

[PATCH 1/7] i2c: i2c-gpio: move header to platform_data

[Wolfram Sang]

This header only contains platform_data. Move it to the proper directory.

Signed-off-by: Wolfram Sang <w...@the-dreams.de>
---
 MAINTAINERS  | 2 +-
 arch/arm/mach-ks8695/board-acs5k.c   | 2 +-
 arch/arm/mach-omap1/board-htcherald.c| 2 +-
 arch/arm/mach-pxa/palmz72.c  | 2 +-
 arch/arm/mach-pxa/viper.c| 2 +-
 arch/arm/mach-sa1100/simpad.c| 2 +-
 arch/mips/alchemy/board-gpr.c| 2 +-
 drivers/i2c/busses/i2c-gpio.c| 2 +-
 drivers/media/platform/marvell-ccic/mmp-driver.c | 2 +-
 drivers/mfd/sm501.c  | 2 +-
 include/linux/{ => platform_data}/i2c-gpio.h | 0
 11 files changed, 10 insertions(+), 10 deletions(-)
 rename include/linux/{ => platform_data}/i2c-gpio.h (100%)

diff --git a/MAINTAINERS b/MAINTAINERS
index 0a1410d5a621..7aad64b62102 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5872,7 +5872,7 @@ GENERIC GPIO I2C DRIVER
 M: Haavard Skinnemoen <hskinnem...@gmail.com>
 S: Supported
 F: drivers/i2c/busses/i2c-gpio.c
-F: include/linux/i2c-gpio.h
+F: include/linux/platform_data/i2c-gpio.h
 
 GENERIC GPIO I2C MULTIPLEXER DRIVER
 M: Peter Korsgaard <peter.korsga...@barco.com>
diff --git a/arch/arm/mach-ks8695/board-acs5k.c 
b/arch/arm/mach-ks8695/board-acs5k.c
index 937eb1d47e7b..ef835d82cdb9 100644
--- a/arch/arm/mach-ks8695/board-acs5k.c
+++ b/arch/arm/mach-ks8695/board-acs5k.c
@@ -19,7 +19,7 @@
 #include 
 #include 
 #include 
-#include 
+#include 
 #include 
 
 #include 
diff --git a/arch/arm/mach-omap1/board-htcherald.c 
b/arch/arm/mach-omap1/board-htcherald.c
index 67d46690a56e..da8f3fc3180f 100644
--- a/arch/arm/mach-omap1/board-htcherald.c
+++ b/arch/arm/mach-omap1/board-htcherald.c
@@ -31,7 +31,7 @@
 #include 
 #include 
 #include 
-#include 
+#include 
 #include 
 #include 
 #include 
diff --git a/arch/arm/mach-pxa/palmz72.c b/arch/arm/mach-pxa/palmz72.c
index 5877e547cecd..c053c8ce1586 100644
--- a/arch/arm/mach-pxa/palmz72.c
+++ b/arch/arm/mach-pxa/palmz72.c
@@ -30,7 +30,7 @@
 #include 
 #include 
 #include 
-#include 
+#include 
 #include 
 
 #include 
diff --git a/arch/arm/mach-pxa/viper.c b/arch/arm/mach-pxa/viper.c
index 90d0f277de55..39e05b7008d8 100644
--- a/arch/arm/mach-pxa/viper.c
+++ b/arch/arm/mach-pxa/viper.c
@@ -35,7 +35,7 @@
 #include 
 #include 
 #include 
-#include 
+#include 
 #include 
 #include 
 #include 
diff --git a/arch/arm/mach-sa1100/simpad.c b/arch/arm/mach-sa1100/simpad.c
index ace010479eb6..49a61e6f3c5f 100644
--- a/arch/arm/mach-sa1100/simpad.c
+++ b/arch/arm/mach-sa1100/simpad.c
@@ -37,7 +37,7 @@
 #include 
 #include 
 #include 
-#include 
+#include 
 
 #include "generic.h"
 
diff --git a/arch/mips/alchemy/board-gpr.c b/arch/mips/alchemy/board-gpr.c
index 4e79dbd54a33..fa75d75b5ba9 100644
--- a/arch/mips/alchemy/board-gpr.c
+++ b/arch/mips/alchemy/board-gpr.c
@@ -29,7 +29,7 @@
 #include 
 #include 
 #include 
-#include 
+#include 
 #include 
 #include 
 #include 
diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
index 58abb3eced58..005e6e0330c2 100644
--- a/drivers/i2c/busses/i2c-gpio.c
+++ b/drivers/i2c/busses/i2c-gpio.c
@@ -11,7 +11,7 @@
 #include 
 #include 
 #include 
-#include 
+#include 
 #include 
 #include 
 #include 
diff --git a/drivers/media/platform/marvell-ccic/mmp-driver.c 
b/drivers/media/platform/marvell-ccic/mmp-driver.c
index 816f4b6a7b8e..d9f0dd0d3525 100644
--- a/drivers/media/platform/marvell-ccic/mmp-driver.c
+++ b/drivers/media/platform/marvell-ccic/mmp-driver.c
@@ -12,7 +12,7 @@
 #include 
 #include 
 #include 
-#include 
+#include 
 #include 
 #include 
 #include 
diff --git a/drivers/mfd/sm501.c b/drivers/mfd/sm501.c
index ad774161a22d..66af659b01b2 100644
--- a/drivers/mfd/sm501.c
+++ b/drivers/mfd/sm501.c
@@ -19,7 +19,7 @@
 #include 
 #include 
 #include 
-#include 
+#include 
 #include 
 #include 
 
diff --git a/include/linux/i2c-gpio.h b/include/linux/platform_data/i2c-gpio.h
similarity index 100%
rename from include/linux/i2c-gpio.h
rename to include/linux/platform_data/i2c-gpio.h
-- 
2.11.0

[PATCH 0/7] i2c: clean up include/linux/i2c-*

[Wolfram Sang]

Move all plain platform_data includes to the platform_data-dir
(except for i2c-pnx which can be moved into the driver itself).

My preference is to take these patches via the i2c tree. I can provide an
immutable branch if needed. But we can also discuss those going in via
arch-trees if dependencies are against us.

The current branch can be found here:

git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/platform_data

and buildbot had no complaints.

Looking forward to comments or Acks, Revs...

Kind regards,

   Wolfram


Wolfram Sang (7):
  i2c: i2c-gpio: move header to platform_data
  i2c: i2c-mux-gpio: move header to platform_data
  i2c: i2c-ocores: move header to platform_data
  i2c: i2c-omap: move header to platform_data
  i2c: i2c-pca-platform: move header to platform_data
  i2c: i2c-xiic: move header to platform_data
  i2c: pnx: move header into the driver

 Documentation/i2c/busses/i2c-ocores|  2 +-
 Documentation/i2c/muxes/i2c-mux-gpio   |  4 +--
 MAINTAINERS|  8 ++---
 arch/arm/mach-ks8695/board-acs5k.c |  2 +-
 arch/arm/mach-omap1/board-htcherald.c  |  2 +-
 arch/arm/mach-omap1/common.h   |  2 +-
 arch/arm/mach-omap1/i2c.c  |  2 +-
 arch/arm/mach-omap2/common.h   |  2 +-
 arch/arm/mach-omap2/omap_hwmod_2420_data.c |  2 +-
 arch/arm/mach-omap2/omap_hwmod_2430_data.c |  2 +-
 arch/arm/mach-omap2/omap_hwmod_33xx_data.c |  2 +-
 arch/arm/mach-omap2/omap_hwmod_3xxx_data.c |  2 +-
 arch/arm/mach-omap2/omap_hwmod_44xx_data.c |  2 +-
 arch/arm/mach-omap2/omap_hwmod_54xx_data.c |  2 +-
 arch/arm/mach-omap2/omap_hwmod_7xx_data.c  |  2 +-
 arch/arm/mach-pxa/palmz72.c|  2 +-
 arch/arm/mach-pxa/viper.c  |  2 +-
 arch/arm/mach-sa1100/simpad.c  |  2 +-
 arch/mips/alchemy/board-gpr.c  |  2 +-
 arch/sh/boards/board-sh7785lcr.c   |  2 +-
 drivers/i2c/busses/i2c-gpio.c  |  2 +-
 drivers/i2c/busses/i2c-i801.c  |  2 +-
 drivers/i2c/busses/i2c-ocores.c|  2 +-
 drivers/i2c/busses/i2c-omap.c  |  2 +-
 drivers/i2c/busses/i2c-pca-platform.c  |  2 +-
 drivers/i2c/busses/i2c-pnx.c   | 21 +++-
 drivers/i2c/busses/i2c-xiic.c  |  2 +-
 drivers/i2c/muxes/i2c-mux-gpio.c   |  2 +-
 drivers/media/platform/marvell-ccic/mmp-driver.c   |  2 +-
 drivers/mfd/sm501.c|  2 +-
 drivers/mfd/timberdale.c   |  4 +--
 include/linux/i2c-pnx.h| 38 --
 include/linux/{ => platform_data}/i2c-gpio.h   |  0
 include/linux/{ => platform_data}/i2c-mux-gpio.h   |  0
 include/linux/{ => platform_data}/i2c-ocores.h |  0
 include/linux/{ => platform_data}/i2c-omap.h   |  0
 .../linux/{ => platform_data}/i2c-pca-platform.h   |  0
 include/linux/{ => platform_data}/i2c-xiic.h   |  0
 38 files changed, 55 insertions(+), 74 deletions(-)
 delete mode 100644 include/linux/i2c-pnx.h
 rename include/linux/{ => platform_data}/i2c-gpio.h (100%)
 rename include/linux/{ => platform_data}/i2c-mux-gpio.h (100%)
 rename include/linux/{ => platform_data}/i2c-ocores.h (100%)
 rename include/linux/{ => platform_data}/i2c-omap.h (100%)
 rename include/linux/{ => platform_data}/i2c-pca-platform.h (100%)
 rename include/linux/{ => platform_data}/i2c-xiic.h (100%)

-- 
2.11.0

[PATCH 00/61] tree-wide: simplify getting .drvdata

[Wolfram Sang]

I got tired of fixing this in Renesas drivers manually, so I took the big
hammer. Remove this cumbersome code pattern which got copy-pasted too much
already:

-   struct platform_device *pdev = to_platform_device(dev);
-   struct ep93xx_keypad *keypad = platform_get_drvdata(pdev);
+   struct ep93xx_keypad *keypad = dev_get_drvdata(dev);

I send this out as one patch per directory per subsystem. I think they should
be applied individually. If you prefer a broken out series per subsystem, I can
provide this as well. Just mail me.

A branch (tested by buildbot; only with all commits squashed into one commit
before) can be found here:

git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git 
coccinelle/get_drvdata

Open for other comments, suggestions, too, of course.

Here is the cocci-script I created (after  iterations by manually checking
samples):

@@
struct device* d;
identifier pdev;
expression *ptr;
@@
(
-   struct platform_device *pdev = to_platform_device(d);
|
-   struct platform_device *pdev;
...
-   pdev = to_platform_device(d);
)
<... when != pdev
-   >dev
+   d
...>

ptr =
-   platform_get_drvdata(pdev)
+   dev_get_drvdata(d)

<... when != pdev
-   >dev
+   d
...>

Kind regards,

   Wolfram


Wolfram Sang (61):
  ARM: plat-samsung: simplify getting .drvdata
  ata: simplify getting .drvdata
  auxdisplay: simplify getting .drvdata
  bus: simplify getting .drvdata
  clk: samsung: simplify getting .drvdata
  crypto: simplify getting .drvdata
  dma: simplify getting .drvdata
  dmaengine: dw: simplify getting .drvdata
  dmaengine: qcom: simplify getting .drvdata
  gpio: simplify getting .drvdata
  gpu: drm: msm: simplify getting .drvdata
  gpu: drm: msm: adreno: simplify getting .drvdata
  gpu: drm: msm: disp: mdp5: simplify getting .drvdata
  gpu: drm: msm: dsi: simplify getting .drvdata
  gpu: drm: omapdrm: displays: simplify getting .drvdata
  gpu: drm: vc4: simplify getting .drvdata
  hid: simplify getting .drvdata
  iio: common: cros_ec_sensors: simplify getting .drvdata
  iio: common: hid-sensors: simplify getting .drvdata
  input: keyboard: simplify getting .drvdata
  input: misc: simplify getting .drvdata
  input: mouse: simplify getting .drvdata
  input: touchscreen: simplify getting .drvdata
  iommu: simplify getting .drvdata
  media: platform: am437x: simplify getting .drvdata
  media: platform: exynos4-is: simplify getting .drvdata
  media: platform: s5p-mfc: simplify getting .drvdata
  mmc: host: simplify getting .drvdata
  mtd: devices: simplify getting .drvdata
  mtd: nand: onenand: simplify getting .drvdata
  net: dsa: simplify getting .drvdata
  net: ethernet: cadence: simplify getting .drvdata
  net: ethernet: davicom: simplify getting .drvdata
  net: ethernet: smsc: simplify getting .drvdata
  net: ethernet: ti: simplify getting .drvdata
  net: ethernet: wiznet: simplify getting .drvdata
  perf: simplify getting .drvdata
  pinctrl: simplify getting .drvdata
  pinctrl: intel: simplify getting .drvdata
  platform: x86: simplify getting .drvdata
  power: supply: simplify getting .drvdata
  ptp: simplify getting .drvdata
  pwm: simplify getting .drvdata
  rtc: simplify getting .drvdata
  slimbus: simplify getting .drvdata
  spi: simplify getting .drvdata
  staging: greybus: simplify getting .drvdata
  staging: iio: adc: simplify getting .drvdata
  staging: nvec: simplify getting .drvdata
  thermal: simplify getting .drvdata
  thermal: int340x_thermal: simplify getting .drvdata
  thermal: st: simplify getting .drvdata
  tty: serial: simplify getting .drvdata
  uio: simplify getting .drvdata
  usb: mtu3: simplify getting .drvdata
  usb: phy: simplify getting .drvdata
  video: fbdev: simplify getting .drvdata
  video: fbdev: omap2: omapfb: displays: simplify getting .drvdata
  watchdog: simplify getting .drvdata
  net: dsa: simplify getting .drvdata
  ASoC: atmel: simplify getting .drvdata

 arch/arm/plat-samsung/adc.c|  3 +-
 drivers/ata/pata_samsung_cf.c  |  8 ++---
 drivers/auxdisplay/arm-charlcd.c   |  6 ++--
 drivers/bus/brcmstb_gisb.c | 12 +++
 drivers/clk/samsung/clk-s3c2410-dclk.c |  6 ++--
 drivers/crypto/exynos-rng.c|  6 ++--
 drivers/crypto/picoxcell_crypto.c  |  6 ++--
 drivers/dma/at_hdmac.c |  9 ++---
 drivers/dma/at_xdmac.c |  9 ++---
 drivers/dma/dw/platform.c  |  6 ++--
 drivers/dma/fsldma.c   |  6 ++--
 drivers/dma/idma64.c   |  6 ++--
 drivers/dma/qcom/hidma.c   |  3 +-
 drivers/dma/qcom/hidma_mgmt_sys.c  |  6 ++--
 drivers/dma/ste_dma40.c| 12 +++
 drivers/dma/txx9dmac.c |  8 ++---

[PATCH 25/61] media: platform: am437x: simplify getting .drvdata

[Wolfram Sang]

We should get drvdata from struct device directly. Going via
platform_device is an unneeded step back and forth.

Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>
---

Build tested only. buildbot is happy. Please apply individually.

 drivers/media/platform/am437x/am437x-vpfe.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/am437x/am437x-vpfe.c 
b/drivers/media/platform/am437x/am437x-vpfe.c
index 601ae6487617..58ebc2220d0e 100644
--- a/drivers/media/platform/am437x/am437x-vpfe.c
+++ b/drivers/media/platform/am437x/am437x-vpfe.c
@@ -2662,8 +2662,7 @@ static void vpfe_save_context(struct vpfe_ccdc *ccdc)
 
 static int vpfe_suspend(struct device *dev)
 {
-   struct platform_device *pdev = to_platform_device(dev);
-   struct vpfe_device *vpfe = platform_get_drvdata(pdev);
+   struct vpfe_device *vpfe = dev_get_drvdata(dev);
struct vpfe_ccdc *ccdc = >ccdc;
 
/* if streaming has not started we don't care */
@@ -2720,8 +2719,7 @@ static void vpfe_restore_context(struct vpfe_ccdc *ccdc)
 
 static int vpfe_resume(struct device *dev)
 {
-   struct platform_device *pdev = to_platform_device(dev);
-   struct vpfe_device *vpfe = platform_get_drvdata(pdev);
+   struct vpfe_device *vpfe = dev_get_drvdata(dev);
struct vpfe_ccdc *ccdc = >ccdc;
 
/* if streaming has not started we don't care */
-- 
2.11.0

[PATCH 26/61] media: platform: exynos4-is: simplify getting .drvdata

[Wolfram Sang]

We should get drvdata from struct device directly. Going via
platform_device is an unneeded step back and forth.

Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>
---

Build tested only. buildbot is happy. Please apply individually.

 drivers/media/platform/exynos4-is/media-dev.c | 6 ++
 drivers/media/platform/exynos4-is/mipi-csis.c | 6 ++
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/media/platform/exynos4-is/media-dev.c 
b/drivers/media/platform/exynos4-is/media-dev.c
index 78b48a1fa26c..deb499f76412 100644
--- a/drivers/media/platform/exynos4-is/media-dev.c
+++ b/drivers/media/platform/exynos4-is/media-dev.c
@@ -1201,8 +1201,7 @@ static const struct media_device_ops fimc_md_ops = {
 static ssize_t fimc_md_sysfs_show(struct device *dev,
  struct device_attribute *attr, char *buf)
 {
-   struct platform_device *pdev = to_platform_device(dev);
-   struct fimc_md *fmd = platform_get_drvdata(pdev);
+   struct fimc_md *fmd = dev_get_drvdata(dev);
 
if (fmd->user_subdev_api)
return strlcpy(buf, "Sub-device API (sub-dev)\n", PAGE_SIZE);
@@ -1214,8 +1213,7 @@ static ssize_t fimc_md_sysfs_store(struct device *dev,
   struct device_attribute *attr,
   const char *buf, size_t count)
 {
-   struct platform_device *pdev = to_platform_device(dev);
-   struct fimc_md *fmd = platform_get_drvdata(pdev);
+   struct fimc_md *fmd = dev_get_drvdata(dev);
bool subdev_api;
int i;
 
diff --git a/drivers/media/platform/exynos4-is/mipi-csis.c 
b/drivers/media/platform/exynos4-is/mipi-csis.c
index cba46a656338..b4e28a299e26 100644
--- a/drivers/media/platform/exynos4-is/mipi-csis.c
+++ b/drivers/media/platform/exynos4-is/mipi-csis.c
@@ -891,8 +891,7 @@ static int s5pcsis_probe(struct platform_device *pdev)
 
 static int s5pcsis_pm_suspend(struct device *dev, bool runtime)
 {
-   struct platform_device *pdev = to_platform_device(dev);
-   struct v4l2_subdev *sd = platform_get_drvdata(pdev);
+   struct v4l2_subdev *sd = dev_get_drvdata(dev);
struct csis_state *state = sd_to_csis_state(sd);
int ret = 0;
 
@@ -921,8 +920,7 @@ static int s5pcsis_pm_suspend(struct device *dev, bool 
runtime)
 
 static int s5pcsis_pm_resume(struct device *dev, bool runtime)
 {
-   struct platform_device *pdev = to_platform_device(dev);
-   struct v4l2_subdev *sd = platform_get_drvdata(pdev);
+   struct v4l2_subdev *sd = dev_get_drvdata(dev);
struct csis_state *state = sd_to_csis_state(sd);
int ret = 0;
 
-- 
2.11.0

[PATCH 27/61] media: platform: s5p-mfc: simplify getting .drvdata

[Wolfram Sang]

We should get drvdata from struct device directly. Going via
platform_device is an unneeded step back and forth.

Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>
---

Build tested only. buildbot is happy. Please apply individually.

 drivers/media/platform/s5p-mfc/s5p_mfc.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c 
b/drivers/media/platform/s5p-mfc/s5p_mfc.c
index a80251ed3143..9ca707cb2a42 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
@@ -1449,8 +1449,7 @@ static int s5p_mfc_remove(struct platform_device *pdev)
 
 static int s5p_mfc_suspend(struct device *dev)
 {
-   struct platform_device *pdev = to_platform_device(dev);
-   struct s5p_mfc_dev *m_dev = platform_get_drvdata(pdev);
+   struct s5p_mfc_dev *m_dev = dev_get_drvdata(dev);
int ret;
 
if (m_dev->num_inst == 0)
@@ -1484,8 +1483,7 @@ static int s5p_mfc_suspend(struct device *dev)
 
 static int s5p_mfc_resume(struct device *dev)
 {
-   struct platform_device *pdev = to_platform_device(dev);
-   struct s5p_mfc_dev *m_dev = platform_get_drvdata(pdev);
+   struct s5p_mfc_dev *m_dev = dev_get_drvdata(dev);
 
if (m_dev->num_inst == 0)
return 0;
-- 
2.11.0

Re: [PATCHv2 6/7] cec-pin-error-inj.rst: document CEC Pin Error Injection

[Wolfram Sang]

> > I'll wait for a v3 with the debugfs ABI documentation in order to merge
> > it. Feel free to put it on a separate patch.
> 
> debugfs ABI? Sounds like an oxymoron to me.

Heh, thought the same :)



signature.asc
Description: PGP signature

Re: [PATCHv2 0/7] cec: add error injection support

[Wolfram Sang]

Hans,

> Special thanks go to Wolfram Sang since his i2c error injection
> presentation at the Embedded Linux Conference Europe 2017 inspired
> me to switch to debugfs for this instead of using ioctls.

You are very welcome! I am glad the presentation did inspire you.

Happy hacking,

   Wolfram



signature.asc
Description: PGP signature

Re: [PATCH 0/4] tree-wide: fix comparison to bitshift when dealing with a mask

[Wolfram Sang]

> I did that too but got no results.  Perhaps right shifting constants is
> pretty uncommon.  I can put that in the complete rule though.

Please do. Even if rare, we would want this bug pointed out, right? :)



signature.asc
Description: PGP signature

Re: [PATCH 0/4] tree-wide: fix comparison to bitshift when dealing with a mask

[Wolfram Sang]

Hi Julia,

> and got the results below.  I can make a version for the kernel shortly.

It should probably take care of right-shifting, too?

Thanks,

   Wolfram



signature.asc
Description: PGP signature

Re: [PATCH 0/4] tree-wide: fix comparison to bitshift when dealing with a mask

[Wolfram Sang]

> I found two more using "git grep 'define.*0x[0-9a-f]* < '":

I added '[0-9]\+' at the end of the regex to reduce the number of false
positives...

> drivers/net/can/m_can/m_can.c:#define RXFC_FWM_MASK (0x7f < 
> RXFC_FWM_SHIFT)
> drivers/usb/gadget/udc/goku_udc.h:#define INT_EPnNAK(n)
> (0x00100 < (n)) /* 0 < n < 4 */

... but you found those two true positives in there. Nice, thanks!



signature.asc
Description: PGP signature

[PATCH 3/4] v4l: dvb-frontends: stb0899: fix comparison to bitshift when dealing with a mask

[Wolfram Sang]

Due to a typo, the mask was destroyed by a comparison instead of a bit
shift.

Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>
---
Only build tested. To be applied individually per subsystem.

 drivers/media/dvb-frontends/stb0899_reg.h | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/dvb-frontends/stb0899_reg.h 
b/drivers/media/dvb-frontends/stb0899_reg.h
index ba1ed56304a0f4..f564269249a682 100644
--- a/drivers/media/dvb-frontends/stb0899_reg.h
+++ b/drivers/media/dvb-frontends/stb0899_reg.h
@@ -374,22 +374,22 @@
 
 #define STB0899_OFF0_IF_AGC_GAIN   0xf30c
 #define STB0899_BASE_IF_AGC_GAIN   0x
-#define STB0899_IF_AGC_GAIN(0x3fff < 0)
+#define STB0899_IF_AGC_GAIN(0x3fff << 0)
 #define STB0899_OFFST_IF_AGC_GAIN  0
 #define STB0899_WIDTH_IF_AGC_GAIN  14
 
 #define STB0899_OFF0_BB_AGC_GAIN   0xf310
 #define STB0899_BASE_BB_AGC_GAIN   0x
-#define STB0899_BB_AGC_GAIN(0x3fff < 0)
+#define STB0899_BB_AGC_GAIN(0x3fff << 0)
 #define STB0899_OFFST_BB_AGC_GAIN  0
 #define STB0899_WIDTH_BB_AGC_GAIN  14
 
 #define STB0899_OFF0_DC_OFFSET 0xf314
 #define STB0899_BASE_DC_OFFSET 0x
-#define STB0899_I  (0xff < 8)
+#define STB0899_I  (0xff << 8)
 #define STB0899_OFFST_I8
 #define STB0899_WIDTH_I8
-#define STB0899_Q  (0xff < 0)
+#define STB0899_Q  (0xff << 0)
 #define STB0899_OFFST_Q8
 #define STB0899_WIDTH_Q8
 
-- 
2.11.0

[PATCH 1/4] v4l: vsp1: fix mask creation for MULT_ALPHA_RATIO

[Wolfram Sang]

Due to a typo, the mask was destroyed by a comparison instead of a bit
shift. No regression since the mask has not been used yet.

Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>
---
Only build tested. To be applied individually per subsystem.

 drivers/media/platform/vsp1/vsp1_regs.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/vsp1/vsp1_regs.h 
b/drivers/media/platform/vsp1/vsp1_regs.h
index 26c4ffad2f4656..b1912c83a1dae2 100644
--- a/drivers/media/platform/vsp1/vsp1_regs.h
+++ b/drivers/media/platform/vsp1/vsp1_regs.h
@@ -225,7 +225,7 @@
 #define VI6_RPF_MULT_ALPHA_P_MMD_RATIO (1 << 8)
 #define VI6_RPF_MULT_ALPHA_P_MMD_IMAGE (2 << 8)
 #define VI6_RPF_MULT_ALPHA_P_MMD_BOTH  (3 << 8)
-#define VI6_RPF_MULT_ALPHA_RATIO_MASK  (0xff < 0)
+#define VI6_RPF_MULT_ALPHA_RATIO_MASK  (0xff << 0)
 #define VI6_RPF_MULT_ALPHA_RATIO_SHIFT 0
 
 /* 
-
-- 
2.11.0

[PATCH 0/4] tree-wide: fix comparison to bitshift when dealing with a mask

[Wolfram Sang]

In one Renesas driver, I found a typo which turned an intended bit shift ('<<')
into a comparison ('<'). Because this is a subtle issue, I looked tree wide for
similar patterns. This small patch series is the outcome.

Buildbot and checkpatch are happy. Only compile-tested. To be applied
individually per sub-system, I think. I'd think only the net: amd: patch needs
to be conisdered for stable, but I leave this to people who actually know this
driver.

CCing Dan. Maybe he has an idea how to add a test to smatch? In my setup, only
cppcheck reported a 'coding style' issue with a low prio.

Wolfram Sang (4):
  v4l: vsp1: fix mask creation for MULT_ALPHA_RATIO
  drm/exynos: fix comparison to bitshift when dealing with a mask
  v4l: dvb-frontends: stb0899: fix comparison to bitshift when dealing
with a mask
  net: amd-xgbe: fix comparison to bitshift when dealing with a mask

 drivers/gpu/drm/exynos/regs-fimc.h| 2 +-
 drivers/media/dvb-frontends/stb0899_reg.h | 8 
 drivers/media/platform/vsp1/vsp1_regs.h   | 2 +-
 drivers/net/ethernet/amd/xgbe/xgbe-drv.c  | 2 +-
 4 files changed, 7 insertions(+), 7 deletions(-)

-- 
2.11.0

Re: [PATCH v6 0/9] i2c: document DMA handling and add helpers for it

[Wolfram Sang]

On Sat, Nov 04, 2017 at 09:20:00PM +0100, Wolfram Sang wrote:
> So, after revisiting old mail threads, taking part in a similar discussion on
> the USB list, and implementing a not-convincing solution before, here is what 
> I
> cooked up to document and ease DMA handling for I2C within Linux. Please have 
> a
> look at the documentation introduced in patch 7 for details. And to make it
> clear again: The stuff below is opt-in. If host drivers are not updated, they
> will continue to work like before.
> 
> While previous versions until v3 tried to magically apply bounce buffers when
> needed, it became clear that detecting DMA safe buffers is too fragile. This
> approach is now opt-in, a DMA_SAFE flag needs to be set on an i2c_msg. The
> outcome so far is very convincing IMO. The core additions are simple and easy
> to understand. The driver changes for the Renesas IP cores became easy to
> understand, too.
> 
> Of course, we must now whitelist DMA safe buffers. This series implements it
> for core functionality:
> 
> a) for the I2C_RDWR when messages come from userspace
> b) for i2c_smbus_xfer_emulated(), DMA safe buffers are now allocated for
>block transfers
> c) i2c_master_{send|recv} have now a *_dmasafe variant
> 
> I am still not sure how we can teach regmap this new flag. regmap is a heavy
> user of I2C, so broonie's opinion here would be great to have. The rest should
> mostly be updating individual drivers which can be done when needed.
> 
> All patches have been tested with a Renesas Salvator-X board (r8a7796/M3-W) 
> and
> Renesas Lager board (r8a7790/H2). But more testing is really really welcome.
> 
> The branch can be found here:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git 
> renesas/topic/i2c-core-dma-v6
> 
> It is planned to land upstream in v4.16 and I want to push it to linux-next
> early after v4.15 to get lots of testing for the core changes.
> 
> Big kudos to Renesas Electronics for funding this work, thank you very much!
> 
> Regards,
> 
>Wolfram

Applied to for-next after fixing some cosmetic checkpatch issues!



signature.asc
Description: PGP signature

Re: [PATCH v6 0/9] i2c: document DMA handling and add helpers for it

[Wolfram Sang]

> > > We pretty much assume everything is DMA safe already, the majority of
> > > transfers go to/from kmalloc()ed scratch buffers so actually are DMA
> > > safe but for bulk transfers we use the caller buffer and there might be
> > > some problem users.
> 
> > So, pretty much the situation I2C was in before this patch set: we
> > pretty much assume DMA safety but there might be problem users.
> 
> It's a bit different in that it's much more likely that a SPI controller
> will actually do DMA than an I2C one since the speeds are higher and
> there's frequent applications that do large transfers so it's more
> likely that people will do the right thing as issues would tend to come
> up if they don't.

Yes, for SPI this is true. I was thinking more of regmap with its
usage of different transport mechanisms. But I guess they should all be
DMA safe because some of them need to be DMA safe?

> > > I can't really think of a particularly good way of
> > > handling it off the top of my head, obviously not setting the flag is
> > > easy but doesn't get the benefit while always using a bounce buffer
> > > would involve lots of unneeded memcpy().  Doing _dmasafe() isn't
> > > particularly appealing either but might be what we end up with.
> 
> > Okay. That sounds you are fine with the approach taken here, in general?
> 
> It's hard to summon enthusiasm but yes, without changes to the DMA stuff
> it's probably as good as we can do.

That sums it up pretty nicely. However, despite even my limited
enthusiasm for this extra flag, I think this set of rules has value. For
some platforms, DMA works more or less by coincidence and can lead to
surprises with e.g. virtual stacks. This is not good engineering
practice, I'd say.

I am going to apply this series now. Thanks for your input!



signature.asc
Description: PGP signature

Re: [PATCH v6 0/9] i2c: document DMA handling and add helpers for it

[Wolfram Sang]

On Wed, Nov 08, 2017 at 10:50:37PM +, Mark Brown wrote:
> On Sat, Nov 04, 2017 at 09:20:00PM +0100, Wolfram Sang wrote:
> 
> > While previous versions until v3 tried to magically apply bounce buffers 
> > when
> > needed, it became clear that detecting DMA safe buffers is too fragile. This
> > approach is now opt-in, a DMA_SAFE flag needs to be set on an i2c_msg. The
> > outcome so far is very convincing IMO. The core additions are simple and 
> > easy
> > to understand. The driver changes for the Renesas IP cores became easy to
> > understand, too.
> 
> It would really help a lot of things if there were a way to detect if a
> given memory block is DMA safe, having to pass the information around
> causes so much pain.

I so agree.

> > I am still not sure how we can teach regmap this new flag. regmap is a heavy
> > user of I2C, so broonie's opinion here would be great to have. The rest 
> > should
> > mostly be updating individual drivers which can be done when needed.
> 
> We pretty much assume everything is DMA safe already, the majority of
> transfers go to/from kmalloc()ed scratch buffers so actually are DMA
> safe but for bulk transfers we use the caller buffer and there might be
> some problem users.

So, pretty much the situation I2C was in before this patch set: we
pretty much assume DMA safety but there might be problem users.

> I can't really think of a particularly good way of
> handling it off the top of my head, obviously not setting the flag is
> easy but doesn't get the benefit while always using a bounce buffer
> would involve lots of unneeded memcpy().  Doing _dmasafe() isn't
> particularly appealing either but might be what we end up with.

Okay. That sounds you are fine with the approach taken here, in general?

Thanks,

   Wolfram



signature.asc
Description: PGP signature

[PATCH v6 0/9] i2c: document DMA handling and add helpers for it

[Wolfram Sang]

So, after revisiting old mail threads, taking part in a similar discussion on
the USB list, and implementing a not-convincing solution before, here is what I
cooked up to document and ease DMA handling for I2C within Linux. Please have a
look at the documentation introduced in patch 7 for details. And to make it
clear again: The stuff below is opt-in. If host drivers are not updated, they
will continue to work like before.

While previous versions until v3 tried to magically apply bounce buffers when
needed, it became clear that detecting DMA safe buffers is too fragile. This
approach is now opt-in, a DMA_SAFE flag needs to be set on an i2c_msg. The
outcome so far is very convincing IMO. The core additions are simple and easy
to understand. The driver changes for the Renesas IP cores became easy to
understand, too.

Of course, we must now whitelist DMA safe buffers. This series implements it
for core functionality:

a) for the I2C_RDWR when messages come from userspace
b) for i2c_smbus_xfer_emulated(), DMA safe buffers are now allocated for
   block transfers
c) i2c_master_{send|recv} have now a *_dmasafe variant

I am still not sure how we can teach regmap this new flag. regmap is a heavy
user of I2C, so broonie's opinion here would be great to have. The rest should
mostly be updating individual drivers which can be done when needed.

All patches have been tested with a Renesas Salvator-X board (r8a7796/M3-W) and
Renesas Lager board (r8a7790/H2). But more testing is really really welcome.

The branch can be found here:

git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git 
renesas/topic/i2c-core-dma-v6

It is planned to land upstream in v4.16 and I want to push it to linux-next
early after v4.15 to get lots of testing for the core changes.

Big kudos to Renesas Electronics for funding this work, thank you very much!

Regards,

   Wolfram

Change since V5:
* i2c_master_{send|recv} have now a *_dmasafe variant
* for i2c_smbus_xfer_emulated(), DMA safe buffers are now allocated for
  block transfers
* updated the documentation
* merged some rewording suggestions from Jonathan Cameron (thanks!)
* rebased the patches v4.14-rc6+i2c/for-next, reordered patches


Wolfram Sang (9):
  i2c: add a message flag for DMA safe buffers
  i2c: add helpers to ease DMA handling
  i2c: dev: mark RDWR buffers as DMA_SAFE
  i2c: refactor i2c_master_{send_recv}
  i2c: add i2c_master_{send|recv}_dmasafe
  i2c: smbus: use DMA safe buffers for emulated SMBus transactions
  i2c: add docs to clarify DMA handling
  i2c: sh_mobile: use core helper to decide when to use DMA
  i2c: rcar: skip DMA if buffer is not safe

 Documentation/i2c/DMA-considerations |  67 +
 drivers/i2c/busses/i2c-rcar.c|   2 +-
 drivers/i2c/busses/i2c-sh_mobile.c   |   8 ++-
 drivers/i2c/i2c-core-base.c  | 110 ---
 drivers/i2c/i2c-core-smbus.c |  45 --
 drivers/i2c/i2c-dev.c|   2 +
 include/linux/i2c.h  |  68 --
 include/uapi/linux/i2c.h |   3 +
 8 files changed, 246 insertions(+), 59 deletions(-)
 create mode 100644 Documentation/i2c/DMA-considerations

-- 
2.11.0

[PATCH v6 5/9] i2c: add i2c_master_{send|recv}_dmasafe

[Wolfram Sang]

Use the new helper to create variants of i2c_master_{send|recv} which
mark their buffers as DMA safe.

Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>
---
 include/linux/i2c.h | 31 +++
 1 file changed, 31 insertions(+)

diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index ef1a8791c1ae24..8c144e3cbfb261 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -81,6 +81,22 @@ static inline int i2c_master_recv(const struct i2c_client 
*client,
 };
 
 /**
+ * i2c_master_recv_dmasafe - issue a single I2C message in master receive mode
+ *  using a DMA safe buffer
+ * @client: Handle to slave device
+ * @buf: Where to store data read from slave, must be safe to use with DMA
+ * @count: How many bytes to read, must be less than 64k since msg.len is u16
+ *
+ * Returns negative errno, or else the number of bytes read.
+ */
+static inline int i2c_master_recv_dmasafe(const struct i2c_client *client,
+ char *buf, int count)
+{
+   return i2c_transfer_buffer_flags(client, buf, count,
+I2C_M_RD | I2C_M_DMA_SAFE);
+};
+
+/**
  * i2c_master_send - issue a single I2C message in master transmit mode
  * @client: Handle to slave device
  * @buf: Data that will be written to the slave
@@ -93,6 +109,21 @@ static inline int i2c_master_send(const struct i2c_client 
*client,
 {
return i2c_transfer_buffer_flags(client, (char *)buf, count, 0);
 };
+/**
+ * i2c_master_send_dmasafe - issue a single I2C message in master transmit mode
+ *  using a DMA safe buffer
+ * @client: Handle to slave device
+ * @buf: Data that will be written to the slave, must be safe to use with DMA
+ * @count: How many bytes to write, must be less than 64k since msg.len is u16
+ *
+ * Returns negative errno, or else the number of bytes written.
+ */
+static inline int i2c_master_send_dmasafe(const struct i2c_client *client,
+ const char *buf, int count)
+{
+   return i2c_transfer_buffer_flags(client, (char *)buf, count,
+I2C_M_DMA_SAFE);
+};
 
 /* Transfer num messages.
  */
-- 
2.11.0

[PATCH v6 6/9] i2c: smbus: use DMA safe buffers for emulated SMBus transactions

[Wolfram Sang]

For all block commands, try to allocate a DMA safe buffer and mark it
accordingly. Only use the stack, if the buffers cannot be allocated.

Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>
---
 drivers/i2c/i2c-core-smbus.c | 45 ++--
 1 file changed, 39 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
index 4bb9927afd0106..931c274fe26809 100644
--- a/drivers/i2c/i2c-core-smbus.c
+++ b/drivers/i2c/i2c-core-smbus.c
@@ -18,6 +18,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #define CREATE_TRACE_POINTS
 #include 
@@ -291,6 +292,22 @@ s32 i2c_smbus_write_i2c_block_data(const struct i2c_client 
*client, u8 command,
 }
 EXPORT_SYMBOL(i2c_smbus_write_i2c_block_data);
 
+static void i2c_smbus_try_get_dmabuf(struct i2c_msg *msg, u8 init_val)
+{
+   bool is_read = msg->flags & I2C_M_RD;
+   unsigned char *dma_buf;
+
+   dma_buf = kzalloc(I2C_SMBUS_BLOCK_MAX + (is_read ? 2 : 3), GFP_KERNEL);
+   if (!dma_buf)
+   return;
+
+   msg->buf = dma_buf;
+   msg->flags |= I2C_M_DMA_SAFE;
+
+   if (init_val)
+   msg->buf[0] = init_val;
+
+}
 /* Simulate a SMBus command using the i2c protocol
No checking of parameters is done!  */
 static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
@@ -368,6 +385,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter 
*adapter, u16 addr,
msg[1].flags |= I2C_M_RECV_LEN;
msg[1].len = 1; /* block length will be added by
   the underlying bus driver */
+   i2c_smbus_try_get_dmabuf([1], 0);
} else {
msg[0].len = data->block[0] + 2;
if (msg[0].len > I2C_SMBUS_BLOCK_MAX + 2) {
@@ -376,8 +394,10 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter 
*adapter, u16 addr,
data->block[0]);
return -EINVAL;
}
+
+   i2c_smbus_try_get_dmabuf([0], command);
for (i = 1; i < msg[0].len; i++)
-   msgbuf0[i] = data->block[i-1];
+   msg[0].buf[i] = data->block[i-1];
}
break;
case I2C_SMBUS_BLOCK_PROC_CALL:
@@ -389,16 +409,21 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter 
*adapter, u16 addr,
data->block[0]);
return -EINVAL;
}
+
msg[0].len = data->block[0] + 2;
+   i2c_smbus_try_get_dmabuf([0], command);
for (i = 1; i < msg[0].len; i++)
-   msgbuf0[i] = data->block[i-1];
+   msg[0].buf[i] = data->block[i-1];
+
msg[1].flags |= I2C_M_RECV_LEN;
msg[1].len = 1; /* block length will be added by
   the underlying bus driver */
+   i2c_smbus_try_get_dmabuf([1], 0);
break;
case I2C_SMBUS_I2C_BLOCK_DATA:
if (read_write == I2C_SMBUS_READ) {
msg[1].len = data->block[0];
+   i2c_smbus_try_get_dmabuf([1], 0);
} else {
msg[0].len = data->block[0] + 1;
if (msg[0].len > I2C_SMBUS_BLOCK_MAX + 1) {
@@ -407,8 +432,10 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter 
*adapter, u16 addr,
data->block[0]);
return -EINVAL;
}
+
+   i2c_smbus_try_get_dmabuf([0], command);
for (i = 1; i <= data->block[0]; i++)
-   msgbuf0[i] = data->block[i];
+   msg[0].buf[i] = data->block[i];
}
break;
default:
@@ -456,14 +483,20 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter 
*adapter, u16 addr,
break;
case I2C_SMBUS_I2C_BLOCK_DATA:
for (i = 0; i < data->block[0]; i++)
-   data->block[i+1] = msgbuf1[i];
+   data->block[i+1] = msg[1].buf[i];
break;
case I2C_SMBUS_BLOCK_DATA:
case I2C_SMBUS_BLOCK_PROC_CALL:
-   for (i = 0; i < msgbuf1[0] + 1; i++)
-   data->block[i] = msgbuf1[i];
+   for (i = 0; i < msg[1].buf[0] + 1; i++)
+   data->block[i] = msg[1].buf[i];
break;
}
+
+   if (msg[0].flags & I2C_M_DMA_SAFE)
+  

[PATCH v6 9/9] i2c: rcar: skip DMA if buffer is not safe

[Wolfram Sang]

This HW is prone to races, so it needs to setup new messages in irq
context. That means we can't alloc bounce buffers if a message buffer is
not DMA safe. So, in that case, simply fall back to PIO.

Reviewed-by: Jonathan Cameron <jonathan.came...@huawei.com>
Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>
---
 drivers/i2c/busses/i2c-rcar.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 15d764afec3b29..8a2ae3e6c561c4 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -359,7 +359,7 @@ static void rcar_i2c_dma(struct rcar_i2c_priv *priv)
int len;
 
/* Do not use DMA if it's not available or for messages < 8 bytes */
-   if (IS_ERR(chan) || msg->len < 8)
+   if (IS_ERR(chan) || msg->len < 8 || !(msg->flags & I2C_M_DMA_SAFE))
return;
 
if (read) {
-- 
2.11.0

[PATCH v6 7/9] i2c: add docs to clarify DMA handling

[Wolfram Sang]

Reviewed-by: Jonathan Cameron <jonathan.came...@huawei.com>
Reviewed-by: Mauro Carvalho Chehab <mche...@s-opensource.com>
Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>
---
 Documentation/i2c/DMA-considerations | 67 
 1 file changed, 67 insertions(+)
 create mode 100644 Documentation/i2c/DMA-considerations

diff --git a/Documentation/i2c/DMA-considerations 
b/Documentation/i2c/DMA-considerations
new file mode 100644
index 00..966610aa4620fa
--- /dev/null
+++ b/Documentation/i2c/DMA-considerations
@@ -0,0 +1,67 @@
+=
+Linux I2C and DMA
+=
+
+Given that i2c is a low-speed bus, over which the majority of messages
+transferred are small, it is not considered a prime user of DMA access. At this
+time of writing, only 10% of I2C bus master drivers have DMA support
+implemented. And the vast majority of transactions are so small that setting up
+DMA for it will likely add more overhead than a plain PIO transfer.
+
+Therefore, it is *not* mandatory that the buffer of an I2C message is DMA safe.
+It does not seem reasonable to apply additional burdens when the feature is so
+rarely used. However, it is recommended to use a DMA-safe buffer if your
+message size is likely applicable for DMA. Most drivers have this threshold
+around 8 bytes (as of today, this is mostly an educated guess, however). For
+any message of 16 byte or larger, it is probably a really good idea. Please
+note that other subsystems you use might add requirements. E.g., if your
+I2C bus master driver is using USB as a bridge, then you need to have DMA
+safe buffers always, because USB requires it.
+
+Clients
+---
+
+For clients, if you use a DMA safe buffer in i2c_msg, set the I2C_M_DMA_SAFE
+flag with it. Then, the I2C core and drivers know they can safely operate DMA
+on it. Note that using this flag is optional. I2C host drivers which are not
+updated to use this flag will work like before. And like before, they risk
+using an unsafe DMA buffer. To improve this situation, using I2C_M_DMA_SAFE in
+more and more clients and host drivers is the planned way forward. Note also
+that setting this flag makes only sense in kernel space. User space data is
+copied into kernel space anyhow. The I2C core makes sure the destination
+buffers in kernel space are always DMA capable. Also, when the core emulates
+SMBus transactions via I2C, the buffers for block transfers are DMA safe. Users
+of i2c_master_send() and i2c_master_recv() functions can now use DMA safe
+variants (i2c_master_send_dmasafe() and i2c_master_recv_dmasafe()) once they
+know their buffers are DMA safe. Users of i2c_transfer() must set the
+I2C_M_DMA_SAFE flag manually.
+
+Masters
+---
+
+Bus master drivers wishing to implement safe DMA can use helper functions from
+the I2C core. One gives you a DMA-safe buffer for a given i2c_msg as long as a
+certain threshold is met::
+
+   dma_buf = i2c_get_dma_safe_msg_buf(msg, threshold_in_byte);
+
+If a buffer is returned, it is either msg->buf for the I2C_M_DMA_SAFE case or a
+bounce buffer. But you don't need to care about that detail, just use the
+returned buffer. If NULL is returned, the threshold was not met or a bounce
+buffer could not be allocated. Fall back to PIO in that case.
+
+In any case, a buffer obtained from above needs to be released. It ensures data
+is copied back to the message and a potentially used bounce buffer is freed::
+
+   i2c_release_dma_safe_msg_buf(msg, dma_buf);
+
+The bounce buffer handling from the core is generic and simple. It will always
+allocate a new bounce buffer. If you want a more sophisticated handling (e.g.
+reusing pre-allocated buffers), you are free to implement your own.
+
+Please also check the in-kernel documentation for details. The i2c-sh_mobile
+driver can be used as a reference example how to use the above helpers.
+
+Final note: If you plan to use DMA with I2C (or with anything else, actually)
+make sure you have CONFIG_DMA_API_DEBUG enabled during development. It can help
+you find various issues which can be complex to debug otherwise.
-- 
2.11.0

[PATCH v6 2/9] i2c: add helpers to ease DMA handling

[Wolfram Sang]

One helper checks if DMA is suitable and optionally creates a bounce
buffer, if not. The other function returns the bounce buffer and makes
sure the data is properly copied back to the message.

Reviewed-by: Jonathan Cameron <jonathan.came...@huawei.com>
Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>
---
 drivers/i2c/i2c-core-base.c | 46 +
 include/linux/i2c.h |  3 +++
 2 files changed, 49 insertions(+)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 706164b4c5be46..de1850bd440659 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -2261,6 +2261,52 @@ void i2c_put_adapter(struct i2c_adapter *adap)
 }
 EXPORT_SYMBOL(i2c_put_adapter);
 
+/**
+ * i2c_get_dma_safe_msg_buf() - get a DMA safe buffer for the given i2c_msg
+ * @msg: the message to be checked
+ * @threshold: the minimum number of bytes for which using DMA makes sense
+ *
+ * Return: NULL if a DMA safe buffer was not obtained. Use msg->buf with PIO.
+ *Or a valid pointer to be used with DMA. After use, release it by
+ *calling i2c_release_dma_safe_msg_buf().
+ *
+ * This function must only be called from process context!
+ */
+u8 *i2c_get_dma_safe_msg_buf(struct i2c_msg *msg, unsigned int threshold)
+{
+   if (msg->len < threshold)
+   return NULL;
+
+   if (msg->flags & I2C_M_DMA_SAFE)
+   return msg->buf;
+
+   pr_debug("using bounce buffer for addr=0x%02x, len=%d\n",
+msg->addr, msg->len);
+
+   if (msg->flags & I2C_M_RD)
+   return kzalloc(msg->len, GFP_KERNEL);
+   else
+   return kmemdup(msg->buf, msg->len, GFP_KERNEL);
+}
+EXPORT_SYMBOL_GPL(i2c_get_dma_safe_msg_buf);
+
+/**
+ * i2c_release_dma_safe_msg_buf - release DMA safe buffer and sync with i2c_msg
+ * @msg: the message to be synced with
+ * @buf: the buffer obtained from i2c_get_dma_safe_msg_buf(). May be NULL.
+ */
+void i2c_release_dma_safe_msg_buf(struct i2c_msg *msg, u8 *buf)
+{
+   if (!buf || buf == msg->buf)
+   return;
+
+   if (msg->flags & I2C_M_RD)
+   memcpy(msg->buf, buf, msg->len);
+
+   kfree(buf);
+}
+EXPORT_SYMBOL_GPL(i2c_release_dma_safe_msg_buf);
+
 MODULE_AUTHOR("Simon G. Vogl <si...@tk.uni-linz.ac.at>");
 MODULE_DESCRIPTION("I2C-Bus main module");
 MODULE_LICENSE("GPL");
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 0f774406fad0e4..a0b57de91e21d3 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -769,6 +769,9 @@ static inline u8 i2c_8bit_addr_from_msg(const struct 
i2c_msg *msg)
return (msg->addr << 1) | (msg->flags & I2C_M_RD ? 1 : 0);
 }
 
+u8 *i2c_get_dma_safe_msg_buf(struct i2c_msg *msg, unsigned int threshold);
+void i2c_release_dma_safe_msg_buf(struct i2c_msg *msg, u8 *buf);
+
 int i2c_handle_smbus_host_notify(struct i2c_adapter *adap, unsigned short 
addr);
 /**
  * module_i2c_driver() - Helper macro for registering a modular I2C driver
-- 
2.11.0

[PATCH v6 8/9] i2c: sh_mobile: use core helper to decide when to use DMA

[Wolfram Sang]

This ensures that we fall back to PIO if the message length is too small
for DMA being useful. Otherwise, we use DMA. A bounce buffer might be
applied by the helper if the original message buffer is not DMA safe.

Reviewed-by: Jonathan Cameron <jonathan.came...@huawei.com>
Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>
---
 drivers/i2c/busses/i2c-sh_mobile.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-sh_mobile.c 
b/drivers/i2c/busses/i2c-sh_mobile.c
index c03acdf7139726..fbadb861672b84 100644
--- a/drivers/i2c/busses/i2c-sh_mobile.c
+++ b/drivers/i2c/busses/i2c-sh_mobile.c
@@ -145,6 +145,7 @@ struct sh_mobile_i2c_data {
struct dma_chan *dma_rx;
struct scatterlist sg;
enum dma_data_direction dma_direction;
+   u8 *dma_buf;
 };
 
 struct sh_mobile_dt_config {
@@ -548,6 +549,8 @@ static void sh_mobile_i2c_dma_callback(void *data)
pd->pos = pd->msg->len;
pd->stop_after_dma = true;
 
+   i2c_release_dma_safe_msg_buf(pd->msg, pd->dma_buf);
+
iic_set_clr(pd, ICIC, 0, ICIC_TDMAE | ICIC_RDMAE);
 }
 
@@ -608,7 +611,7 @@ static void sh_mobile_i2c_xfer_dma(struct 
sh_mobile_i2c_data *pd)
if (IS_ERR(chan))
return;
 
-   dma_addr = dma_map_single(chan->device->dev, pd->msg->buf, 
pd->msg->len, dir);
+   dma_addr = dma_map_single(chan->device->dev, pd->dma_buf, pd->msg->len, 
dir);
if (dma_mapping_error(chan->device->dev, dma_addr)) {
dev_dbg(pd->dev, "dma map failed, using PIO\n");
return;
@@ -665,7 +668,8 @@ static int start_ch(struct sh_mobile_i2c_data *pd, struct 
i2c_msg *usr_msg,
pd->pos = -1;
pd->sr = 0;
 
-   if (pd->msg->len > 8)
+   pd->dma_buf = i2c_get_dma_safe_msg_buf(pd->msg, 8);
+   if (pd->dma_buf)
sh_mobile_i2c_xfer_dma(pd);
 
/* Enable all interrupts to begin with */
-- 
2.11.0

[PATCH v6 1/9] i2c: add a message flag for DMA safe buffers

[Wolfram Sang]

I2C has no requirement that the buffer of a message needs to be DMA
safe. In case it is, it can now be flagged, so drivers wishing to
do DMA can use the buffer directly.

Reviewed-by: Jonathan Cameron <jonathan.came...@huawei.com>
Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>
---
 include/uapi/linux/i2c.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/uapi/linux/i2c.h b/include/uapi/linux/i2c.h
index 009e27bb9abe19..1c683cb319e4b7 100644
--- a/include/uapi/linux/i2c.h
+++ b/include/uapi/linux/i2c.h
@@ -71,6 +71,9 @@ struct i2c_msg {
 #define I2C_M_RD   0x0001  /* read data, from slave to master */
/* I2C_M_RD is guaranteed to be 0x0001! 
*/
 #define I2C_M_TEN  0x0010  /* this is a ten bit chip address */
+#define I2C_M_DMA_SAFE 0x0200  /* the buffer of this message is DMA 
safe */
+   /* makes only sense in kernelspace */
+   /* userspace buffers are copied anyway 
*/
 #define I2C_M_RECV_LEN 0x0400  /* length will be first received byte */
 #define I2C_M_NO_RD_ACK0x0800  /* if 
I2C_FUNC_PROTOCOL_MANGLING */
 #define I2C_M_IGNORE_NAK   0x1000  /* if I2C_FUNC_PROTOCOL_MANGLING */
-- 
2.11.0

[PATCH v6 3/9] i2c: dev: mark RDWR buffers as DMA_SAFE

[Wolfram Sang]

Reviewed-by: Jonathan Cameron <jonathan.came...@huawei.com>
Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>
---
 drivers/i2c/i2c-dev.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c
index 6f638bbc922db4..bbc7aadb4c899d 100644
--- a/drivers/i2c/i2c-dev.c
+++ b/drivers/i2c/i2c-dev.c
@@ -280,6 +280,8 @@ static noinline int i2cdev_ioctl_rdwr(struct i2c_client 
*client,
res = PTR_ERR(rdwr_pa[i].buf);
break;
}
+   /* memdup_user allocates with GFP_KERNEL, so DMA is ok */
+   rdwr_pa[i].flags |= I2C_M_DMA_SAFE;
 
/*
 * If the message length is received from the slave (similar
-- 
2.11.0

[PATCH v6 4/9] i2c: refactor i2c_master_{send_recv}

[Wolfram Sang]

Those two functions are very similar, the only differences are that one
needs the I2C_M_RD flag for its message while the other one needs the
buffer casted to drop the const. Introduce a generic helper which
allows to specify the flags (also needed later for DMA safe variants of
these calls) and let the casting be done in the inlining fuctions which
are now calling the new helper function.

Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>
---
 drivers/i2c/i2c-core-base.c | 64 +
 include/linux/i2c.h | 34 +---
 2 files changed, 48 insertions(+), 50 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index de1850bd440659..206c47c85c98c5 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1972,63 +1972,35 @@ int i2c_transfer(struct i2c_adapter *adap, struct 
i2c_msg *msgs, int num)
 EXPORT_SYMBOL(i2c_transfer);
 
 /**
- * i2c_master_send - issue a single I2C message in master transmit mode
+ * i2c_transfer_buffer_flags - issue a single I2C message transferring data
+ *to/from a buffer
  * @client: Handle to slave device
- * @buf: Data that will be written to the slave
- * @count: How many bytes to write, must be less than 64k since msg.len is u16
+ * @buf: Where the data is stored
+ * @count: How many bytes to transfer, must be less than 64k since msg.len is 
u16
+ * @flags: The flags to be used for the message, e.g. I2C_M_RD for reads
  *
- * Returns negative errno, or else the number of bytes written.
+ * Returns negative errno, or else the number of bytes transferred.
  */
-int i2c_master_send(const struct i2c_client *client, const char *buf, int 
count)
+int i2c_transfer_buffer_flags(const struct i2c_client *client, char *buf,
+ int count, u16 flags)
 {
int ret;
-   struct i2c_adapter *adap = client->adapter;
-   struct i2c_msg msg;
-
-   msg.addr = client->addr;
-   msg.flags = client->flags & I2C_M_TEN;
-   msg.len = count;
-   msg.buf = (char *)buf;
-
-   ret = i2c_transfer(adap, , 1);
-
-   /*
-* If everything went ok (i.e. 1 msg transmitted), return #bytes
-* transmitted, else error code.
-*/
-   return (ret == 1) ? count : ret;
-}
-EXPORT_SYMBOL(i2c_master_send);
-
-/**
- * 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
- * @count: How many bytes to read, must be less than 64k since msg.len is u16
- *
- * Returns negative errno, or else the number of bytes read.
- */
-int i2c_master_recv(const struct i2c_client *client, char *buf, int count)
-{
-   struct i2c_adapter *adap = client->adapter;
-   struct i2c_msg msg;
-   int ret;
-
-   msg.addr = client->addr;
-   msg.flags = client->flags & I2C_M_TEN;
-   msg.flags |= I2C_M_RD;
-   msg.len = count;
-   msg.buf = buf;
+   struct i2c_msg msg = {
+   .addr = client->addr,
+   .flags = flags | (client->flags & I2C_M_TEN),
+   .len = count,
+   .buf = buf,
+   };
 
-   ret = i2c_transfer(adap, , 1);
+   ret = i2c_transfer(client->adapter, , 1);
 
/*
-* If everything went ok (i.e. 1 msg received), return #bytes received,
-* else error code.
+* If everything went ok (i.e. 1 msg transferred), return #bytes
+* transferred, else error code.
 */
return (ret == 1) ? count : ret;
 }
-EXPORT_SYMBOL(i2c_master_recv);
+EXPORT_SYMBOL(i2c_transfer_buffer_flags);
 
 /* 
  * the i2c address scanning function
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index a0b57de91e21d3..ef1a8791c1ae24 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -63,10 +63,36 @@ struct property_entry;
  * transmit an arbitrary number of messages without interruption.
  * @count must be be less than 64k since msg.len is u16.
  */
-extern int i2c_master_send(const struct i2c_client *client, const char *buf,
-  int count);
-extern int i2c_master_recv(const struct i2c_client *client, char *buf,
-  int count);
+extern int i2c_transfer_buffer_flags(const struct i2c_client *client,
+char *buf, int count, u16 flags);
+
+/**
+ * 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
+ * @count: How many bytes to read, must be less than 64k since msg.len is u16
+ *
+ * Returns negative errno, or else the number of bytes read.
+ */
+static inline int i2c_master_recv(const struct i2c_client *client,
+ char *buf, int count)
+{
+   return i2c_trans

Re: [RFC PATCH v5 6/6] i2c: dev: mark RDWR buffers as DMA_SAFE

[Wolfram Sang]

On Thu, Sep 21, 2017 at 03:17:44PM +0100, Jonathan Cameron wrote:
> On Wed, 20 Sep 2017 20:59:56 +0200
> Wolfram Sang <wsa+rene...@sang-engineering.com> wrote:
> 
> > Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>
> 
> Makes sense as do the other drivers.
> 
> Feel free to add
> 
> Reviewed-by: Jonathan Cameron <jonathan.came...@huawei.com>
> 
> to all of them (though they hardly took a lot of reviewing given how simple
> the patches were :)

Well, bugs can slip in everywhere, so thanks for the review!



signature.asc
Description: PGP signature

Re: [RFC PATCH v5 2/6] i2c: add helpers to ease DMA handling

[Wolfram Sang]

> > > +/**
> > > + * i2c_release_dma_safe_msg_buf - release DMA safe buffer and sync with 
> > > i2c_msg
> > > + * @msg: the message to be synced with
> > > + * @buf: the buffer obtained from i2c_get_dma_safe_msg_buf(). May be 
> > > NULL.
> > > + */
> > > +void i2c_release_dma_safe_msg_buf(struct i2c_msg *msg, u8 *buf)
> > > +{
> > > + if (!buf || buf == msg->buf)
> > > + return;
> > > +
> > > + if (msg->flags & I2C_M_RD)
> > > + memcpy(msg->buf, buf, msg->len);
> > > +
> > > + kfree(buf);
> 
> Only free when you actually allocated it.  Seems to me like you need
> to check if (!(msg->flags & I2C_M_DMA_SAFE)) before kfree.
> 
> Otherwise the logic to do this will be needed in every driver
> which will get irritating fast.

Well, I return early if (buf == msg->buf) which is only true for
I2C_M_DMA_SAFE. If not, I allocated the buffer. Am I missing something?
It would be very strange to call this function if the caller allocated
the buffer manually.

Thanks for the review!



signature.asc
Description: PGP signature

[RFC PATCH v5 2/6] i2c: add helpers to ease DMA handling

[Wolfram Sang]

One helper checks if DMA is suitable and optionally creates a bounce
buffer, if not. The other function returns the bounce buffer and makes
sure the data is properly copied back to the message.

Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>
---
 drivers/i2c/i2c-core-base.c | 45 +
 include/linux/i2c.h |  3 +++
 2 files changed, 48 insertions(+)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 56e46581b84bdb..925a22794d3ced 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -2241,6 +2241,51 @@ void i2c_put_adapter(struct i2c_adapter *adap)
 }
 EXPORT_SYMBOL(i2c_put_adapter);
 
+/**
+ * i2c_get_dma_safe_msg_buf() - get a DMA safe buffer for the given i2c_msg
+ * @msg: the message to be checked
+ * @threshold: the amount of byte from which using DMA makes sense
+ *
+ * Return: NULL if a DMA safe buffer was not obtained. Use msg->buf with PIO.
+ *
+ *Or a valid pointer to be used with DMA. Note that it can either be
+ *msg->buf or a bounce buffer. After use, release it by calling
+ *i2c_release_dma_safe_msg_buf().
+ *
+ * This function must only be called from process context!
+ */
+u8 *i2c_get_dma_safe_msg_buf(struct i2c_msg *msg, unsigned int threshold)
+{
+   if (msg->len < threshold)
+   return NULL;
+
+   if (msg->flags & I2C_M_DMA_SAFE)
+   return msg->buf;
+
+   if (msg->flags & I2C_M_RD)
+   return kzalloc(msg->len, GFP_KERNEL);
+   else
+   return kmemdup(msg->buf, msg->len, GFP_KERNEL);
+}
+EXPORT_SYMBOL_GPL(i2c_get_dma_safe_msg_buf);
+
+/**
+ * i2c_release_dma_safe_msg_buf - release DMA safe buffer and sync with i2c_msg
+ * @msg: the message to be synced with
+ * @buf: the buffer obtained from i2c_get_dma_safe_msg_buf(). May be NULL.
+ */
+void i2c_release_dma_safe_msg_buf(struct i2c_msg *msg, u8 *buf)
+{
+   if (!buf || buf == msg->buf)
+   return;
+
+   if (msg->flags & I2C_M_RD)
+   memcpy(msg->buf, buf, msg->len);
+
+   kfree(buf);
+}
+EXPORT_SYMBOL_GPL(i2c_release_dma_safe_msg_buf);
+
 MODULE_AUTHOR("Simon G. Vogl <si...@tk.uni-linz.ac.at>");
 MODULE_DESCRIPTION("I2C-Bus main module");
 MODULE_LICENSE("GPL");
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index d501d3956f13f0..1e99342f180f45 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -767,6 +767,9 @@ static inline u8 i2c_8bit_addr_from_msg(const struct 
i2c_msg *msg)
return (msg->addr << 1) | (msg->flags & I2C_M_RD ? 1 : 0);
 }
 
+u8 *i2c_get_dma_safe_msg_buf(struct i2c_msg *msg, unsigned int threshold);
+void i2c_release_dma_safe_msg_buf(struct i2c_msg *msg, u8 *buf);
+
 int i2c_handle_smbus_host_notify(struct i2c_adapter *adap, unsigned short 
addr);
 /**
  * module_i2c_driver() - Helper macro for registering a modular I2C driver
-- 
2.11.0

[RFC PATCH v5 0/6] i2c: document DMA handling and add helpers for it

[Wolfram Sang]

So, after revisiting old mail threads, taking part in a similar discussion on
the USB list, and implementing a not-convincing solution before, here is what I
cooked up to document and ease DMA handling for I2C within Linux. Please have a
look at the documentation introduced in patch 3 for details. And to make it
clear again: The stuff below is opt-in. If host drivers are not updated, they
will continue to work like before.

While the previous versions tried to magically apply bounce buffers when
needed, it became clear that detecting DMA safe buffers is too fragile. This
approach is now opt-in, a DMA_SAFE flag needs to be set on an i2c_msg. The
outcome so far is very convincing IMO. The core additions are simple and easy
to understand (makes me even think of inlining them again?). The driver changes
for the Renesas IP cores became easier to understand, too. While only a tad for
the i2c-sh_mobile driver, the situation became a LOT better for the i2c-rcar
driver. No more DMA disabling for the whole transfer in case of unsafe buffers,
we are back to per-msg handling. And the code fix is now an easy to understand
one line change. Yay!

Of course, we must now whitelist DMA safe buffers. An example for I2C_RDWR case
is in this series. It makes the i2ctransfer utility have DMA_SAFE buffers,
which is nice for testing as i2cdump will (currently) not use DMA_SAFE buffers.
My plan is to add two new calls: i2c_master_{send|receive}_dma_safe which can
be used if DMA_SAFE buffers are provided. So, drivers can simply switch to
them. Also, the buffers used within i2c_smbus_xfer_emulated() need to be
converted to be DMA_SAFE which will cover a huge bunch of use cases. The rest
is then updating drivers which can be done when needed.

As these conversions are not done yet, this patch series has RFC status. But I
already would like to get opinions on this approach, so I'll cc mailing lists
of the heavier I2C users. Please let me know what you think.

All patches have been tested with a Renesas Salvator-X board (r8a7796/M3-W).

The branch can be found here:

git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git 
renesas/topic/i2c-core-dma-rfc-v5

And big kudos to Renesas Electronics for funding this work, thank you very much!

Regards,

   Wolfram

Changes since v4:
* rebased to v4.14-rc1
* (hopefully) improved the docs to be more clear
* added basic RST syntax to the docs

This is mainly an update to agree on the docs. Missing code is still missing
and will be added in v6.

Changes since v3:
* completely redesigned


Wolfram Sang (6):
  i2c: add a message flag for DMA safe buffers
  i2c: add helpers to ease DMA handling
  i2c: add docs to clarify DMA handling
  i2c: sh_mobile: use helper to decide if DMA is useful
  i2c: rcar: skip DMA if buffer is not safe
  i2c: dev: mark RDWR buffers as DMA_SAFE

 Documentation/i2c/DMA-considerations | 58 
 drivers/i2c/busses/i2c-rcar.c|  2 +-
 drivers/i2c/busses/i2c-sh_mobile.c   |  8 +++--
 drivers/i2c/i2c-core-base.c  | 45 
 drivers/i2c/i2c-dev.c|  2 ++
 include/linux/i2c.h  |  3 ++
 include/uapi/linux/i2c.h |  3 ++
 7 files changed, 118 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/i2c/DMA-considerations

-- 
2.11.0

[RFC PATCH v5 4/6] i2c: sh_mobile: use helper to decide if DMA is useful

[Wolfram Sang]

This ensures that we fall back to PIO if the message length is too small
for DMA being useful. Otherwise, we use DMA. A bounce buffer might be
applied by the helper if the original message buffer is not DMA safe.

Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>
---
 drivers/i2c/busses/i2c-sh_mobile.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-sh_mobile.c 
b/drivers/i2c/busses/i2c-sh_mobile.c
index 6f2aaeb7c4fa15..b76399d8a3abd3 100644
--- a/drivers/i2c/busses/i2c-sh_mobile.c
+++ b/drivers/i2c/busses/i2c-sh_mobile.c
@@ -145,6 +145,7 @@ struct sh_mobile_i2c_data {
struct dma_chan *dma_rx;
struct scatterlist sg;
enum dma_data_direction dma_direction;
+   u8 *dma_buf;
 };
 
 struct sh_mobile_dt_config {
@@ -548,6 +549,8 @@ static void sh_mobile_i2c_dma_callback(void *data)
pd->pos = pd->msg->len;
pd->stop_after_dma = true;
 
+   i2c_release_dma_safe_msg_buf(pd->msg, pd->dma_buf);
+
iic_set_clr(pd, ICIC, 0, ICIC_TDMAE | ICIC_RDMAE);
 }
 
@@ -608,7 +611,7 @@ static void sh_mobile_i2c_xfer_dma(struct 
sh_mobile_i2c_data *pd)
if (IS_ERR(chan))
return;
 
-   dma_addr = dma_map_single(chan->device->dev, pd->msg->buf, 
pd->msg->len, dir);
+   dma_addr = dma_map_single(chan->device->dev, pd->dma_buf, pd->msg->len, 
dir);
if (dma_mapping_error(chan->device->dev, dma_addr)) {
dev_dbg(pd->dev, "dma map failed, using PIO\n");
return;
@@ -665,7 +668,8 @@ static int start_ch(struct sh_mobile_i2c_data *pd, struct 
i2c_msg *usr_msg,
pd->pos = -1;
pd->sr = 0;
 
-   if (pd->msg->len > 8)
+   pd->dma_buf = i2c_get_dma_safe_msg_buf(pd->msg, 8);
+   if (pd->dma_buf)
sh_mobile_i2c_xfer_dma(pd);
 
/* Enable all interrupts to begin with */
-- 
2.11.0

[RFC PATCH v5 3/6] i2c: add docs to clarify DMA handling

[Wolfram Sang]

Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>
---
 Documentation/i2c/DMA-considerations | 58 
 1 file changed, 58 insertions(+)
 create mode 100644 Documentation/i2c/DMA-considerations

diff --git a/Documentation/i2c/DMA-considerations 
b/Documentation/i2c/DMA-considerations
new file mode 100644
index 00..5a63355c6a9b6f
--- /dev/null
+++ b/Documentation/i2c/DMA-considerations
@@ -0,0 +1,58 @@
+=
+Linux I2C and DMA
+=
+
+Given that I2C is a low-speed bus where largely small messages are transferred,
+it is not considered a prime user of DMA access. At this time of writing, only
+10% of I2C bus master drivers have DMA support implemented. And the vast
+majority of transactions are so small that setting up DMA for it will likely
+add more overhead than a plain PIO transfer.
+
+Therefore, it is *not* mandatory that the buffer of an I2C message is DMA safe.
+It does not seem reasonable to apply additional burdens when the feature is so
+rarely used. However, it is recommended to use a DMA-safe buffer if your
+message size is likely applicable for DMA. Most drivers have this threshold
+around 8 bytes (as of today, this is mostly an educated guess, however). For
+any message of 16 byte or larger, it is probably a really good idea. Please
+note that other subsystems you use might add requirements. E.g., if your
+I2C bus master driver is using USB as a bridge, then you need to have DMA
+safe buffers always, because USB requires it.
+
+For clients, if you use a DMA safe buffer in i2c_msg, set the I2C_M_DMA_SAFE
+flag with it. Then, the I2C core and drivers know they can safely operate DMA
+on it. Note that using this flag is optional. I2C host drivers which are not
+updated to use this flag will work like before. And like before, they risk
+using an unsafe DMA buffer. To improve this situation, using I2C_M_DMA_SAFE in
+more and more clients and host drivers is the planned way forward. Note also
+that setting this flag makes only sense in kernel space. User space data is
+copied into kernel space anyhow. The I2C core makes sure the destination
+buffers in kernel space are always DMA capable.
+
+FIXME: Need to implement i2c_master_{send|receive}_dma and proper buffers for 
i2c_smbus_xfer_emulated.
+
+Drivers wishing to implement safe DMA can use helper functions from the I2C
+core. One gives you a DMA-safe buffer for a given i2c_msg as long as a certain
+threshold is met::
+
+   dma_buf = i2c_get_dma_safe_msg_buf(msg, threshold_in_byte);
+
+If a buffer is returned, it is either msg->buf for the I2C_M_DMA_SAFE case or a
+bounce buffer. But you don't need to care about that detail, just use the
+returned buffer. If NULL is returned, the threshold was not met or a bounce
+buffer could not be allocated. Fall back to PIO in that case.
+
+In any case, a buffer obtained from above needs to be released. It ensures data
+is copied back to the message and a potentially used bounce buffer is freed::
+
+   i2c_release_dma_safe_msg_buf(msg, dma_buf);
+
+The bounce buffer handling from the core is generic and simple. It will always
+allocate a new bounce buffer. If you want a more sophisticated handling (e.g.
+reusing pre-allocated buffers), you are free to implement your own.
+
+Please also check the in-kernel documentation for details. The i2c-sh_mobile
+driver can be used as a reference example how to use the above helpers.
+
+Final note: If you plan to use DMA with I2C (or with anything else, actually)
+make sure you have CONFIG_DMA_API_DEBUG enabled during development. It can help
+you find various issues which can be complex to debug otherwise.
-- 
2.11.0

[RFC PATCH v5 1/6] i2c: add a message flag for DMA safe buffers

[Wolfram Sang]

I2C has no requirement that the buffer of a message needs to be DMA
safe. In case it is, it can now be flagged, so drivers wishing to
do DMA can use the buffer directly.

Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>
---
 include/uapi/linux/i2c.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/uapi/linux/i2c.h b/include/uapi/linux/i2c.h
index 009e27bb9abe19..1c683cb319e4b7 100644
--- a/include/uapi/linux/i2c.h
+++ b/include/uapi/linux/i2c.h
@@ -71,6 +71,9 @@ struct i2c_msg {
 #define I2C_M_RD   0x0001  /* read data, from slave to master */
/* I2C_M_RD is guaranteed to be 0x0001! 
*/
 #define I2C_M_TEN  0x0010  /* this is a ten bit chip address */
+#define I2C_M_DMA_SAFE 0x0200  /* the buffer of this message is DMA 
safe */
+   /* makes only sense in kernelspace */
+   /* userspace buffers are copied anyway 
*/
 #define I2C_M_RECV_LEN 0x0400  /* length will be first received byte */
 #define I2C_M_NO_RD_ACK0x0800  /* if 
I2C_FUNC_PROTOCOL_MANGLING */
 #define I2C_M_IGNORE_NAK   0x1000  /* if I2C_FUNC_PROTOCOL_MANGLING */
-- 
2.11.0

[RFC PATCH v5 5/6] i2c: rcar: skip DMA if buffer is not safe

[Wolfram Sang]

This HW is prone to races, so it needs to setup new messages in irq
context. That means we can't alloc bounce buffers if a message buffer is
not DMA safe. So, in that case, simply fall back to PIO.

Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>
---
 drivers/i2c/busses/i2c-rcar.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 15d764afec3b29..8a2ae3e6c561c4 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -359,7 +359,7 @@ static void rcar_i2c_dma(struct rcar_i2c_priv *priv)
int len;
 
/* Do not use DMA if it's not available or for messages < 8 bytes */
-   if (IS_ERR(chan) || msg->len < 8)
+   if (IS_ERR(chan) || msg->len < 8 || !(msg->flags & I2C_M_DMA_SAFE))
return;
 
if (read) {
-- 
2.11.0

[RFC PATCH v5 6/6] i2c: dev: mark RDWR buffers as DMA_SAFE

[Wolfram Sang]

Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>
---
 drivers/i2c/i2c-dev.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c
index 6f638bbc922db4..bbc7aadb4c899d 100644
--- a/drivers/i2c/i2c-dev.c
+++ b/drivers/i2c/i2c-dev.c
@@ -280,6 +280,8 @@ static noinline int i2cdev_ioctl_rdwr(struct i2c_client 
*client,
res = PTR_ERR(rdwr_pa[i].buf);
break;
}
+   /* memdup_user allocates with GFP_KERNEL, so DMA is ok */
+   rdwr_pa[i].flags |= I2C_M_DMA_SAFE;
 
/*
 * If the message length is received from the slave (similar
-- 
2.11.0

Re: [RFC PATCH v4 3/6] i2c: add docs to clarify DMA handling

[Wolfram Sang]

> In order to avoid that, and to place them into a box using monotonic fonts,
> I usually add "::" at the preceding line, e. g.:

Just in time: I added the '::' and will resubmit the new version in a
minute.

Thanks for the pointers!



signature.asc
Description: PGP signature

Re: [RFC PATCH v4 3/6] i2c: add docs to clarify DMA handling

[Wolfram Sang]

Hi Mauro,

> > +Linux I2C and DMA
> > +-
> 
> I would use, instead:
> 
> =
> Linux I2C and DMA
> =
> 
> As this is the way we're starting document titles, after converted to
> ReST. So, better to have it already using the right format, as one day

I did this.

> There are also a couple of things here that Sphinx would complain.

The only complaint I got was

WARNING: document isn't included in any toctree

which makes sense because I renamed it only temporarily to *.rst

> So, it could be worth to rename it to *.rst, while you're writing
> it, and see what:
>   make htmldocs
> will complain and how it will look in html.

So, no complaints from Sphinx and the HTML output looks good IMO. Was
there anything specific you had in mind when saying that Sphinx would
complain?

Regards,

   Wolfram



signature.asc
Description: PGP signature

Re: [RFC PATCH v4 3/6] i2c: add docs to clarify DMA handling

[Wolfram Sang]

> Yes, but the statistics that 10% of I2C bus master drivers
> are DMA-safe is not true, if you take those into account ;-)
> 
> Perhaps you could write it as (or something similar):
> 
>   At this time of writing, only +10% of I2C bus master
>   drivers for non-remote boards have DMA support implemented.  

No, this is confusing IMO. Being remote has nothing to with DMA. What
has to do with DMA is that these are using USB as communication channel.
And USB clearly needs DMA-safe buffers. The encapsulation needs DMA-safe
buffers. So, I think the new sentence "other subsystems might impose"
mentions that.

Let me recap what this series is for:

a) It makes clear that I2C subsystem does not require DMA-safety because
for the reasons given in the textfile.

If I2C is encapsulated over USB, then DMA-safety is of course required,
but this comes from USB. So, those USB-I2C master drivers need to do
something to make sure DMA-safety is guaranteed because i2c_msg buffers
don't need to be DMA safe because of a). They could use the newly
introduced features, but they don't have to.

b) a flag for DMA safe i2c_msgs is added

So, for messages we know to be DMA safe, we can flag that. Drivers
could check that and use a bounce buffer if the flag is not set.
However, this is all optional. Your drivers can still choose to not
check the flag and everything will stay as before. Check patch 5 for a
use case.

c) helper functions for bounce buffers are added

These are *helper* functions for drivers wishing to do DMA. A super
simple bounce buffer support. Check patch 4 for a use case. Again, this
is optional. Drivers can implement their own bounce buffer support. Or,
as in your case, if you know that your buffers are good, then don't use
any of this and things will keep going.


This all is to allow bus master drivers to opt-in for DMA safety if they
want to do DMA directly. Because currently, with a lot of i2c_msgs on
stack, this is more or less working accidently.

And, yes, I know I have to add this new flag to a few central places in
client drivers. Otherwise, master drivers checking for DMA safety will
initially have a performance regression. This is scheduled for V5 and
mentioned in this series.

> In the past, on lots of drivers, the i2c_xfer logic just used to assume
> that the I2C client driver allocated a DMA-safe buffer, as it just used to
> pass whatever buffer it receives directly to USB core. We did an effort to
> change it, as it can be messy, but I'm not sure if this is solved everywhere.

Good, I can imagine this being some effort. But again, this is because
USB needs the DMA-safety, not I2C. AFAICS, most i2c_msgs are register
accesses and thus, small messages.

> The usage of I2C at the media subsystem predates the I2C subsystem.
> at V4L2 drivers, a great effort was done to port it to fully use the
> I2C subsystem when it was added upstream, but there were some problems
> a the initial implementation of the i2c subsystem that prevented its
> usage for the DVB drivers. By the time such restrictions got removed,
> it was too late, as there were already a large amount of drivers relying
> on i2c "low level" functions.

Didn't know that, but this is good to know! Thanks.

> Even on the drivers that use i2c_add_adapter(), the usage of DMA can't
> be get by the above grep, as the DMA usage is actually at drivers/usb.

Ok. But as said before, what works now will continue to work. This
series is about clarifying that i2c_msg buffers need not to be DMA safe
and offering some helpers for those bus master drivers wanting to opt-in
to be sure.

Clearer now?

Regards,

   Wolfram



signature.asc
Description: PGP signature

Re: [RFC PATCH v4 3/6] i2c: add docs to clarify DMA handling

[Wolfram Sang]

Hi Mauro,

thanks for your comments. Much appreciated!

> There are also a couple of things here that Sphinx would complain.
> So, it could be worth to rename it to *.rst, while you're writing
> it, and see what:
>   make htmldocs
> will complain and how it will look in html.

OK, I'll check that.

> > +Given that I2C is a low-speed bus where largely small messages are 
> > transferred,
> > +it is not considered a prime user of DMA access. At this time of writing, 
> > only
> > +10% of I2C bus master drivers have DMA support implemented.
> 
> Are you sure about that? I'd say that, on media, at least half of the
> drivers use DMA for I2C bus access, as the I2C bus is on a remote
> board that talks with CPU via USB, using DMA, and all communication
> with USB should be DMA-safe.

Well, the DMA-safe requirement comes then from the USB subsystem,
doesn't it? Or do you really use DMA on the remote board to transfer
data via I2C to an I2C client?

> I guess what you really wanted to say on most of this section is
> about SoC (and other CPUs) where the I2C bus master is is at the
> mainboard, and not on some peripheral.

I might be biased to that, yes. So, it is good talking about it.

> > And the vast
> > +majority of transactions are so small that setting up DMA for it will 
> > likely
> > +add more overhead than a plain PIO transfer.
> > +
> > +Therefore, it is *not* mandatory that the buffer of an I2C message is DMA 
> > safe.
> 
> Again, that may not be true on media boards. The code that implements the
> I2C transfers there, on most boards, have to be DMA safe, as it won't
> otherwise send/receive commands from the chips that are after the USB
> bridge.

That still sounds to me like the DMA-safe requirement comes from USB
(which is fine, of course.). In any case, a sentence like "Other
subsystem you might use for bridging I2C might impose other DMA
requirements" sounds like to nice to have.

> > +Drivers wishing to implement DMA can use helper functions from the I2C 
> > core.
> > +One gives you a DMA-safe buffer for a given i2c_msg as long as a certain
> > +threshold is met.
> > +
> > +   dma_buf = i2c_get_dma_safe_msg_buf(msg, threshold_in_byte);
> 
> I'm concerned about the new bits added by this call. Right now,
> USB drivers just use kalloc() for transfer buffers used to send and
> receive URB control messages for both setting the main device and
> for I2C messages. Before this changeset, buffers allocated this
> way are DMA save and have been working for years.

Can you give me a pointer to a driver doing this? I glimpsed around in
drivers/media/usb and found that most drivers are having their i2c_msg
buffers on the stack. Which is clearly not DMA-safe.

> When you add some flags that would make the I2C subsystem aware
> that a buffer is now DMA safe, I guess you could be breaking
> those drivers, as a DMA safe buffer will now be considered as
> DMA-unsafe.

Well, this flag is only relevant for i2c master drivers wishing to do
DMA. So, grepping in the above directory

grep dma $(grep -rl i2c_add_adapter *)

only gives one driver which is irrelevant because the i2c master it
registers is not doing any DMA?

Am I missing something? We are clearly not aligned yet...

Regards,

   Wolfram



signature.asc
Description: PGP signature

Re: [PATCH 2/4] [media] media: pci: make i2c_adapter const

[Wolfram Sang]

On Sat, Aug 19, 2017 at 04:04:13PM +0530, Bhumika Goyal wrote:
> Make these const as they are only used in a copy operation.
> Done using Coccinelle
> 
> Signed-off-by: Bhumika Goyal <bhumi...@gmail.com>

Acked-by: Wolfram Sang <w...@the-dreams.de>



signature.asc
Description: PGP signature

Re: [PATCH 3/4] [media] radio-usb-si4713: make i2c_adapter const

[Wolfram Sang]

On Sat, Aug 19, 2017 at 04:04:14PM +0530, Bhumika Goyal wrote:
> Make this const as it is only used in a copy operation.
> Done using Coccinelle
> 
> Signed-off-by: Bhumika Goyal <bhumi...@gmail.com>

Acked-by: Wolfram Sang <w...@the-dreams.de>



signature.asc
Description: PGP signature

Re: [PATCH 4/4] [media] usb: make i2c_adapter const

[Wolfram Sang]

On Sat, Aug 19, 2017 at 04:04:15PM +0530, Bhumika Goyal wrote:
> Make these const as they are only used in a copy operation.
> Done using Coccinelle
> 
> Signed-off-by: Bhumika Goyal <bhumi...@gmail.com>

Acked-by: Wolfram Sang <w...@the-dreams.de>



signature.asc
Description: PGP signature

Re: [PATCH 1/4] i2c: busses: make i2c_adapter const

[Wolfram Sang]

On Sat, Aug 19, 2017 at 04:04:12PM +0530, Bhumika Goyal wrote:
> Make these const as they are only used in a copy operation.
> Done using Coccinelle.
> 
> Signed-off-by: Bhumika Goyal 

Applied to for-next, thanks!



signature.asc
Description: PGP signature

Re: [PATCH 1/2] i2c: busses: make i2c_algorithm const

[Wolfram Sang]

On Fri, Aug 18, 2017 at 09:36:57PM +0530, Bhumika Goyal wrote:
> Make these const as they are only stored in the algo field of
> i2c_adapter structure, which is const.
> 
> Signed-off-by: Bhumika Goyal 

Applied to for-next, thanks!



signature.asc
Description: PGP signature

[RFC PATCH v4 0/6] i2c: document DMA handling and add helpers for it

[Wolfram Sang]

So, after revisiting old mail threads, taking part in a similar discussion on
the USB list, and implementing a not-convincing solution before, here is what I
cooked up to document and ease DMA handling for I2C within Linux. Please have a
look at the documentation introduced in patch 3 for details.

While the previous versions tried to magically apply bounce buffers when
needed, it became clear that detecting DMA safe buffers is too fragile. This
approach is now opt-in, a DMA_SAFE flag needs to be set on an i2c_msg. The
outcome so far is very convincing IMO. The core additions are simple and easy
to understand (makes me even think of inlining them again?). The driver changes
for the Renesas IP cores became easier to understand, too. While only a tad for
the i2c-sh_mobile driver, the situation became a LOT better for the i2c-rcar
driver. No more DMA disabling for the whole transfer in case of unsafe buffers,
we are back to per-msg handling. And the code fix is now an easy to understand
one line change. Yay!

Of course, we must now whitelist DMA safe buffers. An example for I2C_RDWR case
is in this series. It makes the i2ctransfer utility have DMA_SAFE buffers,
which is nice for testing as i2cdump will (currently) not use DMA_SAFE buffers.
My plan is to add two new calls: i2c_master_{send|receive}_dma_safe which can
be used if DMA_SAFE buffers are provided. So, drivers can simply switch to
them. Also, the buffers used within i2c_smbus_xfer_emulated() need to be
converted to be DMA_SAFE which will cover a huge bunch of use cases. The rest
is then updating drivers which can be done when needed.

As these conversions are not done yet, this patch series has RFC status. But I
already would like to get opinions on this approach, so I'll cc mailing lists
of the heavier I2C users. Please let me know what you think.

All patches have been tested with a Renesas Salvator-X board (r8a7796/M3-W).

The branch can be found here:

git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git 
renesas/topic/i2c-core-dma-rfc-v4

And big kudos to Renesas Electronics for funding this work, thank you very much!

Regards,

   Wolfram

Changes since v3:
* completely redesigned

Wolfram Sang (6):
  i2c: add a message flag for DMA safe buffers
  i2c: add helpers to ease DMA handling
  i2c: add docs to clarify DMA handling
  i2c: sh_mobile: use helper to decide if DMA is useful
  i2c: rcar: skip DMA if buffer is not safe
  i2c: dev: mark RDWR buffers as DMA_SAFE

 Documentation/i2c/DMA-considerations | 50 
 drivers/i2c/busses/i2c-rcar.c|  2 +-
 drivers/i2c/busses/i2c-sh_mobile.c   |  8 --
 drivers/i2c/i2c-core-base.c  | 45 
 drivers/i2c/i2c-dev.c|  2 ++
 include/linux/i2c.h  |  3 +++
 include/uapi/linux/i2c.h |  3 +++
 7 files changed, 110 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/i2c/DMA-considerations

-- 
2.11.0

[RFC PATCH v4 4/6] i2c: sh_mobile: use helper to decide if DMA is useful

[Wolfram Sang]

This ensures that we fall back to PIO if the message length is too small
for DMA being useful. Otherwise, we use DMA. A bounce buffer might be
applied by the helper if the original message buffer is not DMA safe.

Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>
---
 drivers/i2c/busses/i2c-sh_mobile.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-sh_mobile.c 
b/drivers/i2c/busses/i2c-sh_mobile.c
index 2e097d97d258bc..5efdb7becd83d6 100644
--- a/drivers/i2c/busses/i2c-sh_mobile.c
+++ b/drivers/i2c/busses/i2c-sh_mobile.c
@@ -145,6 +145,7 @@ struct sh_mobile_i2c_data {
struct dma_chan *dma_rx;
struct scatterlist sg;
enum dma_data_direction dma_direction;
+   u8 *dma_buf;
 };
 
 struct sh_mobile_dt_config {
@@ -548,6 +549,8 @@ static void sh_mobile_i2c_dma_callback(void *data)
pd->pos = pd->msg->len;
pd->stop_after_dma = true;
 
+   i2c_release_dma_safe_msg_buf(pd->msg, pd->dma_buf);
+
iic_set_clr(pd, ICIC, 0, ICIC_TDMAE | ICIC_RDMAE);
 }
 
@@ -608,7 +611,7 @@ static void sh_mobile_i2c_xfer_dma(struct 
sh_mobile_i2c_data *pd)
if (IS_ERR(chan))
return;
 
-   dma_addr = dma_map_single(chan->device->dev, pd->msg->buf, 
pd->msg->len, dir);
+   dma_addr = dma_map_single(chan->device->dev, pd->dma_buf, pd->msg->len, 
dir);
if (dma_mapping_error(chan->device->dev, dma_addr)) {
dev_dbg(pd->dev, "dma map failed, using PIO\n");
return;
@@ -665,7 +668,8 @@ static int start_ch(struct sh_mobile_i2c_data *pd, struct 
i2c_msg *usr_msg,
pd->pos = -1;
pd->sr = 0;
 
-   if (pd->msg->len > 8)
+   pd->dma_buf = i2c_get_dma_safe_msg_buf(pd->msg, 8);
+   if (pd->dma_buf)
sh_mobile_i2c_xfer_dma(pd);
 
/* Enable all interrupts to begin with */
-- 
2.11.0

[RFC PATCH v4 6/6] i2c: dev: mark RDWR buffers as DMA_SAFE

[Wolfram Sang]

Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>
---
 drivers/i2c/i2c-dev.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c
index 6f638bbc922db4..bbc7aadb4c899d 100644
--- a/drivers/i2c/i2c-dev.c
+++ b/drivers/i2c/i2c-dev.c
@@ -280,6 +280,8 @@ static noinline int i2cdev_ioctl_rdwr(struct i2c_client 
*client,
res = PTR_ERR(rdwr_pa[i].buf);
break;
}
+   /* memdup_user allocates with GFP_KERNEL, so DMA is ok */
+   rdwr_pa[i].flags |= I2C_M_DMA_SAFE;
 
/*
 * If the message length is received from the slave (similar
-- 
2.11.0

[RFC PATCH v4 5/6] i2c: rcar: skip DMA if buffer is not safe

[Wolfram Sang]

This HW is prone to races, so it needs to setup new messages in irq
context. That means we can't alloc bounce buffers if a message buffer is
not DMA safe. So, in that case, simply fall back to PIO.

Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>
---
 drivers/i2c/busses/i2c-rcar.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 93c1a54981df08..5654a7142bffec 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -359,7 +359,7 @@ static void rcar_i2c_dma(struct rcar_i2c_priv *priv)
int len;
 
/* Do not use DMA if it's not available or for messages < 8 bytes */
-   if (IS_ERR(chan) || msg->len < 8)
+   if (IS_ERR(chan) || msg->len < 8 || !(msg->flags & I2C_M_DMA_SAFE))
return;
 
if (read) {
-- 
2.11.0

[RFC PATCH v4 1/6] i2c: add a message flag for DMA safe buffers

[Wolfram Sang]

I2C has no requirement that the buffer of a message needs to be DMA
safe. In case it is, it can now be flagged, so drivers wishing to
do DMA can use the buffer directly.

Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>
---
 include/uapi/linux/i2c.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/uapi/linux/i2c.h b/include/uapi/linux/i2c.h
index 009e27bb9abe19..1c683cb319e4b7 100644
--- a/include/uapi/linux/i2c.h
+++ b/include/uapi/linux/i2c.h
@@ -71,6 +71,9 @@ struct i2c_msg {
 #define I2C_M_RD   0x0001  /* read data, from slave to master */
/* I2C_M_RD is guaranteed to be 0x0001! 
*/
 #define I2C_M_TEN  0x0010  /* this is a ten bit chip address */
+#define I2C_M_DMA_SAFE 0x0200  /* the buffer of this message is DMA 
safe */
+   /* makes only sense in kernelspace */
+   /* userspace buffers are copied anyway 
*/
 #define I2C_M_RECV_LEN 0x0400  /* length will be first received byte */
 #define I2C_M_NO_RD_ACK0x0800  /* if 
I2C_FUNC_PROTOCOL_MANGLING */
 #define I2C_M_IGNORE_NAK   0x1000  /* if I2C_FUNC_PROTOCOL_MANGLING */
-- 
2.11.0

[RFC PATCH v4 2/6] i2c: add helpers to ease DMA handling

[Wolfram Sang]

One helper checks if DMA is suitable and optionally creates a bounce
buffer, if not. The other function returns the bounce buffer and makes
sure the data is properly copied back to the message.

Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>
---
 drivers/i2c/i2c-core-base.c | 45 +
 include/linux/i2c.h |  3 +++
 2 files changed, 48 insertions(+)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 12822a4b8f8f09..a104ebc2d05af8 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -2241,6 +2241,51 @@ void i2c_put_adapter(struct i2c_adapter *adap)
 }
 EXPORT_SYMBOL(i2c_put_adapter);
 
+/**
+ * i2c_get_dma_safe_msg_buf() - get a DMA safe buffer for the given i2c_msg
+ * @msg: the message to be checked
+ * @threshold: the amount of byte from which using DMA makes sense
+ *
+ * Return: NULL if a DMA safe buffer was not obtained. Use msg->buf with PIO.
+ *
+ *Or a valid pointer to be used with DMA. Note that it can either be
+ *msg->buf or a bounce buffer. After use, release it by calling
+ *i2c_release_dma_safe_msg_buf().
+ *
+ * This function must only be called from process context!
+ */
+u8 *i2c_get_dma_safe_msg_buf(struct i2c_msg *msg, unsigned int threshold)
+{
+   if (msg->len < threshold)
+   return NULL;
+
+   if (msg->flags & I2C_M_DMA_SAFE)
+   return msg->buf;
+
+   if (msg->flags & I2C_M_RD)
+   return kzalloc(msg->len, GFP_KERNEL);
+   else
+   return kmemdup(msg->buf, msg->len, GFP_KERNEL);
+}
+EXPORT_SYMBOL_GPL(i2c_get_dma_safe_msg_buf);
+
+/**
+ * i2c_release_dma_safe_msg_buf - release DMA safe buffer and sync with i2c_msg
+ * @msg: the message to be synced with
+ * @buf: the buffer obtained from i2c_get_dma_safe_msg_buf(). May be NULL.
+ */
+void i2c_release_dma_safe_msg_buf(struct i2c_msg *msg, u8 *buf)
+{
+   if (!buf || buf == msg->buf)
+   return;
+
+   if (msg->flags & I2C_M_RD)
+   memcpy(msg->buf, buf, msg->len);
+
+   kfree(buf);
+}
+EXPORT_SYMBOL_GPL(i2c_release_dma_safe_msg_buf);
+
 MODULE_AUTHOR("Simon G. Vogl <si...@tk.uni-linz.ac.at>");
 MODULE_DESCRIPTION("I2C-Bus main module");
 MODULE_LICENSE("GPL");
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index d501d3956f13f0..1e99342f180f45 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -767,6 +767,9 @@ static inline u8 i2c_8bit_addr_from_msg(const struct 
i2c_msg *msg)
return (msg->addr << 1) | (msg->flags & I2C_M_RD ? 1 : 0);
 }
 
+u8 *i2c_get_dma_safe_msg_buf(struct i2c_msg *msg, unsigned int threshold);
+void i2c_release_dma_safe_msg_buf(struct i2c_msg *msg, u8 *buf);
+
 int i2c_handle_smbus_host_notify(struct i2c_adapter *adap, unsigned short 
addr);
 /**
  * module_i2c_driver() - Helper macro for registering a modular I2C driver
-- 
2.11.0

[RFC PATCH v4 3/6] i2c: add docs to clarify DMA handling

[Wolfram Sang]

Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>
---
 Documentation/i2c/DMA-considerations | 50 
 1 file changed, 50 insertions(+)
 create mode 100644 Documentation/i2c/DMA-considerations

diff --git a/Documentation/i2c/DMA-considerations 
b/Documentation/i2c/DMA-considerations
new file mode 100644
index 00..a4b4a107102452
--- /dev/null
+++ b/Documentation/i2c/DMA-considerations
@@ -0,0 +1,50 @@
+Linux I2C and DMA
+-
+
+Given that I2C is a low-speed bus where largely small messages are transferred,
+it is not considered a prime user of DMA access. At this time of writing, only
+10% of I2C bus master drivers have DMA support implemented. And the vast
+majority of transactions are so small that setting up DMA for it will likely
+add more overhead than a plain PIO transfer.
+
+Therefore, it is *not* mandatory that the buffer of an I2C message is DMA safe.
+It does not seem reasonable to apply additional burdens when the feature is so
+rarely used. However, it is recommended to use a DMA-safe buffer if your
+message size is likely applicable for DMA. Most drivers have this threshold
+around 8 bytes (as of today, this is mostly an educated guess, however). For
+any message of 16 byte or larger, it is probably a really good idea.
+
+If you use such a buffer in a i2c_msg, set the I2C_M_DMA_SAFE flag with it.
+Then, the I2C core and drivers know they can safely operate DMA on it. Note
+that setting this flag makes only sense in kernel space. User space data is
+copied into kernel space anyhow. The I2C core makes sure the destination
+buffers in kernel space are always DMA capable.
+
+FIXME: Need to implement i2c_master_{send|receive}_dma and proper buffers for 
i2c_smbus_xfer_emulated.
+
+Drivers wishing to implement DMA can use helper functions from the I2C core.
+One gives you a DMA-safe buffer for a given i2c_msg as long as a certain
+threshold is met.
+
+   dma_buf = i2c_get_dma_safe_msg_buf(msg, threshold_in_byte);
+
+If a buffer is returned, it either msg->buf for the I2C_M_DMA_SAFE case or a
+bounce buffer. But you don't need to care about that detail. If NULL is
+returned, the threshold was not met or a bounce buffer could not be allocated.
+Fall back to PIO in that case.
+
+In any case, a buffer obtained from above needs to be released. It ensures data
+is copied back to the message and a potentially used bounce buffer is freed.
+
+   i2c_release_dma_safe_msg_buf(msg, dma_buf);
+
+The bounce buffer handling from the core is generic and simple. It will always
+allocate a new bounce buffer. If you want a more sophisticated handling (e.g.
+reusing pre-allocated buffers), you are free to implement your own.
+
+Please also check the in-kernel documentation for details. The i2c-sh_mobile
+driver can be used as a reference example how to use the above helpers.
+
+Final note: If you plan to use DMA with I2C (or with anything else, actually)
+make sure you have CONFIG_DMA_API_DEBUG enabled during development. It can help
+you find various issues which can be complex to debug otherwise.
-- 
2.11.0

Re: [PATCH v3 1/4] i2c: add helpers to ease DMA handling

[Wolfram Sang]

Hi Jonathan,

> I like the basic idea of this patch set a lot btw!

Thanks!

> Jonathan

Could you delete irrelevant parts of the message, please? I nearly
missed your other comment which would have been a great loss!

> I'm trying to get my head around whether this is a sufficient set of 
> conditions
> for a dma safe buffer and I'm not sure it is.
> 
> We go to a lot of effort with SPI buffers to ensure they are in their own 
> cache
> lines.  Do we not need to do the same here?  The issue would be the
> classic case of embedding a buffer inside a larger structure which is on
> the stack.  Need the magic of __cacheline_aligned to force it into it's
> own line for devices with dma-incoherent caches.
> DMA-API-HOWTO.txt (not read that for a while at it is a rather good
> at describing this stuff these days!)
> 
> So that one is easy enough to check by checking if it is cache line aligned,
> but what do you do to know there is nothing much after it?

Thank you, really!

This patch series addressed all cases I have created, but I also
wondered if there are cases left which I missed. Now, also because you
mentioned cacheline alignments, the faint idea of "maybe it could be
fragile" became a strong "it is too fragile"! lib/dma-debug.c is >40KB
for a reason. I just rewrote this series...

> Not sure there is a way around this short of auditing i2c drivers to
> change any that do this.

... to do something like this, more precisely: an opt-in approach. I
introduce a new flag for i2c_msg, namely I2C_M_DMA_SAFE which can opt-in
DMA if we know the i2c_msg buffer is DMA safe. I finished a
proof-of-concept this evening and pushed it here:

git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git 
renesas/topic/i2c-core-dma-rfc-v4

I will test it on HW tomorrow and send it out, then. There are still
some bits missing, but for getting opinions, it should be good enough.

Thanks and kind regards,

   Wolfram



signature.asc
Description: PGP signature

Re: [PATCH v3 1/4] i2c: add helpers to ease DMA handling

[Wolfram Sang]

> Right:
> 
> drivers/i2c/i2c-core-base.c:2310:15: error: 'i2c_release_bounce_buf'
> undeclared here (not in a function)
>  EXPORT_SYMBOL_GPL(i2c_release_bounce_buf);

Thanks. I am just now working on V4 currently which is a redesign.
I'll write more in an hour or so.



signature.asc
Description: PGP signature

[PATCH v3 3/4] i2c: sh_mobile: use helper to decide if DMA is useful

[Wolfram Sang]

This ensures that we fall back to PIO if the buffer is too small for DMA
being useful. Otherwise, we use DMA. A bounce buffer might be applied if
the original message buffer is not DMA safe

Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>
---
 drivers/i2c/busses/i2c-sh_mobile.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-sh_mobile.c 
b/drivers/i2c/busses/i2c-sh_mobile.c
index 2e097d97d258bc..19f45bcd9b35ca 100644
--- a/drivers/i2c/busses/i2c-sh_mobile.c
+++ b/drivers/i2c/busses/i2c-sh_mobile.c
@@ -145,6 +145,7 @@ struct sh_mobile_i2c_data {
struct dma_chan *dma_rx;
struct scatterlist sg;
enum dma_data_direction dma_direction;
+   u8 *bounce_buf;
 };
 
 struct sh_mobile_dt_config {
@@ -548,6 +549,8 @@ static void sh_mobile_i2c_dma_callback(void *data)
pd->pos = pd->msg->len;
pd->stop_after_dma = true;
 
+   i2c_release_dma_bounce_buf(pd->msg, pd->bounce_buf);
+
iic_set_clr(pd, ICIC, 0, ICIC_TDMAE | ICIC_RDMAE);
 }
 
@@ -595,6 +598,7 @@ static void sh_mobile_i2c_xfer_dma(struct 
sh_mobile_i2c_data *pd)
struct dma_async_tx_descriptor *txdesc;
dma_addr_t dma_addr;
dma_cookie_t cookie;
+   u8 *dma_buf = pd->bounce_buf ?: pd->msg->buf;
 
if (PTR_ERR(chan) == -EPROBE_DEFER) {
if (read)
@@ -608,7 +612,7 @@ static void sh_mobile_i2c_xfer_dma(struct 
sh_mobile_i2c_data *pd)
if (IS_ERR(chan))
return;
 
-   dma_addr = dma_map_single(chan->device->dev, pd->msg->buf, 
pd->msg->len, dir);
+   dma_addr = dma_map_single(chan->device->dev, dma_buf, pd->msg->len, 
dir);
if (dma_mapping_error(chan->device->dev, dma_addr)) {
dev_dbg(pd->dev, "dma map failed, using PIO\n");
return;
@@ -665,7 +669,7 @@ static int start_ch(struct sh_mobile_i2c_data *pd, struct 
i2c_msg *usr_msg,
pd->pos = -1;
pd->sr = 0;
 
-   if (pd->msg->len > 8)
+   if (i2c_check_msg_for_dma(pd->msg, 8, >bounce_buf) == 0)
sh_mobile_i2c_xfer_dma(pd);
 
/* Enable all interrupts to begin with */
-- 
2.11.0

[PATCH v3 1/4] i2c: add helpers to ease DMA handling

[Wolfram Sang]

One helper checks if DMA is suitable and optionally creates a bounce
buffer, if not. The other function returns the bounce buffer and makes
sure the data is properly copied back to the message.

Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>
---
Changes since v2:

* rebased to v4.13-rc1
* helper functions are not inlined anymore but moved to i2c core
* __must_check has been added to the buffer check helper
* the release function has been renamed to contain 'dma' as well

 drivers/i2c/i2c-core-base.c | 68 +
 include/linux/i2c.h |  5 
 2 files changed, 73 insertions(+)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index c89dac7fd2e7b7..7326a9d2e4eb69 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -34,6 +34,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -44,6 +45,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 #include "i2c-core.h"
@@ -2240,6 +2242,72 @@ void i2c_put_adapter(struct i2c_adapter *adap)
 }
 EXPORT_SYMBOL(i2c_put_adapter);
 
+/**
+ * i2c_check_msg_for_dma() - check if a message is suitable for DMA
+ * @msg: the message to be checked
+ * @threshold: the amount of byte from which using DMA makes sense
+ * @ptr_for_bounce_buf: if not NULL, a bounce buffer will be attached to this
+ * ptr, if needed. The bounce buffer must be freed by the
+ * caller using i2c_release_dma_bounce_buf().
+ *
+ * Return: -ERANGE if message is smaller than threshold
+ *-EFAULT if message buffer is not DMA capable and no bounce buffer
+ *was requested
+ *-ENOMEM if a bounce buffer could not be created
+ *0 if message is suitable for DMA
+ *
+ * The return value must be checked.
+ *
+ * Note: This function should only be called from process context! It uses
+ * helper functions which work on the 'current' task.
+ */
+int i2c_check_msg_for_dma(struct i2c_msg *msg, unsigned int threshold,
+   u8 **ptr_for_bounce_buf)
+{
+   if (ptr_for_bounce_buf)
+   *ptr_for_bounce_buf = NULL;
+
+   if (msg->len < threshold)
+   return -ERANGE;
+
+   if (!virt_addr_valid(msg->buf) || object_is_on_stack(msg->buf)) {
+   pr_debug("msg buffer to 0x%04x is not DMA safe%s\n", msg->addr,
+ptr_for_bounce_buf ? ", trying bounce buffer" : "");
+   if (ptr_for_bounce_buf) {
+   if (msg->flags & I2C_M_RD)
+   *ptr_for_bounce_buf = kzalloc(msg->len, 
GFP_KERNEL);
+   else
+   *ptr_for_bounce_buf = kmemdup(msg->buf, 
msg->len,
+ GFP_KERNEL);
+   if (!*ptr_for_bounce_buf)
+   return -ENOMEM;
+   } else {
+   return -EFAULT;
+   }
+   }
+
+   return 0;
+}
+EXPORT_SYMBOL_GPL(i2c_check_msg_for_dma);
+
+/**
+ * i2c_release_bounce_buf - copy data back from bounce buffer and release it
+ * @msg: the message to be copied back to
+ * @bounce_buf: the bounce buffer obtained from i2c_check_msg_for_dma().
+ * May be NULL.
+ */
+void i2c_release_dma_bounce_buf(struct i2c_msg *msg, u8 *bounce_buf)
+{
+   if (!bounce_buf)
+   return;
+
+   if (msg->flags & I2C_M_RD)
+   memcpy(msg->buf, bounce_buf, msg->len);
+
+   kfree(bounce_buf);
+}
+EXPORT_SYMBOL_GPL(i2c_release_bounce_buf);
+
 MODULE_AUTHOR("Simon G. Vogl <si...@tk.uni-linz.ac.at>");
 MODULE_DESCRIPTION("I2C-Bus main module");
 MODULE_LICENSE("GPL");
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 00ca5b86a753f8..ac02287b6c0d8f 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -766,6 +766,11 @@ static inline u8 i2c_8bit_addr_from_msg(const struct 
i2c_msg *msg)
return (msg->addr << 1) | (msg->flags & I2C_M_RD ? 1 : 0);
 }
 
+int __must_check i2c_check_msg_for_dma(struct i2c_msg *msg, unsigned int 
threshold,
+   u8 **ptr_for_bounce_buf);
+
+void i2c_release_dma_bounce_buf(struct i2c_msg *msg, u8 *bounce_buf);
+
 int i2c_handle_smbus_host_notify(struct i2c_adapter *adap, unsigned short 
addr);
 /**
  * module_i2c_driver() - Helper macro for registering a modular I2C driver
-- 
2.11.0

[PATCH v3 4/4] i2c: rcar: check for DMA-capable buffers

[Wolfram Sang]

Handling this is special for this driver. Because the hardware needs to
initialize the next message in interrupt context, we cannot use the
i2c_check_msg_for_dma() directly. This helper only works reliably in
process context. So, we need to check during initial preparation of the
whole transfer and need to disable DMA completely for the whole transfer
once a message with a not-DMA-capable buffer is found.

Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>
---
 drivers/i2c/busses/i2c-rcar.c | 18 +-
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 93c1a54981df08..5d0e820d708853 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -111,8 +111,11 @@
 #define ID_ARBLOST (1 << 3)
 #define ID_NACK(1 << 4)
 /* persistent flags */
+#define ID_P_NODMA (1 << 30)
 #define ID_P_PM_BLOCKED(1 << 31)
-#define ID_P_MASK  ID_P_PM_BLOCKED
+#define ID_P_MASK  (ID_P_PM_BLOCKED | ID_P_NODMA)
+
+#define RCAR_DMA_THRESHOLD 8
 
 enum rcar_i2c_type {
I2C_RCAR_GEN1,
@@ -358,8 +361,7 @@ static void rcar_i2c_dma(struct rcar_i2c_priv *priv)
unsigned char *buf;
int len;
 
-   /* Do not use DMA if it's not available or for messages < 8 bytes */
-   if (IS_ERR(chan) || msg->len < 8)
+   if (IS_ERR(chan) || msg->len < RCAR_DMA_THRESHOLD || priv->flags & 
ID_P_NODMA)
return;
 
if (read) {
@@ -657,11 +659,15 @@ static void rcar_i2c_request_dma(struct rcar_i2c_priv 
*priv,
 struct i2c_msg *msg)
 {
struct device *dev = rcar_i2c_priv_to_dev(priv);
-   bool read;
+   bool read = msg->flags & I2C_M_RD;
struct dma_chan *chan;
enum dma_transfer_direction dir;
 
-   read = msg->flags & I2C_M_RD;
+   /* we need to check here because we need the 'current' context */
+   if (i2c_check_msg_for_dma(msg, RCAR_DMA_THRESHOLD, NULL) == -EFAULT) {
+   dev_dbg(dev, "skipping DMA for this whole transfer\n");
+   priv->flags |= ID_P_NODMA;
+   }
 
chan = read ? priv->dma_rx : priv->dma_tx;
if (PTR_ERR(chan) != -EPROBE_DEFER)
@@ -740,6 +746,8 @@ static int rcar_i2c_master_xfer(struct i2c_adapter *adap,
if (ret < 0 && ret != -ENXIO)
dev_err(dev, "error %d : %x\n", ret, priv->flags);
 
+   priv->flags &= ~ID_P_NODMA;
+
return ret;
 }
 
-- 
2.11.0

[PATCH v3 0/4] i2c: document DMA handling and add helpers for it

[Wolfram Sang]

So, after revisiting old mail threads and taking part in a similar discussion
on the USB list, here is what I cooked up to document and ease DMA handling for
I2C within Linux. Please have a look at the documentation introduced in patch 2
for further details.

All patches have been tested with a Renesas Salvator-X board (r8a7796/M3-W) and
a Renesas Lager board (r8a7790/H2). A more detailed test description can be
found here: http://elinux.org/Tests:I2C-core-DMA

The branch can be found here:

git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git 
renesas/topic/i2c-core-dma-v3

And big kudos to Renesas Electronics for funding this work, thank you very much!

Regards,

   Wolfram


Changes since v2:

* rebased to v4.13-rc1
* helper functions are not inlined anymore but moved to i2c core
* __must_check has been added to the buffer check helper
* the release function has been renamed to contain 'dma' as well
* documentation updates. Hopefully better wording now
* removed the doubled Signed-offs
* adding more potentially interested parties to CC


Wolfram Sang (4):
  i2c: add helpers to ease DMA handling
  i2c: add docs to clarify DMA handling
  i2c: sh_mobile: use helper to decide if DMA is useful
  i2c: rcar: check for DMA-capable buffers

 Documentation/i2c/DMA-considerations | 38 
 drivers/i2c/busses/i2c-rcar.c| 18 +++---
 drivers/i2c/busses/i2c-sh_mobile.c   |  8 +++--
 drivers/i2c/i2c-core-base.c  | 68 
 include/linux/i2c.h  |  5 +++
 5 files changed, 130 insertions(+), 7 deletions(-)
 create mode 100644 Documentation/i2c/DMA-considerations

-- 
2.11.0

[PATCH v3 2/4] i2c: add docs to clarify DMA handling

[Wolfram Sang]

Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>
---
Changes since v2:

* documentation updates. Hopefully better wording now

 Documentation/i2c/DMA-considerations | 38 
 1 file changed, 38 insertions(+)
 create mode 100644 Documentation/i2c/DMA-considerations

diff --git a/Documentation/i2c/DMA-considerations 
b/Documentation/i2c/DMA-considerations
new file mode 100644
index 00..e46c24d65c8556
--- /dev/null
+++ b/Documentation/i2c/DMA-considerations
@@ -0,0 +1,38 @@
+Linux I2C and DMA
+-
+
+Given that I2C is a low-speed bus where largely small messages are transferred,
+it is not considered a prime user of DMA access. At this time of writing, only
+10% of I2C bus master drivers have DMA support implemented. And the vast
+majority of transactions are so small that setting up DMA for it will likely
+add more overhead than a plain PIO transfer.
+
+Therefore, it is *not* mandatory that the buffer of an I2C message is DMA safe.
+It does not seem reasonable to apply additional burdens when the feature is so
+rarely used. However, it is recommended to use a DMA-safe buffer if your
+message size is likely applicable for DMA. Most drivers have this threshold
+around 8 bytes. As of today, this is mostly an educated guess, however.
+
+To support this scenario, drivers wishing to implement DMA can use helper
+functions from the I2C core. One checks if a message is DMA capable in terms of
+size and memory type. It can optionally also create a bounce buffer:
+
+   i2c_check_msg_for_dma(msg, threshold, _buf);
+
+The bounce buffer handling from the core is generic and simple. It will always
+allocate a new bounce buffer. If you want a more sophisticated handling (e.g.
+reusing pre-allocated buffers), you can leave the pointer to the bounce buffer
+empty and implement your own handling based on the return value of the above
+function.
+
+The other helper function releases the bounce buffer. It ensures data is copied
+back to the message:
+
+   i2c_release_dma_bounce_buf(msg, bounce_buf);
+
+Please check the in-kernel documentation for details. The i2c-sh_mobile driver
+can be used as a reference example.
+
+If you plan to use DMA with I2C (or with any other bus, actually) make sure you
+have CONFIG_DMA_API_DEBUG enabled during development. It can help you find
+various issues which can be complex to debug otherwise.
-- 
2.11.0

Re: [PATCH 6/7] [media] soc_camera: rcar_vin: use proper name for the R-Car SoC

[Wolfram Sang]

> Btw, I'm seeing only patches 6 and 7 here at media ML (and patchwork).
> As those are trivial changes, I'll just apply what I have.

Perfect, thanks!



signature.asc
Description: PGP signature

Re: [PATCH 6/7] [media] soc_camera: rcar_vin: use proper name for the R-Car SoC

[Wolfram Sang]

>Why "soc_camera:" in the subject?

I used 'git log $file' and copied the most recent entry. Do I need to
resend?



signature.asc
Description: PGP signature

[PATCH 0/7] tree-wide: use proper 'R-Car' product name

[Wolfram Sang]

Small series to get the R-Car product name proper. Based on
renesas-drivers/master, but can be applied to current linus/master as well.
Except for the MMC patch, which depends on mmc/next.

Please apply.

Wolfram Sang (7):
  dmaengine: use proper name for the R-Car SoC
  i2c: use proper name for the R-Car SoC
  iio: use proper name for the R-Car SoC
  mmc: use proper name for the R-Car SoC
  pci: use proper name for the R-Car SoC
  [media] soc_camera: rcar_vin: use proper name for the R-Car SoC
  [media] v4l: rcar_fdp1: use proper name for the R-Car SoC

 Documentation/devicetree/bindings/dma/shdma.txt   | 2 +-
 Documentation/devicetree/bindings/iio/adc/renesas,gyroadc.txt | 2 +-
 Documentation/devicetree/bindings/media/rcar_vin.txt  | 4 ++--
 Documentation/devicetree/bindings/pci/rcar-pci.txt| 2 +-
 drivers/i2c/busses/i2c-rcar.c | 2 +-
 drivers/media/platform/rcar_fdp1.c| 2 +-
 drivers/mmc/host/renesas_sdhi_core.c  | 2 +-
 include/linux/mfd/tmio.h  | 2 +-
 8 files changed, 9 insertions(+), 9 deletions(-)

-- 
2.11.0

[PATCH 7/7] [media] v4l: rcar_fdp1: use proper name for the R-Car SoC

[Wolfram Sang]

It is 'R-Car', not 'RCar'. No code or binding changes, only descriptive text.

Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>
---
I suggest this trivial patch should be picked individually per susbsystem.

 drivers/media/platform/rcar_fdp1.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/rcar_fdp1.c 
b/drivers/media/platform/rcar_fdp1.c
index 42f25d241edd7c..0da0eba9202cdd 100644
--- a/drivers/media/platform/rcar_fdp1.c
+++ b/drivers/media/platform/rcar_fdp1.c
@@ -1,5 +1,5 @@
 /*
- * Renesas RCar Fine Display Processor
+ * Renesas R-Car Fine Display Processor
  *
  * Video format converter and frame deinterlacer device.
  *
-- 
2.11.0

[PATCH 6/7] [media] soc_camera: rcar_vin: use proper name for the R-Car SoC

[Wolfram Sang]

It is 'R-Car', not 'RCar'. No code or binding changes, only descriptive text.

Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>
---
I suggest this trivial patch should be picked individually per susbsystem.

 Documentation/devicetree/bindings/media/rcar_vin.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/rcar_vin.txt 
b/Documentation/devicetree/bindings/media/rcar_vin.txt
index 25fc933318483d..4af8760b353339 100644
--- a/Documentation/devicetree/bindings/media/rcar_vin.txt
+++ b/Documentation/devicetree/bindings/media/rcar_vin.txt
@@ -1,5 +1,5 @@
-Renesas RCar Video Input driver (rcar_vin)
---
+Renesas R-Car Video Input driver (rcar_vin)
+---
 
 The rcar_vin device provides video input capabilities for the Renesas R-Car
 family of devices.
-- 
2.11.0

Re: [PATCH] media: i2c: ov772x: Force use of SCCB protocol

[Wolfram Sang]

On Fri, May 12, 2017 at 11:52:43AM +0200, Jacopo Mondi wrote:
> Force use of Omnivision's SCCB protocol and make sure the I2c adapter
> supports protocol mangling during probe.
> 
> Testing done on SH4 Migo-R board.
> As commit:
> [e789029761503f0cce03e8767a56ae099b88e1bd]
> "i2c: sh_mobile: don't send a stop condition by default inside transfers"
> makes the i2c adapter emit a stop bit between messages in a single
> transfer only when explicitly required, the ov772x driver fails to
> probe due to i2c transfer timeout without SCCB flag set.
> 
> i2c-sh_mobile i2c-sh_mobile.0: Transfer request timed out
> ov772x 0-0021: Product ID error 92:92
> 
> With this patch applied:
> 
> soc-camera-pdrv soc-camera-pdrv.0: Probing soc-camera-pdrv.0
> ov772x 0-0021: ov7725 Product ID 77:21 Manufacturer ID 7f:a2
> 
> Signed-off-by: Jacopo Mondi <jac...@jmondi.org>

Acked-by: Wolfram Sang <wsa+rene...@sang-engineering.com>



signature.asc
Description: PGP signature

Re: [PATCH 0/9] Unify i2c_mux_add_adapter error reporting

[Wolfram Sang]

On Mon, Apr 03, 2017 at 10:38:29AM +0200, Peter Rosin wrote:
> Hi!
> 
> Many users of the i2c_mux_add_adapter interface log a message
> on failure, but the function already logs such a message. One
> or two of those users actually add more information than already
> provided by the central failure message.
> 
> So, first fix the central error reporting to provide as much
> information as any current user, and then remove the surplus
> error reporting at the call sites.

Yes, I like.

Reviewed-by: Wolfram Sang <w...@the-dreams.de>



signature.asc
Description: PGP signature

Re: [PATCH 9/9] [media] cx231xx: stop double error reporting

[Wolfram Sang]

On Mon, Apr 03, 2017 at 10:38:38AM +0200, Peter Rosin wrote:
> i2c_mux_add_adapter already logs a message on failure.
> 
> Signed-off-by: Peter Rosin 
> ---
>  drivers/media/usb/cx231xx/cx231xx-i2c.c | 15 ---
>  1 file changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/usb/cx231xx/cx231xx-i2c.c 
> b/drivers/media/usb/cx231xx/cx231xx-i2c.c
> index 35e9acfe63d3..dff514e147da 100644
> --- a/drivers/media/usb/cx231xx/cx231xx-i2c.c
> +++ b/drivers/media/usb/cx231xx/cx231xx-i2c.c
> @@ -576,17 +576,10 @@ int cx231xx_i2c_mux_create(struct cx231xx *dev)
>  
>  int cx231xx_i2c_mux_register(struct cx231xx *dev, int mux_no)
>  {
> - int rc;
> -
> - rc = i2c_mux_add_adapter(dev->muxc,
> -  0,
> -  mux_no /* chan_id */,
> -  0 /* class */);
> - if (rc)
> - dev_warn(dev->dev,
> -  "i2c mux %d register FAILED\n", mux_no);
> -
> - return rc;
> + return i2c_mux_add_adapter(dev->muxc,
> +0,
> +mux_no /* chan_id */,
> +0 /* class */);

Could be argued that the whole function is obsolete now and the
c231xx-core can call i2c_mux_add_adapter() directly. But maybe this is a
seperate patch.

>  }
>  
>  void cx231xx_i2c_mux_unregister(struct cx231xx *dev)
> -- 
> 2.1.4
> 


signature.asc
Description: PGP signature

Re: [PATCH 00/11] media: rcar-vin: fix OPS and format/pad index issues

[Wolfram Sang]

> Patch 10-11 fix a OPS when unbinding/binding the video source subdevice.

I can happily confirm that this series finally makes the I2C demuxer
work on the I2C bus with the HDMI clients because rebinding works now!
Note that I didn't test inputting any actual video but only the
rebinding capabilites. But since rebinding was a major motivation for
this series to be factored out of a bigger one:

Tested-by: Wolfram Sang <wsa+rene...@sang-engineering.com>



signature.asc
Description: PGP signature

Re: [PATCH 1/4] exynos4-is: Clear isp-i2c adapter power.ignore_children flag

[Wolfram Sang]

On Thu, Sep 01, 2016 at 01:39:16PM +0200, Sylwester Nawrocki wrote:
> Since commit 04f59143b571161d25315dd52d7a2ecc022cb71a
> ("i2c: let I2C masters ignore their children for PM")
> the power.ignore_children flag is set when registering an I2C
> adapter. Since I2C transfers are not managed by the fimc-isp-i2c
> driver its clients use pm_runtime_* calls directly to communicate
> required power state of the bus controller.
> However when the power.ignore_children flag is set that doesn't
> work, so clear that flag back after registering the adapter.
> While at it drop pm_runtime_enable() call on the i2c_adapter
> as it is already done by the I2C subsystem when registering
> I2C adapter.
> 
> Cc:  # 4.7+
> Reported-by: Marek Szyprowski 
> Signed-off-by: Sylwester Nawrocki 

CCing the authors of the "offending" commit as well as linux-pm for more
PM expertise.

> ---
>  drivers/media/platform/exynos4-is/fimc-is-i2c.c | 25 
> ++---
>  1 file changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/platform/exynos4-is/fimc-is-i2c.c 
> b/drivers/media/platform/exynos4-is/fimc-is-i2c.c
> index 7521aa5..03b4246 100644
> --- a/drivers/media/platform/exynos4-is/fimc-is-i2c.c
> +++ b/drivers/media/platform/exynos4-is/fimc-is-i2c.c
> @@ -55,26 +55,37 @@ static int fimc_is_i2c_probe(struct platform_device *pdev)
>   i2c_adap->algo = _is_i2c_algorithm;
>   i2c_adap->class = I2C_CLASS_SPD;
>  
> + platform_set_drvdata(pdev, isp_i2c);
> + pm_runtime_enable(>dev);
> +
>   ret = i2c_add_adapter(i2c_adap);
>   if (ret < 0) {
>   dev_err(>dev, "failed to add I2C bus %s\n",
>   node->full_name);
> - return ret;
> + goto err_pm_dis;
>   }
>  
> - platform_set_drvdata(pdev, isp_i2c);
> -
> - pm_runtime_enable(>dev);
> - pm_runtime_enable(_adap->dev);
> -
> + /*
> +  * Client drivers of this adapter don't do any I2C transfers as that
> +  * is handled by the ISP firmware.  But we rely on the runtime PM
> +  * state propagation from the clients up to the adapter driver so
> +  * clear the ignore_children flags here.  PM rutnime calls are not
> +  * used in probe() handler of clients of this adapter so there is
> +  * no issues with clearing the flag right after registering the I2C
> +  * adapter.
> +  */
> + pm_suspend_ignore_children(_adap->dev, false);
>   return 0;
> +
> +err_pm_dis:
> + pm_runtime_disable(>dev);
> + return ret;
>  }
>  
>  static int fimc_is_i2c_remove(struct platform_device *pdev)
>  {
>   struct fimc_is_i2c *isp_i2c = platform_get_drvdata(pdev);
>  
> - pm_runtime_disable(_i2c->adapter.dev);
>   pm_runtime_disable(>dev);
>   i2c_del_adapter(_i2c->adapter);
>  
> -- 
> 1.9.1
> 


signature.asc
Description: PGP signature

[PATCH 2/6] staging: media: lirc: lirc_imon: don't print error when allocating urb fails

[Wolfram Sang]

kmalloc will print enough information in case of failure.

Signed-off-by: Wolfram Sang <wsa-...@sang-engineering.com>
---
 drivers/staging/media/lirc/lirc_imon.c | 9 ++---
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/media/lirc/lirc_imon.c 
b/drivers/staging/media/lirc/lirc_imon.c
index ff1926ca1f96f5..a183e68ec32089 100644
--- a/drivers/staging/media/lirc/lirc_imon.c
+++ b/drivers/staging/media/lirc/lirc_imon.c
@@ -797,16 +797,11 @@ static int imon_probe(struct usb_interface *interface,
goto free_rbuf;
}
rx_urb = usb_alloc_urb(0, GFP_KERNEL);
-   if (!rx_urb) {
-   dev_err(dev, "%s: usb_alloc_urb failed for IR urb\n", __func__);
+   if (!rx_urb)
goto free_lirc_buf;
-   }
tx_urb = usb_alloc_urb(0, GFP_KERNEL);
-   if (!tx_urb) {
-   dev_err(dev, "%s: usb_alloc_urb failed for display urb\n",
-   __func__);
+   if (!tx_urb)
goto free_rx_urb;
-   }
 
mutex_init(>ctx_lock);
context->vfd_proto_6p = vfd_proto_6p;
-- 
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/6] staging: media: lirc: lirc_sasem: don't print error when allocating urb fails

[Wolfram Sang]

kmalloc will print enough information in case of failure.

Signed-off-by: Wolfram Sang <wsa-...@sang-engineering.com>
---
 drivers/staging/media/lirc/lirc_sasem.c | 5 -
 1 file changed, 5 deletions(-)

diff --git a/drivers/staging/media/lirc/lirc_sasem.c 
b/drivers/staging/media/lirc/lirc_sasem.c
index 2218d0042030ed..b080fde6d740c9 100644
--- a/drivers/staging/media/lirc/lirc_sasem.c
+++ b/drivers/staging/media/lirc/lirc_sasem.c
@@ -758,17 +758,12 @@ static int sasem_probe(struct usb_interface *interface,
}
rx_urb = usb_alloc_urb(0, GFP_KERNEL);
if (!rx_urb) {
-   dev_err(>dev,
-   "%s: usb_alloc_urb failed for IR urb\n", __func__);
alloc_status = 5;
goto alloc_status_switch;
}
if (vfd_ep_found) {
tx_urb = usb_alloc_urb(0, GFP_KERNEL);
if (!tx_urb) {
-   dev_err(>dev,
-   "%s: usb_alloc_urb failed for VFD urb",
-   __func__);
alloc_status = 6;
goto alloc_status_switch;
}
-- 
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/6] staging: don't print error when allocating urb fails

[Wolfram Sang]

This per-subsystem series is part of a tree wide cleanup. usb_alloc_urb() uses
kmalloc which already prints enough information on failure. So, let's simply
remove those "allocation failed" messages from drivers like we did already for
other -ENOMEM cases. gkh acked this approach when we talked about it at LCJ in
Tokyo a few weeks ago.


Wolfram Sang (6):
  staging: comedi: drivers: usbduxfast: don't print error when
allocating urb fails
  staging: media: lirc: lirc_imon: don't print error when allocating urb
fails
  staging: media: lirc: lirc_sasem: don't print error when allocating
urb fails
  staging: most: hdm-usb: hdm_usb: don't print error when allocating urb
fails
  staging: rtl8192u: r8192U_core: don't print error when allocating urb
fails
  staging: vt6656: main_usb: don't print error when allocating urb fails

 drivers/staging/comedi/drivers/usbduxfast.c |  4 +---
 drivers/staging/media/lirc/lirc_imon.c  |  9 ++---
 drivers/staging/media/lirc/lirc_sasem.c |  5 -
 drivers/staging/most/hdm-usb/hdm_usb.c  |  4 +---
 drivers/staging/rtl8192u/r8192U_core.c  |  5 +
 drivers/staging/vt6656/main_usb.c   | 12 +++-
 6 files changed, 8 insertions(+), 31 deletions(-)

-- 
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 18/28] media: usb: hackrf: hackrf: don't print error when allocating urb fails

[Wolfram Sang]

kmalloc will print enough information in case of failure.

Signed-off-by: Wolfram Sang <wsa-...@sang-engineering.com>
---
 drivers/media/usb/hackrf/hackrf.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/media/usb/hackrf/hackrf.c 
b/drivers/media/usb/hackrf/hackrf.c
index b1e229a44192d3..c2c8d12e949868 100644
--- a/drivers/media/usb/hackrf/hackrf.c
+++ b/drivers/media/usb/hackrf/hackrf.c
@@ -691,7 +691,6 @@ static int hackrf_alloc_urbs(struct hackrf_dev *dev, bool 
rcv)
dev_dbg(dev->dev, "alloc urb=%d\n", i);
dev->urb_list[i] = usb_alloc_urb(0, GFP_ATOMIC);
if (!dev->urb_list[i]) {
-   dev_dbg(dev->dev, "failed\n");
for (j = 0; j < i; j++)
usb_free_urb(dev->urb_list[j]);
return -ENOMEM;
-- 
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 23/28] media: usb: stk1160: stk1160-video: don't print error when allocating urb fails

[Wolfram Sang]

kmalloc will print enough information in case of failure.

Signed-off-by: Wolfram Sang <wsa-...@sang-engineering.com>
---
 drivers/media/usb/stk1160/stk1160-video.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/media/usb/stk1160/stk1160-video.c 
b/drivers/media/usb/stk1160/stk1160-video.c
index 6ecb0b48423f3d..ce8ebbe395a620 100644
--- a/drivers/media/usb/stk1160/stk1160-video.c
+++ b/drivers/media/usb/stk1160/stk1160-video.c
@@ -457,10 +457,8 @@ int stk1160_alloc_isoc(struct stk1160 *dev)
for (i = 0; i < num_bufs; i++) {
 
urb = usb_alloc_urb(max_packets, GFP_KERNEL);
-   if (!urb) {
-   stk1160_err("cannot alloc urb[%d]\n", i);
+   if (!urb)
goto free_i_bufs;
-   }
dev->isoc_ctl.urb[i] = urb;
 
 #ifndef CONFIG_DMA_NONCOHERENT
-- 
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 19/28] media: usb: hdpvr: hdpvr-video: don't print error when allocating urb fails

[Wolfram Sang]

kmalloc will print enough information in case of failure.

Signed-off-by: Wolfram Sang <wsa-...@sang-engineering.com>
---
 drivers/media/usb/hdpvr/hdpvr-video.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/media/usb/hdpvr/hdpvr-video.c 
b/drivers/media/usb/hdpvr/hdpvr-video.c
index 2a3a8b470555b9..6d43d75493ea0e 100644
--- a/drivers/media/usb/hdpvr/hdpvr-video.c
+++ b/drivers/media/usb/hdpvr/hdpvr-video.c
@@ -155,10 +155,8 @@ int hdpvr_alloc_buffers(struct hdpvr_device *dev, uint 
count)
buf->dev = dev;
 
urb = usb_alloc_urb(0, GFP_KERNEL);
-   if (!urb) {
-   v4l2_err(>v4l2_dev, "cannot allocate urb\n");
+   if (!urb)
goto exit_urb;
-   }
buf->urb = urb;
 
mem = usb_alloc_coherent(dev->udev, dev->bulk_in_size, 
GFP_KERNEL,
-- 
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 14/28] media: usb: em28xx: em28xx-core: don't print error when allocating urb fails

[Wolfram Sang]

kmalloc will print enough information in case of failure.

Signed-off-by: Wolfram Sang <wsa-...@sang-engineering.com>
---
 drivers/media/usb/em28xx/em28xx-core.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/media/usb/em28xx/em28xx-core.c 
b/drivers/media/usb/em28xx/em28xx-core.c
index 37456079f490d9..eebd5d7088d009 100644
--- a/drivers/media/usb/em28xx/em28xx-core.c
+++ b/drivers/media/usb/em28xx/em28xx-core.c
@@ -934,7 +934,6 @@ int em28xx_alloc_urbs(struct em28xx *dev, enum em28xx_mode 
mode, int xfer_bulk,
for (i = 0; i < usb_bufs->num_bufs; i++) {
urb = usb_alloc_urb(usb_bufs->num_packets, GFP_KERNEL);
if (!urb) {
-   em28xx_err("cannot alloc usb_ctl.urb %i\n", i);
em28xx_uninit_usb_xfer(dev, mode);
return -ENOMEM;
}
-- 
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html