Re: [PATCH REBASE 1/2] i2c: tegra: I2_M_NOSTART functionality not supported in Tegra20

2012-08-18 Thread Wolfram Sang

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


signature.asc
Description: Digital signature


Re: [PATCH REBASE 1/2] i2c: tegra: I2_M_NOSTART functionality not supported in Tegra20

2012-08-18 Thread Laxman Dewangan

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.


--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH REBASE 1/2] i2c: tegra: I2_M_NOSTART functionality not supported in Tegra20

2012-08-18 Thread Wolfram Sang
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 
> ---
> 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 
>  #include 
>  #include 
> +#include 
>  #include 
>  
>  #include 
> @@ -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);
> + 

Re: [PATCH REBASE 1/2] i2c: tegra: I2_M_NOSTART functionality not supported in Tegra20

2012-08-17 Thread Stephen Warren
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 

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

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html