Re: [RFC PATCH 1/2] picoxcell-otp: add support for picoxcell OTP devices

[Jamie Iles <jamie@jamieiles.com>]

Replying to myself...

On Wed, Mar 23, 2011 at 03:12:42PM +0000, Jamie Iles wrote:
> On Wed, Mar 23, 2011 at 07:42:30AM -0700, Greg KH wrote:
> > On Wed, Mar 23, 2011 at 12:16:58PM +0000, Jamie Iles wrote:
> > > +What:           /sys/bus/picoxcell-otp/devices/.../size
> > > +Date:           March 2011
> > > +KernelVersion:  2.6.40+
> > > +Contact:        Jamie Iles <jamie@jamieiles.com>
> > > +Description:
> > > +                The effective storage size of the region. This is the amount
> > > +		of data that a user can store in the region taking into
> > > +		account the number of regions and the redundancy format of the
> > > +		region itself.
> > > diff --git a/Documentation/ABI/testing/sysfs-platform-picoxcell-otp b/Documentation/ABI/testing/sysfs-platform-picoxcell-otp
> > > new file mode 100644
> > > index 0000000..e5ee711
> > > --- /dev/null
> > > +++ b/Documentation/ABI/testing/sysfs-platform-picoxcell-otp
> > > @@ -0,0 +1,39 @@
> > > +What:		/sys/devices/platform/picoxcell-otp*/write_enable
> > 
> > Why are these in a platform subdirectory?  Shouldn't they be the devices
> > listed above in the previous file?
> 
> So the way I have it is that there is the real OTP device which can be 
> split into a number of regions.  These attributes affect the physical 
> device by programming the number of regions and write enable.
> 
> Each region is a virtual device to provide the character device and the 
> redundancy/size attributes but you can't split these regions down again.

[...]

> > > +/*
> > > + * Add all of the device entries to sysfs. This also includes creating the
> > > + * region device nodes and sysfs entries.
> > > + */
> > > +static int otp_sysfs_add(struct device *dev)
> > > +{
> > > +	int err;
> > > +
> > > +	err = device_create_file(dev, &dev_attr_write_enable);
> > > +	if (err)
> > > +		goto out;
> > > +
> > > +	err = device_create_file(dev, &dev_attr_num_regions);
> > > +	if (err)
> > > +		goto num_regions_fail;
> > > +
> > > +	err = device_create_file(dev, &dev_attr_bad_words);
> > > +	if (err)
> > > +		goto bad_words_fail;
> > > +
> > > +	err = device_create_file(dev, &dev_attr_strict_programming);
> > > +	if (err)
> > > +		goto strict_programming_fail;
> > > +
> > 
> > Shouldn't all of these be in an attribute group like the other sysfs
> > files are in this driver?  That way you add/remove them all at once.
> 
> I did look at doing this but I couldn't see a way to add an attribute 
> group to an existing device in a single step, or is this just the 
> wrong approach all together?

So the crucial thing I was missing was the device_type part of the 
driver model.  I'm now creating a virtual "otpa" device that is in the 
"otp" bus and has the regions has virtual child devices.   Both of these 
virtual devices have different device_types that have different 
attributes so that fits in very nicely.

Jamie