RE: [PATCH] rtc: pcf2127: clear the flag TSF1 before enabling interrupt generation

[Biwen Li (OSS)]

> 
> Hi,
> 
> On 01/12/2020 16:47:46+0800, Biwen Li wrote:
> > From: Biwen Li <biwen.li@xxxxxxx>
> >
> > - clear the flag TSF1 before enabling interrupt generation
> > - properly set flag WD_CD for rtc chips(pcf2129, pca2129)
> >
> 
> This change has to be a separate patch.
Sure, np. Will separate the patch in v2.
> 
> > Signed-off-by: Biwen Li <biwen.li@xxxxxxx>
> > ---
> >  drivers/rtc/rtc-pcf2127.c | 21 ++++++++++++++++++++-
> >  1 file changed, 20 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
> > index 07a5630ec841..0a45e2512258 100644
> > --- a/drivers/rtc/rtc-pcf2127.c
> > +++ b/drivers/rtc/rtc-pcf2127.c
> > @@ -601,6 +601,10 @@ static int pcf2127_probe(struct device *dev, struct
> regmap *regmap,
> >  	 * Watchdog timer enabled and reset pin /RST activated when timed out.
> >  	 * Select 1Hz clock source for watchdog timer.
> >  	 * Note: Countdown timer disabled and not available.
> > +	 * For pca2129, pcf2129, only bit[7] is for Symbol WD_CD
> > +	 * of register watchdg_tim_ctl. The bit[6] is labeled
> > +	 * as T. Bits labeled as T must always be written with
> > +	 * logic 0.
> >  	 */
> >  	ret = regmap_update_bits(pcf2127->regmap, PCF2127_REG_WD_CTL,
> >  				 PCF2127_BIT_WD_CTL_CD1 |
> > @@ -608,7 +612,7 @@ static int pcf2127_probe(struct device *dev, struct
> regmap *regmap,
> >  				 PCF2127_BIT_WD_CTL_TF1 |
> >  				 PCF2127_BIT_WD_CTL_TF0,
> >  				 PCF2127_BIT_WD_CTL_CD1 |
> > -				 PCF2127_BIT_WD_CTL_CD0 |
> > +				 has_nvmem ? (PCF2127_BIT_WD_CTL_CD0) : (0) |
> 
> I don't like that because has_nvmem has nothing to do with
> PCF2127_BIT_WD_CTL_CD0 and nothing guarantees that we won't ever get an
> RTC without RST but with NVRAM and that willprobbly be overlooked.
Okay, got it. Will correct it in v2.
> 
> >  				 PCF2127_BIT_WD_CTL_TF1);
> >  	if (ret) {
> >  		dev_err(dev, "%s: watchdog config (wd_ctl) failed\n", __func__); @@
> > -659,6 +663,21 @@ static int pcf2127_probe(struct device *dev, struct
> regmap *regmap,
> >  		return ret;
> >  	}
> >
> > +	/*
> > +	 * Clear TSF1 field of ctrl1 register to clear interrupt
> > +	 * before enabling interrupt generation when
> > +	 * timestamp flag set. Unless the flag TSF1 won't
> > +	 * be cleared and the interrupt(INT pin) is
> > +	 * triggered continueously.
> > +	 */
> > +	ret = regmap_update_bits(pcf2127->regmap, PCF2127_REG_CTRL1,
> > +				 PCF2127_BIT_CTRL1_TSF1,
> > +				 0);
> > +	if (ret) {
> > +		dev_err(dev, "%s:  control and status register 1 (ctrl1) failed, ret =
> 0x%x\n",
> > +			__func__, ret);
> > +		return ret;
> > +	}
> 
> Doing that means ignoring timestamps taken while the system is offline.
> It also doesn't fully solve the issue because you are not clearing TSF2 here and
> also it never gets cleared by the driver later on so I guess you will get the
> interrupt storm once a timestamp is taken.
Okay, got it. Thanks. Will clear TSF2 flag in v2.
> 
> 
> --
> Alexandre Belloni, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com