| Message ID | 20211101013400.325855-1-dominique.martinet@atmark-techno.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/2] rtc-rv8803: fix writing back ctrl in flag register | expand |
Hi Alexandre, Alessandro, the other patch was proved to be unneeded, but this one is still a valid fix as far as I can understand the code (reusing RV8803_CTRL value to write into RV8803_FLAG does not look correct) (I'm also convinced either mostly work because the original values are usually close enough, but that's not a reason to keep using the wrong one) Would you have time to take a look? Thanks! Dominique Martinet wrote on Mon, Nov 01, 2021 at 10:33:59AM +0900: > ctrl is set from read_regs(..FLAG, 2, ctrl), so ctrl[0] is FLAG > and ctrl[1] is the CTRL register. > Use ctrl[0] to write back to the FLAG register as appropriate. > > Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com> > --- > drivers/rtc/rtc-rv8803.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/rtc/rtc-rv8803.c b/drivers/rtc/rtc-rv8803.c > index 72adef5a5ebe..0d5ed38bf60c 100644 > --- a/drivers/rtc/rtc-rv8803.c > +++ b/drivers/rtc/rtc-rv8803.c > @@ -340,8 +340,8 @@ static int rv8803_set_alarm(struct device *dev, struct rtc_wkalrm *alrm) > } > } > > - ctrl[1] &= ~RV8803_FLAG_AF; > - err = rv8803_write_reg(rv8803->client, RV8803_FLAG, ctrl[1]); > + ctrl[0] &= ~RV8803_FLAG_AF; > + err = rv8803_write_reg(rv8803->client, RV8803_FLAG, ctrl[0]); > mutex_unlock(&rv8803->flags_lock); > if (err) > return err;
On 08/11/2021 12:16:59+0900, Dominique Martinet wrote: > Hi Alexandre, Alessandro, > > the other patch was proved to be unneeded, but this one is still a valid > fix as far as I can understand the code (reusing RV8803_CTRL value to > write into RV8803_FLAG does not look correct) > > (I'm also convinced either mostly work because the original values are > usually close enough, but that's not a reason to keep using the wrong > one) > > > Would you have time to take a look? I did check with the initial review and I'm going to apply it, I just didn't have the time to do that yet. > > > Thanks! > > Dominique Martinet wrote on Mon, Nov 01, 2021 at 10:33:59AM +0900: > > ctrl is set from read_regs(..FLAG, 2, ctrl), so ctrl[0] is FLAG > > and ctrl[1] is the CTRL register. > > Use ctrl[0] to write back to the FLAG register as appropriate. > > > > Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com> > > --- > > drivers/rtc/rtc-rv8803.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/rtc/rtc-rv8803.c b/drivers/rtc/rtc-rv8803.c > > index 72adef5a5ebe..0d5ed38bf60c 100644 > > --- a/drivers/rtc/rtc-rv8803.c > > +++ b/drivers/rtc/rtc-rv8803.c > > @@ -340,8 +340,8 @@ static int rv8803_set_alarm(struct device *dev, struct rtc_wkalrm *alrm) > > } > > } > > > > - ctrl[1] &= ~RV8803_FLAG_AF; > > - err = rv8803_write_reg(rv8803->client, RV8803_FLAG, ctrl[1]); > > + ctrl[0] &= ~RV8803_FLAG_AF; > > + err = rv8803_write_reg(rv8803->client, RV8803_FLAG, ctrl[0]); > > mutex_unlock(&rv8803->flags_lock); > > if (err) > > return err;
Alexandre Belloni wrote on Mon, Nov 08, 2021 at 09:42:53AM +0100: > On 08/11/2021 12:16:59+0900, Dominique Martinet wrote: > > Hi Alexandre, Alessandro, > > > > the other patch was proved to be unneeded, but this one is still a valid > > fix as far as I can understand the code (reusing RV8803_CTRL value to > > write into RV8803_FLAG does not look correct) > > > > (I'm also convinced either mostly work because the original values are > > usually close enough, but that's not a reason to keep using the wrong > > one) > > > > > > Would you have time to take a look? > > I did check with the initial review and I'm going to apply it, I just > didn't have the time to do that yet. Sorry, it wasn't clear to me whether this was dropped with the other or not. There's no hurry on my end, please apply when you can! Thanks,
On Mon, 1 Nov 2021 10:33:59 +0900, Dominique Martinet wrote: > ctrl is set from read_regs(..FLAG, 2, ctrl), so ctrl[0] is FLAG > and ctrl[1] is the CTRL register. > Use ctrl[0] to write back to the FLAG register as appropriate. > > Applied, thanks! [1/2] rtc-rv8803: fix writing back ctrl in flag register commit: 03a86cda4123084c7969387e7e0b69f23c2f8acf Best regards,
diff --git a/drivers/rtc/rtc-rv8803.c b/drivers/rtc/rtc-rv8803.c index 72adef5a5ebe..0d5ed38bf60c 100644 --- a/drivers/rtc/rtc-rv8803.c +++ b/drivers/rtc/rtc-rv8803.c @@ -340,8 +340,8 @@ static int rv8803_set_alarm(struct device *dev, struct rtc_wkalrm *alrm) } } - ctrl[1] &= ~RV8803_FLAG_AF; - err = rv8803_write_reg(rv8803->client, RV8803_FLAG, ctrl[1]); + ctrl[0] &= ~RV8803_FLAG_AF; + err = rv8803_write_reg(rv8803->client, RV8803_FLAG, ctrl[0]); mutex_unlock(&rv8803->flags_lock); if (err) return err;
ctrl is set from read_regs(..FLAG, 2, ctrl), so ctrl[0] is FLAG and ctrl[1] is the CTRL register. Use ctrl[0] to write back to the FLAG register as appropriate. Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com> --- This probably works in practice because the two registers are pretty close, but might as well fix it and use the correct register. drivers/rtc/rtc-rv8803.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)