diff mbox series

[v4,1/5] dm: clk: add stub when CONFIG_CLK is desactivated

Message ID 20200218083503.5468-2-patrick.delaunay@st.com
State New
Headers show
Series usb: host: dwc2: use driver model for PHY and CLOCK | expand

Commit Message

Patrick Delaunay Feb. 18, 2020, 8:34 a.m. UTC
Add stub for functions clk_...() when CONFIG_CLK is desactivated.

This patch avoids compilation issues for driver using these API
without protection (#if CONFIG_IS_ENABLED(CLK))

For example, before this patch we have undefined reference to
`clk_disable_bulk') for code:
  clk_disable_bulk(&priv->clks);
  clk_release_bulk(&priv->clks);

Signed-off-by: Patrick Delaunay <patrick.delaunay at st.com>
---

Changes in v4:
- Add stub for all functions using 'struct clk' or 'struct clk_bulk'
  after remarks on v3

Changes in v3:
- Add stub for clk_disable_bulk

Changes in v2: None

 include/clk.h | 101 +++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 88 insertions(+), 13 deletions(-)

Comments

Simon Goldschmidt March 4, 2020, 7:48 p.m. UTC | #1
Am 18.02.2020 um 09:34 schrieb Patrick Delaunay:
> Add stub for functions clk_...() when CONFIG_CLK is desactivated.
> 
> This patch avoids compilation issues for driver using these API
> without protection (#if CONFIG_IS_ENABLED(CLK))
> 
> For example, before this patch we have undefined reference to
> `clk_disable_bulk') for code:
>   clk_disable_bulk(&priv->clks);
>   clk_release_bulk(&priv->clks);
> 
> Signed-off-by: Patrick Delaunay <patrick.delaunay at st.com>
> ---
> 
> Changes in v4:
> - Add stub for all functions using 'struct clk' or 'struct clk_bulk'
>   after remarks on v3
> 
> Changes in v3:
> - Add stub for clk_disable_bulk
> 
> Changes in v2: None
> 
>  include/clk.h | 101 +++++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 88 insertions(+), 13 deletions(-)
> 
> diff --git a/include/clk.h b/include/clk.h
> index 3336301815..1fb415ddc8 100644
> --- a/include/clk.h
> +++ b/include/clk.h
> @@ -312,6 +312,7 @@ static inline int clk_release_bulk(struct clk_bulk *bulk)
>  	return clk_release_all(bulk->clks, bulk->count);
>  }
>  
> +#if CONFIG_IS_ENABLED(CLK)
>  /**
>   * clk_request - Request a clock by provider-specific ID.
>   *
> @@ -433,19 +434,6 @@ int clk_disable_bulk(struct clk_bulk *bulk);
>   */
>  bool clk_is_match(const struct clk *p, const struct clk *q);
>  
> -int soc_clk_dump(void);
> -
> -/**
> - * clk_valid() - check if clk is valid
> - *
> - * @clk:	the clock to check
> - * @return true if valid, or false
> - */
> -static inline bool clk_valid(struct clk *clk)
> -{
> -	return clk && !!clk->dev;
> -}
> -
>  /**
>   * clk_get_by_id() - Get the clock by its ID
>   *
> @@ -465,6 +453,93 @@ int clk_get_by_id(ulong id, struct clk **clkp);
>   * @return true on binded, or false on no
>   */
>  bool clk_dev_binded(struct clk *clk);
> +
> +#else /* CONFIG_IS_ENABLED(CLK) */
> +
> +static inline int clk_request(struct udevice *dev, struct clk *clk)
> +{
> +	return -ENOSYS;
> +}
> +
> +static inline int clk_free(struct clk *clk)
> +{
> +	return -ENOSYS;
> +}
> +
> +static inline ulong clk_get_rate(struct clk *clk)
> +{
> +	return -ENOSYS;
> +}
> +
> +static inline struct clk *clk_get_parent(struct clk *clk)
> +{
> +	return (struct clk *)-ENOSYS;

This should use ERR_PTR() to care for platforms defining
CONFIG_ERR_PTR_OFFSET.

> +}
> +
> +static inline long long clk_get_parent_rate(struct clk *clk)
> +{
> +	return -ENOSYS;
> +}
> +
> +static inline ulong clk_set_rate(struct clk *clk, ulong rate)
> +{
> +	return -ENOSYS;
> +}
> +
> +static inline int clk_set_parent(struct clk *clk, struct clk *parent)
> +{
> +	return -ENOSYS;
> +}
> +
> +static inline int clk_enable(struct clk *clk)
> +{
> +	return -ENOSYS;
> +}
> +
> +static inline int clk_enable_bulk(struct clk_bulk *bulk)
> +{
> +	return bulk && bulk->count == 0 ? 0 : -ENOSYS;

For this test to work, someone would need to set bulk->count to 0. This
is normally done by clk_get_bulk(), but you defined it to only return an
error.

I guess it works for you because all clk_bulk objects you use are from
the heap (which is currently zeroed out in U-Boot) or if they are on the
stack, you have if/else code that doesn't bring you here. Still it seems
wrong.

Regards,
Simon

> +}
> +
> +static inline int clk_disable(struct clk *clk)
> +{
> +	return -ENOSYS;
> +}
> +
> +static inline int clk_disable_bulk(struct clk_bulk *bulk)
> +{
> +	return bulk && bulk->count == 0 ? 0 : -ENOSYS;
> +}
> +
> +static inline bool clk_is_match(const struct clk *p, const struct clk *q)
> +{
> +	return false;
> +}
> +
> +static inline int clk_get_by_id(ulong id, struct clk **clkp)
> +{
> +	return -ENOSYS;
> +}
> +
> +static inline bool clk_dev_binded(struct clk *clk)
> +{
> +	return false;
> +}
> +#endif /* CONFIG_IS_ENABLED(CLK) */
> +
> +/**
> + * clk_valid() - check if clk is valid
> + *
> + * @clk:	the clock to check
> + * @return true if valid, or false
> + */
> +static inline bool clk_valid(struct clk *clk)
> +{
> +	return clk && !!clk->dev;
> +}
> +
> +int soc_clk_dump(void);
> +
>  #endif
>  
>  #define clk_prepare_enable(clk) clk_enable(clk)
>
Patrick Delaunay March 5, 2020, 5:57 p.m. UTC | #2
Hi Simon,

