[rtc-linux] [PATCH 0/3] rtc: ds3232: Minor cleanup and fix sysfs wakealarm

[rtc-linux] [PATCH 0/3] rtc: ds3232: Minor cleanup and fix sysfs wakealarm

[Phil Reid]

Minor whitespace cleanup and regmap register definition.
Fix so that sysfs wakealarm attribute is visible.

Phil Reid (3):
  rtc: ds3232: Cleanup whitespace around register and bit definitions.
  rtc: ds3232: Add regmap max_register definition.
  rtc: ds3232: Call device_init_wakeup before device_register

 drivers/rtc/rtc-ds3232.c | 56 +++++++++++++++++++++++++-----------------------
 1 file changed, 29 insertions(+), 27 deletions(-)

-- 
1.8.3.1

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] [PATCH 1/3] rtc: ds3232: Cleanup whitespace around register and bit definitions.

[Phil Reid]

Whitespace was a combination of spaces and tabs.
Use spaces and align register / bit definitions.

Signed-off-by: Phil Reid <preid@electromag.com.au>
---
 drivers/rtc/rtc-ds3232.c | 44 ++++++++++++++++++++++----------------------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/drivers/rtc/rtc-ds3232.c b/drivers/rtc/rtc-ds3232.c
index b1f20d8..67066f1 100644
--- a/drivers/rtc/rtc-ds3232.c
+++ b/drivers/rtc/rtc-ds3232.c
@@ -23,28 +23,28 @@
 #include <linux/slab.h>
 #include <linux/regmap.h>
 
-#define DS3232_REG_SECONDS	0x00
-#define DS3232_REG_MINUTES	0x01
-#define DS3232_REG_HOURS	0x02
-#define DS3232_REG_AMPM		0x02
-#define DS3232_REG_DAY		0x03
-#define DS3232_REG_DATE		0x04
-#define DS3232_REG_MONTH	0x05
-#define DS3232_REG_CENTURY	0x05
-#define DS3232_REG_YEAR		0x06
-#define DS3232_REG_ALARM1         0x07	/* Alarm 1 BASE */
-#define DS3232_REG_ALARM2         0x0B	/* Alarm 2 BASE */
-#define DS3232_REG_CR		0x0E	/* Control register */
-#	define DS3232_REG_CR_nEOSC        0x80
-#       define DS3232_REG_CR_INTCN        0x04
-#       define DS3232_REG_CR_A2IE        0x02
-#       define DS3232_REG_CR_A1IE        0x01
-
-#define DS3232_REG_SR	0x0F	/* control/status register */
-#	define DS3232_REG_SR_OSF   0x80
-#       define DS3232_REG_SR_BSY   0x04
-#       define DS3232_REG_SR_A2F   0x02
-#       define DS3232_REG_SR_A1F   0x01
+#define DS3232_REG_SECONDS      0x00
+#define DS3232_REG_MINUTES      0x01
+#define DS3232_REG_HOURS        0x02
+#define DS3232_REG_AMPM         0x02
+#define DS3232_REG_DAY          0x03
+#define DS3232_REG_DATE         0x04
+#define DS3232_REG_MONTH        0x05
+#define DS3232_REG_CENTURY      0x05
+#define DS3232_REG_YEAR         0x06
+#define DS3232_REG_ALARM1       0x07       /* Alarm 1 BASE */
+#define DS3232_REG_ALARM2       0x0B       /* Alarm 2 BASE */
+#define DS3232_REG_CR           0x0E       /* Control register */
+#       define DS3232_REG_CR_nEOSC   0x80
+#       define DS3232_REG_CR_INTCN   0x04
+#       define DS3232_REG_CR_A2IE    0x02
+#       define DS3232_REG_CR_A1IE    0x01
+
+#define DS3232_REG_SR           0x0F       /* control/status register */
+#       define DS3232_REG_SR_OSF     0x80
+#       define DS3232_REG_SR_BSY     0x04
+#       define DS3232_REG_SR_A2F     0x02
+#       define DS3232_REG_SR_A1F     0x01
 
 struct ds3232 {
 	struct device *dev;
-- 
1.8.3.1

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] [PATCH 2/3] rtc: ds3232: Add regmap max_register definition.

[Phil Reid]

Add the max_register  to the regmap_config definition. This allows
dumping of the device's registers via the regmap debugfs interface.

