Tegra20 i2c controller does not support the continue transfer
which implements the I2C_M_NOSTART functionality of i2c
protocol mangling.
Removing the I2C_M_NOSTART functionality for Tegra20.
Signed-off-by: Laxman Dewangan <[email protected]>
---
This is rebased on linux-next-20120816.
drivers/i2c/busses/i2c-tegra.c | 73 +++++++++++++++++++++++++++++++--------
1 files changed, 58 insertions(+), 15 deletions(-)
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 7149625..554f713 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -27,6 +27,7 @@
#include <linux/slab.h>
#include <linux/i2c-tegra.h>
#include <linux/of_i2c.h>
+#include <linux/of_device.h>
#include <linux/module.h>
#include <asm/unaligned.h>
@@ -114,6 +115,15 @@ enum msg_end_type {
};
/**
+ * struct tegra_i2c_hw_feature : Different HW support on Tegra
+ * @has_continue_xfer_support: Continue transfer supports.
+ */
+
+struct tegra_i2c_hw_feature {
+ bool has_continue_xfer_support;
+};
+
+/**
* struct tegra_i2c_dev - per device i2c context
* @dev: device reference for power management
* @adapter: core i2c layer adapter information
@@ -148,6 +158,7 @@ struct tegra_i2c_dev {
int msg_read;
unsigned long bus_clk_rate;
bool is_suspended;
+ bool has_continue_xfer_support;
};
static void dvc_writel(struct tegra_i2c_dev *i2c_dev, u32 val, unsigned long reg)
@@ -563,7 +574,17 @@ static int tegra_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
if (i2c_dev->is_suspended)
return -EBUSY;
- clk_prepare_enable(i2c_dev->div_clk);
+ /* Support I2C_M_NOSTART only if HW support continue xfer. */
+ for (i = 0; i < num - 1; i++) {
+ if ((msgs[i + 1].flags & I2C_M_NOSTART) &&
+ !i2c_dev->has_continue_xfer_support) {
+ dev_err(i2c_dev->dev,
+ "mesg %d have illegal flag\n", i + 1);
+ return -EINVAL;
+ }
+ }
+
+ clk_prepare_enable(i2c_dev->clk);
for (i = 0; i < num; i++) {
enum msg_end_type end_type = MSG_END_STOP;
if (i < (num - 1)) {
@@ -582,8 +603,13 @@ static int tegra_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
static u32 tegra_i2c_func(struct i2c_adapter *adap)
{
- return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_10BIT_ADDR |
- I2C_FUNC_PROTOCOL_MANGLING | I2C_FUNC_NOSTART;
+ struct tegra_i2c_dev *i2c_dev = i2c_get_adapdata(adap);
+ u32 ret = I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_10BIT_ADDR |
+ I2C_FUNC_PROTOCOL_MANGLING;
+
+ if (i2c_dev->has_continue_xfer_support)
+ ret |= I2C_FUNC_NOSTART;
+ return ret;
}
static const struct i2c_algorithm tegra_i2c_algo = {
@@ -591,6 +617,25 @@ static const struct i2c_algorithm tegra_i2c_algo = {
.functionality = tegra_i2c_func,
};
+static struct tegra_i2c_hw_feature tegra20_i2c_hw = {
+ .has_continue_xfer_support = false,
+};
+
+static struct tegra_i2c_hw_feature tegra30_i2c_hw = {
+ .has_continue_xfer_support = true,
+};
+
+#if defined(CONFIG_OF)
+/* Match table for of_platform binding */
+static const struct of_device_id tegra_i2c_of_match[] __devinitconst = {
+ { .compatible = "nvidia,tegra30-i2c", .data = &tegra30_i2c_hw, },
+ { .compatible = "nvidia,tegra20-i2c", .data = &tegra20_i2c_hw, },
+ { .compatible = "nvidia,tegra20-i2c-dvc", .data = &tegra20_i2c_hw, },
+ {},
+};
+MODULE_DEVICE_TABLE(of, tegra_i2c_of_match);
+#endif
+
static int __devinit tegra_i2c_probe(struct platform_device *pdev)
{
struct tegra_i2c_dev *i2c_dev;
@@ -600,6 +645,7 @@ static int __devinit tegra_i2c_probe(struct platform_device *pdev)
struct clk *fast_clk;
const unsigned int *prop;
void __iomem *base;
+ const struct tegra_i2c_hw_feature *hw = &tegra20_i2c_hw;
int irq;
int ret = 0;
@@ -659,11 +705,18 @@ static int __devinit tegra_i2c_probe(struct platform_device *pdev)
i2c_dev->bus_clk_rate = be32_to_cpup(prop);
}
- if (pdev->dev.of_node)
+ if (pdev->dev.of_node) {
+ const struct of_device_id *match;
+ match = of_match_device(of_match_ptr(tegra_i2c_of_match),
+ &pdev->dev);
+ hw = match->data;
i2c_dev->is_dvc = of_device_is_compatible(pdev->dev.of_node,
"nvidia,tegra20-i2c-dvc");
- else if (pdev->id == 3)
+ } else if (pdev->id == 3) {
i2c_dev->is_dvc = 1;
+ }
+
+ i2c_dev->has_continue_xfer_support = hw->has_continue_xfer_support;
init_completion(&i2c_dev->msg_complete);
platform_set_drvdata(pdev, i2c_dev);
@@ -751,16 +804,6 @@ static SIMPLE_DEV_PM_OPS(tegra_i2c_pm, tegra_i2c_suspend, tegra_i2c_resume);
#define TEGRA_I2C_PM NULL
#endif
-#if defined(CONFIG_OF)
-/* Match table for of_platform binding */
-static const struct of_device_id tegra_i2c_of_match[] __devinitconst = {
- { .compatible = "nvidia,tegra20-i2c", },
- { .compatible = "nvidia,tegra20-i2c-dvc", },
- {},
-};
-MODULE_DEVICE_TABLE(of, tegra_i2c_of_match);
-#endif
-
static struct platform_driver tegra_i2c_driver = {
.probe = tegra_i2c_probe,
.remove = __devexit_p(tegra_i2c_remove),
--
1.7.1.1
Tegra I2C driver enables the fast clock during initialization
and does not disable till driver removed.
Enable this clock before transfer and disable after transfer done.
Signed-off-by: Laxman Dewangan <[email protected]>
---
This is rebased on linux-next-20120816.
drivers/i2c/busses/i2c-tegra.c | 35 ++++++++++++++++++++++++++++-------
1 files changed, 28 insertions(+), 7 deletions(-)
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 554f713..24f7cee 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -362,12 +362,36 @@ static void tegra_dvc_init(struct tegra_i2c_dev *i2c_dev)
dvc_writel(i2c_dev, val, DVC_CTRL_REG1);
}
+static inline int tegra_i2c_clock_enable(struct tegra_i2c_dev *i2c_dev)
+{
+ int ret;
+ ret = clk_prepare_enable(i2c_dev->fast_clk);
+ if (ret < 0) {
+ dev_err(i2c_dev->dev,
+ "Enabling fast clk failed, err %d\n", ret);
+ return ret;
+ }
+ ret = clk_prepare_enable(i2c_dev->div_clk);
+ if (ret < 0) {
+ dev_err(i2c_dev->dev,
+ "Enabling div clk failed, err %d\n", ret);
+ clk_disable_unprepare(i2c_dev->fast_clk);
+ }
+ return ret;
+}
+
+static inline void tegra_i2c_clock_disable(struct tegra_i2c_dev *i2c_dev)
+{
+ clk_disable_unprepare(i2c_dev->div_clk);
+ clk_disable_unprepare(i2c_dev->fast_clk);
+}
+
static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
{
u32 val;
int err = 0;
- clk_prepare_enable(i2c_dev->div_clk);
+ tegra_i2c_clock_enable(i2c_dev);
tegra_periph_reset_assert(i2c_dev->div_clk);
udelay(2);
@@ -398,7 +422,7 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
if (tegra_i2c_flush_fifos(i2c_dev))
err = -ETIMEDOUT;
- clk_disable_unprepare(i2c_dev->div_clk);
+ tegra_i2c_clock_disable(i2c_dev);
if (i2c_dev->irq_disabled) {
i2c_dev->irq_disabled = 0;
@@ -584,7 +608,7 @@ static int tegra_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
}
}
- clk_prepare_enable(i2c_dev->clk);
+ tegra_i2c_clock_enable(i2c_dev);
for (i = 0; i < num; i++) {
enum msg_end_type end_type = MSG_END_STOP;
if (i < (num - 1)) {
@@ -597,7 +621,7 @@ static int tegra_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
if (ret)
break;
}
- clk_disable_unprepare(i2c_dev->div_clk);
+ tegra_i2c_clock_disable(i2c_dev);
return ret ?: i;
}
@@ -734,8 +758,6 @@ static int __devinit tegra_i2c_probe(struct platform_device *pdev)
return ret;
}
- clk_prepare_enable(i2c_dev->fast_clk);
-
i2c_set_adapdata(&i2c_dev->adapter, i2c_dev);
i2c_dev->adapter.owner = THIS_MODULE;
i2c_dev->adapter.class = I2C_CLASS_HWMON;
@@ -749,7 +771,6 @@ static int __devinit tegra_i2c_probe(struct platform_device *pdev)
ret = i2c_add_numbered_adapter(&i2c_dev->adapter);
if (ret) {
dev_err(&pdev->dev, "Failed to add I2C adapter\n");
- clk_disable_unprepare(i2c_dev->fast_clk);
return ret;
}
--
1.7.1.1
On 08/17/2012 01:02 PM, Laxman Dewangan wrote:
> Tegra20 i2c controller does not support the continue transfer
> which implements the I2C_M_NOSTART functionality of i2c
> protocol mangling.
> Removing the I2C_M_NOSTART functionality for Tegra20.
>
> Signed-off-by: Laxman Dewangan <[email protected]>
I tested both these patches and they work fine. I'll hold off applying
them for a while for Wolfram to ack them.
On Sat, Aug 18, 2012 at 12:32:34AM +0530, Laxman Dewangan wrote:
> Tegra20 i2c controller does not support the continue transfer
> which implements the I2C_M_NOSTART functionality of i2c
> protocol mangling.
> Removing the I2C_M_NOSTART functionality for Tegra20.
>
> Signed-off-by: Laxman Dewangan <[email protected]>
> ---
> This is rebased on linux-next-20120816.
>
> drivers/i2c/busses/i2c-tegra.c | 73 +++++++++++++++++++++++++++++++--------
> 1 files changed, 58 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index 7149625..554f713 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -27,6 +27,7 @@
> #include <linux/slab.h>
> #include <linux/i2c-tegra.h>
> #include <linux/of_i2c.h>
> +#include <linux/of_device.h>
> #include <linux/module.h>
>
> #include <asm/unaligned.h>
> @@ -114,6 +115,15 @@ enum msg_end_type {
> };
>
> /**
> + * struct tegra_i2c_hw_feature : Different HW support on Tegra
> + * @has_continue_xfer_support: Continue transfer supports.
> + */
> +
> +struct tegra_i2c_hw_feature {
> + bool has_continue_xfer_support;
> +};
> +
> +/**
> * struct tegra_i2c_dev - per device i2c context
> * @dev: device reference for power management
> * @adapter: core i2c layer adapter information
> @@ -148,6 +158,7 @@ struct tegra_i2c_dev {
> int msg_read;
> unsigned long bus_clk_rate;
> bool is_suspended;
> + bool has_continue_xfer_support;
I wonder if it makes sense to carry a pointer here to the
tegra_i2c_hw_feature in use instead of copying all entries by hand,
since they might get more and more.
> };
>
> static void dvc_writel(struct tegra_i2c_dev *i2c_dev, u32 val, unsigned long reg)
> @@ -563,7 +574,17 @@ static int tegra_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
> if (i2c_dev->is_suspended)
> return -EBUSY;
>
> - clk_prepare_enable(i2c_dev->div_clk);
> + /* Support I2C_M_NOSTART only if HW support continue xfer. */
> + for (i = 0; i < num - 1; i++) {
> + if ((msgs[i + 1].flags & I2C_M_NOSTART) &&
> + !i2c_dev->has_continue_xfer_support) {
> + dev_err(i2c_dev->dev,
> + "mesg %d have illegal flag\n", i + 1);
> + return -EINVAL;
> + }
> + }
Drivers are requested to explicitly check for features of the I2C bus
(like M_NOSTART) before using them, so I'd skip this extra check.
> +
> + clk_prepare_enable(i2c_dev->clk);
From a glimpse, this change looks unrelated at least. Even wrong, no?
> for (i = 0; i < num; i++) {
> enum msg_end_type end_type = MSG_END_STOP;
> if (i < (num - 1)) {
> @@ -582,8 +603,13 @@ static int tegra_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
>
> static u32 tegra_i2c_func(struct i2c_adapter *adap)
> {
> - return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_10BIT_ADDR |
> - I2C_FUNC_PROTOCOL_MANGLING | I2C_FUNC_NOSTART;
> + struct tegra_i2c_dev *i2c_dev = i2c_get_adapdata(adap);
> + u32 ret = I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_10BIT_ADDR |
> + I2C_FUNC_PROTOCOL_MANGLING;
> +
> + if (i2c_dev->has_continue_xfer_support)
> + ret |= I2C_FUNC_NOSTART;
> + return ret;
> }
>
> static const struct i2c_algorithm tegra_i2c_algo = {
> @@ -591,6 +617,25 @@ static const struct i2c_algorithm tegra_i2c_algo = {
> .functionality = tegra_i2c_func,
> };
>
> +static struct tegra_i2c_hw_feature tegra20_i2c_hw = {
> + .has_continue_xfer_support = false,
> +};
> +
> +static struct tegra_i2c_hw_feature tegra30_i2c_hw = {
> + .has_continue_xfer_support = true,
> +};
> +
> +#if defined(CONFIG_OF)
> +/* Match table for of_platform binding */
> +static const struct of_device_id tegra_i2c_of_match[] __devinitconst = {
> + { .compatible = "nvidia,tegra30-i2c", .data = &tegra30_i2c_hw, },
> + { .compatible = "nvidia,tegra20-i2c", .data = &tegra20_i2c_hw, },
> + { .compatible = "nvidia,tegra20-i2c-dvc", .data = &tegra20_i2c_hw, },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, tegra_i2c_of_match);
> +#endif
> +
> static int __devinit tegra_i2c_probe(struct platform_device *pdev)
> {
> struct tegra_i2c_dev *i2c_dev;
> @@ -600,6 +645,7 @@ static int __devinit tegra_i2c_probe(struct platform_device *pdev)
> struct clk *fast_clk;
> const unsigned int *prop;
> void __iomem *base;
> + const struct tegra_i2c_hw_feature *hw = &tegra20_i2c_hw;
> int irq;
> int ret = 0;
>
> @@ -659,11 +705,18 @@ static int __devinit tegra_i2c_probe(struct platform_device *pdev)
> i2c_dev->bus_clk_rate = be32_to_cpup(prop);
> }
>
> - if (pdev->dev.of_node)
> + if (pdev->dev.of_node) {
> + const struct of_device_id *match;
> + match = of_match_device(of_match_ptr(tegra_i2c_of_match),
> + &pdev->dev);
> + hw = match->data;
> i2c_dev->is_dvc = of_device_is_compatible(pdev->dev.of_node,
> "nvidia,tegra20-i2c-dvc");
> - else if (pdev->id == 3)
> + } else if (pdev->id == 3) {
> i2c_dev->is_dvc = 1;
> + }
> +
> + i2c_dev->has_continue_xfer_support = hw->has_continue_xfer_support;
> init_completion(&i2c_dev->msg_complete);
>
> platform_set_drvdata(pdev, i2c_dev);
> @@ -751,16 +804,6 @@ static SIMPLE_DEV_PM_OPS(tegra_i2c_pm, tegra_i2c_suspend, tegra_i2c_resume);
> #define TEGRA_I2C_PM NULL
> #endif
>
> -#if defined(CONFIG_OF)
> -/* Match table for of_platform binding */
> -static const struct of_device_id tegra_i2c_of_match[] __devinitconst = {
> - { .compatible = "nvidia,tegra20-i2c", },
> - { .compatible = "nvidia,tegra20-i2c-dvc", },
> - {},
> -};
> -MODULE_DEVICE_TABLE(of, tegra_i2c_of_match);
> -#endif
> -
> static struct platform_driver tegra_i2c_driver = {
> .probe = tegra_i2c_probe,
> .remove = __devexit_p(tegra_i2c_remove),
Thanks,
Wolfram
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Thanks for review.
On Saturday 18 August 2012 06:17 PM, Wolfram Sang wrote:
> * PGP Signed by an unknown key
>
> On Sat, Aug 18, 2012 at 12:32:34AM +0530, Laxman Dewangan wrote:
>> + bool has_continue_xfer_support;
> I wonder if it makes sense to carry a pointer here to the
> tegra_i2c_hw_feature in use instead of copying all entries by hand,
> since they might get more and more.
>
Ok, we can store the hw pointer in device structure. I will send the new
patch.
>> };
>>
>> static void dvc_writel(struct tegra_i2c_dev *i2c_dev, u32 val, unsigned long reg)
>> @@ -563,7 +574,17 @@ static int tegra_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
>> if (i2c_dev->is_suspended)
>> return -EBUSY;
>>
>> - clk_prepare_enable(i2c_dev->div_clk);
>> + /* Support I2C_M_NOSTART only if HW support continue xfer. */
>> + for (i = 0; i< num - 1; i++) {
>> + if ((msgs[i + 1].flags& I2C_M_NOSTART)&&
>> + !i2c_dev->has_continue_xfer_support) {
>> + dev_err(i2c_dev->dev,
>> + "mesg %d have illegal flag\n", i + 1);
>> + return -EINVAL;
>> + }
>> + }
> Drivers are requested to explicitly check for features of the I2C bus
> (like M_NOSTART) before using them, so I'd skip this extra check.
>
Ok, I kept this as part of flag checking so illegal flag should not be
passed. I will remove this on next version patch.
>> +
>> + clk_prepare_enable(i2c_dev->clk);
> From a glimpse, this change looks unrelated at least. Even wrong, no?
>
It was already there, just before above check. Due to insertion of
check, this code shifted, otherwise it is not a new code.
> >>- clk_prepare_enable(i2c_dev->div_clk);
> >>+ /* Support I2C_M_NOSTART only if HW support continue xfer. */
> >>+ for (i = 0; i< num - 1; i++) {
> >>+ if ((msgs[i + 1].flags& I2C_M_NOSTART)&&
> >>+ !i2c_dev->has_continue_xfer_support) {
> >>+ dev_err(i2c_dev->dev,
> >>+ "mesg %d have illegal flag\n", i + 1);
> >>+ return -EINVAL;
> >>+ }
> >>+ }
> >Drivers are requested to explicitly check for features of the I2C bus
> >(like M_NOSTART) before using them, so I'd skip this extra check.
> >
>
> Ok, I kept this as part of flag checking so illegal flag should not
> be passed. I will remove this on next version patch.
>
> >>+
> >>+ clk_prepare_enable(i2c_dev->clk);
> > From a glimpse, this change looks unrelated at least. Even wrong, no?
> >
>
> It was already there, just before above check. Due to insertion of
> check, this code shifted, otherwise it is not a new code.
It used to be ->div_clk and now it is ->clk?
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On Sat, Aug 18, 2012 at 12:32:35AM +0530, Laxman Dewangan wrote:
> Tegra I2C driver enables the fast clock during initialization
> and does not disable till driver removed.
> Enable this clock before transfer and disable after transfer done.
>
> Signed-off-by: Laxman Dewangan <[email protected]>
Except that this patch is affected from the flaw of the previous patch,
it looks good to me.
> - clk_prepare_enable(i2c_dev->clk);
> + tegra_i2c_clock_enable(i2c_dev);
This should be div_clk.
> for (i = 0; i < num; i++) {
> enum msg_end_type end_type = MSG_END_STOP;
> if (i < (num - 1)) {
> @@ -597,7 +621,7 @@ static int tegra_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
> if (ret)
> break;
> }
> - clk_disable_unprepare(i2c_dev->div_clk);
> + tegra_i2c_clock_disable(i2c_dev);
> return ret ?: i;
If that is fixed, you can add
Reviewed-by: Wolfram Sang <[email protected]>
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |