Re: [PATCHv3 08/35] clk: ti: fix ti_clk_get_reg_addr error handling

2015-03-20 Thread Tero Kristo 

On 03/06/2015 09:18 PM, Mike Turquette wrote:

Quoting Tero Kristo (2015-02-25 11:04:18)

There is a case where NULL can be a valid return value for
ti_clk_get_reg_addr, specifically the case where both the provider index
and register offsets are zero. In this case, the current error checking
against a NULL pointer will fail. Thus, change the API to return a
ERR_PTR value in an error case, and change all the users of this API to
check against IS_ERR instead.

Signed-off-by: Tero Kristo t-kri...@ti.com
Cc: Michael Turquette mturque...@linaro.org


Looks good to me.


I'll interpret this as an Acked-by, will add this tag to the latest 
version of the set I will post later today, thanks.


-Tero



Regards,
Mike


---
  drivers/clk/ti/apll.c  |5 +++--
  drivers/clk/ti/autoidle.c  |2 +-
  drivers/clk/ti/clk.c   |7 ---
  drivers/clk/ti/divider.c   |4 ++--
  drivers/clk/ti/dpll.c  |6 +++---
  drivers/clk/ti/gate.c  |4 ++--
  drivers/clk/ti/interface.c |2 +-
  drivers/clk/ti/mux.c   |4 ++--
  8 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/drivers/clk/ti/apll.c b/drivers/clk/ti/apll.c
index 72d9727..49baf38 100644
--- a/drivers/clk/ti/apll.c
+++ b/drivers/clk/ti/apll.c
@@ -203,7 +203,7 @@ static void __init of_dra7_apll_setup(struct device_node 
*node)
 ad-control_reg = ti_clk_get_reg_addr(node, 0);
 ad-idlest_reg = ti_clk_get_reg_addr(node, 1);

-   if (!ad-control_reg || !ad-idlest_reg)
+   if (IS_ERR(ad-control_reg) || IS_ERR(ad-idlest_reg))
 goto cleanup;

 ad-idlest_mask = 0x1;
@@ -384,7 +384,8 @@ static void __init of_omap2_apll_setup(struct device_node 
*node)
 ad-autoidle_reg = ti_clk_get_reg_addr(node, 1);
 ad-idlest_reg = ti_clk_get_reg_addr(node, 2);

-   if (!ad-control_reg || !ad-autoidle_reg || !ad-idlest_reg)
+   if (IS_ERR(ad-control_reg) || IS_ERR(ad-autoidle_reg) ||
+   IS_ERR(ad-idlest_reg))
 goto cleanup;

 clk = clk_register(NULL, clk_hw-hw);
diff --git a/drivers/clk/ti/autoidle.c b/drivers/clk/ti/autoidle.c
index 8912ff8..e75c64c 100644
--- a/drivers/clk/ti/autoidle.c
+++ b/drivers/clk/ti/autoidle.c
@@ -119,7 +119,7 @@ int __init of_ti_clk_autoidle_setup(struct device_node 
*node)
 clk-name = node-name;
 clk-reg = ti_clk_get_reg_addr(node, 0);

-   if (!clk-reg) {
+   if (IS_ERR(clk-reg)) {
 kfree(clk);
 return -EINVAL;
 }
diff --git a/drivers/clk/ti/clk.c b/drivers/clk/ti/clk.c
index e22b956..0ebe5c5 100644
--- a/drivers/clk/ti/clk.c
+++ b/drivers/clk/ti/clk.c
@@ -103,7 +103,8 @@ int __init ti_clk_retry_init(struct device_node *node, 
struct clk_hw *hw,
   * @index: register index from the clock node
   *
   * Builds clock register address from device tree information. This
- * is a struct of type clk_omap_reg.
+ * is a struct of type clk_omap_reg. Returns a pointer to the register
+ * address, or a pointer error value in failure.
   */
  void __iomem *ti_clk_get_reg_addr(struct device_node *node, int index)
  {
@@ -121,14 +122,14 @@ void __iomem *ti_clk_get_reg_addr(struct device_node 
*node, int index)

 if (i == CLK_MAX_MEMMAPS) {
 pr_err(clk-provider not found for %s!\n, node-name);
-   return NULL;
+   return ERR_PTR(-ENOENT);
 }

 reg-index = i;

 if (of_property_read_u32_index(node, reg, index, val)) {
 pr_err(%s must have reg[%d]!\n, node-name, index);
-   return NULL;
+   return ERR_PTR(-EINVAL);
 }

 reg-offset = val;
diff --git a/drivers/clk/ti/divider.c b/drivers/clk/ti/divider.c
index 6211893..ff5f117 100644
--- a/drivers/clk/ti/divider.c
+++ b/drivers/clk/ti/divider.c
@@ -530,8 +530,8 @@ static int __init ti_clk_divider_populate(struct 
device_node *node,
 u32 val;

 *reg = ti_clk_get_reg_addr(node, 0);
-   if (!*reg)
-   return -EINVAL;
+   if (IS_ERR(*reg))
+   return PTR_ERR(*reg);

 if (!of_property_read_u32(node, ti,bit-shift, val))
 *shift = val;
diff --git a/drivers/clk/ti/dpll.c b/drivers/clk/ti/dpll.c
index 81dc469..11478a5 100644
--- a/drivers/clk/ti/dpll.c
+++ b/drivers/clk/ti/dpll.c
@@ -390,18 +390,18 @@ static void __init of_ti_dpll_setup(struct device_node 
*node,
  #endif
 } else {
 dd-idlest_reg = ti_clk_get_reg_addr(node, 1);
-   if (!dd-idlest_reg)
+   if (IS_ERR(dd-idlest_reg))
 goto cleanup;

 dd-mult_div1_reg = ti_clk_get_reg_addr(node, 2);
 }

-   if (!dd-control_reg || !dd-mult_div1_reg)
+   if (IS_ERR(dd-control_reg) || IS_ERR(dd-mult_div1_reg))
 goto cleanup;

 if (dd-autoidle_mask) {
 dd-autoidle_reg = ti_clk_get_reg_addr(node, 3);
-   if (!dd-autoidle_reg)
+

Re: [PATCHv3 08/35] clk: ti: fix ti_clk_get_reg_addr error handling

2015-03-19 Thread Tero Kristo 

On 03/18/2015 07:02 PM, Tony Lindgren wrote:

* Tero Kristo t-kri...@ti.com [150318 00:06]:

On 03/17/2015 08:38 PM, Tony Lindgren wrote:

* Mike Turquette mturque...@linaro.org [150306 11:18]:

Quoting Tero Kristo (2015-02-25 11:04:18)

There is a case where NULL can be a valid return value for
ti_clk_get_reg_addr, specifically the case where both the provider index
and register offsets are zero. In this case, the current error checking
against a NULL pointer will fail. Thus, change the API to return a
ERR_PTR value in an error case, and change all the users of this API to
check against IS_ERR instead.

Signed-off-by: Tero Kristo t-kri...@ti.com
Cc: Michael Turquette mturque...@linaro.org


Looks good to me.

...


---
  drivers/clk/ti/apll.c  |5 +++--
  drivers/clk/ti/autoidle.c  |2 +-
  drivers/clk/ti/clk.c   |7 ---
  drivers/clk/ti/divider.c   |4 ++--
  drivers/clk/ti/dpll.c  |6 +++---
  drivers/clk/ti/gate.c  |4 ++--
  drivers/clk/ti/interface.c |2 +-
  drivers/clk/ti/mux.c   |4 ++--
  8 files changed, 18 insertions(+), 16 deletions(-)


Can this patch be queued separately by Mike or is there some
dependency to this series?


Without this patch, patch #10 in the set causes a boot failure on omap3,
because the specific NULL value is returned for iva2_ck and the clock
register fails. This in turn breaks hwmod registration because iva2_ck is
missing.


Oh OK.


I would just queue this patch as part of this series to avoid any trouble.


Can this patch be applied separately before this series or does
it cause other problems? If it can be separated, Mike can maybe put it
into an immutable branch that I can merge in too.


Yea, that works also if Mike is okay with it.

-Tero




Other than wondering about the above and the dts related comments,
this series works for me with PM tests.


I hope to post a series with the dts related comments fixed later today.


Yes I'll take a look, thanks for doing that.

Regards,

Tony



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

Re: [PATCHv3 08/35] clk: ti: fix ti_clk_get_reg_addr error handling

2015-03-18 Thread Tony Lindgren 
* Tero Kristo t-kri...@ti.com [150318 00:06]:
 On 03/17/2015 08:38 PM, Tony Lindgren wrote:
 * Mike Turquette mturque...@linaro.org [150306 11:18]:
 Quoting Tero Kristo (2015-02-25 11:04:18)
 There is a case where NULL can be a valid return value for
 ti_clk_get_reg_addr, specifically the case where both the provider index
 and register offsets are zero. In this case, the current error checking
 against a NULL pointer will fail. Thus, change the API to return a
 ERR_PTR value in an error case, and change all the users of this API to
 check against IS_ERR instead.
 
 Signed-off-by: Tero Kristo t-kri...@ti.com
 Cc: Michael Turquette mturque...@linaro.org
 
 Looks good to me.
 ...
 
 ---
   drivers/clk/ti/apll.c  |5 +++--
   drivers/clk/ti/autoidle.c  |2 +-
   drivers/clk/ti/clk.c   |7 ---
   drivers/clk/ti/divider.c   |4 ++--
   drivers/clk/ti/dpll.c  |6 +++---
   drivers/clk/ti/gate.c  |4 ++--
   drivers/clk/ti/interface.c |2 +-
   drivers/clk/ti/mux.c   |4 ++--
   8 files changed, 18 insertions(+), 16 deletions(-)
 
 Can this patch be queued separately by Mike or is there some
 dependency to this series?
 
 Without this patch, patch #10 in the set causes a boot failure on omap3,
 because the specific NULL value is returned for iva2_ck and the clock
 register fails. This in turn breaks hwmod registration because iva2_ck is
 missing.

Oh OK.

 I would just queue this patch as part of this series to avoid any trouble.

Can this patch be applied separately before this series or does
it cause other problems? If it can be separated, Mike can maybe put it
into an immutable branch that I can merge in too.
 
 Other than wondering about the above and the dts related comments,
 this series works for me with PM tests.
 
 I hope to post a series with the dts related comments fixed later today.

Yes I'll take a look, thanks for doing that.

Regards,

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

Re: [PATCHv3 08/35] clk: ti: fix ti_clk_get_reg_addr error handling

2015-03-18 Thread Tero Kristo 

On 03/17/2015 08:38 PM, Tony Lindgren wrote:

* Mike Turquette mturque...@linaro.org [150306 11:18]:

Quoting Tero Kristo (2015-02-25 11:04:18)

There is a case where NULL can be a valid return value for
ti_clk_get_reg_addr, specifically the case where both the provider index
and register offsets are zero. In this case, the current error checking
against a NULL pointer will fail. Thus, change the API to return a
ERR_PTR value in an error case, and change all the users of this API to
check against IS_ERR instead.

Signed-off-by: Tero Kristo t-kri...@ti.com
Cc: Michael Turquette mturque...@linaro.org


Looks good to me.

...


---
  drivers/clk/ti/apll.c  |5 +++--
  drivers/clk/ti/autoidle.c  |2 +-
  drivers/clk/ti/clk.c   |7 ---
  drivers/clk/ti/divider.c   |4 ++--
  drivers/clk/ti/dpll.c  |6 +++---
  drivers/clk/ti/gate.c  |4 ++--
  drivers/clk/ti/interface.c |2 +-
  drivers/clk/ti/mux.c   |4 ++--
  8 files changed, 18 insertions(+), 16 deletions(-)


Can this patch be queued separately by Mike or is there some
dependency to this series?


Without this patch, patch #10 in the set causes a boot failure on omap3, 
because the specific NULL value is returned for iva2_ck and the clock 
register fails. This in turn breaks hwmod registration because iva2_ck 
is missing.


I would just queue this patch as part of this series to avoid any trouble.



Other than wondering about the above and the dts related comments,
this series works for me with PM tests.


I hope to post a series with the dts related comments fixed later today.

-Tero



Regards,

Tony



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

Re: [PATCHv3 08/35] clk: ti: fix ti_clk_get_reg_addr error handling

2015-03-17 Thread Tony Lindgren 
* Mike Turquette mturque...@linaro.org [150306 11:18]:
 Quoting Tero Kristo (2015-02-25 11:04:18)
  There is a case where NULL can be a valid return value for
  ti_clk_get_reg_addr, specifically the case where both the provider index
  and register offsets are zero. In this case, the current error checking
  against a NULL pointer will fail. Thus, change the API to return a
  ERR_PTR value in an error case, and change all the users of this API to
  check against IS_ERR instead.
  
  Signed-off-by: Tero Kristo t-kri...@ti.com
  Cc: Michael Turquette mturque...@linaro.org
 
 Looks good to me.
...

  ---
   drivers/clk/ti/apll.c  |5 +++--
   drivers/clk/ti/autoidle.c  |2 +-
   drivers/clk/ti/clk.c   |7 ---
   drivers/clk/ti/divider.c   |4 ++--
   drivers/clk/ti/dpll.c  |6 +++---
   drivers/clk/ti/gate.c  |4 ++--
   drivers/clk/ti/interface.c |2 +-
   drivers/clk/ti/mux.c   |4 ++--
   8 files changed, 18 insertions(+), 16 deletions(-)

Can this patch be queued separately by Mike or is there some
dependency to this series?

Other than wondering about the above and the dts related comments,
this series works for me with PM tests.

Regards,

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

Re: [PATCHv3 08/35] clk: ti: fix ti_clk_get_reg_addr error handling

2015-03-06 Thread Mike Turquette 
Quoting Tero Kristo (2015-02-25 11:04:18)
 There is a case where NULL can be a valid return value for
 ti_clk_get_reg_addr, specifically the case where both the provider index
 and register offsets are zero. In this case, the current error checking
 against a NULL pointer will fail. Thus, change the API to return a
 ERR_PTR value in an error case, and change all the users of this API to
 check against IS_ERR instead.
 
 Signed-off-by: Tero Kristo t-kri...@ti.com
 Cc: Michael Turquette mturque...@linaro.org

Looks good to me.

Regards,
Mike

 ---
  drivers/clk/ti/apll.c  |5 +++--
  drivers/clk/ti/autoidle.c  |2 +-
  drivers/clk/ti/clk.c   |7 ---
  drivers/clk/ti/divider.c   |4 ++--
  drivers/clk/ti/dpll.c  |6 +++---
  drivers/clk/ti/gate.c  |4 ++--
  drivers/clk/ti/interface.c |2 +-
  drivers/clk/ti/mux.c   |4 ++--
  8 files changed, 18 insertions(+), 16 deletions(-)
 
 diff --git a/drivers/clk/ti/apll.c b/drivers/clk/ti/apll.c
 index 72d9727..49baf38 100644
 --- a/drivers/clk/ti/apll.c
 +++ b/drivers/clk/ti/apll.c
 @@ -203,7 +203,7 @@ static void __init of_dra7_apll_setup(struct device_node 
 *node)
 ad-control_reg = ti_clk_get_reg_addr(node, 0);
 ad-idlest_reg = ti_clk_get_reg_addr(node, 1);
  
 -   if (!ad-control_reg || !ad-idlest_reg)
 +   if (IS_ERR(ad-control_reg) || IS_ERR(ad-idlest_reg))
 goto cleanup;
  
 ad-idlest_mask = 0x1;
 @@ -384,7 +384,8 @@ static void __init of_omap2_apll_setup(struct device_node 
 *node)
 ad-autoidle_reg = ti_clk_get_reg_addr(node, 1);
 ad-idlest_reg = ti_clk_get_reg_addr(node, 2);
  
 -   if (!ad-control_reg || !ad-autoidle_reg || !ad-idlest_reg)
 +   if (IS_ERR(ad-control_reg) || IS_ERR(ad-autoidle_reg) ||
 +   IS_ERR(ad-idlest_reg))
 goto cleanup;
  
 clk = clk_register(NULL, clk_hw-hw);
 diff --git a/drivers/clk/ti/autoidle.c b/drivers/clk/ti/autoidle.c
 index 8912ff8..e75c64c 100644
 --- a/drivers/clk/ti/autoidle.c
 +++ b/drivers/clk/ti/autoidle.c
 @@ -119,7 +119,7 @@ int __init of_ti_clk_autoidle_setup(struct device_node 
 *node)
 clk-name = node-name;
 clk-reg = ti_clk_get_reg_addr(node, 0);
  
 -   if (!clk-reg) {
 +   if (IS_ERR(clk-reg)) {
 kfree(clk);
 return -EINVAL;
 }
 diff --git a/drivers/clk/ti/clk.c b/drivers/clk/ti/clk.c
 index e22b956..0ebe5c5 100644
 --- a/drivers/clk/ti/clk.c
 +++ b/drivers/clk/ti/clk.c
 @@ -103,7 +103,8 @@ int __init ti_clk_retry_init(struct device_node *node, 
 struct clk_hw *hw,
   * @index: register index from the clock node
   *
   * Builds clock register address from device tree information. This
 - * is a struct of type clk_omap_reg.
 + * is a struct of type clk_omap_reg. Returns a pointer to the register
 + * address, or a pointer error value in failure.
   */
  void __iomem *ti_clk_get_reg_addr(struct device_node *node, int index)
  {
 @@ -121,14 +122,14 @@ void __iomem *ti_clk_get_reg_addr(struct device_node 
 *node, int index)
  
 if (i == CLK_MAX_MEMMAPS) {
 pr_err(clk-provider not found for %s!\n, node-name);
 -   return NULL;
 +   return ERR_PTR(-ENOENT);
 }
  
 reg-index = i;
  
 if (of_property_read_u32_index(node, reg, index, val)) {
 pr_err(%s must have reg[%d]!\n, node-name, index);
 -   return NULL;
 +   return ERR_PTR(-EINVAL);
 }
  
 reg-offset = val;
 diff --git a/drivers/clk/ti/divider.c b/drivers/clk/ti/divider.c
 index 6211893..ff5f117 100644
 --- a/drivers/clk/ti/divider.c
 +++ b/drivers/clk/ti/divider.c
 @@ -530,8 +530,8 @@ static int __init ti_clk_divider_populate(struct 
 device_node *node,
 u32 val;
  
 *reg = ti_clk_get_reg_addr(node, 0);
 -   if (!*reg)
 -   return -EINVAL;
 +   if (IS_ERR(*reg))
 +   return PTR_ERR(*reg);
  
 if (!of_property_read_u32(node, ti,bit-shift, val))
 *shift = val;
 diff --git a/drivers/clk/ti/dpll.c b/drivers/clk/ti/dpll.c
 index 81dc469..11478a5 100644
 --- a/drivers/clk/ti/dpll.c
 +++ b/drivers/clk/ti/dpll.c
 @@ -390,18 +390,18 @@ static void __init of_ti_dpll_setup(struct device_node 
 *node,
  #endif
 } else {
 dd-idlest_reg = ti_clk_get_reg_addr(node, 1);
 -   if (!dd-idlest_reg)
 +   if (IS_ERR(dd-idlest_reg))
 goto cleanup;
  
 dd-mult_div1_reg = ti_clk_get_reg_addr(node, 2);
 }
  
 -   if (!dd-control_reg || !dd-mult_div1_reg)
 +   if (IS_ERR(dd-control_reg) || IS_ERR(dd-mult_div1_reg))
 goto cleanup;
  
 if (dd-autoidle_mask) {
 dd-autoidle_reg = ti_clk_get_reg_addr(node, 3);
 -   if (!dd-autoidle_reg)
 +   if (IS_ERR(dd-autoidle_reg))
 goto cleanup;