Re: [PATCH 6/6] ARM: at91: add Shutdown Controller (SHDWC) DT support

On 18:49 Wed 07 Mar     , Arnd Bergmann wrote:

On Wednesday 07 March 2012, Jean-Christophe PLAGNIOL-VILLARD wrote:

quoted

On 20:24 Fri 02 Mar     , Arnd Bergmann wrote:

quoted

On Friday 02 March 2012, Jean-Christophe PLAGNIOL-VILLARD wrote:

quoted

+       }
+
+       if (of_device_is_compatible(np, "atmel,at91sam9x5-shdwc")) {
+               have_rtt = false;
+               have_rtc = true;
+       } else if (of_device_is_compatible(np, "atmel,at91sam9rl-shdwc")) {
+               have_rtt = true;
+               have_rtc = true;
+       } else {
+               have_rtt = true;
+               have_rtc = false;
+       }
+
+       if (have_rtc && of_property_read_bool(np, "atmel,wakeup-rtc-timer"))
+                       mode |= AT91_SHDW_RTCWKEN;
+
+       if (have_rtt && of_property_read_bool(np, "atmel,wakeup-rtt-timer"))
+                       mode |= AT91_SHDW_RTTWKEN;
+
+       at91_shdwc_write(AT91_SHDW_MR, wakeup_mode | mode);
+

Hi Jean-Christophe,

I don't understand why you check the specific part here. Isn't it enough to
check the property when you already mandate that they can only be present
on devices that support the specific wakeup?

If there is a good explanation for that, maybe add a code comment why it's
required.

some wake update source exist on few soc and we are not supposed to set the
bit otherwise

I still don't understand: Doesn't the property already give the information?

Yes

In general, you should try to encode these things in specific properties instead of
checking the compatible property.

But I check that no mistake is done in the DT as the source of wakeup is
availlable on different version of the IP

Just more cautious

Best Regards,
J.