[U-Boot] Fixed clock driver (clk: convert API to match reset/mailbox style)

[Michal Simek]

Hi Simon,

On 15.11.2016 20:23, Simon Glass wrote:
> Hi Michal,
> 
> On 15 November 2016 at 12:07, Michal Simek <monstr at 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