Re: [PATCH] arch/tile: add arch/tile/drivers/ directory with SROM driver

[Arnd Bergmann <arnd@arndb.de>]

On Saturday 21 May 2011 05:21:02 Greg KH wrote:
> On Fri, May 20, 2011 at 07:39:10PM -0400, Chris Metcalf wrote:
> > On 5/20/2011 6:40 PM, Eric Biederman wrote:
> > 
> > > You are vastly exceeding the one value per file rule of sysfs.
> > 
> > True, but bin_attribute has always been an exception to that rule anyway.
> 
> No it hasn't.  A bin attribute is for something that is "one" value, it
> is a binary file that the kernel doesn't know anything about, nor does
> it intrepret it.
> 
> It sounds like you want this binary attribute file to be a bit different
> than the "normal" one, and because of that you are wanting to modify the
> sysfs file, which proves that you are doing things differently here.

I initially argued that the file is the same as all the other ones
in drivers/misc/eeprom/*.c. None of them care what the data is and
they pass it through to the user. It would also be possible to
always flush after each write, which makes the interface act more
like you would expect it to, at the cost of lower performance in
case the user writes it in small blocks.

> > Originally I proposed a straightforward character device for this
> > role.
> 
> Ah, ok, I think you should do that.
> 
> > Arnd Bergmann encouraged me to look at kernel precedents like
> > drivers/char/eeprom/, which is why I converted this driver to sysfs.
> > The first post in this thread is here:
> > https://lkml.org/lkml/2011/5/4/415 .  Since then I've come around to
> > believing that it's more valuable to group this driver with the other
> > eeprom drivers, rather than with the other paravirtualized tile
> > drivers.
> 
> See above why I don't think that is so.

What I only now noticed is that the other eeprom drivers only support
reading the eeprom, not writing it, so there is a significant difference.

Using the bin_attribute doesn't sound completely wrong to me, especially
if you put it in your /sys/hypervisor/* direcory together with the
regular attributes we talked about. The character device would also
be an option (better than /proc/ppc/update_flash that is used on pSeries),
but if we do that, I would group it together with the other similar
files (ps3flash, nwflash, ...) in a new subdirectory.

	Arnd