[RFC 0/6] watchdog drivers converted to the new framework

[RFC 0/6] watchdog drivers converted to the new framework

[Wolfram Sang]

As promised, here is a RFC with two examples demonstrating how watchdog drivers
can be converted to use the new watchdog framework (using the current version
Wim posted two days ago). There is also conversion guide put to the
documentation folder. Being RFC, all this is not final yet, but presentable, I
hope.

Although there are a few more consolidation options left, there is already a
gain of ~100 lines per driver. Promising, but there are a few issues to be
sorted out, too, yet nothing which can be dealt with.

I have two other drivers in the making (stmp3xxx and imx2), but they need some
more preparation; the first one needs some internal cleanups (like a lot of
watchdog drivers); the latter one needs an addition to the framework
(installing a timer for non-stoppable devices). I will also prepare a new
driver (mx1) to show how small new drivers can be now :) The aim for all these
driver conversions is inclusion in Linux 3.2. I still hope we can get the basic
framework into Linux 3.1.

Many thanks to CELF/LF for supporting this work and to Wim and Alan for making
the framework!

Looking forward to comments,

   Wolfram

They are also available in the git repository at:
  git://git.pengutronix.de/git/wsa/linux-2.6.git generic-watchdog-with-drivers

Wolfram Sang (6):
  Documentation: watchdog: add guide how to convert drivers to new framework
  watchdog: s3c2410: convert to use the watchdog framework
  watchdog: pnx4008: cleanup resource handling using managed devices
  watchdog: pnx4008: don't use __raw_-accessors
  watchdog: pnx4008: convert driver to use the watchdog framework
  watchdog: pnx4008: WIP refactor disabling device

 .../watchdog/convert_drivers_to_kernel_api.txt     |  195 ++++++++++++++++
 drivers/watchdog/Kconfig                           |    2 +
 drivers/watchdog/pnx4008_wdt.c                     |  246 +++++++-------------
 drivers/watchdog/s3c2410_wdt.c                     |  176 +++-----------
 4 files changed, 313 insertions(+), 306 deletions(-)
 create mode 100644 Documentation/watchdog/convert_drivers_to_kernel_api.txt

-- 
1.7.2.5

[RFC 0/6] watchdog drivers converted to the new framework

[H Hartley Sweeten]

On Wednesday, July 13, 2011 1:26 PM, Wolfram Sang wrote:
> As promised, here is a RFC with two examples demonstrating how watchdog drivers
> can be converted to use the new watchdog framework (using the current version
> Wim posted two days ago). There is also conversion guide put to the
> documentation folder. Being RFC, all this is not final yet, but presentable, I
> hope.
> 
> Although there are a few more consolidation options left, there is already a
> gain of ~100 lines per driver. Promising, but there are a few issues to be
> sorted out, too, yet nothing which can be dealt with.
> 
> I have two other drivers in the making (stmp3xxx and imx2), but they need some
> more preparation; the first one needs some internal cleanups (like a lot of
> watchdog drivers); the latter one needs an addition to the framework
> (installing a timer for non-stoppable devices). I will also prepare a new
> driver (mx1) to show how small new drivers can be now :) The aim for all these
> driver conversions is inclusion in Linux 3.2. I still hope we can get the basic
> framework into Linux 3.1.
> 
> Many thanks to CELF/LF for supporting this work and to Wim and Alan for making
> the framework!
> 
> Looking forward to comments,

What's the status on Wim's watchdog framework patches?  It would be nice to have
them re-posted and CC the linux-arm-kernel list.  Not everyone follows lkml
regularly.

I found Wim's patches on lkml and converted the ep93xx watchdog driver and get:

 drivers/watchdog/ep93xx_wdt.c |  174 +++++++++-------------------------------
 1 files changed, 39 insertions(+), 135 deletions(-)

Overall I like the results.  It also makes the driver a lot easier to follow.

I would also like to convert this driver into a proper platform_driver using
ioremap'ed addresses instead of the static mappings.  Converting the driver
to the new watchdog framework would make this a bit cleaner.

Regards,
Hartley

[RFC 0/6] watchdog drivers converted to the new framework

[Wolfram Sang]

