Re: [RFC PATCHv2 1/4] drivers/otp: add initial support for OTP memory

[Arnd Bergmann <arnd@arndb.de>]

On Thursday 24 March 2011 16:21:08 Jamie Iles wrote:
> OTP memory is typically found in some embedded devices and can be
> used for storing boot code, cryptographic keys and other persistent
> information onchip.  This patch adds a generic layer that devices can
> register OTP with and allows access through a set of character
> device nodes.

Looks very nice overall, but I have a few minor comments.

> diff --git a/Documentation/ABI/testing/sysfs-bus-otp b/Documentation/ABI/testing/sysfs-bus-otp
> new file mode 100644
> index 0000000..4dbc652
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-otp
> @@ -0,0 +1,68 @@
> +What:           /sys/bus/otp/
> +Date:           March 2011
> +KernelVersion:  2.6.40+
> +Contact:        Jamie Iles <jamie@jamieiles.com>
> +Description:
> +                The otp bus presents a number of devices where each
> +		device represents a region or device in the SoC. Each region
> +		will create a device node which allows the region to be
> +		written with read()/write() calls and the device on the bus
> +		has attributes for controlling the redundancy format and
> +		getting the region size.

Why is this a bus? You don't have any device matching code or similar,
and the devices typically are on an existing bus_type (e.g. platform_bus).
I think it would make more sense to do this as a class.

> +
> +menuconfig OTP
> +	bool "OTP memory support"
> +	help
> +	  Say y here to support OTP memory found in some embedded devices.
> +	  This memory can commonly be used to store boot code, cryptographic
> +	  keys and other persistent data.

Please spell out OTP at least once here, a lot of people might not
know the TLA.

> +
> +static int otp_open(struct inode *inode, struct file *filp);
> +static int otp_release(struct inode *inode, struct file *filp);
> +static ssize_t otp_write(struct file *filp, const char __user *buf,
> +			 size_t len, loff_t *offs);
> +static ssize_t otp_read(struct file *filp, char __user *buf, size_t len,
> +			loff_t *offs);
> +static loff_t otp_llseek(struct file *filp, loff_t offs, int origin);
> +
> +static const struct file_operations otp_fops = {
> +	.owner	    = THIS_MODULE,
> +	.open	    = otp_open,
> +	.release    = otp_release,
> +	.write	    = otp_write,
> +	.read	    = otp_read,
> +	.llseek	    = otp_llseek,
> +};

The common way to organize source files is to put all definitions in an
order that requires no forward declarations.

Just define the operations first, then the struct file_operations, and
then the rest.

> +
> +static DEFINE_SEMAPHORE(otp_sem);

Semaphores are basically deprecated. Use a mutex instead.

> +static int otp_we, otp_strict_programming;
> +static struct otp_device *otp;
> +static dev_t otp_devno;

Making these global variables limits you to a single device. I think
you can put everything here into struct otp_device and find that from
the device or chardev you get passed when entering the otp code.

> +static void otp_dev_release(struct device *dev)
> +{
> +	struct otp_device *otp = to_otp_device(dev);
> +
> +	kfree(otp);
> +	otp = NULL;
> +}

The final assignment is not needed.

	Arnd