Re: [PATCH v4 2/2] I2C: mediatek: Add driver for MediaTek I2C controller
Hi,
On Wed, 2015-01-21 at 16:31 +0100, Uwe Kleine-König wrote:
> Hello,
>
> On Wed, Jan 21, 2015 at 08:49:40PM +0800, Yingjoe Chen wrote:
> > On Wed, 2015-01-21 at 09:15 +0100, Uwe Kleine-König wrote:
<...>
> > > > > > > +static int mtk_i2c_remove(struct platform_device *pdev)
> > > > > > > +{
> > > > > > > + struct mtk_i2c *i2c = platform_get_drvdata(pdev);
> > > > > > > +
> > > > > > > + i2c_del_adapter(>adap);
> > > > > > > + free_i2c_dma_bufs(i2c);
> > > > > > > + platform_set_drvdata(pdev, NULL);
> > > > > > > +
> > > > > > Here you need to make sure that no irq is running when
> > > > > > i2c_del_adapter
> > > > > > is called.
> > > > > OK, add check here
> > > >
> > > > I thought after i2c_del_adapter() is complete, all i2c_transfer for this
> > > > adapter is completed. If this is true, then i2c clock is already off and
> > > > we won't have any on-going transfer/pending irq.
> > > Consider that there is an ongoing transaction and before it completes
> > > the adapter-device is unbound from the driver. Then i2c_del_adapter is
> > > called which frees the resources managed by the core, then the device's
> > > completion irq triggers and the freed adapter is used which probably
> > > results in an oops.
> >
> > Not sure if I missed anything. i2c_transfer() is a synchronize call. If
> > we fixed timeout issue you mentioned in mtk_i2c_transfer(), it will turn
> > off clock before it return, which disable any transaction and clear all
> > pending irq.
> There is no synchronization to prevent unbinding the i2c-bus device
> while there is a i2c transfer on the wire. i2c_del_adapter only takes
> i2c-core.c's _lock while i2c_transfer takes >bus_lock.
> If you want to test for it: do something like that:
>
> while true; do dd if=/sys/bus/i2c/.../eeprom of=/dev/null; done
>
> and while this is running do:
>
> cd /sys/bus/platform/drivers/mt-i2c
> while true; do
> echo 1100d000.i2c > unbind;
> sleep 1;
> echo 1100d000.i2c > bind;
> sleep 1;
> done
>
> > Your scenario can only happens when one thread is still running in
> > i2c_transfer/algo->master_xfer and the other thread is trying to remove
> > the device. If that happened, then every device data access in
> > mtk_i2c_transfer might cause oops. I looked at some i2c drivers and
> > can't find any checking for this case, I can't find anything prevent i2c
> > device removal before pending i2c_transfer complete either. Would you
> > give me an example?
> I just noticed that even "my" driver is affected. If the above recipe
> makes your driver barf there is something to fix, if not ... hmm, then
> maybe there is more synchronization than I'm aware of or my recipe is
> wrong.
>
> At least another driver author believed me:
> http://thread.gmane.org/gmane.linux.drivers.i2c/21531/focus=21662
If there is no synchronization mechanism then every driver is affected.
We should add the check in the core instead of fixing it in every
driver. We could take the bus_lock in i2c_del_adapter() and check if the
adapter is removed in __i2c_transfer(), this should fix the problem.
Wolfram, what do you think about this? Is there anything we are missing?
Joe.C
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 2/2] I2C: mediatek: Add driver for MediaTek I2C controller
Hello,
On Wed, Jan 21, 2015 at 08:49:40PM +0800, Yingjoe Chen wrote:
> On Wed, 2015-01-21 at 09:15 +0100, Uwe Kleine-König wrote:
> > On Wed, Jan 21, 2015 at 02:30:09PM +0800, Yingjoe Chen wrote:
> > > On Wed, 2015-01-21 at 11:13 +0800, Eddie Huang wrote:
> > > <...>
> > > > > > + ret = -EINVAL;
> > > > > > + goto err_exit;
> > > > > > + }
> > > > > > +
> > > > > > + if (msgs->buf == NULL) {
> > > > > > + dev_dbg(i2c->dev, " data buffer is NULL.\n");
> > > > > > + ret = -EINVAL;
> > > > > > + goto err_exit;
> > > > > > + }
> > > > > > +
> > > > > > + i2c->addr = msgs->addr;
> > > > > > + i2c->msg_len = msgs->len;
> > > > > > + i2c->msg_buf = msgs->buf;
> > > > > > +
> > > > > > + if (msgs->flags & I2C_M_RD)
> > > > > > + i2c->op = I2C_MASTER_RD;
> > > > > > + else
> > > > > > + i2c->op = I2C_MASTER_WR;
> > > > > > +
> > > > > > + /* combined two messages into one transaction */
> > > > > > + if (num > 1) {
> > > > > > + i2c->msg_aux_len = (msgs + 1)->len;
> > > > > > + i2c->op = I2C_MASTER_WRRD;
> > > > > > + }
> > > > > This means "write then read", right? You should check here that the
> > > > > first message is really a write and the 2nd a read then.
> > > > > Can this happen at all with the quirks defined below (.max_num_msgs =
> > > > > 1)?
> > > > Yes, mean write then read. Indeed, add check is better.
> > > > If msg number is 1, means normal write or read, not "write then read".
> > >
> > > The quirks will increase the message count and check 'write then read'
> > > for us. We don't have to add check here.
> > I have to admit I don't know that quirks stuff, so it's well possible
> > that I'm wrong here.
> >
> > > > > > +static int mtk_i2c_remove(struct platform_device *pdev)
> > > > > > +{
> > > > > > + struct mtk_i2c *i2c = platform_get_drvdata(pdev);
> > > > > > +
> > > > > > + i2c_del_adapter(>adap);
> > > > > > + free_i2c_dma_bufs(i2c);
> > > > > > + platform_set_drvdata(pdev, NULL);
> > > > > > +
> > > > > Here you need to make sure that no irq is running when i2c_del_adapter
> > > > > is called.
> > > > OK, add check here
> > >
> > > I thought after i2c_del_adapter() is complete, all i2c_transfer for this
> > > adapter is completed. If this is true, then i2c clock is already off and
> > > we won't have any on-going transfer/pending irq.
> > Consider that there is an ongoing transaction and before it completes
> > the adapter-device is unbound from the driver. Then i2c_del_adapter is
> > called which frees the resources managed by the core, then the device's
> > completion irq triggers and the freed adapter is used which probably
> > results in an oops.
>
> Not sure if I missed anything. i2c_transfer() is a synchronize call. If
> we fixed timeout issue you mentioned in mtk_i2c_transfer(), it will turn
> off clock before it return, which disable any transaction and clear all
> pending irq.
There is no synchronization to prevent unbinding the i2c-bus device
while there is a i2c transfer on the wire. i2c_del_adapter only takes
i2c-core.c's _lock while i2c_transfer takes >bus_lock.
If you want to test for it: do something like that:
while true; do dd if=/sys/bus/i2c/.../eeprom of=/dev/null; done
and while this is running do:
cd /sys/bus/platform/drivers/mt-i2c
while true; do
echo 1100d000.i2c > unbind;
sleep 1;
echo 1100d000.i2c > bind;
sleep 1;
done
> Your scenario can only happens when one thread is still running in
> i2c_transfer/algo->master_xfer and the other thread is trying to remove
> the device. If that happened, then every device data access in
> mtk_i2c_transfer might cause oops. I looked at some i2c drivers and
> can't find any checking for this case, I can't find anything prevent i2c
> device removal before pending i2c_transfer complete either. Would you
> give me an example?
I just noticed that even "my" driver is affected. If the above recipe
makes your driver barf there is something to fix, if not ... hmm, then
maybe there is more synchronization than I'm aware of or my recipe is
wrong.
At least another driver author believed me:
http://thread.gmane.org/gmane.linux.drivers.i2c/21531/focus=21662
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 2/2] I2C: mediatek: Add driver for MediaTek I2C controller
On Wed, 2015-01-21 at 09:15 +0100, Uwe Kleine-König wrote:
> Hello,
>
> On Wed, Jan 21, 2015 at 02:30:09PM +0800, Yingjoe Chen wrote:
> > On Wed, 2015-01-21 at 11:13 +0800, Eddie Huang wrote:
> > <...>
> > > > > + ret = -EINVAL;
> > > > > + goto err_exit;
> > > > > + }
> > > > > +
> > > > > + if (msgs->buf == NULL) {
> > > > > + dev_dbg(i2c->dev, " data buffer is NULL.\n");
> > > > > + ret = -EINVAL;
> > > > > + goto err_exit;
> > > > > + }
> > > > > +
> > > > > + i2c->addr = msgs->addr;
> > > > > + i2c->msg_len = msgs->len;
> > > > > + i2c->msg_buf = msgs->buf;
> > > > > +
> > > > > + if (msgs->flags & I2C_M_RD)
> > > > > + i2c->op = I2C_MASTER_RD;
> > > > > + else
> > > > > + i2c->op = I2C_MASTER_WR;
> > > > > +
> > > > > + /* combined two messages into one transaction */
> > > > > + if (num > 1) {
> > > > > + i2c->msg_aux_len = (msgs + 1)->len;
> > > > > + i2c->op = I2C_MASTER_WRRD;
> > > > > + }
> > > > This means "write then read", right? You should check here that the
> > > > first message is really a write and the 2nd a read then.
> > > > Can this happen at all with the quirks defined below (.max_num_msgs =
> > > > 1)?
> > > Yes, mean write then read. Indeed, add check is better.
> > > If msg number is 1, means normal write or read, not "write then read".
> >
> > The quirks will increase the message count and check 'write then read'
> > for us. We don't have to add check here.
> I have to admit I don't know that quirks stuff, so it's well possible
> that I'm wrong here.
>
> > > > > +static int mtk_i2c_remove(struct platform_device *pdev)
> > > > > +{
> > > > > + struct mtk_i2c *i2c = platform_get_drvdata(pdev);
> > > > > +
> > > > > + i2c_del_adapter(>adap);
> > > > > + free_i2c_dma_bufs(i2c);
> > > > > + platform_set_drvdata(pdev, NULL);
> > > > > +
> > > > Here you need to make sure that no irq is running when i2c_del_adapter
> > > > is called.
> > > OK, add check here
> >
> > I thought after i2c_del_adapter() is complete, all i2c_transfer for this
> > adapter is completed. If this is true, then i2c clock is already off and
> > we won't have any on-going transfer/pending irq.
> Consider that there is an ongoing transaction and before it completes
> the adapter-device is unbound from the driver. Then i2c_del_adapter is
> called which frees the resources managed by the core, then the device's
> completion irq triggers and the freed adapter is used which probably
> results in an oops.
Not sure if I missed anything. i2c_transfer() is a synchronize call. If
we fixed timeout issue you mentioned in mtk_i2c_transfer(), it will turn
off clock before it return, which disable any transaction and clear all
pending irq.
Your scenario can only happens when one thread is still running in
i2c_transfer/algo->master_xfer and the other thread is trying to remove
the device. If that happened, then every device data access in
mtk_i2c_transfer might cause oops. I looked at some i2c drivers and
can't find any checking for this case, I can't find anything prevent i2c
device removal before pending i2c_transfer complete either. Would you
give me an example?
Joe.C
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 2/2] I2C: mediatek: Add driver for MediaTek I2C controller
Hi Uwe, On Wed, 2015-01-21 at 09:20 +0100, Uwe Kleine-König wrote: > Hello, > > On Wed, Jan 21, 2015 at 11:13:24AM +0800, Eddie Huang wrote: > > > > + /* set when doing the transfer */ > > > > + u16 irq_stat; /* interrupt status */ > > > > + unsigned int speed_hz; /* The speed in transfer */ > > > > + bool trans_stop;/* i2c transfer stop */ > > > > + enum mtk_trans_op op; > > > > + u16 msg_len; > > > > + u8 *msg_buf;/* pointer to msg data */ > > > > + u16 msg_aux_len;/* WRRD mode to set AUX_LEN > > > > register*/ > > > > + u16 addr; /* 7bit slave address, without read/write bit */ > > > Wouldn't it be easier to maintain a pointer to the message to be > > > transferred? > > I think use mtk_i2c pointer is more flexible than maintain a pointer to > > message. > Not sure you understood what I intended to suggest. My idea was to drop > > u16 msg_len; > u8 *msg_buf; > u16 msg_aux_len; // maybe > u16 addr; > > from struct mtk_i2c and add a struct i2c_msg *msg instead. Up to you to > decide. > Because this driver pass mtk_i2c pointer between functions, it's flexible to use any member of struct mtk_i2c. The good thing is avoid passing one more struct i2c_msg parameter between functions, or another struct copy. The bad thing is make struct mtk_i2c larger. I prefer to keep as it is now. Best Regards Eddie -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 2/2] I2C: mediatek: Add driver for MediaTek I2C controller
Hello, On Wed, Jan 21, 2015 at 11:13:24AM +0800, Eddie Huang wrote: > > > + /* set when doing the transfer */ > > > + u16 irq_stat; /* interrupt status */ > > > + unsigned int speed_hz; /* The speed in transfer */ > > > + bool trans_stop;/* i2c transfer stop */ > > > + enum mtk_trans_op op; > > > + u16 msg_len; > > > + u8 *msg_buf;/* pointer to msg data */ > > > + u16 msg_aux_len;/* WRRD mode to set AUX_LEN register*/ > > > + u16 addr; /* 7bit slave address, without read/write bit */ > > Wouldn't it be easier to maintain a pointer to the message to be > > transferred? > I think use mtk_i2c pointer is more flexible than maintain a pointer to > message. Not sure you understood what I intended to suggest. My idea was to drop u16 msg_len; u8 *msg_buf; u16 msg_aux_len; // maybe u16 addr; from struct mtk_i2c and add a struct i2c_msg *msg instead. Up to you to decide. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | http://www.pengutronix.de/ | -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 2/2] I2C: mediatek: Add driver for MediaTek I2C controller
Hello,
On Wed, Jan 21, 2015 at 02:30:09PM +0800, Yingjoe Chen wrote:
> On Wed, 2015-01-21 at 11:13 +0800, Eddie Huang wrote:
> <...>
> > > > + ret = -EINVAL;
> > > > + goto err_exit;
> > > > + }
> > > > +
> > > > + if (msgs->buf == NULL) {
> > > > + dev_dbg(i2c->dev, " data buffer is NULL.\n");
> > > > + ret = -EINVAL;
> > > > + goto err_exit;
> > > > + }
> > > > +
> > > > + i2c->addr = msgs->addr;
> > > > + i2c->msg_len = msgs->len;
> > > > + i2c->msg_buf = msgs->buf;
> > > > +
> > > > + if (msgs->flags & I2C_M_RD)
> > > > + i2c->op = I2C_MASTER_RD;
> > > > + else
> > > > + i2c->op = I2C_MASTER_WR;
> > > > +
> > > > + /* combined two messages into one transaction */
> > > > + if (num > 1) {
> > > > + i2c->msg_aux_len = (msgs + 1)->len;
> > > > + i2c->op = I2C_MASTER_WRRD;
> > > > + }
> > > This means "write then read", right? You should check here that the
> > > first message is really a write and the 2nd a read then.
> > > Can this happen at all with the quirks defined below (.max_num_msgs =
> > > 1)?
> > Yes, mean write then read. Indeed, add check is better.
> > If msg number is 1, means normal write or read, not "write then read".
>
> The quirks will increase the message count and check 'write then read'
> for us. We don't have to add check here.
I have to admit I don't know that quirks stuff, so it's well possible
that I'm wrong here.
> > > > +static int mtk_i2c_remove(struct platform_device *pdev)
> > > > +{
> > > > + struct mtk_i2c *i2c = platform_get_drvdata(pdev);
> > > > +
> > > > + i2c_del_adapter(>adap);
> > > > + free_i2c_dma_bufs(i2c);
> > > > + platform_set_drvdata(pdev, NULL);
> > > > +
> > > Here you need to make sure that no irq is running when i2c_del_adapter
> > > is called.
> > OK, add check here
>
> I thought after i2c_del_adapter() is complete, all i2c_transfer for this
> adapter is completed. If this is true, then i2c clock is already off and
> we won't have any on-going transfer/pending irq.
Consider that there is an ongoing transaction and before it completes
the adapter-device is unbound from the driver. Then i2c_del_adapter is
called which frees the resources managed by the core, then the device's
completion irq triggers and the freed adapter is used which probably
results in an oops.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 2/2] I2C: mediatek: Add driver for MediaTek I2C controller
Hi,
On Wed, 2015-01-21 at 16:31 +0100, Uwe Kleine-König wrote:
Hello,
On Wed, Jan 21, 2015 at 08:49:40PM +0800, Yingjoe Chen wrote:
On Wed, 2015-01-21 at 09:15 +0100, Uwe Kleine-König wrote:
...
+static int mtk_i2c_remove(struct platform_device *pdev)
+{
+ struct mtk_i2c *i2c = platform_get_drvdata(pdev);
+
+ i2c_del_adapter(i2c-adap);
+ free_i2c_dma_bufs(i2c);
+ platform_set_drvdata(pdev, NULL);
+
Here you need to make sure that no irq is running when
i2c_del_adapter
is called.
OK, add check here
I thought after i2c_del_adapter() is complete, all i2c_transfer for this
adapter is completed. If this is true, then i2c clock is already off and
we won't have any on-going transfer/pending irq.
Consider that there is an ongoing transaction and before it completes
the adapter-device is unbound from the driver. Then i2c_del_adapter is
called which frees the resources managed by the core, then the device's
completion irq triggers and the freed adapter is used which probably
results in an oops.
Not sure if I missed anything. i2c_transfer() is a synchronize call. If
we fixed timeout issue you mentioned in mtk_i2c_transfer(), it will turn
off clock before it return, which disable any transaction and clear all
pending irq.
There is no synchronization to prevent unbinding the i2c-bus device
while there is a i2c transfer on the wire. i2c_del_adapter only takes
i2c-core.c's core_lock while i2c_transfer takes adapter-bus_lock.
If you want to test for it: do something like that:
while true; do dd if=/sys/bus/i2c/.../eeprom of=/dev/null; done
and while this is running do:
cd /sys/bus/platform/drivers/mt-i2c
while true; do
echo 1100d000.i2c unbind;
sleep 1;
echo 1100d000.i2c bind;
sleep 1;
done
Your scenario can only happens when one thread is still running in
i2c_transfer/algo-master_xfer and the other thread is trying to remove
the device. If that happened, then every device data access in
mtk_i2c_transfer might cause oops. I looked at some i2c drivers and
can't find any checking for this case, I can't find anything prevent i2c
device removal before pending i2c_transfer complete either. Would you
give me an example?
I just noticed that even my driver is affected. If the above recipe
makes your driver barf there is something to fix, if not ... hmm, then
maybe there is more synchronization than I'm aware of or my recipe is
wrong.
At least another driver author believed me:
http://thread.gmane.org/gmane.linux.drivers.i2c/21531/focus=21662
If there is no synchronization mechanism then every driver is affected.
We should add the check in the core instead of fixing it in every
driver. We could take the bus_lock in i2c_del_adapter() and check if the
adapter is removed in __i2c_transfer(), this should fix the problem.
Wolfram, what do you think about this? Is there anything we are missing?
Joe.C
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 2/2] I2C: mediatek: Add driver for MediaTek I2C controller
Hello, On Wed, Jan 21, 2015 at 11:13:24AM +0800, Eddie Huang wrote: + /* set when doing the transfer */ + u16 irq_stat; /* interrupt status */ + unsigned int speed_hz; /* The speed in transfer */ + bool trans_stop;/* i2c transfer stop */ + enum mtk_trans_op op; + u16 msg_len; + u8 *msg_buf;/* pointer to msg data */ + u16 msg_aux_len;/* WRRD mode to set AUX_LEN register*/ + u16 addr; /* 7bit slave address, without read/write bit */ Wouldn't it be easier to maintain a pointer to the message to be transferred? I think use mtk_i2c pointer is more flexible than maintain a pointer to message. Not sure you understood what I intended to suggest. My idea was to drop u16 msg_len; u8 *msg_buf; u16 msg_aux_len; // maybe u16 addr; from struct mtk_i2c and add a struct i2c_msg *msg instead. Up to you to decide. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | http://www.pengutronix.de/ | -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 2/2] I2C: mediatek: Add driver for MediaTek I2C controller
Hi Uwe, On Wed, 2015-01-21 at 09:20 +0100, Uwe Kleine-König wrote: Hello, On Wed, Jan 21, 2015 at 11:13:24AM +0800, Eddie Huang wrote: + /* set when doing the transfer */ + u16 irq_stat; /* interrupt status */ + unsigned int speed_hz; /* The speed in transfer */ + bool trans_stop;/* i2c transfer stop */ + enum mtk_trans_op op; + u16 msg_len; + u8 *msg_buf;/* pointer to msg data */ + u16 msg_aux_len;/* WRRD mode to set AUX_LEN register*/ + u16 addr; /* 7bit slave address, without read/write bit */ Wouldn't it be easier to maintain a pointer to the message to be transferred? I think use mtk_i2c pointer is more flexible than maintain a pointer to message. Not sure you understood what I intended to suggest. My idea was to drop u16 msg_len; u8 *msg_buf; u16 msg_aux_len; // maybe u16 addr; from struct mtk_i2c and add a struct i2c_msg *msg instead. Up to you to decide. Because this driver pass mtk_i2c pointer between functions, it's flexible to use any member of struct mtk_i2c. The good thing is avoid passing one more struct i2c_msg parameter between functions, or another struct copy. The bad thing is make struct mtk_i2c larger. I prefer to keep as it is now. Best Regards Eddie -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 2/2] I2C: mediatek: Add driver for MediaTek I2C controller
Hello,
On Wed, Jan 21, 2015 at 02:30:09PM +0800, Yingjoe Chen wrote:
On Wed, 2015-01-21 at 11:13 +0800, Eddie Huang wrote:
...
+ ret = -EINVAL;
+ goto err_exit;
+ }
+
+ if (msgs-buf == NULL) {
+ dev_dbg(i2c-dev, data buffer is NULL.\n);
+ ret = -EINVAL;
+ goto err_exit;
+ }
+
+ i2c-addr = msgs-addr;
+ i2c-msg_len = msgs-len;
+ i2c-msg_buf = msgs-buf;
+
+ if (msgs-flags I2C_M_RD)
+ i2c-op = I2C_MASTER_RD;
+ else
+ i2c-op = I2C_MASTER_WR;
+
+ /* combined two messages into one transaction */
+ if (num 1) {
+ i2c-msg_aux_len = (msgs + 1)-len;
+ i2c-op = I2C_MASTER_WRRD;
+ }
This means write then read, right? You should check here that the
first message is really a write and the 2nd a read then.
Can this happen at all with the quirks defined below (.max_num_msgs =
1)?
Yes, mean write then read. Indeed, add check is better.
If msg number is 1, means normal write or read, not write then read.
The quirks will increase the message count and check 'write then read'
for us. We don't have to add check here.
I have to admit I don't know that quirks stuff, so it's well possible
that I'm wrong here.
+static int mtk_i2c_remove(struct platform_device *pdev)
+{
+ struct mtk_i2c *i2c = platform_get_drvdata(pdev);
+
+ i2c_del_adapter(i2c-adap);
+ free_i2c_dma_bufs(i2c);
+ platform_set_drvdata(pdev, NULL);
+
Here you need to make sure that no irq is running when i2c_del_adapter
is called.
OK, add check here
I thought after i2c_del_adapter() is complete, all i2c_transfer for this
adapter is completed. If this is true, then i2c clock is already off and
we won't have any on-going transfer/pending irq.
Consider that there is an ongoing transaction and before it completes
the adapter-device is unbound from the driver. Then i2c_del_adapter is
called which frees the resources managed by the core, then the device's
completion irq triggers and the freed adapter is used which probably
results in an oops.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 2/2] I2C: mediatek: Add driver for MediaTek I2C controller
On Wed, 2015-01-21 at 09:15 +0100, Uwe Kleine-König wrote:
Hello,
On Wed, Jan 21, 2015 at 02:30:09PM +0800, Yingjoe Chen wrote:
On Wed, 2015-01-21 at 11:13 +0800, Eddie Huang wrote:
...
+ ret = -EINVAL;
+ goto err_exit;
+ }
+
+ if (msgs-buf == NULL) {
+ dev_dbg(i2c-dev, data buffer is NULL.\n);
+ ret = -EINVAL;
+ goto err_exit;
+ }
+
+ i2c-addr = msgs-addr;
+ i2c-msg_len = msgs-len;
+ i2c-msg_buf = msgs-buf;
+
+ if (msgs-flags I2C_M_RD)
+ i2c-op = I2C_MASTER_RD;
+ else
+ i2c-op = I2C_MASTER_WR;
+
+ /* combined two messages into one transaction */
+ if (num 1) {
+ i2c-msg_aux_len = (msgs + 1)-len;
+ i2c-op = I2C_MASTER_WRRD;
+ }
This means write then read, right? You should check here that the
first message is really a write and the 2nd a read then.
Can this happen at all with the quirks defined below (.max_num_msgs =
1)?
Yes, mean write then read. Indeed, add check is better.
If msg number is 1, means normal write or read, not write then read.
The quirks will increase the message count and check 'write then read'
for us. We don't have to add check here.
I have to admit I don't know that quirks stuff, so it's well possible
that I'm wrong here.
+static int mtk_i2c_remove(struct platform_device *pdev)
+{
+ struct mtk_i2c *i2c = platform_get_drvdata(pdev);
+
+ i2c_del_adapter(i2c-adap);
+ free_i2c_dma_bufs(i2c);
+ platform_set_drvdata(pdev, NULL);
+
Here you need to make sure that no irq is running when i2c_del_adapter
is called.
OK, add check here
I thought after i2c_del_adapter() is complete, all i2c_transfer for this
adapter is completed. If this is true, then i2c clock is already off and
we won't have any on-going transfer/pending irq.
Consider that there is an ongoing transaction and before it completes
the adapter-device is unbound from the driver. Then i2c_del_adapter is
called which frees the resources managed by the core, then the device's
completion irq triggers and the freed adapter is used which probably
results in an oops.
Not sure if I missed anything. i2c_transfer() is a synchronize call. If
we fixed timeout issue you mentioned in mtk_i2c_transfer(), it will turn
off clock before it return, which disable any transaction and clear all
pending irq.
Your scenario can only happens when one thread is still running in
i2c_transfer/algo-master_xfer and the other thread is trying to remove
the device. If that happened, then every device data access in
mtk_i2c_transfer might cause oops. I looked at some i2c drivers and
can't find any checking for this case, I can't find anything prevent i2c
device removal before pending i2c_transfer complete either. Would you
give me an example?
Joe.C
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 2/2] I2C: mediatek: Add driver for MediaTek I2C controller
Hello,
On Wed, Jan 21, 2015 at 08:49:40PM +0800, Yingjoe Chen wrote:
On Wed, 2015-01-21 at 09:15 +0100, Uwe Kleine-König wrote:
On Wed, Jan 21, 2015 at 02:30:09PM +0800, Yingjoe Chen wrote:
On Wed, 2015-01-21 at 11:13 +0800, Eddie Huang wrote:
...
+ ret = -EINVAL;
+ goto err_exit;
+ }
+
+ if (msgs-buf == NULL) {
+ dev_dbg(i2c-dev, data buffer is NULL.\n);
+ ret = -EINVAL;
+ goto err_exit;
+ }
+
+ i2c-addr = msgs-addr;
+ i2c-msg_len = msgs-len;
+ i2c-msg_buf = msgs-buf;
+
+ if (msgs-flags I2C_M_RD)
+ i2c-op = I2C_MASTER_RD;
+ else
+ i2c-op = I2C_MASTER_WR;
+
+ /* combined two messages into one transaction */
+ if (num 1) {
+ i2c-msg_aux_len = (msgs + 1)-len;
+ i2c-op = I2C_MASTER_WRRD;
+ }
This means write then read, right? You should check here that the
first message is really a write and the 2nd a read then.
Can this happen at all with the quirks defined below (.max_num_msgs =
1)?
Yes, mean write then read. Indeed, add check is better.
If msg number is 1, means normal write or read, not write then read.
The quirks will increase the message count and check 'write then read'
for us. We don't have to add check here.
I have to admit I don't know that quirks stuff, so it's well possible
that I'm wrong here.
+static int mtk_i2c_remove(struct platform_device *pdev)
+{
+ struct mtk_i2c *i2c = platform_get_drvdata(pdev);
+
+ i2c_del_adapter(i2c-adap);
+ free_i2c_dma_bufs(i2c);
+ platform_set_drvdata(pdev, NULL);
+
Here you need to make sure that no irq is running when i2c_del_adapter
is called.
OK, add check here
I thought after i2c_del_adapter() is complete, all i2c_transfer for this
adapter is completed. If this is true, then i2c clock is already off and
we won't have any on-going transfer/pending irq.
Consider that there is an ongoing transaction and before it completes
the adapter-device is unbound from the driver. Then i2c_del_adapter is
called which frees the resources managed by the core, then the device's
completion irq triggers and the freed adapter is used which probably
results in an oops.
Not sure if I missed anything. i2c_transfer() is a synchronize call. If
we fixed timeout issue you mentioned in mtk_i2c_transfer(), it will turn
off clock before it return, which disable any transaction and clear all
pending irq.
There is no synchronization to prevent unbinding the i2c-bus device
while there is a i2c transfer on the wire. i2c_del_adapter only takes
i2c-core.c's core_lock while i2c_transfer takes adapter-bus_lock.
If you want to test for it: do something like that:
while true; do dd if=/sys/bus/i2c/.../eeprom of=/dev/null; done
and while this is running do:
cd /sys/bus/platform/drivers/mt-i2c
while true; do
echo 1100d000.i2c unbind;
sleep 1;
echo 1100d000.i2c bind;
sleep 1;
done
Your scenario can only happens when one thread is still running in
i2c_transfer/algo-master_xfer and the other thread is trying to remove
the device. If that happened, then every device data access in
mtk_i2c_transfer might cause oops. I looked at some i2c drivers and
can't find any checking for this case, I can't find anything prevent i2c
device removal before pending i2c_transfer complete either. Would you
give me an example?
I just noticed that even my driver is affected. If the above recipe
makes your driver barf there is something to fix, if not ... hmm, then
maybe there is more synchronization than I'm aware of or my recipe is
wrong.
At least another driver author believed me:
http://thread.gmane.org/gmane.linux.drivers.i2c/21531/focus=21662
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 2/2] I2C: mediatek: Add driver for MediaTek I2C controller
Hi Uwe,
Thanks for your review,
On Wed, 2015-01-21 at 11:13 +0800, Eddie Huang wrote:
<...>
> > > + ret = -EINVAL;
> > > + goto err_exit;
> > > + }
> > > +
> > > + if (msgs->buf == NULL) {
> > > + dev_dbg(i2c->dev, " data buffer is NULL.\n");
> > > + ret = -EINVAL;
> > > + goto err_exit;
> > > + }
> > > +
> > > + i2c->addr = msgs->addr;
> > > + i2c->msg_len = msgs->len;
> > > + i2c->msg_buf = msgs->buf;
> > > +
> > > + if (msgs->flags & I2C_M_RD)
> > > + i2c->op = I2C_MASTER_RD;
> > > + else
> > > + i2c->op = I2C_MASTER_WR;
> > > +
> > > + /* combined two messages into one transaction */
> > > + if (num > 1) {
> > > + i2c->msg_aux_len = (msgs + 1)->len;
> > > + i2c->op = I2C_MASTER_WRRD;
> > > + }
> > This means "write then read", right? You should check here that the
> > first message is really a write and the 2nd a read then.
> > Can this happen at all with the quirks defined below (.max_num_msgs =
> > 1)?
> Yes, mean write then read. Indeed, add check is better.
> If msg number is 1, means normal write or read, not "write then read".
The quirks will increase the message count and check 'write then read'
for us. We don't have to add check here.
<...>
> > > +static int mtk_i2c_remove(struct platform_device *pdev)
> > > +{
> > > + struct mtk_i2c *i2c = platform_get_drvdata(pdev);
> > > +
> > > + i2c_del_adapter(>adap);
> > > + free_i2c_dma_bufs(i2c);
> > > + platform_set_drvdata(pdev, NULL);
> > > +
> > Here you need to make sure that no irq is running when i2c_del_adapter
> > is called.
> OK, add check here
I thought after i2c_del_adapter() is complete, all i2c_transfer for this
adapter is completed. If this is true, then i2c clock is already off and
we won't have any on-going transfer/pending irq.
Joe.C
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 2/2] I2C: mediatek: Add driver for MediaTek I2C controller
Hi Uwe,
On Sun, 2015-01-18 at 11:18 +0100, Uwe Kleine-König wrote:
> Hello,
>
> On Fri, Jan 16, 2015 at 06:33:38PM +0800, Eddie Huang wrote:
> > +config I2C_MT65XX
> > + tristate "MediaTek I2C adapter"
> > + depends on ARCH_MEDIATEK
> depends on ARCH_MEDIATEK || COMPILE_TEST
> default ARCH_MEDIATEK
>
> would be nice to have to get better compile coverage.
OK
>
> > +struct mtk_i2c {
> > + struct i2c_adapter adap;/* i2c host adapter */
> > + struct device *dev;
> > + wait_queue_head_t wait; /* i2c transfer wait queue */
> > + /* set in i2c probe */
> > + void __iomem *base; /* i2c base addr */
> > + void __iomem *pdmabase; /* dma base address*/
> > + int irqnr; /* i2c interrupt number */
> irqs are unsigned quantities
OK
>
> > + struct i2c_dma_buf dma_buf; /* memory alloc for DMA mode */
> > + struct clk *clk_main; /* main clock for i2c bus */
> > + struct clk *clk_dma;/* DMA clock for i2c via DMA */
> > + struct clk *clk_pmic; /* PMIC clock for i2c from PMIC */
> > + bool have_pmic; /* can use i2c pins from PMIC */
> > + bool use_push_pull; /* IO config push-pull mode */
> > + u32 platform_compat;/* platform compatible data */
> > + /* set when doing the transfer */
> > + u16 irq_stat; /* interrupt status */
> > + unsigned int speed_hz; /* The speed in transfer */
> > + bool trans_stop;/* i2c transfer stop */
> > + enum mtk_trans_op op;
> > + u16 msg_len;
> > + u8 *msg_buf;/* pointer to msg data */
> > + u16 msg_aux_len;/* WRRD mode to set AUX_LEN register*/
> > + u16 addr; /* 7bit slave address, without read/write bit */
> Wouldn't it be easier to maintain a pointer to the message to be
> transferred?
I think use mtk_i2c pointer is more flexible than maintain a pointer to
message.
>
> > + u16 timing_reg;
> > + u16 high_speed_reg;
> > +};
> > +
> > +static const struct of_device_id mtk_i2c_of_match[] = {
> > + { .compatible = "mediatek,mt6577-i2c", .data = (void *)COMPAT_MT6577 },
> > + { .compatible = "mediatek,mt6589-i2c", .data = (void *)COMPAT_MT6589 },
> > + {},
> There is usually no , after the sentinel entry.
OK
>
> > +};
> > +MODULE_DEVICE_TABLE(of, mtk_i2c_match);
> > +
> > +static inline void i2c_writew(u16 value, struct mtk_i2c *i2c, u8 offset)
> > +{
> > + writew(value, i2c->base + offset);
> > +}
> hmm, these simple wrappers are fine in general for me because they might
> ease debugging or changing the accessor-function. Still "i2c_writew"
> sounds too generic. IMHO you should spend the few chars to make this
> mtk_i2c_writew. And to match my taste completely, move the driver data
> parameter to the front. (But there are too much different tastes out
> there to really request a certain style here.) Same for the other
> wrappers of course.
Sure, I will add mtk_ prefix.
>
> > + /* Prepare buffer data to start transfer */
> > + if (i2c->op == I2C_MASTER_RD) {
> > + i2c_writel_dma(I2C_DMA_INT_FLAG_NONE, i2c, OFFSET_INT_FLAG);
> > + i2c_writel_dma(I2C_DMA_CON_RX, i2c, OFFSET_CON);
> > + i2c_writel_dma((u32)i2c->msg_buf, i2c, OFFSET_RX_MEM_ADDR);
> > + i2c_writel_dma(i2c->msg_len, i2c, OFFSET_RX_LEN);
> > + } else if (i2c->op == I2C_MASTER_WR) {
> > + i2c_writel_dma(I2C_DMA_INT_FLAG_NONE, i2c, OFFSET_INT_FLAG);
> > + i2c_writel_dma(I2C_DMA_CON_TX, i2c, OFFSET_CON);
> > + i2c_writel_dma((u32)i2c->msg_buf, i2c, OFFSET_TX_MEM_ADDR);
> > + i2c_writel_dma(i2c->msg_len, i2c, OFFSET_TX_LEN);
> > + } else {
> /* i2c->op == I2C_MASTER_WRRD */
>
> > + i2c_writel_dma(I2C_DMA_CLR_FLAG, i2c, OFFSET_INT_FLAG);
> > + i2c_writel_dma(I2C_DMA_CLR_FLAG, i2c, OFFSET_CON);
> > + i2c_writel_dma((u32)i2c->msg_buf, i2c, OFFSET_TX_MEM_ADDR);
> > + i2c_writel_dma((u32)i2c->dma_buf.paddr, i2c,
> > + OFFSET_RX_MEM_ADDR);
> > + i2c_writel_dma(i2c->msg_len, i2c, OFFSET_TX_LEN);
> > + i2c_writel_dma(i2c->msg_aux_len, i2c, OFFSET_RX_LEN);
> > + }
> > +
> > + /* flush before sending start */
> > + mb();
> > + i2c_writel_dma(I2C_DMA_START_EN, i2c, OFFSET_EN);
> > + i2c_writew(I2C_TRANSAC_START, i2c, OFFSET_START);
> > +
> > + tmo = wait_event_timeout(i2c->wait, i2c->trans_stop, tmo);
> If the request completes just when wait_event_timeout returned 0 you
> shouldn't throw it away.
OK, add check transfer complete in tmo == 0 case.
> > + if (tmo == 0) {
> > + dev_dbg(i2c->dev, "addr: %x, transfer timeout\n", i2c->addr);
> > + mtk_i2c_init_hw(i2c);
> > + return -ETIMEDOUT;
> > + }
> > [...]
> > + if (msgs->addr == 0) {
> > + dev_dbg(i2c->dev, " addr is invalid.\n");
> Is this a hardware limitation?
No. addr 0
Re: [PATCH v4 2/2] I2C: mediatek: Add driver for MediaTek I2C controller
Hi Uwe,
On Sun, 2015-01-18 at 11:18 +0100, Uwe Kleine-König wrote:
Hello,
On Fri, Jan 16, 2015 at 06:33:38PM +0800, Eddie Huang wrote:
+config I2C_MT65XX
+ tristate MediaTek I2C adapter
+ depends on ARCH_MEDIATEK
depends on ARCH_MEDIATEK || COMPILE_TEST
default ARCH_MEDIATEK
would be nice to have to get better compile coverage.
OK
+struct mtk_i2c {
+ struct i2c_adapter adap;/* i2c host adapter */
+ struct device *dev;
+ wait_queue_head_t wait; /* i2c transfer wait queue */
+ /* set in i2c probe */
+ void __iomem *base; /* i2c base addr */
+ void __iomem *pdmabase; /* dma base address*/
+ int irqnr; /* i2c interrupt number */
irqs are unsigned quantities
OK
+ struct i2c_dma_buf dma_buf; /* memory alloc for DMA mode */
+ struct clk *clk_main; /* main clock for i2c bus */
+ struct clk *clk_dma;/* DMA clock for i2c via DMA */
+ struct clk *clk_pmic; /* PMIC clock for i2c from PMIC */
+ bool have_pmic; /* can use i2c pins from PMIC */
+ bool use_push_pull; /* IO config push-pull mode */
+ u32 platform_compat;/* platform compatible data */
+ /* set when doing the transfer */
+ u16 irq_stat; /* interrupt status */
+ unsigned int speed_hz; /* The speed in transfer */
+ bool trans_stop;/* i2c transfer stop */
+ enum mtk_trans_op op;
+ u16 msg_len;
+ u8 *msg_buf;/* pointer to msg data */
+ u16 msg_aux_len;/* WRRD mode to set AUX_LEN register*/
+ u16 addr; /* 7bit slave address, without read/write bit */
Wouldn't it be easier to maintain a pointer to the message to be
transferred?
I think use mtk_i2c pointer is more flexible than maintain a pointer to
message.
+ u16 timing_reg;
+ u16 high_speed_reg;
+};
+
+static const struct of_device_id mtk_i2c_of_match[] = {
+ { .compatible = mediatek,mt6577-i2c, .data = (void *)COMPAT_MT6577 },
+ { .compatible = mediatek,mt6589-i2c, .data = (void *)COMPAT_MT6589 },
+ {},
There is usually no , after the sentinel entry.
OK
+};
+MODULE_DEVICE_TABLE(of, mtk_i2c_match);
+
+static inline void i2c_writew(u16 value, struct mtk_i2c *i2c, u8 offset)
+{
+ writew(value, i2c-base + offset);
+}
hmm, these simple wrappers are fine in general for me because they might
ease debugging or changing the accessor-function. Still i2c_writew
sounds too generic. IMHO you should spend the few chars to make this
mtk_i2c_writew. And to match my taste completely, move the driver data
parameter to the front. (But there are too much different tastes out
there to really request a certain style here.) Same for the other
wrappers of course.
Sure, I will add mtk_ prefix.
+ /* Prepare buffer data to start transfer */
+ if (i2c-op == I2C_MASTER_RD) {
+ i2c_writel_dma(I2C_DMA_INT_FLAG_NONE, i2c, OFFSET_INT_FLAG);
+ i2c_writel_dma(I2C_DMA_CON_RX, i2c, OFFSET_CON);
+ i2c_writel_dma((u32)i2c-msg_buf, i2c, OFFSET_RX_MEM_ADDR);
+ i2c_writel_dma(i2c-msg_len, i2c, OFFSET_RX_LEN);
+ } else if (i2c-op == I2C_MASTER_WR) {
+ i2c_writel_dma(I2C_DMA_INT_FLAG_NONE, i2c, OFFSET_INT_FLAG);
+ i2c_writel_dma(I2C_DMA_CON_TX, i2c, OFFSET_CON);
+ i2c_writel_dma((u32)i2c-msg_buf, i2c, OFFSET_TX_MEM_ADDR);
+ i2c_writel_dma(i2c-msg_len, i2c, OFFSET_TX_LEN);
+ } else {
/* i2c-op == I2C_MASTER_WRRD */
+ i2c_writel_dma(I2C_DMA_CLR_FLAG, i2c, OFFSET_INT_FLAG);
+ i2c_writel_dma(I2C_DMA_CLR_FLAG, i2c, OFFSET_CON);
+ i2c_writel_dma((u32)i2c-msg_buf, i2c, OFFSET_TX_MEM_ADDR);
+ i2c_writel_dma((u32)i2c-dma_buf.paddr, i2c,
+ OFFSET_RX_MEM_ADDR);
+ i2c_writel_dma(i2c-msg_len, i2c, OFFSET_TX_LEN);
+ i2c_writel_dma(i2c-msg_aux_len, i2c, OFFSET_RX_LEN);
+ }
+
+ /* flush before sending start */
+ mb();
+ i2c_writel_dma(I2C_DMA_START_EN, i2c, OFFSET_EN);
+ i2c_writew(I2C_TRANSAC_START, i2c, OFFSET_START);
+
+ tmo = wait_event_timeout(i2c-wait, i2c-trans_stop, tmo);
If the request completes just when wait_event_timeout returned 0 you
shouldn't throw it away.
OK, add check transfer complete in tmo == 0 case.
+ if (tmo == 0) {
+ dev_dbg(i2c-dev, addr: %x, transfer timeout\n, i2c-addr);
+ mtk_i2c_init_hw(i2c);
+ return -ETIMEDOUT;
+ }
[...]
+ if (msgs-addr == 0) {
+ dev_dbg(i2c-dev, addr is invalid.\n);
Is this a hardware limitation?
No. addr 0 should be reserved for special purpose. No client should use
addr 0. I think driver should not block transfer addr 0, I will remove
this.
I'd remove the leading space in the message. (Applies also to other
Re: [PATCH v4 2/2] I2C: mediatek: Add driver for MediaTek I2C controller
Hi Uwe,
Thanks for your review,
On Wed, 2015-01-21 at 11:13 +0800, Eddie Huang wrote:
...
+ ret = -EINVAL;
+ goto err_exit;
+ }
+
+ if (msgs-buf == NULL) {
+ dev_dbg(i2c-dev, data buffer is NULL.\n);
+ ret = -EINVAL;
+ goto err_exit;
+ }
+
+ i2c-addr = msgs-addr;
+ i2c-msg_len = msgs-len;
+ i2c-msg_buf = msgs-buf;
+
+ if (msgs-flags I2C_M_RD)
+ i2c-op = I2C_MASTER_RD;
+ else
+ i2c-op = I2C_MASTER_WR;
+
+ /* combined two messages into one transaction */
+ if (num 1) {
+ i2c-msg_aux_len = (msgs + 1)-len;
+ i2c-op = I2C_MASTER_WRRD;
+ }
This means write then read, right? You should check here that the
first message is really a write and the 2nd a read then.
Can this happen at all with the quirks defined below (.max_num_msgs =
1)?
Yes, mean write then read. Indeed, add check is better.
If msg number is 1, means normal write or read, not write then read.
The quirks will increase the message count and check 'write then read'
for us. We don't have to add check here.
...
+static int mtk_i2c_remove(struct platform_device *pdev)
+{
+ struct mtk_i2c *i2c = platform_get_drvdata(pdev);
+
+ i2c_del_adapter(i2c-adap);
+ free_i2c_dma_bufs(i2c);
+ platform_set_drvdata(pdev, NULL);
+
Here you need to make sure that no irq is running when i2c_del_adapter
is called.
OK, add check here
I thought after i2c_del_adapter() is complete, all i2c_transfer for this
adapter is completed. If this is true, then i2c clock is already off and
we won't have any on-going transfer/pending irq.
Joe.C
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 2/2] I2C: mediatek: Add driver for MediaTek I2C controller
Hello,
On Fri, Jan 16, 2015 at 06:33:38PM +0800, Eddie Huang wrote:
> +config I2C_MT65XX
> + tristate "MediaTek I2C adapter"
> + depends on ARCH_MEDIATEK
depends on ARCH_MEDIATEK || COMPILE_TEST
default ARCH_MEDIATEK
would be nice to have to get better compile coverage.
> +struct mtk_i2c {
> + struct i2c_adapter adap;/* i2c host adapter */
> + struct device *dev;
> + wait_queue_head_t wait; /* i2c transfer wait queue */
> + /* set in i2c probe */
> + void __iomem *base; /* i2c base addr */
> + void __iomem *pdmabase; /* dma base address*/
> + int irqnr; /* i2c interrupt number */
irqs are unsigned quantities
> + struct i2c_dma_buf dma_buf; /* memory alloc for DMA mode */
> + struct clk *clk_main; /* main clock for i2c bus */
> + struct clk *clk_dma;/* DMA clock for i2c via DMA */
> + struct clk *clk_pmic; /* PMIC clock for i2c from PMIC */
> + bool have_pmic; /* can use i2c pins from PMIC */
> + bool use_push_pull; /* IO config push-pull mode */
> + u32 platform_compat;/* platform compatible data */
> + /* set when doing the transfer */
> + u16 irq_stat; /* interrupt status */
> + unsigned int speed_hz; /* The speed in transfer */
> + bool trans_stop;/* i2c transfer stop */
> + enum mtk_trans_op op;
> + u16 msg_len;
> + u8 *msg_buf;/* pointer to msg data */
> + u16 msg_aux_len;/* WRRD mode to set AUX_LEN register*/
> + u16 addr; /* 7bit slave address, without read/write bit */
Wouldn't it be easier to maintain a pointer to the message to be
transferred?
> + u16 timing_reg;
> + u16 high_speed_reg;
> +};
> +
> +static const struct of_device_id mtk_i2c_of_match[] = {
> + { .compatible = "mediatek,mt6577-i2c", .data = (void *)COMPAT_MT6577 },
> + { .compatible = "mediatek,mt6589-i2c", .data = (void *)COMPAT_MT6589 },
> + {},
There is usually no , after the sentinel entry.
> +};
> +MODULE_DEVICE_TABLE(of, mtk_i2c_match);
> +
> +static inline void i2c_writew(u16 value, struct mtk_i2c *i2c, u8 offset)
> +{
> + writew(value, i2c->base + offset);
> +}
hmm, these simple wrappers are fine in general for me because they might
ease debugging or changing the accessor-function. Still "i2c_writew"
sounds too generic. IMHO you should spend the few chars to make this
mtk_i2c_writew. And to match my taste completely, move the driver data
parameter to the front. (But there are too much different tastes out
there to really request a certain style here.) Same for the other
wrappers of course.
> + /* Prepare buffer data to start transfer */
> + if (i2c->op == I2C_MASTER_RD) {
> + i2c_writel_dma(I2C_DMA_INT_FLAG_NONE, i2c, OFFSET_INT_FLAG);
> + i2c_writel_dma(I2C_DMA_CON_RX, i2c, OFFSET_CON);
> + i2c_writel_dma((u32)i2c->msg_buf, i2c, OFFSET_RX_MEM_ADDR);
> + i2c_writel_dma(i2c->msg_len, i2c, OFFSET_RX_LEN);
> + } else if (i2c->op == I2C_MASTER_WR) {
> + i2c_writel_dma(I2C_DMA_INT_FLAG_NONE, i2c, OFFSET_INT_FLAG);
> + i2c_writel_dma(I2C_DMA_CON_TX, i2c, OFFSET_CON);
> + i2c_writel_dma((u32)i2c->msg_buf, i2c, OFFSET_TX_MEM_ADDR);
> + i2c_writel_dma(i2c->msg_len, i2c, OFFSET_TX_LEN);
> + } else {
/* i2c->op == I2C_MASTER_WRRD */
> + i2c_writel_dma(I2C_DMA_CLR_FLAG, i2c, OFFSET_INT_FLAG);
> + i2c_writel_dma(I2C_DMA_CLR_FLAG, i2c, OFFSET_CON);
> + i2c_writel_dma((u32)i2c->msg_buf, i2c, OFFSET_TX_MEM_ADDR);
> + i2c_writel_dma((u32)i2c->dma_buf.paddr, i2c,
> + OFFSET_RX_MEM_ADDR);
> + i2c_writel_dma(i2c->msg_len, i2c, OFFSET_TX_LEN);
> + i2c_writel_dma(i2c->msg_aux_len, i2c, OFFSET_RX_LEN);
> + }
> +
> + /* flush before sending start */
> + mb();
> + i2c_writel_dma(I2C_DMA_START_EN, i2c, OFFSET_EN);
> + i2c_writew(I2C_TRANSAC_START, i2c, OFFSET_START);
> +
> + tmo = wait_event_timeout(i2c->wait, i2c->trans_stop, tmo);
If the request completes just when wait_event_timeout returned 0 you
shouldn't throw it away.
> + if (tmo == 0) {
> + dev_dbg(i2c->dev, "addr: %x, transfer timeout\n", i2c->addr);
> + mtk_i2c_init_hw(i2c);
> + return -ETIMEDOUT;
> + }
> [...]
> + if (msgs->addr == 0) {
> + dev_dbg(i2c->dev, " addr is invalid.\n");
Is this a hardware limitation?
I'd remove the leading space in the message. (Applies also to other
places.)
> + ret = -EINVAL;
> + goto err_exit;
> + }
> +
> + if (msgs->buf == NULL) {
> + dev_dbg(i2c->dev, " data buffer is NULL.\n");
> + ret = -EINVAL;
> + goto err_exit;
> + }
> +
> +
Re: [PATCH v4 2/2] I2C: mediatek: Add driver for MediaTek I2C controller
Hello,
On Fri, Jan 16, 2015 at 06:33:38PM +0800, Eddie Huang wrote:
+config I2C_MT65XX
+ tristate MediaTek I2C adapter
+ depends on ARCH_MEDIATEK
depends on ARCH_MEDIATEK || COMPILE_TEST
default ARCH_MEDIATEK
would be nice to have to get better compile coverage.
+struct mtk_i2c {
+ struct i2c_adapter adap;/* i2c host adapter */
+ struct device *dev;
+ wait_queue_head_t wait; /* i2c transfer wait queue */
+ /* set in i2c probe */
+ void __iomem *base; /* i2c base addr */
+ void __iomem *pdmabase; /* dma base address*/
+ int irqnr; /* i2c interrupt number */
irqs are unsigned quantities
+ struct i2c_dma_buf dma_buf; /* memory alloc for DMA mode */
+ struct clk *clk_main; /* main clock for i2c bus */
+ struct clk *clk_dma;/* DMA clock for i2c via DMA */
+ struct clk *clk_pmic; /* PMIC clock for i2c from PMIC */
+ bool have_pmic; /* can use i2c pins from PMIC */
+ bool use_push_pull; /* IO config push-pull mode */
+ u32 platform_compat;/* platform compatible data */
+ /* set when doing the transfer */
+ u16 irq_stat; /* interrupt status */
+ unsigned int speed_hz; /* The speed in transfer */
+ bool trans_stop;/* i2c transfer stop */
+ enum mtk_trans_op op;
+ u16 msg_len;
+ u8 *msg_buf;/* pointer to msg data */
+ u16 msg_aux_len;/* WRRD mode to set AUX_LEN register*/
+ u16 addr; /* 7bit slave address, without read/write bit */
Wouldn't it be easier to maintain a pointer to the message to be
transferred?
+ u16 timing_reg;
+ u16 high_speed_reg;
+};
+
+static const struct of_device_id mtk_i2c_of_match[] = {
+ { .compatible = mediatek,mt6577-i2c, .data = (void *)COMPAT_MT6577 },
+ { .compatible = mediatek,mt6589-i2c, .data = (void *)COMPAT_MT6589 },
+ {},
There is usually no , after the sentinel entry.
+};
+MODULE_DEVICE_TABLE(of, mtk_i2c_match);
+
+static inline void i2c_writew(u16 value, struct mtk_i2c *i2c, u8 offset)
+{
+ writew(value, i2c-base + offset);
+}
hmm, these simple wrappers are fine in general for me because they might
ease debugging or changing the accessor-function. Still i2c_writew
sounds too generic. IMHO you should spend the few chars to make this
mtk_i2c_writew. And to match my taste completely, move the driver data
parameter to the front. (But there are too much different tastes out
there to really request a certain style here.) Same for the other
wrappers of course.
+ /* Prepare buffer data to start transfer */
+ if (i2c-op == I2C_MASTER_RD) {
+ i2c_writel_dma(I2C_DMA_INT_FLAG_NONE, i2c, OFFSET_INT_FLAG);
+ i2c_writel_dma(I2C_DMA_CON_RX, i2c, OFFSET_CON);
+ i2c_writel_dma((u32)i2c-msg_buf, i2c, OFFSET_RX_MEM_ADDR);
+ i2c_writel_dma(i2c-msg_len, i2c, OFFSET_RX_LEN);
+ } else if (i2c-op == I2C_MASTER_WR) {
+ i2c_writel_dma(I2C_DMA_INT_FLAG_NONE, i2c, OFFSET_INT_FLAG);
+ i2c_writel_dma(I2C_DMA_CON_TX, i2c, OFFSET_CON);
+ i2c_writel_dma((u32)i2c-msg_buf, i2c, OFFSET_TX_MEM_ADDR);
+ i2c_writel_dma(i2c-msg_len, i2c, OFFSET_TX_LEN);
+ } else {
/* i2c-op == I2C_MASTER_WRRD */
+ i2c_writel_dma(I2C_DMA_CLR_FLAG, i2c, OFFSET_INT_FLAG);
+ i2c_writel_dma(I2C_DMA_CLR_FLAG, i2c, OFFSET_CON);
+ i2c_writel_dma((u32)i2c-msg_buf, i2c, OFFSET_TX_MEM_ADDR);
+ i2c_writel_dma((u32)i2c-dma_buf.paddr, i2c,
+ OFFSET_RX_MEM_ADDR);
+ i2c_writel_dma(i2c-msg_len, i2c, OFFSET_TX_LEN);
+ i2c_writel_dma(i2c-msg_aux_len, i2c, OFFSET_RX_LEN);
+ }
+
+ /* flush before sending start */
+ mb();
+ i2c_writel_dma(I2C_DMA_START_EN, i2c, OFFSET_EN);
+ i2c_writew(I2C_TRANSAC_START, i2c, OFFSET_START);
+
+ tmo = wait_event_timeout(i2c-wait, i2c-trans_stop, tmo);
If the request completes just when wait_event_timeout returned 0 you
shouldn't throw it away.
+ if (tmo == 0) {
+ dev_dbg(i2c-dev, addr: %x, transfer timeout\n, i2c-addr);
+ mtk_i2c_init_hw(i2c);
+ return -ETIMEDOUT;
+ }
[...]
+ if (msgs-addr == 0) {
+ dev_dbg(i2c-dev, addr is invalid.\n);
Is this a hardware limitation?
I'd remove the leading space in the message. (Applies also to other
places.)
+ ret = -EINVAL;
+ goto err_exit;
+ }
+
+ if (msgs-buf == NULL) {
+ dev_dbg(i2c-dev, data buffer is NULL.\n);
+ ret = -EINVAL;
+ goto err_exit;
+ }
+
+ i2c-addr = msgs-addr;
+ i2c-msg_len = msgs-len;
+ i2c-msg_buf = msgs-buf;
+
+ if (msgs-flags I2C_M_RD)
+
[PATCH v4 2/2] I2C: mediatek: Add driver for MediaTek I2C controller
From: Xudong Chen
The mediatek SoCs have I2C controller that handle I2C transfer.
This patch include common I2C bus driver.
This driver is compatible with I2C controller on mt65xx/mt81xx.
Signed-off-by: Xudong Chen
Signed-off-by: Eddie Huang
---
drivers/i2c/busses/Kconfig | 9 +
drivers/i2c/busses/Makefile | 1 +
drivers/i2c/busses/i2c-mt65xx.c | 710
3 files changed, 720 insertions(+)
create mode 100644 drivers/i2c/busses/i2c-mt65xx.c
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 31e8308..7c76693 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -582,6 +582,15 @@ config I2C_MPC
This driver can also be built as a module. If so, the module
will be called i2c-mpc.
+config I2C_MT65XX
+ tristate "MediaTek I2C adapter"
+ depends on ARCH_MEDIATEK
+ help
+ This selects the MediaTek(R) Integrated Inter Circuit bus driver
+ for MT65xx and MT81xx.
+ If you want to use MediaTek(R) I2C interface, say Y or M here.
+ If unsure, say N.
+
config I2C_MV64XXX
tristate "Marvell mv64xxx I2C Controller"
depends on MV64X60 || PLAT_ORION || ARCH_SUNXI
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 56388f6..59cbb4b 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -56,6 +56,7 @@ obj-$(CONFIG_I2C_IOP3XX) += i2c-iop3xx.o
obj-$(CONFIG_I2C_KEMPLD) += i2c-kempld.o
obj-$(CONFIG_I2C_MESON)+= i2c-meson.o
obj-$(CONFIG_I2C_MPC) += i2c-mpc.o
+obj-$(CONFIG_I2C_MT65XX) += i2c-mt65xx.o
obj-$(CONFIG_I2C_MV64XXX) += i2c-mv64xxx.o
obj-$(CONFIG_I2C_MXS) += i2c-mxs.o
obj-$(CONFIG_I2C_NOMADIK) += i2c-nomadik.o
diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c
new file mode 100644
index 000..21bbe3b
--- /dev/null
+++ b/drivers/i2c/busses/i2c-mt65xx.c
@@ -0,0 +1,710 @@
+/*
+ * Copyright (c) 2014 MediaTek Inc.
+ * Author: Xudong.chen
+ *
+ * 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.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define I2C_HS_NACKERR (1 << 2)
+#define I2C_ACKERR (1 << 1)
+#define I2C_TRANSAC_COMP (1 << 0)
+#define I2C_TRANSAC_START (1 << 0)
+#define I2C_TIMING_STEP_DIV_MASK (0x3f << 0)
+#define I2C_TIMING_SAMPLE_COUNT_MASK (0x7 << 0)
+#define I2C_TIMING_SAMPLE_DIV_MASK (0x7 << 8)
+#define I2C_TIMING_DATA_READ_MASK (0x7 << 12)
+#define I2C_DCM_DISABLE0x
+#define I2C_IO_CONFIG_OPEN_DRAIN 0x0003
+#define I2C_IO_CONFIG_PUSH_PULL0x
+#define I2C_SOFT_RST 0x0001
+#define I2C_FIFO_ADDR_CLR 0x0001
+#define I2C_DELAY_LEN 0x0002
+#define I2C_ST_START_CON 0x8001
+#define I2C_FS_START_CON 0x1800
+#define I2C_TIME_CLR_VALUE 0x
+#define I2C_TIME_DEFAULT_VALUE 0x0003
+#define I2C_FS_TIME_INIT_VALUE 0x1303
+#define I2C_WRRD_TRANAC_VALUE 0x0002
+#define I2C_RD_TRANAC_VALUE0x0001
+
+#define I2C_DMA_CON_TX 0x
+#define I2C_DMA_CON_RX 0x0001
+#define I2C_DMA_START_EN 0x0001
+#define I2C_DMA_INT_FLAG_NONE 0x
+#define I2C_DMA_CLR_FLAG 0x
+
+#define I2C_DEFAUT_SPEED 10 /* hz */
+#define MAX_FS_MODE_SPEED 40
+#define MAX_HS_MODE_SPEED 340
+#define MAX_DMA_TRANS_SIZE 255
+#define MAX_WRRD_TRANS_SIZE31
+#define MAX_SAMPLE_CNT_DIV 8
+#define MAX_STEP_CNT_DIV 64
+#define MAX_HS_STEP_CNT_DIV8
+
+#define I2C_CONTROL_RS (0x1 << 1)
+#define I2C_CONTROL_DMA_EN (0x1 << 2)
+#define I2C_CONTROL_CLK_EXT_EN (0x1 << 3)
+#define I2C_CONTROL_DIR_CHANGE (0x1 << 4)
+#define I2C_CONTROL_ACKERR_DET_EN (0x1 << 5)
+#define I2C_CONTROL_TRANSFER_LEN_CHANGE (0x1 << 6)
+#define I2C_CONTROL_WRAPPER (0x1 << 0)
+
+#define COMPAT_MT6577 (0x1 << 0)
+#define COMPAT_MT6589 (0x1 << 1)
+
+#define I2C_DRV_NAME "mt-i2c"
+
+enum DMA_REGS_OFFSET {
+ OFFSET_INT_FLAG = 0x0,
+ OFFSET_INT_EN = 0x04,
+
[PATCH v4 2/2] I2C: mediatek: Add driver for MediaTek I2C controller
From: Xudong Chen xudong.c...@mediatek.com The mediatek SoCs have I2C controller that handle I2C transfer. This patch include common I2C bus driver. This driver is compatible with I2C controller on mt65xx/mt81xx. Signed-off-by: Xudong Chen xudong.c...@mediatek.com Signed-off-by: Eddie Huang eddie.hu...@mediatek.com --- drivers/i2c/busses/Kconfig | 9 + drivers/i2c/busses/Makefile | 1 + drivers/i2c/busses/i2c-mt65xx.c | 710 3 files changed, 720 insertions(+) create mode 100644 drivers/i2c/busses/i2c-mt65xx.c diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index 31e8308..7c76693 100644 --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -582,6 +582,15 @@ config I2C_MPC This driver can also be built as a module. If so, the module will be called i2c-mpc. +config I2C_MT65XX + tristate MediaTek I2C adapter + depends on ARCH_MEDIATEK + help + This selects the MediaTek(R) Integrated Inter Circuit bus driver + for MT65xx and MT81xx. + If you want to use MediaTek(R) I2C interface, say Y or M here. + If unsure, say N. + config I2C_MV64XXX tristate Marvell mv64xxx I2C Controller depends on MV64X60 || PLAT_ORION || ARCH_SUNXI diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile index 56388f6..59cbb4b 100644 --- a/drivers/i2c/busses/Makefile +++ b/drivers/i2c/busses/Makefile @@ -56,6 +56,7 @@ obj-$(CONFIG_I2C_IOP3XX) += i2c-iop3xx.o obj-$(CONFIG_I2C_KEMPLD) += i2c-kempld.o obj-$(CONFIG_I2C_MESON)+= i2c-meson.o obj-$(CONFIG_I2C_MPC) += i2c-mpc.o +obj-$(CONFIG_I2C_MT65XX) += i2c-mt65xx.o obj-$(CONFIG_I2C_MV64XXX) += i2c-mv64xxx.o obj-$(CONFIG_I2C_MXS) += i2c-mxs.o obj-$(CONFIG_I2C_NOMADIK) += i2c-nomadik.o diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c new file mode 100644 index 000..21bbe3b --- /dev/null +++ b/drivers/i2c/busses/i2c-mt65xx.c @@ -0,0 +1,710 @@ +/* + * Copyright (c) 2014 MediaTek Inc. + * Author: Xudong.chen xudong.c...@mediatek.com + * + * 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. + */ + +#include linux/kernel.h +#include linux/module.h +#include linux/slab.h +#include linux/i2c.h +#include linux/init.h +#include linux/interrupt.h +#include linux/sched.h +#include linux/delay.h +#include linux/errno.h +#include linux/err.h +#include linux/device.h +#include linux/platform_device.h +#include linux/wait.h +#include linux/mm.h +#include linux/dma-mapping.h +#include linux/scatterlist.h +#include linux/io.h +#include linux/of_address.h +#include linux/of_irq.h +#include linux/clk.h + +#define I2C_HS_NACKERR (1 2) +#define I2C_ACKERR (1 1) +#define I2C_TRANSAC_COMP (1 0) +#define I2C_TRANSAC_START (1 0) +#define I2C_TIMING_STEP_DIV_MASK (0x3f 0) +#define I2C_TIMING_SAMPLE_COUNT_MASK (0x7 0) +#define I2C_TIMING_SAMPLE_DIV_MASK (0x7 8) +#define I2C_TIMING_DATA_READ_MASK (0x7 12) +#define I2C_DCM_DISABLE0x +#define I2C_IO_CONFIG_OPEN_DRAIN 0x0003 +#define I2C_IO_CONFIG_PUSH_PULL0x +#define I2C_SOFT_RST 0x0001 +#define I2C_FIFO_ADDR_CLR 0x0001 +#define I2C_DELAY_LEN 0x0002 +#define I2C_ST_START_CON 0x8001 +#define I2C_FS_START_CON 0x1800 +#define I2C_TIME_CLR_VALUE 0x +#define I2C_TIME_DEFAULT_VALUE 0x0003 +#define I2C_FS_TIME_INIT_VALUE 0x1303 +#define I2C_WRRD_TRANAC_VALUE 0x0002 +#define I2C_RD_TRANAC_VALUE0x0001 + +#define I2C_DMA_CON_TX 0x +#define I2C_DMA_CON_RX 0x0001 +#define I2C_DMA_START_EN 0x0001 +#define I2C_DMA_INT_FLAG_NONE 0x +#define I2C_DMA_CLR_FLAG 0x + +#define I2C_DEFAUT_SPEED 10 /* hz */ +#define MAX_FS_MODE_SPEED 40 +#define MAX_HS_MODE_SPEED 340 +#define MAX_DMA_TRANS_SIZE 255 +#define MAX_WRRD_TRANS_SIZE31 +#define MAX_SAMPLE_CNT_DIV 8 +#define MAX_STEP_CNT_DIV 64 +#define MAX_HS_STEP_CNT_DIV8 + +#define I2C_CONTROL_RS (0x1 1) +#define I2C_CONTROL_DMA_EN (0x1 2) +#define I2C_CONTROL_CLK_EXT_EN (0x1 3) +#define I2C_CONTROL_DIR_CHANGE (0x1 4) +#define I2C_CONTROL_ACKERR_DET_EN (0x1
