On 07/23/2013 01:53 PM, Gerhard Sittig wrote:

On Mon, Jul 22, 2013 at 14:31 +0200, Marc Kleine-Budde wrote:

quoted

On 07/22/2013 02:14 PM, Gerhard Sittig wrote:

quoted

the .get_clock() callback is run from probe() and might allocate
resources, introduce a .put_clock() callback that is run from remove()
to undo any allocation activities

looks good

quoted

use devm_get_clk() upon lookup (for SYS and REF) to have the clocks put
upon driver unload

fine

quoted

assume that resources get prepared but not necessarily enabled in the
setup phase, make the open() and close() callbacks of the CAN network
device enable and disable a previously acquired and prepared clock

I think you should call prepare_enable and disable_unprepare in the
open/close functions.

After more local research, which totally eliminated the need to
pre-enable the CAN related clocks, but might need more discussion
as it touches the common gate support, I've learned something
more:

The CAN clock needs to get enabled during probe() already, since
registers get accessed between probe() for the driver and open()
for the network device -- while access to peripheral registers
crashes the kernel when clocks still are disabled (other hardware
may just hang or provide fake data, neither of this is OK).

Then call prepare_enable(); before and disable_unprepare(); after
accessing the registers. Have a look at the flexcan driver.

But I see the point in your suggestion to prepare _and_ enable
the clock during open() as well -- to have open() cope with
whatever probe() did, after all the driver is shared among
platforms, which may differ in what they do during probe().

If you enable a clock to access the registers before open() (and disable
it afterwards), it should not harm any architecture that doesn't need
this clock enabled.

So I will:
- make open() of the network device prepare _and_ enable the
  clock for the peripheral (if acquired during probe())

good

- adjust open() because ATM it leaves the clock enabled when the
  network device operation fails (the error path is incomplete in
  v3)

yes, clock should be disabled if open() fails.

- make the MPC512x specific probe() time .get_clock() routine not
  just prepare but enable the clock as well

If needed enable the clock, but disable after probe() has finished.

- and of course address all the shutdown counter parts of the
  above setup paths

This results in:
- specific chip drivers only need to balance their private get
  and put clock routines which are called from probe and remove,
  common paths DTRT for all of them

Yes, but clock should not stay enabled between probe() and open().

[...]

Removing unnecessary devm_put_clk() calls is orthogonal to that.
Putting these in isn't totally wrong (they won't harm, and they
do signal "visual balance" more clearly such that the next person
won't stop and wonder), but it's true that they are redundant.
"Trained persons" will wonder as much about their presence as
untrained persons wonder about their absence. :)  Apparently I'm
not well trained yet.

The whole point about devm_* is to get rid of auto manually tear down
functions. So please remove all devm_put_clk() calls, as it will be
called automatically if a driver instance is removed.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |