Re: [PATCH v3] UIO: Implement a UIO interface for the SMX Cryptengine

[Paul Mundt <lethal@linux-sh.org>]

On Thu, Mar 13, 2008 at 08:35:40PM +1100, Ben Nizette wrote:
> diff --git a/drivers/uio/uio_smx.c b/drivers/uio/uio_smx.c
> new file mode 100644
> index 0000000..fda8b8b
> --- /dev/null
> +++ b/drivers/uio/uio_smx.c
> @@ -0,0 +1,118 @@
> +/*
> + * UIO SMX Cryptengine driver.
> + *
> + * (C) 2008 Nias Digital P/L <bn@niasdigital.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + */
> +
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/uio_driver.h>
> +
> +#include <asm/io.h>
> +
linux/io.h is preferable here.

> +static int __devinit smx_ce_probe(struct platform_device *dev)
> +{
> +	struct uio_info *info;
> +	struct resource *regs;
> +
> +	info = kzalloc(sizeof(struct uio_info), GFP_KERNEL);
> +	if (!info)
> +		return -ENOMEM;
> +
> +	regs = platform_get_resource(dev, IORESOURCE_MEM, 0);
> +	if (!regs)
> +		goto out_free;
> +
> +	info->mem[0].addr = regs->start;
> +	if (!info->mem[0].addr)
> +		goto out_free;
> +
> +	info->mem[0].size = regs->end - regs->start + 1;
> +	info->mem[0].internal_addr = ioremap(regs->start, info->mem[0].size);
> +	if (!info->mem[0].internal_addr)
> +		goto out_free;
> +
> +	info->mem[0].memtype = UIO_MEM_PHYS;
> +
> +	info->name = "smx-ce";
> +	info->version = "0.03";

You may as well use DRV_NAME and DRV_VERSION, then you can toss that over
to MODULE_VERSION() if you're so inclined. It's generally nice to keep
modinfo happy.

> +	info->irq = platform_get_irq(dev, 0);
> +	info->irq_flags = IRQF_SHARED;
> +	info->handler = smx_handler;
> +
platform_get_irq() can fail, which you don't presently handle (though I
guess uio_register_device() will error out if you hand off -ENXIO as your
IRQ anyways, so it's not technically an issue). That's a pretty minor
issue, though, but might be something you wish to fix up if you are
planning on using this as an example driver for others.

> +	platform_set_drvdata(dev, info);
> +
> +	if (uio_register_device(&dev->dev, info))
> +		goto out_unmap;
> +
> +	return 0;
> +out_unmap:
> +	iounmap(info->mem[0].internal_addr);
> +out_free:
> +	kfree(info);
> +
> +	return -ENODEV;
> +}
> +
Your error paths are also pretty quiet. A dev_err() or so in the few
failure cases you do have might not be a terrible idea.

uio_register_device() also has quite a few different error cases, so it's
at least worth preserving the error value and returning that back,
rather than always returning -ENODEV.