Re: [PATCH v3 3/5] i2c: i2c-mpc: make I2C bus speed configurable
Wolfram Sang wrote: > Ping. Somebody wants to send a patch? I already have a patch that fixes it, I just want confirmation that I'm right first. -- Timur Tabi Linux kernel developer at Freescale -- 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 3/5] i2c: i2c-mpc: make I2C bus speed configurable
Wolfgang,
I know it's been 3 1/2 years since you wrote this code, but I think I
found a bug.
On Tue, Apr 7, 2009 at 3:20 AM, Wolfgang Grandegger wrote:
> This patch makes the I2C bus speed configurable by using the I2C node
> property "clock-frequency". If the property is not defined, the old
> fixed clock settings will be used for backward comptibility.
>
> The generic I2C clock properties, especially the CPU-specific source
> clock pre-scaler are defined via the OF match table:
>
> static const struct of_device_id mpc_i2c_of_match[] = {
> ...
> {.compatible = "fsl,mpc8543-i2c",
> .data = &(struct fsl_i2c_match_data) {
> .setclock = mpc_i2c_setclock_8xxx,
> .prescaler = 2,
> },
> },
>
> The "data" field defines the relevant I2C setclock function and the
> relevant pre-scaler for the I2C source clock frequency.
>
> It uses arch-specific tables and functions to determine resonable
> Freqency Divider Register (fdr) values for MPC83xx, MPC85xx, MPC86xx,
> MPC5200 and MPC5200B.
>
> The i2c->flags field and the corresponding FSL_I2C_DEV_* definitions
> have been removed as they are obsolete.
>
> Signed-off-by: Wolfgang Grandegger
...
> +u32 mpc_i2c_get_sec_cfg_8xxx(void)
> +{
> + struct device_node *node = NULL;
> + u32 __iomem *reg;
> + u32 val = 0;
> +
> + node = of_find_node_by_name(NULL, "global-utilities");
> + if (node) {
> + const u32 *prop = of_get_property(node, "reg", NULL);
> + if (prop) {
> + /*
> +* Map and check POR Device Status Register 2
> +* (PORDEVSR2) at 0xE0014
> +*/
> + reg = ioremap(get_immrbase() + *prop + 0x14, 0x4);
> + if (!reg)
> + printk(KERN_ERR
> + "Error: couldn't map PORDEVSR2\n");
> + else
> + val = in_be32(reg) & 0x0080; /* sec-cfg */
I'm looking at the MPC8544 reference manual, and PORDEVSR2[SEC_CFG] is
in position 26, which means that this line should be "& 0x20", not "&
0x80".
Can you check this for me and let me know if I'm right?
> + iounmap(reg);
> + }
> + }
> + if (node)
> + of_node_put(node);
> +
> + return val;
> +}
--
Timur Tabi
Linux kernel developer at Freescale
--
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 V2 06/11] ASoC: fsl_ssi: convert to use devm_clk_get
Richard Zhao wrote: > Signed-off-by: Richard Zhao > --- It's a simple change that only affects i.MX, so ... Acked-by: Timur Tabi -- Timur Tabi Linux kernel developer at Freescale -- 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] i2c/busses: (mpc) Add support for SMBUS_READ_BLOCK_DATA
Guenter Roeck wrote: > I simply don't have time for this, and spent way too much time on it > already. Jean, please drop this patch. If someone else needs it, he or > she can revive and re-submit it. If folks can complain about the patch > w/o understanding how SMBus block reads work, but are not willing to > test it, something is wrong anyway. I don't see York's posts as a complaint at all, just someone with a vested interest in this code trying to understand the patch. I definitely appreciate the effort you have put into it already, and I'm disappointed that you think that this is now a waste of your time. -- Timur Tabi Linux kernel developer at Freescale -- 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] I2C: Add support for 64bit system.
On Tue, Mar 15, 2011 at 10:44 AM, Kumar Gala wrote: > > On Dec 20, 2010, at 8:59 AM, Ben Dooks wrote: > >> On Mon, Dec 20, 2010 at 03:37:34PM +0800, Xulei wrote: >>> Currently I2C_MPC supports 32bit system only, then this >>> modification makes it support 32bit and 64bit system both. >>> >>> Signed-off-by: Xulei >> >> This been build or run tested? > > Yes, Any issues with me applying this via the powerpc.git tree? I'm concerned about the fact that we have to have two defines to declare code that is 32-bit and 64-bit clean. Technically speaking, all drivers should work in both environments. It seems silly to have "PPC32 || PPC64" for everything. Isn't there a generic "PPC" config option that covers this? -- Timur Tabi Linux kernel developer at Freescale -- 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] i2c-mpc: do not allow interruptions when waiting for I2C to complete
Jean Delvare wrote: > To the best of my knowledge i2c-mpc falls into the "embedded platforms" > category and is thus under Ben's responsibility. I don't see his Signed-off-by on any patch to i2c-mpc.c for the past three years (at least). I do see yours, however. You have directly applied similar patches in the recent past, so of course I will assume that you would apply this one. -- Timur Tabi Linux kernel developer at Freescale -- 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] i2c-mpc: do not allow interruptions when waiting for I2C to complete
Jean Delvare wrote: > No, that's something for either Ben Dooks (Cc'd) or the powerpc tree. This patch has nothing to do with ARM, so Kumar will pick it up, if you ACK it. -- Timur Tabi Linux kernel developer at Freescale -- 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] i2c-mpc: do not allow interruptions when waiting for I2C to complete
On Fri, Feb 6, 2009 at 8:00 AM, Timur Tabi wrote: > The i2c_wait() function is using wait_event_interruptible_timeout() to wait > for > the I2C controller to signal that it has completed an I2C bus operation. If > the process that causes the I2C operation terminated abruptly, the wait will > be interrupted, returning an error. It is better to let the I2C operation > finished before the process exits. > > It is safe to use wait_event_timeout() instead, because the timeout will allow > the process to exit if the I2C bus hangs. It's also better to allow the > I2C operation to finish, because unacknowledged I2C operations can cause the > I2C bus to hang. > > Signed-off-by: Timur Tabi Jean, Could you pick up this patch for 2.6.30? -- Timur Tabi Linux kernel developer at Freescale -- 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] i2c-mpc: do not allow interruptions when waiting for I2C to complete
The i2c_wait() function is using wait_event_interruptible_timeout() to wait for
the I2C controller to signal that it has completed an I2C bus operation. If
the process that causes the I2C operation terminated abruptly, the wait will
be interrupted, returning an error. It is better to let the I2C operation
finished before the process exits.
It is safe to use wait_event_timeout() instead, because the timeout will allow
the process to exit if the I2C bus hangs. It's also better to allow the
I2C operation to finish, because unacknowledged I2C operations can cause the
I2C bus to hang.
Signed-off-by: Timur Tabi
---
A similar change should probably be done to i2c-cpm.c, and maybe all other
I2C drivers. Not many use wait_event_interruptible_timeout().
drivers/i2c/busses/i2c-mpc.c |9 +++--
1 files changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index a9a45fc..c0ace48 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -70,7 +70,7 @@ static irqreturn_t mpc_i2c_isr(int irq, void *dev_id)
/* Read again to allow register to stabilise */
i2c->interrupt = readb(i2c->base + MPC_I2C_SR);
writeb(0, i2c->base + MPC_I2C_SR);
- wake_up_interruptible(&i2c->queue);
+ wake_up(&i2c->queue);
}
return IRQ_HANDLED;
}
@@ -115,13 +115,10 @@ static int i2c_wait(struct mpc_i2c *i2c, unsigned
timeout, int writing)
writeb(0, i2c->base + MPC_I2C_SR);
} else {
/* Interrupt mode */
- result = wait_event_interruptible_timeout(i2c->queue,
+ result = wait_event_timeout(i2c->queue,
(i2c->interrupt & CSR_MIF), timeout * HZ);
- if (unlikely(result < 0)) {
- pr_debug("I2C: wait interrupted\n");
- writeccr(i2c, 0);
- } else if (unlikely(!(i2c->interrupt & CSR_MIF))) {
+ if (unlikely(!(i2c->interrupt & CSR_MIF))) {
pr_debug("I2C: wait timeout\n");
writeccr(i2c, 0);
result = -ETIMEDOUT;
--
1.5.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: Calling wait_event_interruptible_timeout() in I2C wait functions
On Thu, Feb 5, 2009 at 5:51 AM, Mark Brown wrote: > This is exactly the problem for users that caused Timur to run into this > - further up the stack we're trying to do cleanup that involves writing > via I2C but the I2C writes error out due to the signal. Well, there's not much discussion on this issue, so I'm going to make the change to an uninterruptible wait and see if it fixes my problem. If so, I'll post a patch for i2c-mpc.c and see how far I get. -- Timur Tabi Linux kernel developer at Freescale -- 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
Calling wait_event_interruptible_timeout() in I2C wait functions
In i2c-mpc.c, the i2c_wait() function has this:
} else {
/* Interrupt mode */
result = wait_event_interruptible_timeout(i2c->queue,
(i2c->interrupt & CSR_MIF), timeout * HZ);
if (unlikely(result < 0)) {
pr_debug("I2C: wait interrupted\n");
writeccr(i2c, 0);
} else if (unlikely(!(i2c->interrupt & CSR_MIF))) {
That is, the driver calls wait_event_interruptible_timeout() to wait for
a response from the I2C controller after a read or write operation.
However, it appears that this is not common behavior for I2C driver. In
fact, only these six drivers ever call wait_event_interruptible_timeout():
i2c-cpm.c
i2c-ibm_iic.c
i2c-mpc.c
i2c-taos-evm.c
i2c-iop3xx.c
i2c-mv64xxx.c
Although one would think that calling wait_event_interruptible_timeout()
is a good idea, it fails in one situation: when the abrupt termination
of a process causes an I2C operation to occur. That is, you press ^C in
your application, and the driver issues a final I2C operation to shut
down the device. In this situation, wait_event_interruptible_timeout()
returns -ERESTARTSYS, which is then passed up through
i2c_smbus_write_byte_data().
So my question is, is i2c-mpc.c wrong in using
wait_event_interruptible_timeout()?
--
Timur Tabi
Linux kernel developer at Freescale
--
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: Dynamically-allocated i2c_device_id vs MODULE_DEVICE_TABLE
On Sat, Jan 24, 2009 at 10:50 AM, Ben Dooks wrote: > you seem to have managed to short-circuit part of the device > creation process. You do not need to pass the data via the device > driver, you should pass it when creating the device. Yes, I think my code is broken. > If you look at i2c_board_info which can be passed into i2c_new_device > or similar functions, there is a platform_data field you can fill out > and this is passed in to your probe routine in the i2c device. I don't think this will work for me. I'm running this on a PowerPC system, and we use a device tree to represent the I2C devices on the various I2C buses. My driver does not call i2c_new_device. This is done for me in of_register_i2c_devices(). As soon as my driver calls i2c_add_driver(), my I2C probe function will be called, once for each I2C device defined in the device tree. So I don't think I can update the i2c_board_info structure. > > -- > Ben (b...@fluff.org, http://www.fluff.org/) > > 'a smiley only costs 4 bytes' > -- > 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 > -- Timur Tabi Linux kernel developer at Freescale -- 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: Dynamically-allocated i2c_device_id vs MODULE_DEVICE_TABLE
Mark Brown wrote: Really what you're having to do here is fairly nasty and there's no good reason for it to be supported - if something is being registered dynamically and passing instantiation specific data around it really should be the device and not the driver. What we need is a way for the codec driver to do its stuff without needing a socdev. I think I see your point, though. I'm calling i2c_add_driver, which technically could result in my i2c_probe function being called multiple times, if there are multiple matches in the device tree. I'll have to check that out on Monday. > The fact that attempting to do this looks bad is a win on the part of the I2C core. For now an idiomatic solution for ASoC drivers is to have a single static variable that you use to get the socdev through. Well, that's what I do today. I was hoping to avoid that, but if I'm right about i2c_add_driver, then this trick doesn't really work either. -- Timur Tabi Linux Kernel Developer @ Freescale -- 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
Dynamically-allocated i2c_device_id vs MODULE_DEVICE_TABLE
I currently have this in my code:
static const struct i2c_device_id cs4270_id[] = {
{"cs4270", 0},
{}
};
MODULE_DEVICE_TABLE(i2c, cs4270_id);
static struct i2c_driver cs4270_i2c_driver = {
.driver = {
.name = "cs4270",
.owner = THIS_MODULE,
},
.id_table = cs4270_id,
.probe = cs4270_i2c_probe,
.remove = cs4270_i2c_remove,
};
ret = i2c_add_driver(&cs4270_i2c_driver);
I would like to use the i2c_device_id.driver_data variable to pass private data
to my cs4270_i2c_probe() function. So it will look like this:
socdev = kmalloc(...);
c24270_id.driver_data = socdev;
i2c_add_driver(&cs4270_i2c_driver);
And then in my cs4270_i2c_probe(), I would do this:
static int cs4270_i2c_probe(struct i2c_client *i2c_client,
const struct i2c_device_id *id)
{
struct X *socdev = (struct X *) id->driver_data.
The problem I'm having is the MODULE_DEVICE_TABLE. What I really should be
doing is this:
socdev = kmalloc(...);
struct i2c_driver *cs4270_i2c_driver = kmalloc(...);
struct i2c_device_id *cs4270_id = kmalloc(...);
cs4270_i2c_driver->id_table = cs4270_id;
c24270_id->driver_data = socdev;
i2c_add_driver(cs4270_i2c_driver);
But if I do this, then I can't use MODULE_DEVICE_TABLE. So I have two
questions:
1) What happens if I don't use MODULE_DEVICE_TABLE to identify my I2C ID table?
2) Is there a way to mimic the behavior of MODULE_DEVICE_TABLE on a
dynamically-created ID table?
--
Timur Tabi
Linux kernel developer at Freescale
--
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
