Re: rtc: ds1307: call the platform's logic for handle IRQs.

Re: rtc: ds1307: call the platform's logic for handle IRQs.

[Heiner Kallweit]

Hi Enric,

I just saw your submitted patch on patchwork. As I haven't been subscribed
to linux-rtc list yet, I can't reply to the original mail.

Few remarks:
I think the same can be achieved easier (apart from the fact that member
irq was just removed from struct ds1307).
The curent call to device_set_wakeup_capable has to be replaced with
device_init_wakeup, in addition we have to call dev_pm_set_wake_irq to
register the interrupt with the Linux wakeup core. Then the core takes
care of everything.

See also rtc-ds1343, although I think the calls to enable/disable_irq_wake
are not needed there because the core takes care of this already
(enable_irq_wake is called from dev_pm_arm_wake_irq).

Rgds, Heiner

Re: rtc: ds1307: call the platform's logic for handle IRQs.

[Heiner Kallweit]

Am 24.08.2017 um 20:46 schrieb Heiner Kallweit:
> Hi Enric,
> 
> I just saw your submitted patch on patchwork. As I haven't been subscribed
> to linux-rtc list yet, I can't reply to the original mail.
> 
> Few remarks:
> I think the same can be achieved easier (apart from the fact that member
> irq was just removed from struct ds1307).
> The curent call to device_set_wakeup_capable has to be replaced with
> device_init_wakeup, in addition we have to call dev_pm_set_wake_irq to
> register the interrupt with the Linux wakeup core. Then the core takes
> care of everything.
> 
After further checking not even this may be necessary.
If flag I2C_CLIENT_WAKE is set for the i2c client then i2c_device_probe()
enables the device as wakeup device and registers the interrupt with the
wakeup core.
If the i2c client is defined via DT, then of_i2c_register_device() sets
this flag based on the "wakeup-source" property.
Is your device configured via DT? If yes, did you check whether wakeup
works w/o your patch with just setting property wakeup-source ?


> See also rtc-ds1343, although I think the calls to enable/disable_irq_wake
> are not needed there because the core takes care of this already
> (enable_irq_wake is called from dev_pm_arm_wake_irq).
> 
> Rgds, Heiner
> 

Re: rtc: ds1307: call the platform's logic for handle IRQs.

[Enric Balletbo i Serra]

Hi Heiner

On 24/08/17 21:16, Heiner Kallweit wrote:
> Am 24.08.2017 um 20:46 schrieb Heiner Kallweit:
>> Hi Enric,
>>
>> I just saw your submitted patch on patchwork. As I haven't been subscribed
>> to linux-rtc list yet, I can't reply to the original mail.
>>
>> Few remarks:
>> I think the same can be achieved easier (apart from the fact that member
>> irq was just removed from struct ds1307).
>> The curent call to device_set_wakeup_capable has to be replaced with
>> device_init_wakeup, in addition we have to call dev_pm_set_wake_irq to
>> register the interrupt with the Linux wakeup core. Then the core takes
>> care of everything.
>>

Oh I just saw your patches, thanks to advice me about them.

> After further checking not even this may be necessary.
> If flag I2C_CLIENT_WAKE is set for the i2c client then i2c_device_probe()
> enables the device as wakeup device and registers the interrupt with the
> wakeup core.
> If the i2c client is defined via DT, then of_i2c_register_device() sets
> this flag based on the "wakeup-source" property.
> Is your device configured via DT? If yes, did you check whether wakeup
> works w/o your patch with just setting property wakeup-source ?
> 

Yes, device is configured via DT and without my patch, just setting property
wakeup-source didn't work, the reason is that code:

http://elixir.free-electrons.com/linux/v4.13-rc6/source/drivers/rtc/rtc-ds1307.c#L1382

I didn't test on top of your patches so let me rebase/rethink my patch on top of
yours.

> 
>> See also rtc-ds1343, although I think the calls to enable/disable_irq_wake
>> are not needed there because the core takes care of this already
>> (enable_irq_wake is called from dev_pm_arm_wake_irq).
>>
>> Rgds, Heiner
>>
> 

Thanks,
 Enric

Re: rtc: ds1307: call the platform's logic for handle IRQs.

[Heiner Kallweit]

