linux-i2c.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH REBASE 1/2] i2c: tegra: I2_M_NOSTART functionality not supported in Tegra20
@ 2012-08-17 19:02 Laxman Dewangan
       [not found] ` <1345230155-4252-1-git-send-email-ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
  0 siblings, 1 reply; 7+ messages in thread
From: Laxman Dewangan @ 2012-08-17 19:02 UTC (permalink / raw)
  To: khali-PUYAD+kWke1g9hUCZPvPmw, w.sang-bIcnvbaLZ9MEGnE8C9+IrQ,
	swarren-DDmLM1+adcrQT0dZR+AlfA
  Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-tegra-u79uwXL29TY76Z2rM5mHXA, olof-nZhT3qVonbNeoWH0uzbU5w,
	Laxman Dewangan

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 <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
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

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH REBASE 2/2] i2c: tegra: dynamically control fast clk
       [not found] ` <1345230155-4252-1-git-send-email-ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
@ 2012-08-17 19:02   ` Laxman Dewangan
       [not found]     ` <1345230155-4252-2-git-send-email-ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
  2012-08-17 21:35   ` [PATCH REBASE 1/2] i2c: tegra: I2_M_NOSTART functionality not supported in Tegra20 Stephen Warren
  2012-08-18 12:47   ` Wolfram Sang
  2 siblings, 1 reply; 7+ messages in thread
From: Laxman Dewangan @ 2012-08-17 19:02 UTC (permalink / raw)
  To: khali-PUYAD+kWke1g9hUCZPvPmw, w.sang-bIcnvbaLZ9MEGnE8C9+IrQ,
	swarren-DDmLM1+adcrQT0dZR+AlfA
  Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-tegra-u79uwXL29TY76Z2rM5mHXA, olof-nZhT3qVonbNeoWH0uzbU5w,
	Laxman Dewangan

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 <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
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

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH REBASE 1/2] i2c: tegra: I2_M_NOSTART functionality not supported in Tegra20
       [not found] ` <1345230155-4252-1-git-send-email-ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
  2012-08-17 19:02   ` [PATCH REBASE 2/2] i2c: tegra: dynamically control fast clk Laxman Dewangan
@ 2012-08-17 21:35   ` Stephen Warren
  2012-08-18 12:47   ` Wolfram Sang
  2 siblings, 0 replies; 7+ messages in thread
From: Stephen Warren @ 2012-08-17 21:35 UTC (permalink / raw)
  To: Laxman Dewangan, w.sang-bIcnvbaLZ9MEGnE8C9+IrQ
  Cc: khali-PUYAD+kWke1g9hUCZPvPmw, swarren-DDmLM1+adcrQT0dZR+AlfA,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-tegra-u79uwXL29TY76Z2rM5mHXA, olof-nZhT3qVonbNeoWH0uzbU5w

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 <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

I tested both these patches and they work fine. I'll hold off applying
them for a while for Wolfram to ack them.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH REBASE 1/2] i2c: tegra: I2_M_NOSTART functionality not supported in Tegra20
       [not found] ` <1345230155-4252-1-git-send-email-ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
  2012-08-17 19:02   ` [PATCH REBASE 2/2] i2c: tegra: dynamically control fast clk Laxman Dewangan
  2012-08-17 21:35   ` [PATCH REBASE 1/2] i2c: tegra: I2_M_NOSTART functionality not supported in Tegra20 Stephen Warren
@ 2012-08-18 12:47   ` Wolfram Sang
       [not found]     ` <20120818124703.GC12839-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
  2 siblings, 1 reply; 7+ messages in thread
From: Wolfram Sang @ 2012-08-18 12:47 UTC (permalink / raw)
  To: Laxman Dewangan
  Cc: khali-PUYAD+kWke1g9hUCZPvPmw, swarren-DDmLM1+adcrQT0dZR+AlfA,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-tegra-u79uwXL29TY76Z2rM5mHXA, olof-nZhT3qVonbNeoWH0uzbU5w

[-- Attachment #1: Type: text/plain, Size: 6057 bytes --]

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 <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
> 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/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH REBASE 1/2] i2c: tegra: I2_M_NOSTART functionality not supported in Tegra20
       [not found]     ` <20120818124703.GC12839-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
@ 2012-08-18 12:50       ` Laxman Dewangan
       [not found]         ` <502F8F83.5030902-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
  0 siblings, 1 reply; 7+ messages in thread
From: Laxman Dewangan @ 2012-08-18 12:50 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org, Stephen Warren,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	olof-nZhT3qVonbNeoWH0uzbU5w@public.gmane.org

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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH REBASE 1/2] i2c: tegra: I2_M_NOSTART functionality not supported in Tegra20
       [not found]         ` <502F8F83.5030902-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
@ 2012-08-18 18:29           ` Wolfram Sang
  0 siblings, 0 replies; 7+ messages in thread
From: Wolfram Sang @ 2012-08-18 18:29 UTC (permalink / raw)
  To: Laxman Dewangan
  Cc: khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org, Stephen Warren,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	olof-nZhT3qVonbNeoWH0uzbU5w@public.gmane.org

[-- Attachment #1: Type: text/plain, Size: 1131 bytes --]


> >>-	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/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH REBASE 2/2] i2c: tegra: dynamically control fast clk
       [not found]     ` <1345230155-4252-2-git-send-email-ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
@ 2012-08-18 18:52       ` Wolfram Sang
  0 siblings, 0 replies; 7+ messages in thread
From: Wolfram Sang @ 2012-08-18 18:52 UTC (permalink / raw)
  To: Laxman Dewangan
  Cc: khali-PUYAD+kWke1g9hUCZPvPmw, swarren-DDmLM1+adcrQT0dZR+AlfA,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-tegra-u79uwXL29TY76Z2rM5mHXA, olof-nZhT3qVonbNeoWH0uzbU5w

[-- Attachment #1: Type: text/plain, Size: 1161 bytes --]

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 <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

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 <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2012-08-18 18:52 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-08-17 19:02 [PATCH REBASE 1/2] i2c: tegra: I2_M_NOSTART functionality not supported in Tegra20 Laxman Dewangan
     [not found] ` <1345230155-4252-1-git-send-email-ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
2012-08-17 19:02   ` [PATCH REBASE 2/2] i2c: tegra: dynamically control fast clk Laxman Dewangan
     [not found]     ` <1345230155-4252-2-git-send-email-ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
2012-08-18 18:52       ` Wolfram Sang
2012-08-17 21:35   ` [PATCH REBASE 1/2] i2c: tegra: I2_M_NOSTART functionality not supported in Tegra20 Stephen Warren
2012-08-18 12:47   ` Wolfram Sang
     [not found]     ` <20120818124703.GC12839-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
2012-08-18 12:50       ` Laxman Dewangan
     [not found]         ` <502F8F83.5030902-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
2012-08-18 18:29           ` Wolfram Sang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).