Hi Simon, On 15.11.2016 20:23, Simon Glass wrote: > Hi Michal, > > On 15 November 2016 at 12:07, Michal Simek <mon...@monstr.eu> wrote: >> Hi guys, >> >> I just found today with playing with clock drivers that the patch >> clk: convert API to match reset/mailbox style >> (sha1: 135aa95002646c46e89de93fa36adad1b010548f) >> >> added this part of code to fixed clock driver >> -static ulong clk_fixed_rate_get_rate(struct udevice *dev) >> +static ulong clk_fixed_rate_get_rate(struct clk *clk) >> { >> - return to_clk_fixed_rate(dev)->fixed_rate; >> -} >> + if (clk->id != 0) >> + return -EINVAL; >> >> >> which is returning -EINVAL when ulong should be returned. > > This is intended - you can use IS_ERR_VALUE() to check if it is an error.
ok then some drivers needs to be fixed. drivers/mmc/atmel_sdhci.c:90: ret = clk_set_rate(&clk, gck_rate); drivers/mmc/atmel_sdhci.c-91- if (ret) drivers/mmc/atmel_sdhci.c-92- return ret; drivers/mmc/msm_sdhci.c:79: ret = clk_set_rate(&clk, clk_rate); drivers/mmc/msm_sdhci.c-80- clk_free(&clk); drivers/mmc/msm_sdhci.c-81- if (ret < 0) drivers/net/dwc_eth_qos.c:484: ret = clk_set_rate(&eqos->clk_ptp_ref, 125 * 1000 * 1000); drivers/net/dwc_eth_qos.c-485- if (ret < 0) { drivers/net/dwc_eth_qos.c:486: error("clk_set_rate(clk_ptp_ref) failed: %d", ret); drivers/net/dwc_eth_qos.c-487- goto err_disable_clk_ptp_ref; drivers/net/dwc_eth_qos.c-488- } drivers/serial/serial_msm.c:175: ret = clk_set_rate(&clk, clk_rate); drivers/serial/serial_msm.c-176- clk_free(&clk); drivers/serial/serial_msm.c-177- if (ret < 0) drivers/spi/rk_spi.c:176: ret = clk_set_rate(&priv->clk, 99000000); drivers/spi/rk_spi.c-177- if (ret < 0) { drivers/spi/rk_spi.c-178- debug("%s: Failed to set clock: %d\n", __func__, ret); > >> >> The next thing I have found is that fixed clock driver has no set_rate >> function which is fine but when I was testing one driver which tries to >> set rate then error code was generated but without any useful >> information what happened. > > It should return -ENOSYS, right? I think -EINVAL in driver as reaction for incorrect id looks good. It was more about returning minus value where you should return unsigned. > >> Are you ok with adding empty set_rate function with returning error >> message that set rate is not supported for fixed clocks? > > What would it return that is different? If you are asking for a > printed error message, that would bloat the code. So long as the > caller checks the error we should be OK. Caller checks the return code for sure but it is a question if this is enough for people to know what's wrong. When this happen you have no clue that this problem is coming from clock subsystem. I can add one print message to the driver but the same message will end up in all these drivers. Thanks, Michal _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot