lkml.org 
[lkml]   [2020]   [Jan]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] rtc: pcf85063: Add pcf85063 clkout control to common clock framework
    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@enatel.net>
    > ---
    > 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@bootlin.com>
    > */
    > +#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

    \
     
     \ /
      Last update: 2020-01-22 23:08    [W:3.508 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site