Signed-off-by: Phil Reid <preid@electromag.com.au>
---
 drivers/rtc/rtc-ds3232.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/rtc/rtc-ds3232.c b/drivers/rtc/rtc-ds3232.c
index 67066f1..60de3a0 100644
--- a/drivers/rtc/rtc-ds3232.c
+++ b/drivers/rtc/rtc-ds3232.c
@@ -420,6 +420,7 @@ static int ds3232_i2c_probe(struct i2c_client *client,
 	static const struct regmap_config config = {
 		.reg_bits = 8,
 		.val_bits = 8,
+		.max_register = 0x13,
 	};
 
 	regmap = devm_regmap_init_i2c(client, &config);
@@ -479,6 +480,7 @@ static int ds3234_probe(struct spi_device *spi)
 	static const struct regmap_config config = {
 		.reg_bits = 8,
 		.val_bits = 8,
+		.max_register = 0x13,
 		.write_flag_mask = 0x80,
 	};
 	struct regmap *regmap;
-- 
1.8.3.1

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] [PATCH 3/3] rtc: ds3232: Call device_init_wakeup before device_register

[Phil Reid]

The wakealarm attribute is currently not exposed in the sysfs interface
as the device has not been set as doing wakealarm when device_register
is called. Changing the order of the calls fixes that problem. Interrupts
are cleared in check_rtc_status prior to requesting the interrupt.

Signed-off-by: Phil Reid <preid@electromag.com.au>
---
 drivers/rtc/rtc-ds3232.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/rtc/rtc-ds3232.c b/drivers/rtc/rtc-ds3232.c
index 60de3a0..ce943d0 100644
--- a/drivers/rtc/rtc-ds3232.c
+++ b/drivers/rtc/rtc-ds3232.c
@@ -363,11 +363,6 @@ static int ds3232_probe(struct device *dev, struct regmap *regmap, int irq,
 	if (ret)
 		return ret;
 
-	ds3232->rtc = devm_rtc_device_register(dev, name, &ds3232_rtc_ops,
-						THIS_MODULE);
-	if (IS_ERR(ds3232->rtc))
-		return PTR_ERR(ds3232->rtc);
-
 	if (ds3232->irq > 0) {
 		ret = devm_request_threaded_irq(dev, ds3232->irq, NULL,
 						ds3232_irq,
@@ -380,6 +375,11 @@ static int ds3232_probe(struct device *dev, struct regmap *regmap, int irq,
 			device_init_wakeup(dev, 1);
 	}
 
+	ds3232->rtc = devm_rtc_device_register(dev, name, &ds3232_rtc_ops,
+		THIS_MODULE);
+	if (IS_ERR(ds3232->rtc))
+		return PTR_ERR(ds3232->rtc);
+
 	return 0;
 }
 
-- 
1.8.3.1

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] Re: [PATCH 3/3] rtc: ds3232: Call device_init_wakeup before device_register

[Alexandre Belloni]

On 17/02/2017 at 09:44:58 +0800, Phil Reid wrote:
> The wakealarm attribute is currently not exposed in the sysfs interface
> as the device has not been set as doing wakealarm when device_register
> is called. Changing the order of the calls fixes that problem. Interrupts
> are cleared in check_rtc_status prior to requesting the interrupt.
> 

This basically revert b4b77f3c280e38cec178f81d7a4d7e65f4045913 So I'm
not sure this is sufficient to ensure the IRQ will never fire.

(I don't know whether this was a real bug or something reported by a
static analysis tool).

> Signed-off-by: Phil Reid <preid@electromag.com.au>
> ---
>  drivers/rtc/rtc-ds3232.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-ds3232.c b/drivers/rtc/rtc-ds3232.c
> index 60de3a0..ce943d0 100644
> --- a/drivers/rtc/rtc-ds3232.c
> +++ b/drivers/rtc/rtc-ds3232.c
> @@ -363,11 +363,6 @@ static int ds3232_probe(struct device *dev, struct regmap *regmap, int irq,
>  	if (ret)
>  		return ret;
>  
> -	ds3232->rtc = devm_rtc_device_register(dev, name, &ds3232_rtc_ops,
> -						THIS_MODULE);
> -	if (IS_ERR(ds3232->rtc))
> -		return PTR_ERR(ds3232->rtc);
> -
>  	if (ds3232->irq > 0) {
>  		ret = devm_request_threaded_irq(dev, ds3232->irq, NULL,
>  						ds3232_irq,
> @@ -380,6 +375,11 @@ static int ds3232_probe(struct device *dev, struct regmap *regmap, int irq,
>  			device_init_wakeup(dev, 1);
>  	}
>  
> +	ds3232->rtc = devm_rtc_device_register(dev, name, &ds3232_rtc_ops,
> +		THIS_MODULE);
> +	if (IS_ERR(ds3232->rtc))
> +		return PTR_ERR(ds3232->rtc);
> +
>  	return 0;
>  }
>  
> -- 
> 1.8.3.1
> 

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] Re: [PATCH 1/3] rtc: ds3232: Cleanup whitespace around register and bit definitions.

[Alexandre Belloni]

On 17/02/2017 at 09:44:56 +0800, Phil Reid wrote:
> Whitespace was a combination of spaces and tabs.
> Use spaces and align register / bit definitions.
> 
> Signed-off-by: Phil Reid <preid@electromag.com.au>
> ---
>  drivers/rtc/rtc-ds3232.c | 44 ++++++++++++++++++++++----------------------
>  1 file changed, 22 insertions(+), 22 deletions(-)
Applied, thanks.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] Re: [PATCH 2/3] rtc: ds3232: Add regmap max_register definition.

[Alexandre Belloni]

On 17/02/2017 at 09:44:57 +0800, Phil Reid wrote:
> Add the max_register  to the regmap_config definition. This allows
> dumping of the device's registers via the regmap debugfs interface.
> 
> Signed-off-by: Phil Reid <preid@electromag.com.au>
> ---
>  drivers/rtc/rtc-ds3232.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
Applied, thanks.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] Re: [PATCH 3/3] rtc: ds3232: Call device_init_wakeup before device_register

