Hi Greg! Thanks for having a look! On 2015-09-21 07:35, Greg Kroah-Hartman wrote: > On Mon, Aug 31, 2015 at 10:08:08PM +0200, peda@xxxxxxxxxxxxxx wrote: >> From: Peter Rosin <peda@xxxxxxxxxx> >> >> Signed-off-by: Peter Rosin <peda@xxxxxxxxxx> > > A whole new driver with exactly no changelog description at all? I > can't take that :( I didn't really expect anyone to take it even with a decent changelog, it was more of a "let's see in how many ways it's wrong"-thing. I modeled the driver after the only potentiometer driver I could find, but it didn't feel right, which I hint at below... >> --- >> Documentation/misc-devices/mcp4xxx_dpot.txt | 47 +++++ >> MAINTAINERS | 5 + >> drivers/misc/Kconfig | 15 ++ >> drivers/misc/Makefile | 1 + >> drivers/misc/mcp4xxx_dpot.c | 269 +++++++++++++++++++++++++++ >> drivers/misc/mcp4xxx_dpot.h | 44 +++++ >> 6 files changed, 381 insertions(+) >> create mode 100644 Documentation/misc-devices/mcp4xxx_dpot.txt >> create mode 100644 drivers/misc/mcp4xxx_dpot.c >> create mode 100644 drivers/misc/mcp4xxx_dpot.h >> >> This patch has two checkpatch errors but I really don't know how to fix them. >> The offending code was copied from drivers/misc/ad525x_dpot.c and the errors >> are: >> >> ERROR: Macros with complex values should be enclosed in parentheses >> #266: FILE: drivers/misc/mcp4xxx_dpot.c:129: >> +#define MCP4XXX_DPOT_DEVICE_SHOW_SET(name, reg) \ >> +MCP4XXX_DPOT_DEVICE_SHOW(name, reg) \ >> +MCP4XXX_DPOT_DEVICE_SET(name, reg) \ >> +static DEVICE_ATTR(name, S_IWUSR | S_IRUGO, show_##name, set_##name) >> >> ERROR: Macros with complex values should be enclosed in parentheses >> #271: FILE: drivers/misc/mcp4xxx_dpot.c:134: >> +#define MCP4XXX_DPOT_DEVICE_SHOW_ONLY(name, reg) \ >> +MCP4XXX_DPOT_DEVICE_SHOW(name, reg) \ >> +static DEVICE_ATTR(name, S_IWUSR | S_IRUGO, show_##name, NULL) >> >> I have tried to add various parentheses, but no luck. >> >> I am also unsure if I have modelled this on deprecated stuff. Is >> there a better place to add a digital potentiometer driver? ...around here, but you seem to have missed that completely? >> Industrial IO perhaps? In any case, this is a sufficient implementation >> for my needs, and a more complex user-space api is only going to be a >> burden. >> >> Cheers, >> Peter >> >> diff --git a/Documentation/misc-devices/mcp4xxx_dpot.txt b/Documentation/misc-devices/mcp4xxx_dpot.txt >> new file mode 100644 >> index 000000000000..10ed02958775 >> --- /dev/null >> +++ b/Documentation/misc-devices/mcp4xxx_dpot.txt >> @@ -0,0 +1,47 @@ >> +--------------------------------- >> + MCP4XXX Digital Potentiometers >> +--------------------------------- >> + >> +The mcp4xxx_dpot driver exports a simple sysfs interface. This allows you to >> +work with the immediate resistance settings. > > sysfs files all need to be documented in Documentation/ABI/ not in > random Documentation files :) > > Also, why isn't this just an IIO driver? Creating one-off sysfs files > for each driver is a path to madness, please work on using standard > interfaces if at all possible. I suspected IIO might be the right place, but when I looked at IIO I only noticed drivers for things with a real-world connection, e.g. movement or temperature. And a potentiometer seemed like a more general thing, more like a component that you build some real-world thing from. So, since I couldn't find an existing pot in IIO, I did a dirty driver based on the only pot-driver I could find, more or less just to see what I should have been doing. After having a closer look at IIO I also notice DACs and ADCs, which are kind of low-level like pots. Oh well, it's not always easy to know where a new driver should go, and it's also not easy to knew where one should ask... I apologize for not being more clear about the nature of the patch, and I hope I didn't waste too much of your time. Anyway, let's scrap that attempt. I'll come back with an IIO driver in a bit. I haven't previously worked with IIO, so I'm bound to fall into some trap, please bear with me... Cheers, Peter -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
