Re: [PATCH] misc: mcp4xxx_dpot: Driver for Microchip digital potentiometers

[Peter Rosin <peda@lysator.liu.se>]

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 <peda@axentia.se>
>>
>> Signed-off-by: Peter Rosin <peda@axentia.se>
> 
> 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