* [PATCH 1/2] rtc-rv8803: fix writing back ctrl in flag register @ 2021-11-01 1:33 Dominique Martinet 2021-11-01 1:34 ` [PATCH 2/2] rv8803: add irq-gpio optional dts attribute Dominique Martinet ` (2 more replies) 0 siblings, 3 replies; 10+ messages in thread From: Dominique Martinet @ 2021-11-01 1:33 UTC (permalink / raw) To: Alexandre Belloni, Alessandro Zummo Cc: linux-rtc, linux-kernel, Marek Vasut, Rob Herring, Dominique Martinet 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(-) 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; -- 2.30.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] rv8803: add irq-gpio optional dts attribute 2021-11-01 1:33 [PATCH 1/2] rtc-rv8803: fix writing back ctrl in flag register Dominique Martinet @ 2021-11-01 1:34 ` Dominique Martinet 2021-11-01 12:53 ` Rob Herring 2021-11-01 22:48 ` Alexandre Belloni 2021-11-08 3:16 ` [PATCH 1/2] rtc-rv8803: fix writing back ctrl in flag register Dominique Martinet 2021-11-09 23:44 ` Alexandre Belloni 2 siblings, 2 replies; 10+ messages in thread From: Dominique Martinet @ 2021-11-01 1:34 UTC (permalink / raw) To: Alexandre Belloni, Alessandro Zummo Cc: linux-rtc, linux-kernel, Marek Vasut, Rob Herring, Dominique Martinet Some device cannot be woken up from i2c signal. Add a new irq-gpio attribute for devices which have a gpio connected to the rv8803 INT line so the rtc can be used for suspend to mem Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com> --- Our board does not have an upstream dts so I cannot provide a real example for it, but I've tested this on something close to the imx8mp-evk. It should not break anything for people having no alarm at all or using the i2c irq. .../devicetree/bindings/rtc/epson,rx8900.yaml | 5 ++ drivers/rtc/rtc-rv8803.c | 73 +++++++++++++++++-- 2 files changed, 73 insertions(+), 5 deletions(-) diff --git a/Documentation/devicetree/bindings/rtc/epson,rx8900.yaml b/Documentation/devicetree/bindings/rtc/epson,rx8900.yaml index 29fe39bb08ad..0d7912b984c7 100644 --- a/Documentation/devicetree/bindings/rtc/epson,rx8900.yaml +++ b/Documentation/devicetree/bindings/rtc/epson,rx8900.yaml @@ -28,6 +28,10 @@ properties: trickle-diode-disable: true + irq-gpio: + description: | + gpio for INT signal. Set up gpio for irq and device wakeup. + required: - compatible - reg @@ -45,5 +49,6 @@ examples: reg = <0x32>; epson,vdet-disable; trickle-diode-disable; + irq-gpio = <&gpio1 11 GPIO_ACTIVE_LOW>; }; }; diff --git a/drivers/rtc/rtc-rv8803.c b/drivers/rtc/rtc-rv8803.c index 0d5ed38bf60c..1c4b96bc110e 100644 --- a/drivers/rtc/rtc-rv8803.c +++ b/drivers/rtc/rtc-rv8803.c @@ -16,6 +16,7 @@ #include <linux/module.h> #include <linux/of_device.h> #include <linux/rtc.h> +#include <linux/pm_wakeirq.h> #define RV8803_I2C_TRY_COUNT 4 @@ -509,6 +510,61 @@ static int rx8900_trickle_charger_init(struct rv8803_data *rv8803) flags); } +static int rv8803_setup_gpio_irq(struct i2c_client *client) +{ + struct device *dev = &client->dev; + int err; + int irq; + unsigned long irqflags; + struct gpio_desc *gpiod; + + + gpiod = devm_gpiod_get_from_of_node(dev, dev->of_node, "irq-gpio", + 0, GPIOD_IN, "RTC irq pin"); + if (!gpiod || IS_ERR(gpiod)) { + dev_dbg(dev, "no gpio for rtc: skipping\n"); + return -ENOENT; + } + + irq = gpiod_to_irq(gpiod); + if (irq < 0) { + dev_err(dev, "gpio found but no irq\n"); + err = irq; + goto error_gpio; + } + + irqflags = IRQF_ONESHOT; + irqflags |= gpiod_is_active_low(gpiod) ? + IRQF_TRIGGER_FALLING : IRQF_TRIGGER_RISING; + + err = devm_request_threaded_irq(dev, irq, NULL, rv8803_handle_irq, + irqflags, "rtc-rv8803-gpio", client); + if (err) { + dev_warn(dev, "unable to request IRQ\n"); + goto error_gpio; + } + + err = device_init_wakeup(dev, true); + if (err) { + dev_warn(dev, "unable to set as wakeup source\n"); + goto error_irq; + } + + err = dev_pm_set_wake_irq(dev, irq); + if (err) { + dev_warn(dev, "unable to set wake irq\n"); + goto error_irq; + } + + return 0; + +error_irq: + devm_free_irq(dev, irq, client); +error_gpio: + devm_gpiod_put(dev, gpiod); + return err; +} + static int rv8803_probe(struct i2c_client *client, const struct i2c_device_id *id) { @@ -524,6 +580,7 @@ static int rv8803_probe(struct i2c_client *client, .reg_write = rv8803_nvram_write, .priv = client, }; + bool irq_setup = false; if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_I2C_BLOCK)) { @@ -562,17 +619,23 @@ static int rv8803_probe(struct i2c_client *client, if (IS_ERR(rv8803->rtc)) return PTR_ERR(rv8803->rtc); - if (client->irq > 0) { + if (client->dev.of_node) { + err = rv8803_setup_gpio_irq(client); + if (!err) + irq_setup = true; + } + + if (!irq_setup && client->irq > 0) { err = devm_request_threaded_irq(&client->dev, client->irq, NULL, rv8803_handle_irq, IRQF_TRIGGER_LOW | IRQF_ONESHOT, "rv8803", client); - if (err) { + if (err) dev_warn(&client->dev, "unable to request IRQ, alarms disabled\n"); - client->irq = 0; - } + else + irq_setup = true; } - if (!client->irq) + if (!irq_setup) clear_bit(RTC_FEATURE_ALARM, rv8803->rtc->features); err = rv8803_write_reg(rv8803->client, RV8803_EXT, RV8803_EXT_WADA); -- 2.30.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] rv8803: add irq-gpio optional dts attribute 2021-11-01 1:34 ` [PATCH 2/2] rv8803: add irq-gpio optional dts attribute Dominique Martinet @ 2021-11-01 12:53 ` Rob Herring 2021-11-01 22:48 ` Alexandre Belloni 1 sibling, 0 replies; 10+ messages in thread From: Rob Herring @ 2021-11-01 12:53 UTC (permalink / raw) To: Dominique Martinet Cc: Alexandre Belloni, Alessandro Zummo, open list:REAL TIME CLOCK (RTC) SUBSYSTEM, linux-kernel, Marek Vasut On Sun, Oct 31, 2021 at 8:40 PM Dominique Martinet <dominique.martinet@atmark-techno.com> wrote: > > Some device cannot be woken up from i2c signal. > Add a new irq-gpio attribute for devices which have a gpio connected to > the rv8803 INT line so the rtc can be used for suspend to mem Please send DT patches to the DT list. Binding changes should be a separate patch. > > Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com> > --- > > Our board does not have an upstream dts so I cannot provide a real > example for it, but I've tested this on something close to the > imx8mp-evk. > > It should not break anything for people having no alarm at all or using > the i2c irq. > > .../devicetree/bindings/rtc/epson,rx8900.yaml | 5 ++ > drivers/rtc/rtc-rv8803.c | 73 +++++++++++++++++-- > 2 files changed, 73 insertions(+), 5 deletions(-) > > diff --git a/Documentation/devicetree/bindings/rtc/epson,rx8900.yaml b/Documentation/devicetree/bindings/rtc/epson,rx8900.yaml > index 29fe39bb08ad..0d7912b984c7 100644 > --- a/Documentation/devicetree/bindings/rtc/epson,rx8900.yaml > +++ b/Documentation/devicetree/bindings/rtc/epson,rx8900.yaml > @@ -28,6 +28,10 @@ properties: > > trickle-diode-disable: true > > + irq-gpio: > + description: | > + gpio for INT signal. Set up gpio for irq and device wakeup. No, use 'interrupts' as interrupt capable GPIO controllers are also an interrupt provider. > + > required: > - compatible > - reg > @@ -45,5 +49,6 @@ examples: > reg = <0x32>; > epson,vdet-disable; > trickle-diode-disable; > + irq-gpio = <&gpio1 11 GPIO_ACTIVE_LOW>; > }; > }; > diff --git a/drivers/rtc/rtc-rv8803.c b/drivers/rtc/rtc-rv8803.c > index 0d5ed38bf60c..1c4b96bc110e 100644 > --- a/drivers/rtc/rtc-rv8803.c > +++ b/drivers/rtc/rtc-rv8803.c > @@ -16,6 +16,7 @@ > #include <linux/module.h> > #include <linux/of_device.h> > #include <linux/rtc.h> > +#include <linux/pm_wakeirq.h> > > #define RV8803_I2C_TRY_COUNT 4 > > @@ -509,6 +510,61 @@ static int rx8900_trickle_charger_init(struct rv8803_data *rv8803) > flags); > } > > +static int rv8803_setup_gpio_irq(struct i2c_client *client) > +{ > + struct device *dev = &client->dev; > + int err; > + int irq; > + unsigned long irqflags; > + struct gpio_desc *gpiod; > + > + > + gpiod = devm_gpiod_get_from_of_node(dev, dev->of_node, "irq-gpio", > + 0, GPIOD_IN, "RTC irq pin"); > + if (!gpiod || IS_ERR(gpiod)) { > + dev_dbg(dev, "no gpio for rtc: skipping\n"); > + return -ENOENT; > + } > + > + irq = gpiod_to_irq(gpiod); > + if (irq < 0) { > + dev_err(dev, "gpio found but no irq\n"); > + err = irq; > + goto error_gpio; > + } > + > + irqflags = IRQF_ONESHOT; > + irqflags |= gpiod_is_active_low(gpiod) ? > + IRQF_TRIGGER_FALLING : IRQF_TRIGGER_RISING; > + > + err = devm_request_threaded_irq(dev, irq, NULL, rv8803_handle_irq, > + irqflags, "rtc-rv8803-gpio", client); > + if (err) { > + dev_warn(dev, "unable to request IRQ\n"); > + goto error_gpio; > + } > + > + err = device_init_wakeup(dev, true); > + if (err) { > + dev_warn(dev, "unable to set as wakeup source\n"); > + goto error_irq; > + } > + > + err = dev_pm_set_wake_irq(dev, irq); > + if (err) { > + dev_warn(dev, "unable to set wake irq\n"); > + goto error_irq; > + } > + > + return 0; > + > +error_irq: > + devm_free_irq(dev, irq, client); > +error_gpio: > + devm_gpiod_put(dev, gpiod); > + return err; > +} > + > static int rv8803_probe(struct i2c_client *client, > const struct i2c_device_id *id) > { > @@ -524,6 +580,7 @@ static int rv8803_probe(struct i2c_client *client, > .reg_write = rv8803_nvram_write, > .priv = client, > }; > + bool irq_setup = false; > > if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA | > I2C_FUNC_SMBUS_I2C_BLOCK)) { > @@ -562,17 +619,23 @@ static int rv8803_probe(struct i2c_client *client, > if (IS_ERR(rv8803->rtc)) > return PTR_ERR(rv8803->rtc); > > - if (client->irq > 0) { > + if (client->dev.of_node) { > + err = rv8803_setup_gpio_irq(client); > + if (!err) > + irq_setup = true; > + } > + > + if (!irq_setup && client->irq > 0) { > err = devm_request_threaded_irq(&client->dev, client->irq, > NULL, rv8803_handle_irq, > IRQF_TRIGGER_LOW | IRQF_ONESHOT, > "rv8803", client); > - if (err) { > + if (err) > dev_warn(&client->dev, "unable to request IRQ, alarms disabled\n"); > - client->irq = 0; > - } > + else > + irq_setup = true; > } > - if (!client->irq) > + if (!irq_setup) > clear_bit(RTC_FEATURE_ALARM, rv8803->rtc->features); > > err = rv8803_write_reg(rv8803->client, RV8803_EXT, RV8803_EXT_WADA); > -- > 2.30.2 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] rv8803: add irq-gpio optional dts attribute 2021-11-01 1:34 ` [PATCH 2/2] rv8803: add irq-gpio optional dts attribute Dominique Martinet 2021-11-01 12:53 ` Rob Herring @ 2021-11-01 22:48 ` Alexandre Belloni 2021-11-01 23:40 ` Dominique Martinet 1 sibling, 1 reply; 10+ messages in thread From: Alexandre Belloni @ 2021-11-01 22:48 UTC (permalink / raw) To: Dominique Martinet Cc: Alessandro Zummo, linux-rtc, linux-kernel, Marek Vasut, Rob Herring On 01/11/2021 10:34:00+0900, Dominique Martinet wrote: > Some device cannot be woken up from i2c signal. > Add a new irq-gpio attribute for devices which have a gpio connected to > the rv8803 INT line so the rtc can be used for suspend to mem > I don't think this is right, the interrupts property of the rtc node can point to a gpio and this is expected to be the one connected on INT. You don't need another property. > Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com> > --- > > Our board does not have an upstream dts so I cannot provide a real > example for it, but I've tested this on something close to the > imx8mp-evk. > > It should not break anything for people having no alarm at all or using > the i2c irq. > > .../devicetree/bindings/rtc/epson,rx8900.yaml | 5 ++ > drivers/rtc/rtc-rv8803.c | 73 +++++++++++++++++-- > 2 files changed, 73 insertions(+), 5 deletions(-) > > diff --git a/Documentation/devicetree/bindings/rtc/epson,rx8900.yaml b/Documentation/devicetree/bindings/rtc/epson,rx8900.yaml > index 29fe39bb08ad..0d7912b984c7 100644 > --- a/Documentation/devicetree/bindings/rtc/epson,rx8900.yaml > +++ b/Documentation/devicetree/bindings/rtc/epson,rx8900.yaml > @@ -28,6 +28,10 @@ properties: > > trickle-diode-disable: true > > + irq-gpio: > + description: | > + gpio for INT signal. Set up gpio for irq and device wakeup. > + > required: > - compatible > - reg > @@ -45,5 +49,6 @@ examples: > reg = <0x32>; > epson,vdet-disable; > trickle-diode-disable; > + irq-gpio = <&gpio1 11 GPIO_ACTIVE_LOW>; > }; > }; > diff --git a/drivers/rtc/rtc-rv8803.c b/drivers/rtc/rtc-rv8803.c > index 0d5ed38bf60c..1c4b96bc110e 100644 > --- a/drivers/rtc/rtc-rv8803.c > +++ b/drivers/rtc/rtc-rv8803.c > @@ -16,6 +16,7 @@ > #include <linux/module.h> > #include <linux/of_device.h> > #include <linux/rtc.h> > +#include <linux/pm_wakeirq.h> > > #define RV8803_I2C_TRY_COUNT 4 > > @@ -509,6 +510,61 @@ static int rx8900_trickle_charger_init(struct rv8803_data *rv8803) > flags); > } > > +static int rv8803_setup_gpio_irq(struct i2c_client *client) > +{ > + struct device *dev = &client->dev; > + int err; > + int irq; > + unsigned long irqflags; > + struct gpio_desc *gpiod; > + > + > + gpiod = devm_gpiod_get_from_of_node(dev, dev->of_node, "irq-gpio", > + 0, GPIOD_IN, "RTC irq pin"); > + if (!gpiod || IS_ERR(gpiod)) { > + dev_dbg(dev, "no gpio for rtc: skipping\n"); > + return -ENOENT; > + } > + > + irq = gpiod_to_irq(gpiod); > + if (irq < 0) { > + dev_err(dev, "gpio found but no irq\n"); > + err = irq; > + goto error_gpio; > + } > + > + irqflags = IRQF_ONESHOT; > + irqflags |= gpiod_is_active_low(gpiod) ? > + IRQF_TRIGGER_FALLING : IRQF_TRIGGER_RISING; > + > + err = devm_request_threaded_irq(dev, irq, NULL, rv8803_handle_irq, > + irqflags, "rtc-rv8803-gpio", client); > + if (err) { > + dev_warn(dev, "unable to request IRQ\n"); > + goto error_gpio; > + } > + > + err = device_init_wakeup(dev, true); > + if (err) { > + dev_warn(dev, "unable to set as wakeup source\n"); > + goto error_irq; > + } > + > + err = dev_pm_set_wake_irq(dev, irq); > + if (err) { > + dev_warn(dev, "unable to set wake irq\n"); > + goto error_irq; > + } > + > + return 0; > + > +error_irq: > + devm_free_irq(dev, irq, client); > +error_gpio: > + devm_gpiod_put(dev, gpiod); > + return err; > +} > + > static int rv8803_probe(struct i2c_client *client, > const struct i2c_device_id *id) > { > @@ -524,6 +580,7 @@ static int rv8803_probe(struct i2c_client *client, > .reg_write = rv8803_nvram_write, > .priv = client, > }; > + bool irq_setup = false; > > if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA | > I2C_FUNC_SMBUS_I2C_BLOCK)) { > @@ -562,17 +619,23 @@ static int rv8803_probe(struct i2c_client *client, > if (IS_ERR(rv8803->rtc)) > return PTR_ERR(rv8803->rtc); > > - if (client->irq > 0) { > + if (client->dev.of_node) { > + err = rv8803_setup_gpio_irq(client); > + if (!err) > + irq_setup = true; > + } > + > + if (!irq_setup && client->irq > 0) { > err = devm_request_threaded_irq(&client->dev, client->irq, > NULL, rv8803_handle_irq, > IRQF_TRIGGER_LOW | IRQF_ONESHOT, > "rv8803", client); > - if (err) { > + if (err) > dev_warn(&client->dev, "unable to request IRQ, alarms disabled\n"); > - client->irq = 0; > - } > + else > + irq_setup = true; > } > - if (!client->irq) > + if (!irq_setup) > clear_bit(RTC_FEATURE_ALARM, rv8803->rtc->features); > > err = rv8803_write_reg(rv8803->client, RV8803_EXT, RV8803_EXT_WADA); > -- > 2.30.2 > -- Alexandre Belloni, co-owner and COO, Bootlin Embedded Linux and Kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] rv8803: add irq-gpio optional dts attribute 2021-11-01 22:48 ` Alexandre Belloni @ 2021-11-01 23:40 ` Dominique Martinet 2021-11-02 8:40 ` Alexandre Belloni 0 siblings, 1 reply; 10+ messages in thread From: Dominique Martinet @ 2021-11-01 23:40 UTC (permalink / raw) To: Alexandre Belloni Cc: Alessandro Zummo, linux-rtc, linux-kernel, Marek Vasut, Rob Herring Alexandre Belloni wrote on Mon, Nov 01, 2021 at 11:48:46PM +0100: > On 01/11/2021 10:34:00+0900, Dominique Martinet wrote: > > Some device cannot be woken up from i2c signal. > > Add a new irq-gpio attribute for devices which have a gpio connected to > > the rv8803 INT line so the rtc can be used for suspend to mem > > I don't think this is right, the interrupts property of the rtc node can > point to a gpio and this is expected to be the one connected on INT. You > don't need another property. Oh, why didn't I know about such a useful property. I thought I'd have a problem with the device wakeup part but there also is another 'wakeup-source' property, so there is really nothing left to do for this patch. Thank you for the pointer, no code is the best code! Rob Herring wrote on Mon, Nov 01, 2021 at 07:53:52AM -0500: > Please send DT patches to the DT list. > > Binding changes should be a separate patch. Ack, I'll do that for new patches onwards. It looks like a DT change will not be required here but I will remember this. Thanks, -- Dominique ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] rv8803: add irq-gpio optional dts attribute 2021-11-01 23:40 ` Dominique Martinet @ 2021-11-02 8:40 ` Alexandre Belloni 0 siblings, 0 replies; 10+ messages in thread From: Alexandre Belloni @ 2021-11-02 8:40 UTC (permalink / raw) To: Dominique Martinet Cc: Alessandro Zummo, linux-rtc, linux-kernel, Marek Vasut, Rob Herring On 02/11/2021 08:40:53+0900, Dominique Martinet wrote: > Alexandre Belloni wrote on Mon, Nov 01, 2021 at 11:48:46PM +0100: > > On 01/11/2021 10:34:00+0900, Dominique Martinet wrote: > > > Some device cannot be woken up from i2c signal. > > > Add a new irq-gpio attribute for devices which have a gpio connected to > > > the rv8803 INT line so the rtc can be used for suspend to mem > > > > I don't think this is right, the interrupts property of the rtc node can > > point to a gpio and this is expected to be the one connected on INT. You > > don't need another property. > > Oh, why didn't I know about such a useful property. > > I thought I'd have a problem with the device wakeup part but there also > is another 'wakeup-source' property, so there is really nothing left to > do for this patch. > Thank you for the pointer, no code is the best code! > I'd say that you may still need the device_init_wakeup and dev_pm_set_wake_irq calls. -- Alexandre Belloni, co-owner and COO, Bootlin Embedded Linux and Kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] rtc-rv8803: fix writing back ctrl in flag register 2021-11-01 1:33 [PATCH 1/2] rtc-rv8803: fix writing back ctrl in flag register Dominique Martinet 2021-11-01 1:34 ` [PATCH 2/2] rv8803: add irq-gpio optional dts attribute Dominique Martinet @ 2021-11-08 3:16 ` Dominique Martinet 2021-11-08 8:42 ` Alexandre Belloni 2021-11-09 23:44 ` Alexandre Belloni 2 siblings, 1 reply; 10+ messages in thread From: Dominique Martinet @ 2021-11-08 3:16 UTC (permalink / raw) To: Alexandre Belloni, Alessandro Zummo; +Cc: linux-rtc, linux-kernel 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; ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] rtc-rv8803: fix writing back ctrl in flag register 2021-11-08 3:16 ` [PATCH 1/2] rtc-rv8803: fix writing back ctrl in flag register Dominique Martinet @ 2021-11-08 8:42 ` Alexandre Belloni 2021-11-08 23:41 ` Dominique Martinet 0 siblings, 1 reply; 10+ messages in thread From: Alexandre Belloni @ 2021-11-08 8:42 UTC (permalink / raw) To: Dominique Martinet; +Cc: Alessandro Zummo, linux-rtc, linux-kernel 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, co-owner and COO, Bootlin Embedded Linux and Kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] rtc-rv8803: fix writing back ctrl in flag register 2021-11-08 8:42 ` Alexandre Belloni @ 2021-11-08 23:41 ` Dominique Martinet 0 siblings, 0 replies; 10+ messages in thread From: Dominique Martinet @ 2021-11-08 23:41 UTC (permalink / raw) To: Alexandre Belloni; +Cc: Alessandro Zummo, linux-rtc, linux-kernel 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, -- Dominique ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] rtc-rv8803: fix writing back ctrl in flag register 2021-11-01 1:33 [PATCH 1/2] rtc-rv8803: fix writing back ctrl in flag register Dominique Martinet 2021-11-01 1:34 ` [PATCH 2/2] rv8803: add irq-gpio optional dts attribute Dominique Martinet 2021-11-08 3:16 ` [PATCH 1/2] rtc-rv8803: fix writing back ctrl in flag register Dominique Martinet @ 2021-11-09 23:44 ` Alexandre Belloni 2 siblings, 0 replies; 10+ messages in thread From: Alexandre Belloni @ 2021-11-09 23:44 UTC (permalink / raw) To: Alessandro Zummo, Dominique Martinet Cc: Alexandre Belloni, Marek Vasut, linux-rtc, Rob Herring, linux-kernel 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, -- Alexandre Belloni <alexandre.belloni@bootlin.com> ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-11-09 23:45 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-11-01 1:33 [PATCH 1/2] rtc-rv8803: fix writing back ctrl in flag register Dominique Martinet 2021-11-01 1:34 ` [PATCH 2/2] rv8803: add irq-gpio optional dts attribute Dominique Martinet 2021-11-01 12:53 ` Rob Herring 2021-11-01 22:48 ` Alexandre Belloni 2021-11-01 23:40 ` Dominique Martinet 2021-11-02 8:40 ` Alexandre Belloni 2021-11-08 3:16 ` [PATCH 1/2] rtc-rv8803: fix writing back ctrl in flag register Dominique Martinet 2021-11-08 8:42 ` Alexandre Belloni 2021-11-08 23:41 ` Dominique Martinet 2021-11-09 23:44 ` Alexandre Belloni
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).