| Message ID | 20181204163403.32321-1-jbrunet@baylibre.com |
|---|---|
| State | New |
| Headers | show |
| Series | clk: fix clk_mux_val_to_index() error value | expand |
Quoting Jerome Brunet (2018-12-04 08:34:03) > clk_mux_val_to_index() is meant to be used by .get_parent(), which > returns a u8, so when the value provided does not map to any valid index, > it is not a good idea to return a negative error value. > > Instead, return num_parents which we know is an invalid index and let > CCF deal with it. > > Fixes: 77deb66d262f ("clk: mux: add helper function for index/value translation") > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> > --- Thanks! > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h > index 60c51871b04b..fc20886ef069 100644 > --- a/include/linux/clk-provider.h > +++ b/include/linux/clk-provider.h > @@ -550,8 +550,8 @@ struct clk_hw *clk_hw_register_mux_table(struct device *dev, const char *name, > void __iomem *reg, u8 shift, u32 mask, > u8 clk_mux_flags, u32 *table, spinlock_t *lock); > > -int clk_mux_val_to_index(struct clk_hw *hw, u32 *table, unsigned int flags, > - unsigned int val); > +u8 clk_mux_val_to_index(struct clk_hw *hw, u32 *table, unsigned int flags, I wonder if we should just make this unsigned int? Does it hurt at all to have it be a wider type even though it doesn't match the CCF decision to make this a u8 for the parent index number space?
On Tue, 2018-12-04 at 10:43 -0800, Stephen Boyd wrote: > Quoting Jerome Brunet (2018-12-04 08:34:03) > > clk_mux_val_to_index() is meant to be used by .get_parent(), which > > returns a u8, so when the value provided does not map to any valid index, > > it is not a good idea to return a negative error value. > > > > Instead, return num_parents which we know is an invalid index and let > > CCF deal with it. > > > > Fixes: 77deb66d262f ("clk: mux: add helper function for index/value > > translation") > > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> > > --- > > Thanks! > > > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h > > index 60c51871b04b..fc20886ef069 100644 > > --- a/include/linux/clk-provider.h > > +++ b/include/linux/clk-provider.h > > @@ -550,8 +550,8 @@ struct clk_hw *clk_hw_register_mux_table(struct device > > *dev, const char *name, > > void __iomem *reg, u8 shift, u32 mask, > > u8 clk_mux_flags, u32 *table, spinlock_t *lock); > > > > -int clk_mux_val_to_index(struct clk_hw *hw, u32 *table, unsigned int > > flags, > > - unsigned int val); > > +u8 clk_mux_val_to_index(struct clk_hw *hw, u32 *table, unsigned int > > flags, > > I wonder if we should just make this unsigned int? Does it hurt at all > to have it be a wider type even though it doesn't match the CCF decision > to make this a u8 for the parent index number space? > I also wondered about this but since the target is get_parent(), I just aligned the two. In the end, I don't really care, as you prefer. Just let me know if you would like a v2 with this change
Quoting Jerome Brunet (2018-12-04 11:55:10) > On Tue, 2018-12-04 at 10:43 -0800, Stephen Boyd wrote: > > Quoting Jerome Brunet (2018-12-04 08:34:03) > > > clk_mux_val_to_index() is meant to be used by .get_parent(), which > > > returns a u8, so when the value provided does not map to any valid index, > > > it is not a good idea to return a negative error value. > > > > > > Instead, return num_parents which we know is an invalid index and let > > > CCF deal with it. > > > > > > Fixes: 77deb66d262f ("clk: mux: add helper function for index/value > > > translation") > > > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> > > > --- > > > > Thanks! > > > > > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h > > > index 60c51871b04b..fc20886ef069 100644 > > > --- a/include/linux/clk-provider.h > > > +++ b/include/linux/clk-provider.h > > > @@ -550,8 +550,8 @@ struct clk_hw *clk_hw_register_mux_table(struct device > > > *dev, const char *name, > > > void __iomem *reg, u8 shift, u32 mask, > > > u8 clk_mux_flags, u32 *table, spinlock_t *lock); > > > > > > -int clk_mux_val_to_index(struct clk_hw *hw, u32 *table, unsigned int > > > flags, > > > - unsigned int val); > > > +u8 clk_mux_val_to_index(struct clk_hw *hw, u32 *table, unsigned int > > > flags, > > > > I wonder if we should just make this unsigned int? Does it hurt at all > > to have it be a wider type even though it doesn't match the CCF decision > > to make this a u8 for the parent index number space? > > > > I also wondered about this but since the target is get_parent(), I just > aligned the two. > > In the end, I don't really care, as you prefer. Just let me know if you would > like a v2 with this change > Ok I may just make it unsigned int when applying then.
diff --git a/drivers/clk/clk-mux.c b/drivers/clk/clk-mux.c index 1628b93655ed..b64e475802cc 100644 --- a/drivers/clk/clk-mux.c +++ b/drivers/clk/clk-mux.c @@ -26,8 +26,8 @@ * parent - parent is adjustable through clk_set_parent */ -int clk_mux_val_to_index(struct clk_hw *hw, u32 *table, unsigned int flags, - unsigned int val) +u8 clk_mux_val_to_index(struct clk_hw *hw, u32 *table, unsigned int flags, + unsigned int val) { int num_parents = clk_hw_get_num_parents(hw); @@ -37,7 +37,7 @@ int clk_mux_val_to_index(struct clk_hw *hw, u32 *table, unsigned int flags, for (i = 0; i < num_parents; i++) if (table[i] == val) return i; - return -EINVAL; + return num_parents; } if (val && (flags & CLK_MUX_INDEX_BIT)) @@ -47,7 +47,7 @@ int clk_mux_val_to_index(struct clk_hw *hw, u32 *table, unsigned int flags, val--; if (val >= num_parents) - return -EINVAL; + return num_parents; return val; } diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h index 60c51871b04b..fc20886ef069 100644 --- a/include/linux/clk-provider.h +++ b/include/linux/clk-provider.h @@ -550,8 +550,8 @@ struct clk_hw *clk_hw_register_mux_table(struct device *dev, const char *name, void __iomem *reg, u8 shift, u32 mask, u8 clk_mux_flags, u32 *table, spinlock_t *lock); -int clk_mux_val_to_index(struct clk_hw *hw, u32 *table, unsigned int flags, - unsigned int val); +u8 clk_mux_val_to_index(struct clk_hw *hw, u32 *table, unsigned int flags, + unsigned int val); unsigned int clk_mux_index_to_val(u32 *table, unsigned int flags, u8 index); void clk_unregister_mux(struct clk *clk);
clk_mux_val_to_index() is meant to be used by .get_parent(), which returns a u8, so when the value provided does not map to any valid index, it is not a good idea to return a negative error value. Instead, return num_parents which we know is an invalid index and let CCF deal with it. Fixes: 77deb66d262f ("clk: mux: add helper function for index/value translation") Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> --- drivers/clk/clk-mux.c | 8 ++++---- include/linux/clk-provider.h | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) -- 2.19.1