[Phil Reid]

On 22/02/2017 04:33, Alexandre Belloni wrote:
> On 17/02/2017 at 09:44:58 +0800, Phil Reid wrote:
>> The wakealarm attribute is currently not exposed in the sysfs interface
>> as the device has not been set as doing wakealarm when device_register
>> is called. Changing the order of the calls fixes that problem. Interrupts
>> are cleared in check_rtc_status prior to requesting the interrupt.
>>
>
> This basically revert b4b77f3c280e38cec178f81d7a4d7e65f4045913 So I'm
> not sure this is sufficient to ensure the IRQ will never fire.
>
> (I don't know whether this was a real bug or something reported by a
> static analysis tool).

G'day Alexandre,
Without this change the wakealarm sysfs is never created.
This is done in rtc_device_register, but a filter to wakealarm attribute effectively on
dev->power.can_wakeup flag. Which is set via the init wakeup call.

But looking at that commit and thinking about it some more I can see the problem
b4b77f3c280e38cec178f81d7a4d7e65f4045913 was try to solve.

Looking at other drivers some of them have similar order but use their own mutex.
eg rtc-rc8803 and don't use the ops_lock

Or others set device_init_wakeup and then fail if the irq fails to register.

Currently if the ds3232 has an irq set but fails to register the irq it falls back
to being a non wakeup source. To me it makes more sense to just fall if the config
has an irq defined.

Then we could set device_init_wakeup based on there being an irq prior to device_register
and then request the irq after device register. But fail the probe if the irq request fails.

Thoughts?