Am 24.08.2017 um 22:13 schrieb Enric Balletbo i Serra:
> Hi Heiner
> 
> On 24/08/17 21:16, Heiner Kallweit wrote:
>> Am 24.08.2017 um 20:46 schrieb Heiner Kallweit:
>>> Hi Enric,
>>>
>>> I just saw your submitted patch on patchwork. As I haven't been subscribed
>>> to linux-rtc list yet, I can't reply to the original mail.
>>>
>>> Few remarks:
>>> I think the same can be achieved easier (apart from the fact that member
>>> irq was just removed from struct ds1307).
>>> The curent call to device_set_wakeup_capable has to be replaced with
>>> device_init_wakeup, in addition we have to call dev_pm_set_wake_irq to
>>> register the interrupt with the Linux wakeup core. Then the core takes
>>> care of everything.
>>>
> 
> Oh I just saw your patches, thanks to advice me about them.
> 
>> After further checking not even this may be necessary.
>> If flag I2C_CLIENT_WAKE is set for the i2c client then i2c_device_probe()
>> enables the device as wakeup device and registers the interrupt with the
>> wakeup core.
>> If the i2c client is defined via DT, then of_i2c_register_device() sets
>> this flag based on the "wakeup-source" property.
>> Is your device configured via DT? If yes, did you check whether wakeup
>> works w/o your patch with just setting property wakeup-source ?
>>
> 
> Yes, device is configured via DT and without my patch, just setting property
> wakeup-source didn't work, the reason is that code:
> 
> http://elixir.free-electrons.com/linux/v4.13-rc6/source/drivers/rtc/rtc-ds1307.c#L1382
> 
This shouldn't be the reason because registering the interrupt with the wakeup core
happens earlier in i2c_device_probe().
Can you check whether dev_pm_set_wake_irq() is actually called with the right
irq number in i2c_device_probe()?

Would be interesting to see the DT config of your rtc.

> I didn't test on top of your patches so let me rebase/rethink my patch on top of
> yours.
> 
>>
>>> See also rtc-ds1343, although I think the calls to enable/disable_irq_wake
>>> are not needed there because the core takes care of this already
>>> (enable_irq_wake is called from dev_pm_arm_wake_irq).
>>>
>>> Rgds, Heiner
>>>
>>
> 
> Thanks,
>  Enric
> 

Re: rtc: ds1307: call the platform's logic for handle IRQs.

[Heiner Kallweit]

Am 24.08.2017 um 22:44 schrieb Heiner Kallweit:
> Am 24.08.2017 um 22:13 schrieb Enric Balletbo i Serra:
>> Hi Heiner
>>
>> On 24/08/17 21:16, Heiner Kallweit wrote:
>>> Am 24.08.2017 um 20:46 schrieb Heiner Kallweit:
>>>> Hi Enric,
>>>>
>>>> I just saw your submitted patch on patchwork. As I haven't been subscribed
>>>> to linux-rtc list yet, I can't reply to the original mail.
>>>>
>>>> Few remarks:
>>>> I think the same can be achieved easier (apart from the fact that member
>>>> irq was just removed from struct ds1307).
>>>> The curent call to device_set_wakeup_capable has to be replaced with
>>>> device_init_wakeup, in addition we have to call dev_pm_set_wake_irq to
>>>> register the interrupt with the Linux wakeup core. Then the core takes
>>>> care of everything.
>>>>
>>
>> Oh I just saw your patches, thanks to advice me about them.
>>
>>> After further checking not even this may be necessary.
>>> If flag I2C_CLIENT_WAKE is set for the i2c client then i2c_device_probe()
>>> enables the device as wakeup device and registers the interrupt with the
>>> wakeup core.
>>> If the i2c client is defined via DT, then of_i2c_register_device() sets
>>> this flag based on the "wakeup-source" property.
>>> Is your device configured via DT? If yes, did you check whether wakeup
>>> works w/o your patch with just setting property wakeup-source ?
>>>
>>
>> Yes, device is configured via DT and without my patch, just setting property
>> wakeup-source didn't work, the reason is that code:
>>
>> http://elixir.free-electrons.com/linux/v4.13-rc6/source/drivers/rtc/rtc-ds1307.c#L1382
>>
> This shouldn't be the reason because registering the interrupt with the wakeup core
> happens earlier in i2c_device_probe().
> Can you check whether dev_pm_set_wake_irq() is actually called with the right
> irq number in i2c_device_probe()?
> 
See also description of commit 4990d4fe327b9d9a7a
"PM / Wakeirq: Add automated device wake IRQ handling" from 2015.
It explains why the code you want to add shouldn't be needed any longer.

