Re: [PATCH] rtc: pcf85063: Add pcf85063 clkout control to common clock framework

[Alexandre Belloni]

Hi,

I'm sorry for the very late review, please Cc me on RTC patches.

It seems the patch has been mangled and tabs became spaces. Also, a few
alignments are off.

On 02/09/2019 16:37:27+1200, Michael McCormick wrote:
> Signed-off-by: Michael McCormick <michael.mccormick@xxxxxxxxxx>
> ---
>  drivers/rtc/rtc-pcf85063.c | 153 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 153 insertions(+)
> 
> diff --git a/drivers/rtc/rtc-pcf85063.c b/drivers/rtc/rtc-pcf85063.c
> index 1afa6d9fa9fb..f47d3a6b997d 100644
> --- a/drivers/rtc/rtc-pcf85063.c
> +++ b/drivers/rtc/rtc-pcf85063.c
> @@ -9,6 +9,7 @@
>   * Copyright (C) 2019 Micro Crystal AG
>   * Author: Alexandre Belloni <alexandre.belloni@xxxxxxxxxxx>
>   */
> +#include <linux/clk-provider.h>
>  #include <linux/i2c.h>
>  #include <linux/bcd.h>
>  #include <linux/rtc.h>
> @@ -44,6 +45,16 @@
>  #define PCF85063_OFFSET_STEP0          4340
>  #define PCF85063_OFFSET_STEP1          4069
> 
> +#define PCF85063_REG_CLKO_F_MASK       0x07 /* frequency mask */
> +#define PCF85063_REG_CLKO_F_32768HZ    0x00
> +#define PCF85063_REG_CLKO_F_16384HZ    0x01
> +#define PCF85063_REG_CLKO_F_8192HZ     0x02
> +#define PCF85063_REG_CLKO_F_4096HZ     0x03
> +#define PCF85063_REG_CLKO_F_2048HZ     0x04
> +#define PCF85063_REG_CLKO_F_1024HZ     0x05
> +#define PCF85063_REG_CLKO_F_1HZ                0x06

Most of those defines are not used, I don't think they are necessary.

> +static int pcf85063_clkout_control(struct clk_hw *hw, bool enable)
> +{
> +       struct pcf85063 *pcf85063 = clkout_hw_to_pcf85063(hw);
> +       unsigned char buf;
> +
> +       if (enable)
> +               buf = PCF85063_REG_CLKO_F_32768HZ;

This doesn't work properly if the clock was already enabled and had a
proper rate already set. I think you need to read out the currently set
rate and update PCF85063_REG_CTRL2 only if PCF85063_REG_CLKO_F_OFF is
set.

> +       else
> +               buf = PCF85063_REG_CLKO_F_OFF;
> +
> +       return regmap_update_bits(pcf85063->regmap, PCF85063_REG_CTRL2,
> +                                 PCF85063_REG_CLKO_F_MASK, buf);
> +}
> +
> +static struct clk *pcf85063_clkout_register_clk(struct pcf85063 *pcf85063)
> +{
> +       struct clk *clk;
> +       struct clk_init_data init;
> +       int ret;
> +
> +       /* disable the clkout output */
> +       ret = regmap_update_bits(pcf85063->regmap, PCF85063_REG_CTRL2,
> +                                PCF85063_REG_CLKO_F_MASK, PCF85063_REG_CLKO_F_OFF);

This has to be left to the common clock framework to figure that nothing
is using this clock and disable it. I guess this is the whole point of
this patch but you can't hardcode it here because this will introduce a
glitch for all the actual users of the clock.


-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com