>
>> Signed-off-by: Phil Reid <preid@electromag.com.au>
>> ---
>>  drivers/rtc/rtc-ds3232.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/rtc/rtc-ds3232.c b/drivers/rtc/rtc-ds3232.c
>> index 60de3a0..ce943d0 100644
>> --- a/drivers/rtc/rtc-ds3232.c
>> +++ b/drivers/rtc/rtc-ds3232.c
>> @@ -363,11 +363,6 @@ static int ds3232_probe(struct device *dev, struct regmap *regmap, int irq,
>>  	if (ret)
>>  		return ret;
>>
>> -	ds3232->rtc = devm_rtc_device_register(dev, name, &ds3232_rtc_ops,
>> -						THIS_MODULE);
>> -	if (IS_ERR(ds3232->rtc))
>> -		return PTR_ERR(ds3232->rtc);
>> -
>>  	if (ds3232->irq > 0) {
>>  		ret = devm_request_threaded_irq(dev, ds3232->irq, NULL,
>>  						ds3232_irq,
>> @@ -380,6 +375,11 @@ static int ds3232_probe(struct device *dev, struct regmap *regmap, int irq,
>>  			device_init_wakeup(dev, 1);
>>  	}
>>
>> +	ds3232->rtc = devm_rtc_device_register(dev, name, &ds3232_rtc_ops,
>> +		THIS_MODULE);
>> +	if (IS_ERR(ds3232->rtc))
>> +		return PTR_ERR(ds3232->rtc);
>> +
>>  	return 0;
>>  }
>>
>> --
>> 1.8.3.1
>>
>


-- 
Regards
Phil Reid

ElectroMagnetic Imaging Technology Pty Ltd
Development of Geophysical Instrumentation & Software
www.electromag.com.au

3 The Avenue, Midland WA 6056, AUSTRALIA
Ph: +61 8 9250 8100
Fax: +61 8 9250 7100
Email: preid@electromag.com.au

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] Re: [PATCH 3/3] rtc: ds3232: Call device_init_wakeup before device_register

[Alexandre Belloni]

On 22/02/2017 at 09:33:14 +0800, Phil Reid wrote:
> On 22/02/2017 04:33, Alexandre Belloni wrote:
> > On 17/02/2017 at 09:44:58 +0800, Phil Reid wrote:
> > > The wakealarm attribute is currently not exposed in the sysfs interface
> > > as the device has not been set as doing wakealarm when device_register
> > > is called. Changing the order of the calls fixes that problem. Interrupts
> > > are cleared in check_rtc_status prior to requesting the interrupt.
> > > 
> > 
> > This basically revert b4b77f3c280e38cec178f81d7a4d7e65f4045913 So I'm
> > not sure this is sufficient to ensure the IRQ will never fire.
> > 
> > (I don't know whether this was a real bug or something reported by a
> > static analysis tool).
> 
> G'day Alexandre,
> Without this change the wakealarm sysfs is never created.
> This is done in rtc_device_register, but a filter to wakealarm attribute effectively on
> dev->power.can_wakeup flag. Which is set via the init wakeup call.
> 
> But looking at that commit and thinking about it some more I can see the problem
> b4b77f3c280e38cec178f81d7a4d7e65f4045913 was try to solve.
> 
> Looking at other drivers some of them have similar order but use their own mutex.
> eg rtc-rc8803 and don't use the ops_lock
> 
> Or others set device_init_wakeup and then fail if the irq fails to register.
> 
> Currently if the ds3232 has an irq set but fails to register the irq it falls back
> to being a non wakeup source. To me it makes more sense to just fall if the config
> has an irq defined.
> 
> Then we could set device_init_wakeup based on there being an irq prior to device_register
> and then request the irq after device register. But fail the probe if the irq request fails.
> 
> Thoughts?
> 

I think you can use device_set_wakeup_capable() afterwards which would
fit this use case. Can you try that?

Anyway, I have a pending rework of the rtc registration that will solve
this issue (and the sysfs attribute creation race).

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] Re: [PATCH 3/3] rtc: ds3232: Call device_init_wakeup before device_register

[Phil Reid]

On 24/02/2017 09:46, Alexandre Belloni wrote:
> On 22/02/2017 at 09:33:14 +0800, Phil Reid wrote:
>> On 22/02/2017 04:33, Alexandre Belloni wrote:
>>> On 17/02/2017 at 09:44:58 +0800, Phil Reid wrote:
>>>> The wakealarm attribute is currently not exposed in the sysfs interface
>>>> as the device has not been set as doing wakealarm when device_register
>>>> is called. Changing the order of the calls fixes that problem. Interrupts
>>>> are cleared in check_rtc_status prior to requesting the interrupt.
>>>>
>>>
>>> This basically revert b4b77f3c280e38cec178f81d7a4d7e65f4045913 So I'm
>>> not sure this is sufficient to ensure the IRQ will never fire.
>>>
>>> (I don't know whether this was a real bug or something reported by a
>>> static analysis tool).
>>
>> G'day Alexandre,
>> Without this change the wakealarm sysfs is never created.
>> This is done in rtc_device_register, but a filter to wakealarm attribute effectively on
>> dev->power.can_wakeup flag. Which is set via the init wakeup call.
>>
>> But looking at that commit and thinking about it some more I can see the problem
>> b4b77f3c280e38cec178f81d7a4d7e65f4045913 was try to solve.
>>
>> Looking at other drivers some of them have similar order but use their own mutex.
>> eg rtc-rc8803 and don't use the ops_lock
>>
>> Or others set device_init_wakeup and then fail if the irq fails to register.
>>
>> Currently if the ds3232 has an irq set but fails to register the irq it falls back
>> to being a non wakeup source. To me it makes more sense to just fall if the config
>> has an irq defined.
>>
>> Then we could set device_init_wakeup based on there being an irq prior to device_register
>> and then request the irq after device register. But fail the probe if the irq request fails.
>>
>> Thoughts?
>>
>
> I think you can use device_set_wakeup_capable() afterwards which would
> fit this use case. Can you try that?
>
> Anyway, I have a pending rework of the rtc registration that will solve
> this issue (and the sysfs attribute creation race).
>
Nope, doesn't help.
device_init_wakeup already calls device_set_wakeup_capable.

