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