> I would also like to convert this driver into a proper platform_driver using
> ioremap'ed addresses instead of the static mappings.  Converting the driver
> to the new watchdog framework would make this a bit cleaner.

Try managed devices (devm_* and friends) for an extra cleanup bonus.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110714/b945e391/attachment.sig>

[RFC 0/6] watchdog drivers converted to the new framework

[H Hartley Sweeten]

On Thursday, July 14, 2011 11:28 AM, Wolfram Sang wrote:
>> I would also like to convert this driver into a proper platform_driver using
>> ioremap'ed addresses instead of the static mappings.  Converting the driver
>> to the new watchdog framework would make this a bit cleaner.
>
> Try managed devices (devm_* and friends) for an extra cleanup bonus.

Already done, just waiting for the framework... ;-)

Regards,
Hartley

[RFC 0/6] watchdog drivers converted to the new framework

[Wolfram Sang]

On Thu, Jul 14, 2011 at 01:42:02PM -0500, H Hartley Sweeten wrote:
> On Thursday, July 14, 2011 11:28 AM, Wolfram Sang wrote:
> >> I would also like to convert this driver into a proper platform_driver using
> >> ioremap'ed addresses instead of the static mappings.  Converting the driver
> >> to the new watchdog framework would make this a bit cleaner.
> >
> > Try managed devices (devm_* and friends) for an extra cleanup bonus.
> 
> Already done, just waiting for the framework... ;-)

Nice :) Did you use my conversion-guide as a reference? Any comments if so?

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110714/55026e44/attachment.sig>

[RFC 0/6] watchdog drivers converted to the new framework

[H Hartley Sweeten]

On Thursday, July 14, 2011 1:01 PM, Wolfram Sang wrote:
> On Thu, Jul 14, 2011 at 01:42:02PM -0500, H Hartley Sweeten wrote:
>> On Thursday, July 14, 2011 11:28 AM, Wolfram Sang wrote:
>>>> I would also like to convert this driver into a proper platform_driver using
>>>> ioremap'ed addresses instead of the static mappings.  Converting the driver
>>>> to the new watchdog framework would make this a bit cleaner.
>>>
>>> Try managed devices (devm_* and friends) for an extra cleanup bonus.
>> 
>> Already done, just waiting for the framework... ;-)
>
> Nice :) Did you use my conversion-guide as a reference? Any comments if so?

Sorry, I didn't.  I just worked it out from the framework core and looking
at your examples.

When I found the framework patches on lkml I couldn't get them into a state
to apply to my tree.  I ended up manually applying all the pieces so I skipped
the Documentation stuff to save some time.

The conversion is pretty straight forward after examining the watchdog
framework files.

Regards,
Hartley

[RFC 0/6] watchdog drivers converted to the new framework

[Wim Van Sebroeck]

Hi Hartley,

> >>>> I would also like to convert this driver into a proper platform_driver using
> >>>> ioremap'ed addresses instead of the static mappings.  Converting the driver
> >>>> to the new watchdog framework would make this a bit cleaner.
> >>>
> >>> Try managed devices (devm_* and friends) for an extra cleanup bonus.
> >> 
> >> Already done, just waiting for the framework... ;-)
> >
> > Nice :) Did you use my conversion-guide as a reference? Any comments if so?
> 
> Sorry, I didn't.  I just worked it out from the framework core and looking
> at your examples.
> 
> When I found the framework patches on lkml I couldn't get them into a state
> to apply to my tree.  I ended up manually applying all the pieces so I skipped
> the Documentation stuff to save some time.
> 
> The conversion is pretty straight forward after examining the watchdog
> framework files.

Maybe you could allready post your patches to the linux watchdog mailing list.

Kind regards,
Wim.

[RFC 1/6] Documentation: watchdog: add guide how to convert drivers to new framework

[Randy Dunlap]

On Wed, 13 Jul 2011 22:26:01 +0200 Wolfram Sang wrote:

> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> ---
>  .../watchdog/convert_drivers_to_kernel_api.txt     |  195 ++++++++++++++++++++
>  1 files changed, 195 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/watchdog/convert_drivers_to_kernel_api.txt
> 
> diff --git a/Documentation/watchdog/convert_drivers_to_kernel_api.txt b/Documentation/watchdog/convert_drivers_to_kernel_api.txt
> new file mode 100644
> index 0000000..9c0cff9
> --- /dev/null
> +++ b/Documentation/watchdog/convert_drivers_to_kernel_api.txt
> @@ -0,0 +1,195 @@
> +Converting old watchdog drivers to the watchdog framework
> +by Wolfram Sang <w.sang@pengutronix.de>
> +=========================================================
> +
> +Before the watchdog framework came into the kernel, every driver had to
> +implement the API on its own. Now, as the framework factored out the common
> +components, those drivers can be lightened making it a user of the framework.
> +This document shall guide you for this task. The necessary steps are described
> +as well as things to look out for.
> +
> +
> +Remove the file_operations struct
> +---------------------------------
> +
> +Old drivers define their own file_operations for actions like open(), write(),
> +etc... These are now handled by the framework and just call the driver when
> +needed. So, in general, the file operations-struct and assorted functions can

                               file operations struct

> +go. Only very few driver-specific details have to be moved to other functions.
> +Here is a overview of the functions and probably needed actions:
> +
> +- open: Everything dealing with resource management (file-open checks, magic
> +  close preparations) can simply go. Device specific stuff needs to go to the
> +  driver specific start-function. Note that for some drivers, the start-function
> +  also serves as the ping-function. If that is the case and you need start/stop
> +  to be balanced (clocks!), you are better off refactoring a separate start-function.
> +
> +- close: Same hints as for open apply.
> +
> +- write: Can simply go, all defined behaviour is taken care of by the framework,
> +  i.e. ping on write and magic char ('V') handling.
> +
> +- ioctl: While the driver is allowed to have extensions to the IOCTL interface,
> +  the most common ones are handled by the framework, supported by some assistance
> +  from the driver:
> +
[snip]
> +
> +  Other IOCTLs can be served using the ioctl-callback. Note that this is mainly
> +  intended for porting old drivers, new drivers should not invent private IOCTLs.

                          old drivers; new drivers

> +  Private IOCTLs are processed first. When the callback returns with
> +  -ENOIOCTLCMD, the IOCTLs of the framework will be tried, too. Any other error
> +  is directly given to the user.
> +
> +Example conversion:
> +
> +-static const struct file_operations s3c2410wdt_fops = {
> +-       .owner          = THIS_MODULE,
> +-       .llseek         = no_llseek,
> +-       .write          = s3c2410wdt_write,
> +-       .unlocked_ioctl = s3c2410wdt_ioctl,
> +-       .open           = s3c2410wdt_open,
> +-       .release        = s3c2410wdt_release,
> +-};
> +
> +Check the functions for device-specific stuff and keep it for later
> +refactoring. The rest can go.
> +
> +
> +Remove the miscdevice
> +---------------------
[snip]

> +Remove obsolete includes and defines
> +------------------------------------
[snip]
> +
> +Add the watchdog operations
> +---------------------------
> +
> +All possible callbacks are defined in 'struct watchdog_ops'. You can find it
> +explained in 'watchdog-kernel-api.txt' in this directory. start(), stop() and
> +owner must be set, the rest is optional. You will easily find corresponding

I would say:          the rest are optional.

> +functions in the old driver. Note that you will now get a pointer to the
> +watchdog_device as a parameter to these functions, so you probably have to
> +change the function header. Other changes are most likely not needed, because
> +here simply happens the direct hardware access. If you have device-specific
> +code left from the above steps, it should be refactored into these callbacks.
> +
> +Here is a simple example:
[snip]

> +Add the watchdog device
> +-----------------------
[snip]

> +Register the watchdog device
> +----------------------------
[snip]

> +Update the Kconfig-entry
> +------------------------
> +
> +The entry for the driver now needs to select WATCHDOG_CORE:
> +
> ++       select WATCHDOG_CORE
> +
> +
> +Create a patch and send it to upstream
> +--------------------------------------
> +
> +Make sure you understood Documentation/SubmittingPatches and send your patch to
> +linux-watchdog at vger.kernel.org. We are looking forward to it :)

Good job.  Thanks.

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***