From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757137AbbIURlB (ORCPT ); Mon, 21 Sep 2015 13:41:01 -0400 Received: from mail.lysator.liu.se ([130.236.254.3]:42605 "EHLO mail.lysator.liu.se" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753873AbbIURk6 (ORCPT ); Mon, 21 Sep 2015 13:40:58 -0400 Subject: Re: [PATCH] misc: mcp4xxx_dpot: Driver for Microchip digital potentiometers To: Greg Kroah-Hartman References: <1441051689-25456-1-git-send-email-peda@lysator.liu.se> <20150921053537.GF24350@kroah.com> Cc: linux-kernel@vger.kernel.org, Peter Rosin , Jonathan Corbet , Arnd Bergmann , linux-doc@vger.kernel.org From: Peter Rosin Message-ID: <56004125.7090801@lysator.liu.se> Date: Mon, 21 Sep 2015 19:40:53 +0200 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 MIME-Version: 1.0 In-Reply-To: <20150921053537.GF24350@kroah.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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@lysator.liu.se wrote: >> From: Peter Rosin >> >> Signed-off-by: Peter Rosin > > 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