Thanks Again Lee,

On Tue, Feb 12, 2019 at 09:17:23AM +0000, Lee Jones wrote:

On Fri, 08 Feb 2019, Matti Vaittinen wrote:

quoted

quoted

I think an exported function with comments would be better.

So do you mean you would prefer exported function over the pointer from

Yes please.  Call-back pointers for non-subsystem level actions are a
bit messy IMHO.

That's fine. I'll go with exported function then =)

quoted

case it just hides the meaning of values we are passing as arguments.
With raw assignment we at least have some idea what the 0x40 or 0x20 are
referring to =)

Well I do agree with your last comment.

Maybe doing the following would help with the ugliness (i.e. the shear
number of chars):

 unsigned int type_reg_offset_inc = 0;
 for (i = BD70528_INT_GPIO0; i <=  BD70528_INT_GPIO3; i++) {
        <blar> *t = irqs[i].type;
        t->type_reg_offset     = type_reg_offset_inc;
 	t->type_rising_val     = 0x20;
 	t->type_falling_val    = 0x10;
 	t->type_level_high_val = 0x40;
 	t->type_level_low_val  = 0x50;
 	t->types_supported =
 		(IRQ_TYPE_EDGE_BOTH | IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW);
 	type_reg_offset_inc += 2;
 }

I'll go with this for next version.

quoted

quoted

quoted

quoted

quoted

+	/* wdt_set must be called rtc_timer_lock held */

This doesn't make sense.

Umm.. The comment does not make sense? Maybe I can explain it further.

"wdt_set must be called when the rtc_timer_lock is held"

Yes. I wanted to say that who-ever is calling the wdt_set function
below, should have locked the rtc_timer_lock mutex (last in this
struct). The function does not do locking inside because we want the RTC
to be able to perform:

lock
disable wdt (store original state)
set RTC
return wdt original state
unlock

Locking is needed so that we can exclude the watchdog enabling or
disabling the WDT timer between moments when RTC is getting the original
WDT state and re-turning back the old state. Without the lock we have a
risk that WDT-driver enables or disables the timer when RTC is being
set, and RTC overwrites the watchdog driver changes when writing back
the old state. I hope this makes sense now... Any suggestions how to
explain this nicely in english?

I think I did already:

 "wdt_set must be called when the rtc_timer_lock is held"

Actually, this is a little ambiguous.  A better sentence could read:

 "rtc_timer_lock must be taken before calling wdt_set()"

Sure. I'll ruthlessy plagiarize that sentence. (And as I am not at all
sure if "ruthlessy plagiarize" actually means what I think it does -
I tried to say that I'll take your suggestion and use it :] )

Once again, thanks for the help =)

Br,
	Matti

-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes.  Just then, he vanished ~~~