rtc_attr_groups is fetched via rtc_get_dev_attribute_groups
and assigned to the rtc->dev.groups in rtc_device_register
The is_visible callback checks device_can_wakeup.
And this is not set until after device creation.

Is there a way to retrigger the sysfs groups creation after device
registration? Thou this would result in sysfs attrib. races I guess.

Failing to probe when irq requested seems the easiest.
Would anyone want the irq registration to fail but device creation succeed if requested?

-- 
Regards
Phil Reid

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] Re: [PATCH 3/3] rtc: ds3232: Call device_init_wakeup before device_register

[Alexandre Belloni]

On 24/02/2017 at 10:32:40 +0800, Phil Reid wrote:
> On 24/02/2017 09:46, Alexandre Belloni wrote:
> > On 22/02/2017 at 09:33:14 +0800, Phil Reid wrote:
> > > On 22/02/2017 04:33, Alexandre Belloni wrote:
> > > > On 17/02/2017 at 09:44:58 +0800, Phil Reid wrote:
> > > > > The wakealarm attribute is currently not exposed in the sysfs interface
> > > > > as the device has not been set as doing wakealarm when device_register
> > > > > is called. Changing the order of the calls fixes that problem. Interrupts
> > > > > are cleared in check_rtc_status prior to requesting the interrupt.
> > > > > 
> > > > 
> > > > This basically revert b4b77f3c280e38cec178f81d7a4d7e65f4045913 So I'm
> > > > not sure this is sufficient to ensure the IRQ will never fire.
> > > > 
> > > > (I don't know whether this was a real bug or something reported by a
> > > > static analysis tool).
> > > 
> > > G'day Alexandre,
> > > Without this change the wakealarm sysfs is never created.
> > > This is done in rtc_device_register, but a filter to wakealarm attribute effectively on
> > > dev->power.can_wakeup flag. Which is set via the init wakeup call.
> > > 
> > > But looking at that commit and thinking about it some more I can see the problem
> > > b4b77f3c280e38cec178f81d7a4d7e65f4045913 was try to solve.
> > > 
> > > Looking at other drivers some of them have similar order but use their own mutex.
> > > eg rtc-rc8803 and don't use the ops_lock
> > > 
> > > Or others set device_init_wakeup and then fail if the irq fails to register.
> > > 
> > > Currently if the ds3232 has an irq set but fails to register the irq it falls back
> > > to being a non wakeup source. To me it makes more sense to just fall if the config
> > > has an irq defined.
> > > 
> > > Then we could set device_init_wakeup based on there being an irq prior to device_register
> > > and then request the irq after device register. But fail the probe if the irq request fails.
> > > 
> > > Thoughts?
> > > 
> > 
> > I think you can use device_set_wakeup_capable() afterwards which would
> > fit this use case. Can you try that?
> > 
> > Anyway, I have a pending rework of the rtc registration that will solve
> > this issue (and the sysfs attribute creation race).
> > 
> Nope, doesn't help.
> device_init_wakeup already calls device_set_wakeup_capable.
> 
> rtc_attr_groups is fetched via rtc_get_dev_attribute_groups
> and assigned to the rtc->dev.groups in rtc_device_register
> The is_visible callback checks device_can_wakeup.
> And this is not set until after device creation.
> 
> Is there a way to retrigger the sysfs groups creation after device
> registration? Thou this would result in sysfs attrib. races I guess.
> 
> Failing to probe when irq requested seems the easiest.
> Would anyone want the irq registration to fail but device creation succeed if requested?
> 

Yes because th RTC is still functional and can give the time (and that
may break userspace).

What you can do is call device_init_wakeup before registering the rtc
and then device_set_wakeup_capable(&client->dev, false); if the irq
request fails.


> -- 
> Regards
> Phil Reid
> 

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.