> From: Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com>
> Sent: mercredi 4 mars 2020 20:49
> 
> Am 18.02.2020 um 09:34 schrieb Patrick Delaunay:
> > Add stub for functions clk_...() when CONFIG_CLK is desactivated.
> >
> > This patch avoids compilation issues for driver using these API
> > without protection (#if CONFIG_IS_ENABLED(CLK))
> >
> > For example, before this patch we have undefined reference to
> > `clk_disable_bulk') for code:
> >   clk_disable_bulk(&priv->clks);
> >   clk_release_bulk(&priv->clks);
> >
> > Signed-off-by: Patrick Delaunay <patrick.delaunay at st.com>
> > ---
> >
> > Changes in v4:
> > - Add stub for all functions using 'struct clk' or 'struct clk_bulk'
> >   after remarks on v3
> >
> > Changes in v3:
> > - Add stub for clk_disable_bulk
> >
> > Changes in v2: None
> >
> >  include/clk.h | 101
> > +++++++++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 88 insertions(+), 13 deletions(-)
> >
> > diff --git a/include/clk.h b/include/clk.h index
> > 3336301815..1fb415ddc8 100644
> > --- a/include/clk.h
> > +++ b/include/clk.h
> > @@ -312,6 +312,7 @@ static inline int clk_release_bulk(struct clk_bulk *bulk)
> >  	return clk_release_all(bulk->clks, bulk->count);  }
> >
> > +#if CONFIG_IS_ENABLED(CLK)
> >  /**
> >   * clk_request - Request a clock by provider-specific ID.
> >   *
> > @@ -433,19 +434,6 @@ int clk_disable_bulk(struct clk_bulk *bulk);
> >   */
> >  bool clk_is_match(const struct clk *p, const struct clk *q);
> >
> > -int soc_clk_dump(void);
> > -
> > -/**
> > - * clk_valid() - check if clk is valid
> > - *
> > - * @clk:	the clock to check
> > - * @return true if valid, or false
> > - */
> > -static inline bool clk_valid(struct clk *clk) -{
> > -	return clk && !!clk->dev;
> > -}
> > -
> >  /**
> >   * clk_get_by_id() - Get the clock by its ID
> >   *
> > @@ -465,6 +453,93 @@ int clk_get_by_id(ulong id, struct clk **clkp);
> >   * @return true on binded, or false on no
> >   */
> >  bool clk_dev_binded(struct clk *clk);
> > +
> > +#else /* CONFIG_IS_ENABLED(CLK) */
> > +
> > +static inline int clk_request(struct udevice *dev, struct clk *clk) {
> > +	return -ENOSYS;
> > +}
> > +
> > +static inline int clk_free(struct clk *clk) {
> > +	return -ENOSYS;
> > +}
> > +
> > +static inline ulong clk_get_rate(struct clk *clk) {
> > +	return -ENOSYS;
> > +}
> > +
> > +static inline struct clk *clk_get_parent(struct clk *clk) {
> > +	return (struct clk *)-ENOSYS;
> 
> This should use ERR_PTR() to care for platforms defining
> CONFIG_ERR_PTR_OFFSET.

Yes I take the point for V5.

> > +}
> > +
> > +static inline long long clk_get_parent_rate(struct clk *clk) {
> > +	return -ENOSYS;
> > +}
> > +
> > +static inline ulong clk_set_rate(struct clk *clk, ulong rate) {
> > +	return -ENOSYS;
> > +}
> > +
> > +static inline int clk_set_parent(struct clk *clk, struct clk *parent)
> > +{
> > +	return -ENOSYS;
> > +}
> > +
> > +static inline int clk_enable(struct clk *clk) {
> > +	return -ENOSYS;
> > +}
> > +
> > +static inline int clk_enable_bulk(struct clk_bulk *bulk) {
> > +	return bulk && bulk->count == 0 ? 0 : -ENOSYS;
> 
> For this test to work, someone would need to set bulk->count to 0. This is normally
> done by clk_get_bulk(), but you defined it to only return an error.
> 
> I guess it works for you because all clk_bulk objects you use are from the heap
> (which is currently zeroed out in U-Boot) or if they are on the stack, you have
> if/else code that doesn't bring you here. Still it seems wrong.

Yes exactly, it is working for me as the bulk was in private date, so zero allocated.

I will update on V5

> Regards,
> Simon

Thanks for the review
diff mbox series

Patch

diff --git a/include/clk.h b/include/clk.h
index 3336301815..1fb415ddc8 100644
--- a/include/clk.h
+++ b/include/clk.h
@@ -312,6 +312,7 @@  static inline int clk_release_bulk(struct clk_bulk *bulk)
 	return clk_release_all(bulk->clks, bulk->count);
 }
 
+#if CONFIG_IS_ENABLED(CLK)
 /**
  * clk_request - Request a clock by provider-specific ID.
  *
@@ -433,19 +434,6 @@  int clk_disable_bulk(struct clk_bulk *bulk);
  */
 bool clk_is_match(const struct clk *p, const struct clk *q);
 
-int soc_clk_dump(void);
-
-/**
- * clk_valid() - check if clk is valid
- *
- * @clk:	the clock to check
- * @return true if valid, or false
- */
-static inline bool clk_valid(struct clk *clk)
-{
-	return clk && !!clk->dev;
-}
-
 /**
  * clk_get_by_id() - Get the clock by its ID
  *
@@ -465,6 +453,93 @@  int clk_get_by_id(ulong id, struct clk **clkp);
  * @return true on binded, or false on no
  */
 bool clk_dev_binded(struct clk *clk);
+
+#else /* CONFIG_IS_ENABLED(CLK) */
+
+static inline int clk_request(struct udevice *dev, struct clk *clk)
+{
+	return -ENOSYS;
+}
+
+static inline int clk_free(struct clk *clk)
+{
+	return -ENOSYS;
+}
+
+static inline ulong clk_get_rate(struct clk *clk)
+{
+	return -ENOSYS;
+}
+
+static inline struct clk *clk_get_parent(struct clk *clk)
+{
+	return (struct clk *)-ENOSYS;
+}
+
+static inline long long clk_get_parent_rate(struct clk *clk)
+{
+	return -ENOSYS;
+}
+
+static inline ulong clk_set_rate(struct clk *clk, ulong rate)
+{
+	return -ENOSYS;
+}
+
+static inline int clk_set_parent(struct clk *clk, struct clk *parent)
+{
+	return -ENOSYS;
+}
+
+static inline int clk_enable(struct clk *clk)
+{
+	return -ENOSYS;
+}
+
+static inline int clk_enable_bulk(struct clk_bulk *bulk)
+{
+	return bulk && bulk->count == 0 ? 0 : -ENOSYS;
+}
+
+static inline int clk_disable(struct clk *clk)
+{
+	return -ENOSYS;
+}
+
+static inline int clk_disable_bulk(struct clk_bulk *bulk)
+{
+	return bulk && bulk->count == 0 ? 0 : -ENOSYS;
+}
+
+static inline bool clk_is_match(const struct clk *p, const struct clk *q)
+{
+	return false;
+}
+
+static inline int clk_get_by_id(ulong id, struct clk **clkp)
+{
+	return -ENOSYS;
+}
+
+static inline bool clk_dev_binded(struct clk *clk)
+{
+	return false;
+}
+#endif /* CONFIG_IS_ENABLED(CLK) */
+
+/**
+ * clk_valid() - check if clk is valid
+ *
+ * @clk:	the clock to check
+ * @return true if valid, or false
+ */
+static inline bool clk_valid(struct clk *clk)
+{
+	return clk && !!clk->dev;
+}
+
+int soc_clk_dump(void);
+
 #endif
 
 #define clk_prepare_enable(clk) clk_enable(clk)