linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [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	[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	[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).