> Would be interesting to see the DT config of your rtc.
> 
>> I didn't test on top of your patches so let me rebase/rethink my patch on top of
>> yours.
>>
>>>
>>>> See also rtc-ds1343, although I think the calls to enable/disable_irq_wake
>>>> are not needed there because the core takes care of this already
>>>> (enable_irq_wake is called from dev_pm_arm_wake_irq).
>>>>
>>>> Rgds, Heiner
>>>>
>>>
>>
>> Thanks,
>>  Enric
>>
> 

Re: rtc: ds1307: call the platform's logic for handle IRQs.

[Enric Balletbo i Serra]

Hi Heiner,

On 24/08/17 22:53, Heiner Kallweit wrote:
> Am 24.08.2017 um 22:44 schrieb Heiner Kallweit:
>> Am 24.08.2017 um 22:13 schrieb Enric Balletbo i Serra:
>>> Hi Heiner
>>>
>>> On 24/08/17 21:16, Heiner Kallweit wrote:
>>>> Am 24.08.2017 um 20:46 schrieb Heiner Kallweit:
>>>>> Hi Enric,
>>>>>
>>>>> I just saw your submitted patch on patchwork. As I haven't been subscribed
>>>>> to linux-rtc list yet, I can't reply to the original mail.
>>>>>
>>>>> Few remarks:
>>>>> I think the same can be achieved easier (apart from the fact that member
>>>>> irq was just removed from struct ds1307).
>>>>> The curent call to device_set_wakeup_capable has to be replaced with
>>>>> device_init_wakeup, in addition we have to call dev_pm_set_wake_irq to
>>>>> register the interrupt with the Linux wakeup core. Then the core takes
>>>>> care of everything.
>>>>>
>>>
>>> Oh I just saw your patches, thanks to advice me about them.
>>>
>>>> After further checking not even this may be necessary.
>>>> If flag I2C_CLIENT_WAKE is set for the i2c client then i2c_device_probe()
>>>> enables the device as wakeup device and registers the interrupt with the
>>>> wakeup core.
>>>> If the i2c client is defined via DT, then of_i2c_register_device() sets
>>>> this flag based on the "wakeup-source" property.
>>>> Is your device configured via DT? If yes, did you check whether wakeup
>>>> works w/o your patch with just setting property wakeup-source ?
>>>>
>>>
>>> Yes, device is configured via DT and without my patch, just setting property
>>> wakeup-source didn't work, the reason is that code:
>>>
>>> http://elixir.free-electrons.com/linux/v4.13-rc6/source/drivers/rtc/rtc-ds1307.c#L1382
>>>
>> This shouldn't be the reason because registering the interrupt with the wakeup core
>> happens earlier in i2c_device_probe().
>> Can you check whether dev_pm_set_wake_irq() is actually called with the right
>> irq number in i2c_device_probe()?
>>

You have reason again, I did something wrong on my tests and read the code in a
wrong way, to sum up, adding both, the interrupt pin and the wakeup-source
propriety, works as expected. So forget this patch. Sorry for the noise and
thanks for replying and look at this.

> See also description of commit 4990d4fe327b9d9a7a
> "PM / Wakeirq: Add automated device wake IRQ handling" from 2015.
> It explains why the code you want to add shouldn't be needed any longer.
> 
>> Would be interesting to see the DT config of your rtc.


        rtc0: rtc@68 {
                compatible = "dallas,ds1339";
                pinctrl-names = "default";
                pinctrl-0 = <&rtc0_irq_pins>;
                interrupt-parent = <&gpio0>;
                interrupts = <23 IRQ_TYPE_EDGE_FALLING>; /* gpio 23 */
                wakeup-source;
                trickle-resistor-ohms = <2000>;
                reg = <0x68>;
        };

>>
>>> I didn't test on top of your patches so let me rebase/rethink my patch on top of
>>> yours.
>>>
>>>>
>>>>> See also rtc-ds1343, although I think the calls to enable/disable_irq_wake
>>>>> are not needed there because the core takes care of this already
>>>>> (enable_irq_wake is called from dev_pm_arm_wake_irq).
>>>>>
>>>>> Rgds, Heiner
>>>>>
>>>>
>>>
>>> Thanks,
>>>  Enric
>>>
>>
>