[PATCH V5 11/17] i2c: nomadik: Convert to devm functions
Use devm_* functions to simplify code and error handling.
Cc: Alessandro Rubini
Cc: Linus Walleij
Cc: Wolfram Sang
Signed-off-by: Ulf Hansson
---
Change in v5:
Fix the screwed up rebase for the second time.
Note, still the other v3 patches in this patchset, applies on top of
this one.
---
drivers/i2c/busses/i2c-nomadik.c | 31 ++-
1 file changed, 10 insertions(+), 21 deletions(-)
diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index b0b9390..b1a5225 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -976,7 +976,7 @@ static int nmk_i2c_probe(struct amba_device *adev, const
struct amba_id *id)
struct i2c_vendor_data *vendor = id->data;
u32 max_fifo_threshold = (vendor->fifodepth / 2) - 1;
- dev = kzalloc(sizeof(struct nmk_i2c_dev), GFP_KERNEL);
+ dev = devm_kzalloc(&adev->dev, sizeof(struct nmk_i2c_dev), GFP_KERNEL);
if (!dev) {
dev_err(&adev->dev, "cannot allocate memory\n");
ret = -ENOMEM;
@@ -1006,27 +1006,28 @@ static int nmk_i2c_probe(struct amba_device *adev,
const struct amba_id *id)
/* If possible, let's go to idle until the first transfer */
pinctrl_pm_select_idle_state(&adev->dev);
- dev->virtbase = ioremap(adev->res.start, resource_size(&adev->res));
- if (!dev->virtbase) {
+ dev->virtbase = devm_ioremap(&adev->dev, adev->res.start,
+ resource_size(&adev->res));
+ if (IS_ERR(dev->virtbase)) {
ret = -ENOMEM;
- goto err_no_ioremap;
+ goto err_no_mem;
}
dev->irq = adev->irq[0];
- ret = request_irq(dev->irq, i2c_irq_handler, 0,
+ ret = devm_request_irq(&adev->dev, dev->irq, i2c_irq_handler, 0,
DRIVER_NAME, dev);
if (ret) {
dev_err(&adev->dev, "cannot claim the irq %d\n", dev->irq);
- goto err_irq;
+ goto err_no_mem;
}
pm_suspend_ignore_children(&adev->dev, true);
- dev->clk = clk_get(&adev->dev, NULL);
+ dev->clk = devm_clk_get(&adev->dev, NULL);
if (IS_ERR(dev->clk)) {
dev_err(&adev->dev, "could not get i2c clock\n");
ret = PTR_ERR(dev->clk);
- goto err_no_clk;
+ goto err_no_mem;
}
adap = &dev->adap;
@@ -1048,21 +1049,13 @@ static int nmk_i2c_probe(struct amba_device *adev,
const struct amba_id *id)
ret = i2c_add_adapter(adap);
if (ret) {
dev_err(&adev->dev, "failed to add adapter\n");
- goto err_add_adap;
+ goto err_no_mem;
}
pm_runtime_put(&adev->dev);
return 0;
- err_add_adap:
- clk_put(dev->clk);
- err_no_clk:
- free_irq(dev->irq, dev);
- err_irq:
- iounmap(dev->virtbase);
- err_no_ioremap:
- kfree(dev);
err_no_mem:
return ret;
@@ -1079,13 +1072,9 @@ static int nmk_i2c_remove(struct amba_device *adev)
clear_all_interrupts(dev);
/* disable the controller */
i2c_clr_bit(dev->virtbase + I2C_CR, I2C_CR_PE);
- free_irq(dev->irq, dev);
- iounmap(dev->virtbase);
if (res)
release_mem_region(res->start, resource_size(res));
- clk_put(dev->clk);
pm_runtime_disable(&adev->dev);
- kfree(dev);
return 0;
}
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V4 11/17] i2c: nomadik: Convert to devm functions
On 18 February 2014 23:20, Ulf Hansson wrote:
> Use devm_* functions to simplify code and error handling.
>
> Cc: Alessandro Rubini
> Cc: Linus Walleij
> Cc: Wolfram Sang
> Signed-off-by: Ulf Hansson
> ---
>
> Changes in v4:
> Rebased on top of Linus Walleij's v2 patch:
> "i2c: nomadik: factor platform data into state container"
>
> Note, only this patch needed to be rebased in the patchset.
>
> ---
> drivers/i2c/busses/i2c-nomadik.c | 36 +++-
> 1 file changed, 15 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-nomadik.c
> b/drivers/i2c/busses/i2c-nomadik.c
> index b0b9390..cd15c03 100644
> --- a/drivers/i2c/busses/i2c-nomadik.c
> +++ b/drivers/i2c/busses/i2c-nomadik.c
> @@ -976,7 +976,12 @@ static int nmk_i2c_probe(struct amba_device *adev, const
> struct amba_id *id)
> struct i2c_vendor_data *vendor = id->data;
> u32 max_fifo_threshold = (vendor->fifodepth / 2) - 1;
>
> - dev = kzalloc(sizeof(struct nmk_i2c_dev), GFP_KERNEL);
> + if (!np) {
> + dev_err(&adev->dev, "no device node\n");
> + return -ENODEV;
> + }
> +
Argh, I managed to screw up again. The above code should not be there.
The rebase of Linus v2 failed.
Hold on, v5 coming soon. :-)
Kind regards
Uffe
> + dev = devm_kzalloc(&adev->dev, sizeof(struct nmk_i2c_dev),
> GFP_KERNEL);
> if (!dev) {
> dev_err(&adev->dev, "cannot allocate memory\n");
> ret = -ENOMEM;
> @@ -1006,27 +1011,28 @@ static int nmk_i2c_probe(struct amba_device *adev,
> const struct amba_id *id)
> /* If possible, let's go to idle until the first transfer */
> pinctrl_pm_select_idle_state(&adev->dev);
>
> - dev->virtbase = ioremap(adev->res.start, resource_size(&adev->res));
> - if (!dev->virtbase) {
> + dev->virtbase = devm_ioremap(&adev->dev, adev->res.start,
> + resource_size(&adev->res));
> + if (IS_ERR(dev->virtbase)) {
> ret = -ENOMEM;
> - goto err_no_ioremap;
> + goto err_no_mem;
> }
>
> dev->irq = adev->irq[0];
> - ret = request_irq(dev->irq, i2c_irq_handler, 0,
> + ret = devm_request_irq(&adev->dev, dev->irq, i2c_irq_handler, 0,
> DRIVER_NAME, dev);
> if (ret) {
> dev_err(&adev->dev, "cannot claim the irq %d\n", dev->irq);
> - goto err_irq;
> + goto err_no_mem;
> }
>
> pm_suspend_ignore_children(&adev->dev, true);
>
> - dev->clk = clk_get(&adev->dev, NULL);
> + dev->clk = devm_clk_get(&adev->dev, NULL);
> if (IS_ERR(dev->clk)) {
> dev_err(&adev->dev, "could not get i2c clock\n");
> ret = PTR_ERR(dev->clk);
> - goto err_no_clk;
> + goto err_no_mem;
> }
>
> adap = &dev->adap;
> @@ -1048,21 +1054,13 @@ static int nmk_i2c_probe(struct amba_device *adev,
> const struct amba_id *id)
> ret = i2c_add_adapter(adap);
> if (ret) {
> dev_err(&adev->dev, "failed to add adapter\n");
> - goto err_add_adap;
> + goto err_no_mem;
> }
>
> pm_runtime_put(&adev->dev);
>
> return 0;
>
> - err_add_adap:
> - clk_put(dev->clk);
> - err_no_clk:
> - free_irq(dev->irq, dev);
> - err_irq:
> - iounmap(dev->virtbase);
> - err_no_ioremap:
> - kfree(dev);
> err_no_mem:
>
> return ret;
> @@ -1079,13 +1077,9 @@ static int nmk_i2c_remove(struct amba_device *adev)
> clear_all_interrupts(dev);
> /* disable the controller */
> i2c_clr_bit(dev->virtbase + I2C_CR, I2C_CR_PE);
> - free_irq(dev->irq, dev);
> - iounmap(dev->virtbase);
> if (res)
> release_mem_region(res->start, resource_size(res));
> - clk_put(dev->clk);
> pm_runtime_disable(&adev->dev);
> - kfree(dev);
>
> return 0;
> }
> --
> 1.7.9.5
>
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH V4 11/17] i2c: nomadik: Convert to devm functions
Use devm_* functions to simplify code and error handling.
Cc: Alessandro Rubini
Cc: Linus Walleij
Cc: Wolfram Sang
Signed-off-by: Ulf Hansson
---
Changes in v4:
Rebased on top of Linus Walleij's v2 patch:
"i2c: nomadik: factor platform data into state container"
Note, only this patch needed to be rebased in the patchset.
---
drivers/i2c/busses/i2c-nomadik.c | 36 +++-
1 file changed, 15 insertions(+), 21 deletions(-)
diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index b0b9390..cd15c03 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -976,7 +976,12 @@ static int nmk_i2c_probe(struct amba_device *adev, const
struct amba_id *id)
struct i2c_vendor_data *vendor = id->data;
u32 max_fifo_threshold = (vendor->fifodepth / 2) - 1;
- dev = kzalloc(sizeof(struct nmk_i2c_dev), GFP_KERNEL);
+ if (!np) {
+ dev_err(&adev->dev, "no device node\n");
+ return -ENODEV;
+ }
+
+ dev = devm_kzalloc(&adev->dev, sizeof(struct nmk_i2c_dev), GFP_KERNEL);
if (!dev) {
dev_err(&adev->dev, "cannot allocate memory\n");
ret = -ENOMEM;
@@ -1006,27 +1011,28 @@ static int nmk_i2c_probe(struct amba_device *adev,
const struct amba_id *id)
/* If possible, let's go to idle until the first transfer */
pinctrl_pm_select_idle_state(&adev->dev);
- dev->virtbase = ioremap(adev->res.start, resource_size(&adev->res));
- if (!dev->virtbase) {
+ dev->virtbase = devm_ioremap(&adev->dev, adev->res.start,
+ resource_size(&adev->res));
+ if (IS_ERR(dev->virtbase)) {
ret = -ENOMEM;
- goto err_no_ioremap;
+ goto err_no_mem;
}
dev->irq = adev->irq[0];
- ret = request_irq(dev->irq, i2c_irq_handler, 0,
+ ret = devm_request_irq(&adev->dev, dev->irq, i2c_irq_handler, 0,
DRIVER_NAME, dev);
if (ret) {
dev_err(&adev->dev, "cannot claim the irq %d\n", dev->irq);
- goto err_irq;
+ goto err_no_mem;
}
pm_suspend_ignore_children(&adev->dev, true);
- dev->clk = clk_get(&adev->dev, NULL);
+ dev->clk = devm_clk_get(&adev->dev, NULL);
if (IS_ERR(dev->clk)) {
dev_err(&adev->dev, "could not get i2c clock\n");
ret = PTR_ERR(dev->clk);
- goto err_no_clk;
+ goto err_no_mem;
}
adap = &dev->adap;
@@ -1048,21 +1054,13 @@ static int nmk_i2c_probe(struct amba_device *adev,
const struct amba_id *id)
ret = i2c_add_adapter(adap);
if (ret) {
dev_err(&adev->dev, "failed to add adapter\n");
- goto err_add_adap;
+ goto err_no_mem;
}
pm_runtime_put(&adev->dev);
return 0;
- err_add_adap:
- clk_put(dev->clk);
- err_no_clk:
- free_irq(dev->irq, dev);
- err_irq:
- iounmap(dev->virtbase);
- err_no_ioremap:
- kfree(dev);
err_no_mem:
return ret;
@@ -1079,13 +1077,9 @@ static int nmk_i2c_remove(struct amba_device *adev)
clear_all_interrupts(dev);
/* disable the controller */
i2c_clr_bit(dev->virtbase + I2C_CR, I2C_CR_PE);
- free_irq(dev->irq, dev);
- iounmap(dev->virtbase);
if (res)
release_mem_region(res->start, resource_size(res));
- clk_put(dev->clk);
pm_runtime_disable(&adev->dev);
- kfree(dev);
return 0;
}
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 11/17] i2c: nomadik: Convert to devm functions
On 18 February 2014 21:50, Ulf Hansson wrote: > On 18 February 2014 19:43, Wolfram Sang wrote: >> >>> I added the IS_ERR, check as you suggested. That caused a rebase of >>> the other patches as well. >>> >>> Sorry if it was unclear. >> >> No problem. I am about to apply all patches except 17 as you suggested. >> It fails here, however, although I applied Linus' patch first. What >> exactly is your base? Any chance to publish your branch? >> > > I have several branches. :-) > > My patches applies on top 3.14-rc1 + Linus' patch. > > It also applies on > "git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-nomadik.git" > on the i2c-nomadik, which already holds Linus's patch. This is what I > thought was important due what's stated in the MAINATAINERS file. > > Now, I guess I should use your tree instead, > "git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git". Any > particular branch I should use? Maybe it's also better if I just > resend the patches in a new patchset, once I rebased them towards your > tree? Sorry, I have taken Linus v1 patch as base, it should have been v2 of course. Sorry for wasting your time. I will rebase and send v4 patches asap. Kind regards Uffe > > Kind regards > Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] i2c: Add message transfer tracepoints for SMBUS
> + TP_printk("i2c-%d a=%02x f=%02x c=%x %s l=%u [%*phN]",
Although SMBus has no 10-bit addresses, we probably should also use %03x
there for consistency reasons?
Also, the I2C tracing has first 'f' then 'a', that should be consistent,
too.
'flags' should be %04x again and I'd prefer %*ph (or %*phD) for the buffer.
> + __print_symbolic(__entry->protocol,
> +{ I2C_SMBUS_QUICK, "QUICK" },
> +{ I2C_SMBUS_BYTE,"BYTE" },
> +{ I2C_SMBUS_BYTE_DATA,
> "BYTE_DATA" },
> +{ I2C_SMBUS_WORD_DATA,
> "WORD_DATA" },
> +{ I2C_SMBUS_PROC_CALL,
> "PROC_CALL" },
> +{ I2C_SMBUS_BLOCK_DATA,
> "BLOCK_DATA" },
> +{ I2C_SMBUS_I2C_BLOCK_BROKEN,
> "I2C_BLOCK_BROKEN" },
> +{ I2C_SMBUS_BLOCK_PROC_CALL,
> "BLOCK_PROC_CALL" },
> +{ I2C_SMBUS_I2C_BLOCK_DATA,
> "I2C_BLOCK_DATA" }),
Can we have something like this for 'flags'?
signature.asc
Description: Digital signature
Re: [PATCH V3 11/17] i2c: nomadik: Convert to devm functions
On 18 February 2014 19:43, Wolfram Sang wrote: > >> I added the IS_ERR, check as you suggested. That caused a rebase of >> the other patches as well. >> >> Sorry if it was unclear. > > No problem. I am about to apply all patches except 17 as you suggested. > It fails here, however, although I applied Linus' patch first. What > exactly is your base? Any chance to publish your branch? > I have several branches. :-) My patches applies on top 3.14-rc1 + Linus' patch. It also applies on "git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-nomadik.git" on the i2c-nomadik, which already holds Linus's patch. This is what I thought was important due what's stated in the MAINATAINERS file. Now, I guess I should use your tree instead, "git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git". Any particular branch I should use? Maybe it's also better if I just resend the patches in a new patchset, once I rebased them towards your tree? Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] i2c: Add message transfer tracepoints for I2C
Hi David,
On Thu, Jan 09, 2014 at 09:49:54PM +, David Howells wrote:
> Add tracepoints into the I2C message transfer function to retrieve the message
> sent or received. The following config options must be turned on to make use
> of the facility:
>
> CONFIG_FTRACE
> CONFIG_ENABLE_DEFAULT_TRACERS
>
> The I2C tracepoint can be enabled thusly:
>
> echo 1 >/sys/kernel/debug/tracing/events/i2c/i2c_transfer/enable
>
> and will dump messages that can be viewed in /sys/kernel/debug/tracing/trace
> that look like:
>
> ... i2c_write: i2c-5 #0 f=00 a=44 l=2 [0214]
> ... i2c_read: i2c-5 #1 f=01 a=44 l=4
> ... i2c_reply: i2c-5 #1 f=01 a=44 l=4 [3300]
> ... i2c_result: i2c-5 n=2 ret=2
>
> formatted as:
>
> i2c-
> #
> f=
> a=
> l=
> n=
> ret=
> []
>
> The operation is done between the i2c_write/i2c_read lines and the i2c_reply
> and i2c_result lines so that if the hardware hangs, the trace buffer can be
> consulted to determine the problematic operation.
>
> The adapters to be traced can be selected by something like:
>
> echo adapter_nr==1 >/sys/kernel/debug/tracing/events/i2c/filter
>
> These changes are based on code from Steven Rostedt.
>
> Signed-off-by: Steven Rostedt
> Signed-off-by: David Howells
> Reviewed-by: Steven Rostedt
I like it very much, just have some comments about the format.
> +TRACE_EVENT_FN(i2c_write,
> +TP_PROTO(const struct i2c_adapter *adap, const struct i2c_msg
> *msg,
> + int num),
> +TP_ARGS(adap, msg, num),
> +TP_STRUCT__entry(
> +__field(int, adapter_nr )
> +__field(__u16, msg_nr )
> +__field(__u16, addr)
> +__field(__u16, flags )
> +__field(__u16, len )
> +__dynamic_array(__u8, buf, msg->len) ),
> +TP_fast_assign(
> +__entry->adapter_nr = adap->nr;
> +__entry->msg_nr = num;
> +__entry->addr = msg->addr;
> +__entry->flags = msg->flags;
> +__entry->len = msg->len;
> +memcpy(__get_dynamic_array(buf), msg->buf, msg->len);
> + ),
> +TP_printk("i2c-%d #%u f=%02x a=%02x l=%u [%*phN]",
'flags' are u16 and the whole range is needed -> %04x
'addr' is u16 and either 7 or 10 bits are needed. The core does the
following when assigning names because it is possible to have devices
0x50 (7-bit) and 0x050 (10-bit) on the bus:
/* For 10-bit clients, add an arbitrary offset to avoid collisions */
dev_set_name(&client->dev, "%d-%04x", i2c_adapter_id(adap),
client->addr | ((client->flags & I2C_CLIENT_TEN)
? 0xa000 : 0));
I don't know if this can be implemented. Actually, I don't think it
needs to be implemented since flags are printed, too. So, with this the
10-bit case is visible and for the address simply %03x should do.
And for the buffer: %*phN is difficult to read IMO. What about %*ph? Or
%*phD at least?
Thanks,
Wolfram
signature.asc
Description: Digital signature
Re: [PATCH V3 11/17] i2c: nomadik: Convert to devm functions
> I added the IS_ERR, check as you suggested. That caused a rebase of > the other patches as well. > > Sorry if it was unclear. No problem. I am about to apply all patches except 17 as you suggested. It fails here, however, although I applied Linus' patch first. What exactly is your base? Any chance to publish your branch? signature.asc Description: Digital signature
Re: [PATCH v2] i2c: nomadik: factor platform data into state container
> - of_property_read_u32(np, "clock-frequency", &pdata->clk_freq); > + /* Default to 400 kHz if no frequency is given in the node */ > + if (of_property_read_u32(np, "clock-frequency", &nmk->clk_freq)) > + nmk->clk_freq = 40; I can't really recommend this. 100kHz is something all devices support, but, although common, 400 kHz is already an extension of the standard. Let me know if I should fix this up for you. signature.asc Description: Digital signature
Re: [PATCH] i2c: imx: retry on NAK
On Wed, Sep 11, 2013 at 01:58:10AM +0200, Luka Perkov wrote: > From: Tim Harvey > > In case of busy i2c try again to get ACK. > > Signed-off-by: Tim Harvey > Tested-by: Luka Perkov -EAGAIN is for lost arbitration (see Documentation/i2c/fault-codes). In that case, the core will try again. NACK needs to be reported, so the caller can decide what to do in that case. -ENXIO would be the best response here. signature.asc Description: Digital signature
Re: [PATCH 1/2] I2C: EMMA Mobile I2C master driver
On Tue, Sep 03, 2013 at 05:49:29PM +0100, Ian Molton wrote:
> Add a driver for the EMMA mobile I2C block.
>
> The driver supports low and high-speed interrupt driven PIO transfers.
>
> Signed-off-by: Ian Molton
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -777,6 +777,16 @@ config I2C_RCAR
> This driver can also be built as a module. If so, the module
> will be called i2c-rcar.
>
> +config I2C_EM
> + tristate "EMMA Mobile series I2C adapter"
> + depends on I2C && HAVE_CLK
> + help
> + If you say yes to this option, support will be included for the
> + I2C interface on the Renesas Electronics EM/EV family of processors.
> +
> + This driver can also be built as a module. If so, the module
> + will be called i2c-em
> +
Please keep it sorted.
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-em.c
> @@ -0,0 +1,501 @@
> +/*
> + * Copyright 2013 Codethink Ltd.
> + * Parts Copyright 2010 Renesas Electronics Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2
> + * as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software Foundation,
> + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1335, USA.
> + */
Skip the address please.
> +static int em_i2c_xfer(struct i2c_adapter *, struct i2c_msg[], int);
You can skip this forward declaration by reordering.
> +
> +struct em_i2c_device {
> + struct i2c_adapter adap;
> + wait_queue_head_t i2c_wait;
> + void __iomem*membase;
> + struct clk *clk;
> + struct clk *sclk;
> + int irq;
> + int flags;
> + int pending;
> + spinlock_t irq_lock;
> +};
> +
> +#define to_em_i2c(adap) (struct em_i2c_device *)i2c_get_adapdata(adap)
> +
> +static u32 em_i2c_func(struct i2c_adapter *adap)
> +{
> + return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
> +}
Have you tried SMBUS_QUICK (via 'i2cdetect -q')?
> +
> +static struct i2c_algorithm em_i2c_algo = {
> + .master_xfer = em_i2c_xfer,
> + .smbus_xfer = NULL,
No need to specify the NULL.
> + .functionality = em_i2c_func,
> +};
> +
> +static int em_i2c_wait_for_event(struct em_i2c_device *i2c_dev, u16 *status)
> +{
> + int interrupted;
> +
> + do {
> + interrupted = wait_event_interruptible_timeout(
> + i2c_dev->i2c_wait, i2c_dev->pending,
> + i2c_dev->adap.timeout);
Have you tested signals extensively? It can be done right, yet it is
complex. Most drivers decide to skip the interruptible.
> +
> + if (i2c_dev->pending) {
> + spin_lock_irq(&i2c_dev->irq_lock);
> + i2c_dev->pending = 0;
> + spin_unlock_irq(&i2c_dev->irq_lock);
> + *status = readl(i2c_dev->membase + I2C_OFS_IICSE0);
> + return 0;
> + }
> +
> + } while (interrupted);
> +
> + *status = 0;
> +
> + return -ETIMEDOUT;
> +}
> +
> +static int em_i2c_stop(struct em_i2c_device *i2c_dev)
> +{
> + u16 status;
> +
> + /* Send Stop condition */
> + writel((readl(i2c_dev->membase + I2C_OFS_IICC0) | I2C_BIT_SPT0 |
> + I2C_BIT_SPIE0), i2c_dev->membase + I2C_OFS_IICC0);
I'd think a em_set_bit() function would make the code more readable.
> +
> + /* Wait for stop condition */
> + em_i2c_wait_for_event(i2c_dev, &status);
> + /* FIXME - check status? */
What about the FIXME?
> +
> + if ((readl(i2c_dev->membase + I2C_OFS_IICSE0) & I2C_BIT_SPD0) != 0)
!= 0 is superfluous, same for == 1 later.
> + return 0;
> +
> + return -EBUSY;
> +}
> +
> + /* Send / receive data */
> + do {
> + /* Arbitration, Extension mode or Slave mode are errors*/
> + if (status & (I2C_BIT_EXC0 | I2C_BIT_COI0 | I2C_BIT_ALD0)
> + || !(status & I2C_BIT_MSTS0))
> + goto out_reset;
> +
> + if (!(status & I2C_BIT_TRC0)) { /* Read transaction */
> +
> + /* msg->flags is Write type */
> + if (!(msg->flags & I2C_M_RD))
> + goto out_reset;
> +
> + if (count == msg->len)
> + break;
> +
> + msg->buf[count++] =
> + readl(i2c_dev->membase + I2C_OFS_IIC0);
> +
Re: [PATCH 03/17] mmc: mmci: Mask IRQs for all variants during runtime suspend
On 18 February 2014 17:46, Russell King - ARM Linux wrote: > On Tue, Feb 18, 2014 at 05:36:52PM +0100, Ulf Hansson wrote: >> If we add SDIO irq support to mmci in future; parts of that >> implementation includes a re-route of DAT1 to a GPIO irq when entering >> runtime suspend state. The mmci HW will in runtime suspend state, not >> be responsible for handling irqs, which is the same as of today. > > Note that the "irq thread" in sdio_irq is scheduled for destruction - > it's buggy when the system is under high load and a driver claims a > SDIO irq. Sched people hate it too... We kind of realized that too, in the early days for ux500. At that point we converted it to a use a dedicated work queue, which improved the behaviour, don't remember the details why, but maybe I should collect the patches and send them out. :-) Finally we moved to use a separate GPIO irq as a dedicated SDIO irq line for our cw1200 wlan chip, mostly due to gain a bit of performance. That happened to simplified the mmci part as well, which was welcome. > > -- > FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up. Estimation > in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad. > Estimate before purchase was "up to 13.2Mbit". -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/17] mmc: mmci: Mask IRQs for all variants during runtime suspend
On Tue, Feb 18, 2014 at 05:36:52PM +0100, Ulf Hansson wrote: > If we add SDIO irq support to mmci in future; parts of that > implementation includes a re-route of DAT1 to a GPIO irq when entering > runtime suspend state. The mmci HW will in runtime suspend state, not > be responsible for handling irqs, which is the same as of today. Note that the "irq thread" in sdio_irq is scheduled for destruction - it's buggy when the system is under high load and a driver claims a SDIO irq. Sched people hate it too... -- FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up. Estimation in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad. Estimate before purchase was "up to 13.2Mbit". -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/17] mmc: mmci: Mask IRQs for all variants during runtime suspend
On 18 February 2014 17:05, Russell King - ARM Linux wrote: > On Tue, Feb 04, 2014 at 04:58:44PM +0100, Ulf Hansson wrote: >> In runtime suspended state, we are not expecting IRQs and thus we can >> safely mask them, not only for pwrreg_nopower variants but for all. >> >> Obviously we then also need to make sure we restore the IRQ mask while >> becoming runtime resumed. > > So, what happens when this patch is applied, and a SDIO card is attached > which expects to receive interrupts at any moment? Currently, no variant implements SDIO irq. The SDIO irq polling mode from the sdio core will still be functional, as of today. So, this patch will not break SDIO. > > Given that I've run into this during the last week with a SDHCI controller, > I'm not that thrilled with other interfaces doing the same broken thing. If we add SDIO irq support to mmci in future; parts of that implementation includes a re-route of DAT1 to a GPIO irq when entering runtime suspend state. The mmci HW will in runtime suspend state, not be responsible for handling irqs, which is the same as of today. Kind regards Ulf Hansson > > -- > FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up. Estimation > in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad. > Estimate before purchase was "up to 13.2Mbit". -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/17] mmc: mmci: Mask IRQs for all variants during runtime suspend
On Tue, Feb 04, 2014 at 04:58:44PM +0100, Ulf Hansson wrote: > In runtime suspended state, we are not expecting IRQs and thus we can > safely mask them, not only for pwrreg_nopower variants but for all. > > Obviously we then also need to make sure we restore the IRQ mask while > becoming runtime resumed. So, what happens when this patch is applied, and a SDIO card is attached which expects to receive interrupts at any moment? Given that I've run into this during the last week with a SDHCI controller, I'm not that thrilled with other interfaces doing the same broken thing. -- FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up. Estimation in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad. Estimate before purchase was "up to 13.2Mbit". -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 11/17] i2c: nomadik: Convert to devm functions
On 18 February 2014 12:01, Wolfram Sang wrote: > On Mon, Feb 17, 2014 at 04:20:18PM +0100, Ulf Hansson wrote: >> Use devm_* functions to simplify code and error handling. >> >> Cc: Alessandro Rubini >> Cc: Linus Walleij >> Cc: Wolfram Sang >> Signed-off-by: Ulf Hansson > > What has changed in V3? I didn't get the cover letter. I added the IS_ERR, check as you suggested. That caused a rebase of the other patches as well. Sorry if it was unclear. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/5] mv64xxx updates
Hi, On Thu, Feb 13, 2014 at 09:51:00PM +0100, Thomas Petazzoni wrote: > Dear Wolfram Sang, > > On Thu, 13 Feb 2014 21:36:28 +0100, Wolfram Sang wrote: > > So, this is a series I came up with trying to fix the issue found by Kevin. > > Patches 1+2 are hopefully fixing the bug (in theory, I don't have the HW). > > Patches 3-5 are RFC, and if patch 3 actually works (see the CHECKME), then > > 4+5 > > are further cleanup possibilities. And there is still more potential, I > > mainly > > wanted to give some inspiration and awareness that the driver could need > > some > > more love. Please test at least 1+2, comments to 3-5 very welcome. > > > > Sorry for the delay, I got distracted by an NMI. > > I'm adding Maxime Ripard in Cc, since he is the maintainer of the > Allwinner platform, which also uses this I2C driver. So I guess he > would like to be notified of such changes in order to test that the > driver still works for him. I just gave a try to these patches, on my A31 board with the extra patches sent previously. Tested-by: Maxime Ripard Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com signature.asc Description: Digital signature
Re: [PATCH V3 11/17] i2c: nomadik: Convert to devm functions
On Mon, Feb 17, 2014 at 04:20:18PM +0100, Ulf Hansson wrote: > Use devm_* functions to simplify code and error handling. > > Cc: Alessandro Rubini > Cc: Linus Walleij > Cc: Wolfram Sang > Signed-off-by: Ulf Hansson What has changed in V3? I didn't get the cover letter. signature.asc Description: Digital signature
