Re: [PATCH v2 0/1] PN544 NFC driver.
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.aalto...@nokia.com 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/1] PN544 NFC driver.
On Fri, 2010-10-29 at 23:50 +0100, ext Mark Brown wrote: On Fri, Oct 29, 2010 at 02:33:33PM -0700, Andrew Morton wrote: 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. There's other NFC devices out there being used with Linux. But NFC drivers are not easy to find. In the kernel code drivers/parisc/pdc_stable.c contains the following line: { USB_DEVICE(0x10C4, 0x8115) }, /* Arygon NFC/Mifare Reader */ That's the only reference to NFC I found. The NFC/Mifare DDS product datasheet claims that The Linux driver is already distributed with recent 2.6 series kernels but it doesn't seem to be there... B.R. Matti -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/1] PN544 NFC driver.
On Mon, Nov 01, 2010 at 02:25:25PM +0200, Matti J. Aaltonen wrote: On Fri, 2010-10-29 at 23:50 +0100, ext Mark Brown wrote: There's other NFC devices out there being used with Linux. But NFC drivers are not easy to find. In the kernel code drivers/parisc/pdc_stable.c contains the following line: ... The NFC/Mifare DDS product datasheet claims that The Linux driver is already distributed with recent 2.6 series kernels but it doesn't seem to be there... I said nothing about mainline; I've not noticing anything there. I was commenting on the question of needing to have an interface which can be reused by other devices. I've never actually worked with such parts so I've no idea what that would look like. -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/1] PN544 NFC driver.
On Fri, 29 Oct 2010 09:26:08 +0300 Matti J. Aaltonen matti.j.aalto...@nokia.com 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? +/* 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? 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? 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? 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. 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. 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. 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? 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. -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/1] PN544 NFC driver.
On Fri, Oct 29, 2010 at 02:33:33PM -0700, Andrew Morton wrote: 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. There's other NFC devices out there being used with Linux. -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html