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 :( > --- > 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? > > 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. > +--------- > + Files > +--------- > + > +Each dpot device will have its own rdac files. How many depends on the actual > +part you have, as will the range of allowed values. > + > +This rdac file is used to program the immediate value of the device. > + > +----------- > + Example > +----------- > + > +Locate the device in your sysfs tree. This is probably easiest by going into > +the common i2c directory and locating the device by the i2c slave address. > + > + # ls /sys/bus/i2c/devices/ > + 0-0028 0-0050 > + > +So assuming the device in question is on the first i2c bus and has the slave > +address of 0x28, we descend (unrelated sysfs entries have been trimmed). > + > + # ls /sys/bus/i2c/devices/0-0028/ > + rdac0 rdac1 > + > +You can use simple reads/writes to access these files: > + > + # cd /sys/bus/i2c/devices/0-0028/ > + > + # cat rdac0 > + 0 > + # echo 128 > rdac0 > + # cat rdac0 > + 128 > + > + # cat rdac1 > + 5 > + # echo 35 > rdac1 > + # cat rdac1 > + 35 You never say what the contents of these files really are. Nor what the units are, if any, or what this driver even does or is used for. For all I know it just is a sysfs file value storage driver :) > diff --git a/MAINTAINERS b/MAINTAINERS > index b60e2b2369d2..c7bc2cc8051d 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -6600,6 +6600,11 @@ W: http://linuxtv.org > S: Maintained > F: drivers/media/radio/radio-maxiradio* > > +MPC4XXX MICROCHIP DIGITAL POTENTIOMETERS DRIVER > +M: Peter Rosin <peda@xxxxxxxxxx> > +S: Maintained > +F: drivers/misc/mcp4xxx_dpot.* > + > MEDIA DRIVERS FOR RENESAS - VSP1 > M: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> > L: linux-media@xxxxxxxxxxxxxxx > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig > index 42c38525904b..a4e5e42b6b92 100644 > --- a/drivers/misc/Kconfig > +++ b/drivers/misc/Kconfig > @@ -51,6 +51,21 @@ config AD525X_DPOT_SPI > To compile this driver as a module, choose M here: the > module will be called ad525x_dpot-spi. > > +config MCP4XXX_DPOT > + tristate "Microchip Digital Potentiometer" > + depends on I2C && SYSFS > + help > + If you say yes here, you get support for the Microchip > + MCP4531, MCP4532, MCP4551, MCP4552, > + MCP4631, MCP4632, MCP4651, MCP4652, > + digital potentiometer chips. > + > + See Documentation/misc-devices/mcp4xxx_dpot.txt for the > + userspace interface. > + > + This driver can also be built as a module. If so, the module > + will be called mcp4xxx_dpot. > + > config ATMEL_TCLIB > bool "Atmel AT32/AT91 Timer/Counter Library" > depends on (AVR32 || ARCH_AT91) > diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile > index d056fb7186fe..bb8c2994fd98 100644 > --- a/drivers/misc/Makefile > +++ b/drivers/misc/Makefile > @@ -15,6 +15,7 @@ obj-$(CONFIG_BMP085_SPI) += bmp085-spi.o > obj-$(CONFIG_DUMMY_IRQ) += dummy-irq.o > obj-$(CONFIG_ICS932S401) += ics932s401.o > obj-$(CONFIG_LKDTM) += lkdtm.o > +obj-$(CONFIG_MCP4XXX_DPOT) += mcp4xxx_dpot.o > obj-$(CONFIG_TIFM_CORE) += tifm_core.o > obj-$(CONFIG_TIFM_7XX1) += tifm_7xx1.o > obj-$(CONFIG_PHANTOM) += phantom.o > diff --git a/drivers/misc/mcp4xxx_dpot.c b/drivers/misc/mcp4xxx_dpot.c > new file mode 100644 > index 000000000000..abacae99ae42 > --- /dev/null > +++ b/drivers/misc/mcp4xxx_dpot.c > @@ -0,0 +1,269 @@ > +/* > + * mcp4xxx_dpot: Driver for Microchip digital potentiometers > + * Copyright (c) 2015 Axentia Technologies AB > + * Author: Peter Rosin <peda@xxxxxxxxxx> > + * > + * DEVID #Wipers #Positions Resistor Options (kOhm) > + * mcp4531 1 129 5, 10, 50, 100 > + * mcp4532 1 129 5, 10, 50, 100 > + * mcp4551 1 257 5, 10, 50, 100 > + * mcp4552 1 257 5, 10, 50, 100 > + * mcp4631 2 129 5, 10, 50, 100 > + * mcp4632 2 129 5, 10, 50, 100 > + * mcp4651 2 257 5, 10, 50, 100 > + * mcp4652 2 257 5, 10, 50, 100 > + * > + * See Documentation/misc-devices/mcp4xxx_dpot.txt for more info. > + * > + * derived from ad525x.c > + * Copyright (c) 2009-2010 Analog Devices, Inc. > + * Author: Michael Hennerich <hennerich@xxxxxxxxxxxxxxxxxxxx> > + * > + * derived from ad5258.c > + * Copyright (c) 2009 Cyber Switching, Inc. > + * Author: Chris Verges <chrisv@xxxxxxxxxxxxxxxxxx> > + * > + * derived from ad5252.c > + * Copyright (c) 2006-2011 Michael Hennerich <hennerich@xxxxxxxxxxxxxxxxxxxx> > + * > + * Licensed under the GPL-2 or later. > + */ > + > +#include <linux/i2c.h> > +#include <linux/module.h> > +#include <linux/device.h> > +#include <linux/kernel.h> > +#include <linux/slab.h> > + > +#include "mcp4xxx_dpot.h" > + > +/* > + * Client data (each client gets its own) > + */ > + > +struct mcp4xxx_dpot_data { > + struct i2c_client *client; > + struct mutex update_lock; > + unsigned max_pos; > + unsigned long devid; > + unsigned uid; > + unsigned wipers; > +}; > + > +static s32 mcp4xxx_dpot_read(struct mcp4xxx_dpot_data *dpot, u8 reg) > +{ > + s32 value; > + > + value = i2c_smbus_read_word_data(dpot->client, (reg << 4) | 0xc); > + if (value < 0) > + return value; > + return ((value >> 8) & 0xff) | ((value & 0xff) << 8); > +} > + > +static s32 mcp4xxx_dpot_write(struct mcp4xxx_dpot_data *dpot, > + u8 reg, u16 value) > +{ > + return i2c_smbus_write_byte_data(dpot->client, > + (reg << 4) | value >> 8, > + value & 0xff); > +} > + > +/* sysfs functions */ > + > +static ssize_t mcp4xxx_sysfs_show_reg(struct device *dev, > + struct device_attribute *attr, > + char *buf, u32 reg) > +{ > + struct mcp4xxx_dpot_data *data = dev_get_drvdata(dev); > + s32 value; > + > + mutex_lock(&data->update_lock); > + value = mcp4xxx_dpot_read(data, reg); > + mutex_unlock(&data->update_lock); > + > + if (value < 0) > + return -EINVAL; > + > + return sprintf(buf, "%u\n", value); > +} > + > +static ssize_t mcp4xxx_sysfs_set_reg(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count, u32 reg) > +{ > + struct mcp4xxx_dpot_data *data = dev_get_drvdata(dev); > + unsigned long value; > + int err; > + > + err = kstrtoul(buf, 10, &value); > + if (err) > + return err; > + > + if (value >= data->max_pos) > + value = data->max_pos - 1; > + > + mutex_lock(&data->update_lock); > + mcp4xxx_dpot_write(data, reg, value); > + mutex_unlock(&data->update_lock); > + > + return count; > +} > + > +/* ------------------------------------------------------------------------- */ > + > +#define MCP4XXX_DPOT_DEVICE_SHOW(_name, _reg) \ > +static ssize_t show_##_name(struct device *dev, \ > + struct device_attribute *attr, char *buf) \ > +{ \ > + return mcp4xxx_sysfs_show_reg(dev, attr, buf, _reg); \ > +} > + > +#define MCP4XXX_DPOT_DEVICE_SET(_name, _reg) \ > +static ssize_t set_##_name(struct device *dev, \ > + struct device_attribute *attr, \ > + const char *buf, size_t count) \ > +{ \ > + return mcp4xxx_sysfs_set_reg(dev, attr, buf, count, _reg); \ > +} > + > +#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) DEVICE_ATTR_RW()? > + > +#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) DEVICE_ATTR_RW()? > + > +MCP4XXX_DPOT_DEVICE_SHOW_SET(rdac0, DPOT_ADDR_RDAC | DPOT_RDAC0); > +MCP4XXX_DPOT_DEVICE_SHOW_SET(rdac1, DPOT_ADDR_RDAC | DPOT_RDAC1); That's a bunch of macros for just 2 files, you could just spell them out which would make things easier... > + > +static const struct attribute *mcp4xxx_dpot_attrib_wipers[] = { > + &dev_attr_rdac0.attr, > + &dev_attr_rdac1.attr, > + NULL > +}; > + > +/* ------------------------------------------------------------------------- */ > + > +static int mcp4xxx_dpot_add_files(struct device *dev, unsigned rdac) > +{ > + int err = sysfs_create_file(&dev->kobj, > + mcp4xxx_dpot_attrib_wipers[rdac]); > + Oops, you just raced with userspace and it never saw your sysfs file created :( You need to use an attribute group instead. Or better yet, the IIO interface please. > + if (err) > + dev_err(dev, "failed to register sysfs hooks for RDAC%d\n", > + rdac); > + > + return err; > +} > + > +static inline void mcp4xxx_dpot_remove_files(struct device *dev, unsigned rdac) > +{ > + sysfs_remove_file(&dev->kobj, > + mcp4xxx_dpot_attrib_wipers[rdac]); > +} > + > +static int mcp4xxx_dpot_i2c_probe(struct i2c_client *client, > + const struct i2c_device_id *id) > +{ > + struct device *dev = &client->dev; > + unsigned long devid = id->driver_data; > + const char *name = id->name; > + struct mcp4xxx_dpot_data *data; > + int i, err = 0; > + > + if (!i2c_check_functionality(client->adapter, > + I2C_FUNC_SMBUS_WORD_DATA)) { > + dev_err(&client->dev, "SMBUS Word Data not Supported\n"); > + return -EIO; > + } > + > + data = kzalloc(sizeof(struct mcp4xxx_dpot_data), GFP_KERNEL); > + if (!data) { > + err = -ENOMEM; > + goto exit; > + } > + > + dev_set_drvdata(dev, data); > + mutex_init(&data->update_lock); > + > + data->client = client; > + data->devid = devid; > + > + data->max_pos = (1 << DPOT_MAX_POS(devid)) + 1; > + data->uid = DPOT_UID(devid); > + data->wipers = DPOT_WIPERS(devid); > + > + for (i = DPOT_RDAC0; i < MAX_RDACS; i++) { > + if (!(data->wipers & (1 << i))) > + continue; > + if (mcp4xxx_dpot_add_files(dev, i)) > + goto exit_remove_files; > + } > + > + dev_info(dev, "%s %d-Position Digital Potentiometer registered\n", > + name, data->max_pos); Don't be "noisy", if a driver is working properly, nothing should show up in the kernel log. Otherwise it's a total mess. thanks, greg k-h -- 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
