[PATCH 1/2] rtc-rv8803: fix writing back ctrl in flag register

[PATCH 1/2] rtc-rv8803: fix writing back ctrl in flag register

[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

[PATCH 2/2] rv8803: add irq-gpio optional dts attribute

[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

Re: [PATCH 2/2] rv8803: add irq-gpio optional dts attribute

[Rob Herring]

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
>

Re: [PATCH 2/2] rv8803: add irq-gpio optional dts attribute

[Alexandre Belloni]

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

Re: [PATCH 2/2] rv8803: add irq-gpio optional dts attribute

[Dominique Martinet]

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

Re: [PATCH 2/2] rv8803: add irq-gpio optional dts attribute

[Alexandre Belloni]

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

Re: [PATCH 1/2] rtc-rv8803: fix writing back ctrl in flag register

[Dominique Martinet]

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;

Re: [PATCH 1/2] rtc-rv8803: fix writing back ctrl in flag register

[Alexandre Belloni]

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

Re: [PATCH 1/2] rtc-rv8803: fix writing back ctrl in flag register

[Dominique Martinet]

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

Re: [PATCH 1/2] rtc-rv8803: fix writing back ctrl in flag register

[Alexandre Belloni]

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>