Hi. On Fri, 2010-10-29 at 14:33 -0700, ext Andrew Morton wrote: > On Fri, 29 Oct 2010 09:26:08 +0300 > "Matti J. Aaltonen" <matti.j.aaltonen@xxxxxxxxx> wrote: > > > Hello. > > > > And thanks to Andrew for the comments. > > > > >> include/linux/pn544.h | 99 ++++++ > > > > > > Is drivers/misc/ the best place for this? > > > > > > Don't be afraid to create a new drivers/nfc/ even if it has only one > > > driver in it. If someone later comes in and adds a new NFC driver then > > > things will all fall into place. > > > > OK. Now I've created directories drivers/nfc, include/linux/nfc and Documentation/nfc. > > I beefed up the changelog a bit, telling people what "nfc" is. > > We really should tell more people that we're adding a new subsystem. > Can you please resend the patchset, cc'ing linux-kernel? OK... > > >> +/* sysfs interface */ > > > > > > OK, this is more serious. > > > > > > You're proposing a permanent addition to the kernel ABI. This is the > > > most important part of the driver because it is something we can never > > > change. This interface is the very first thing we'll want to > > > understand and review, before we even look at the implementation. > > > > > > And it isn't even described! Not in the changelog, not in a > > > documentation file, not even in code comments. > > > > > > Please, provide a description of this proposed interface. Sufficient > > > for reviewers to understand it and for users to use it. Pobably this > > > will require some description of the hardware functions as well. > > > > > > Please also consider updating Documentation/ABI/ > > > > I've added a documentation file. But I didn't make changes to the interface > > yet. > > So we can expect some updates? Yes in the sense that I'm not yet sure what the community wants and so making a big leap could just go in the wrong direction... > > > > > > And an ioctl interface as well! An undocmented one. > > > > > > ioctls are pretty unpopular for various reasons. To a large extent, > > > people have been using sysfs interfaces as a repalcement for ioctl()s. > > > But this driver has both! > > > > Some explanatory text written into the documentation file. > > So, the sysfs interface is purely for running a device test? Yes, and if the board data doesn't contain the test the sysfs interface doesn't appear. > And primary communication is via the /dev node, and that node supports > an ioctl() which changes the meaning of reads and writes to that node? There is the "normal" mode, which is practice is used something like 99% of the time and there the special firmware upload mode. > If so, that's a bit of a peculiar interface. Perhaps there should have > been two /dev nodes. Certainly it would be most popular if the ioctl() > interface were to simply disappear. What would be the most popular choice? > Please, do spell all of this out in some detail within the changelog > when resending the code for wider review. There are people out there > (eg Greg, Alan) who are better at these things than I and we should > provide them with all the details up-front without going through > another question-and-answer session or expecting them to grovel through > code to reverse-engineer the interfaces. OK I'll try to do that, but I really thought the driver was simple and the documentation clear enough... > Another consideration here is that if we do expect more NFC devices and > drivers for them, then we should aim for some standardisation of the > interface, from day one. Some discussion of this would also be helpful > for reviewers. I personally think we should aim for standardization. > One other thing: these messages which flow between userspace and the > device. Are they documented or sufficiently well understood so that > non-Nokia people can use this driver? You can get the documentation from www.etsi.org as I said in the document file. > Because we had a driver come up a couple of weeks ago. It was a > simple, clean driver but all it did was to shuffle opaque data between > userspace and the device, and the contents of that data was not > publicly available. I discussed the driver with Linus and he said "no". Yes I remember, I am the contact person for that driver as well. And I also though that these two drivers were very similar - except for the message protocol: "simple and clean". Cheers, Matti -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
