* [PATCH v4 0/4] Support for the TS-5500 platform
@ 2012-01-18 23:52 Vivien Didelot
2012-01-18 23:52 ` [PATCH v4 1/4] x86/platform: (TS-5500) add platform base support Vivien Didelot
` (3 more replies)
0 siblings, 4 replies; 18+ messages in thread
From: Vivien Didelot @ 2012-01-18 23:52 UTC (permalink / raw)
To: x86
Cc: Vivien Didelot, Ingo Molnar, Thomas Gleixner, H. Peter Anvin,
linux-kernel
Support for the Technologic Systems TS-5500 Single Board Computer.
This fourth version of the patches set fixes a small Kconfig dependency issue,
pointed out by Ingo, and turns GPIO, LED, and ADC drivers into modules, as
suggested by HPA. The code is rebased on v3.2.
The first patch adds the base support for the board in
/arch/x86/platform/ts5500 and a documentation file in
Documentation/ABI/testing/sysfs-platform-ts5500.
The second patch adds support for GPIO.
The third patch adds support for the on-board LED.
The fourth patch brings support for the Analogic/Digital converter.
Jerome Oufella (1):
x86/platform: (TS-5500) add GPIO support
Jonas Fonseca (2):
x86/platform: (TS-5500) add LED support
x86/platform: (TS-5500) add ADC support
Vivien Didelot (1):
x86/platform: (TS-5500) add platform base support
Documentation/ABI/testing/sysfs-platform-ts5500 | 46 +++
MAINTAINERS | 5 +
arch/x86/Kconfig | 2 +
arch/x86/platform/Makefile | 1 +
arch/x86/platform/ts5500/Kconfig | 29 ++
arch/x86/platform/ts5500/Makefile | 4 +
arch/x86/platform/ts5500/ts5500.c | 302 ++++++++++++++
arch/x86/platform/ts5500/ts5500.h | 35 ++
arch/x86/platform/ts5500/ts5500_adc.c | 478 ++++++++++++++++++++++
arch/x86/platform/ts5500/ts5500_gpio.c | 479 +++++++++++++++++++++++
arch/x86/platform/ts5500/ts5500_gpio.h | 60 +++
arch/x86/platform/ts5500/ts5500_led.c | 180 +++++++++
12 files changed, 1621 insertions(+), 0 deletions(-)
create mode 100644 Documentation/ABI/testing/sysfs-platform-ts5500
create mode 100644 arch/x86/platform/ts5500/Kconfig
create mode 100644 arch/x86/platform/ts5500/Makefile
create mode 100644 arch/x86/platform/ts5500/ts5500.c
create mode 100644 arch/x86/platform/ts5500/ts5500.h
create mode 100644 arch/x86/platform/ts5500/ts5500_adc.c
create mode 100644 arch/x86/platform/ts5500/ts5500_gpio.c
create mode 100644 arch/x86/platform/ts5500/ts5500_gpio.h
create mode 100644 arch/x86/platform/ts5500/ts5500_led.c
--
1.7.6.5
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v4 1/4] x86/platform: (TS-5500) add platform base support
2012-01-18 23:52 [PATCH v4 0/4] Support for the TS-5500 platform Vivien Didelot
@ 2012-01-18 23:52 ` Vivien Didelot
2012-01-18 23:52 ` [PATCH v4 2/4] x86/platform: (TS-5500) add GPIO support Vivien Didelot
` (2 subsequent siblings)
3 siblings, 0 replies; 18+ messages in thread
From: Vivien Didelot @ 2012-01-18 23:52 UTC (permalink / raw)
To: x86
Cc: Vivien Didelot, Ingo Molnar, Thomas Gleixner, H. Peter Anvin,
linux-kernel
Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
Documentation/ABI/testing/sysfs-platform-ts5500 | 46 ++++
MAINTAINERS | 5 +
arch/x86/Kconfig | 2 +
arch/x86/platform/Makefile | 1 +
arch/x86/platform/ts5500/Kconfig | 8 +
arch/x86/platform/ts5500/Makefile | 1 +
arch/x86/platform/ts5500/ts5500.c | 302 +++++++++++++++++++++++
arch/x86/platform/ts5500/ts5500.h | 35 +++
8 files changed, 400 insertions(+), 0 deletions(-)
create mode 100644 Documentation/ABI/testing/sysfs-platform-ts5500
create mode 100644 arch/x86/platform/ts5500/Kconfig
create mode 100644 arch/x86/platform/ts5500/Makefile
create mode 100644 arch/x86/platform/ts5500/ts5500.c
create mode 100644 arch/x86/platform/ts5500/ts5500.h
diff --git a/Documentation/ABI/testing/sysfs-platform-ts5500 b/Documentation/ABI/testing/sysfs-platform-ts5500
new file mode 100644
index 0000000..d6774c05
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-platform-ts5500
@@ -0,0 +1,46 @@
+What: /sys/devices/platform/ts5500/id
+Date: January 2012
+KernelVersion: 3.2
+Contact: "Savoir-faire Linux, Inc." <kernel-ts@savoirfairelinux.com>
+Description:
+ Product ID of the TS board. TS-5500 ID is 0x60.
+
+What: /sys/devices/platform/ts5500/sram
+Date: January 2012
+KernelVersion: 3.2
+Contact: "Savoir-faire Linux, Inc." <kernel-ts@savoirfairelinux.com>
+Description:
+ Say if the SRAM feature is present. If it is enabled, it will
+ display "1", otherwise "0".
+
+What: /sys/devices/platform/ts5500/ereset
+Date: January 2012
+KernelVersion: 3.2
+Contact: "Savoir-faire Linux, Inc." <kernel-ts@savoirfairelinux.com>
+Description:
+ Say if an external reset is present. If it is present, it will
+ display "1", otherwise "0".
+
+What: /sys/devices/platform/ts5500/jp{1,2,3,4,5,6}
+Date: January 2012
+KernelVersion: 3.2
+Contact: "Savoir-faire Linux, Inc." <kernel-ts@savoirfairelinux.com>
+Description:
+ Show the state of a plugged jumper. If it is present, it will
+ display "1", otherwise "0".
+
+What: /sys/devices/platform/ts5500/adc
+Date: January 2012
+KernelVersion: 3.2
+Contact: "Savoir-faire Linux, Inc." <kernel-ts@savoirfairelinux.com>
+Description:
+ Say if the Analogic to Digital Converter is present. If it is
+ enabled, it will display "1", otherwise "0".
+
+What: /sys/devices/platform/ts5500/rs485
+Date: January 2012
+KernelVersion: 3.2
+Contact: "Savoir-faire Linux, Inc." <kernel-ts@savoirfairelinux.com>
+Description:
+ Say if the RS485 feature is present. If it is enabled, it
+ will display "1", otherwise "0".
diff --git a/MAINTAINERS b/MAINTAINERS
index 62f1cd3..84b9244 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6500,6 +6500,11 @@ W: http://tcp-lp-mod.sourceforge.net/
S: Maintained
F: net/ipv4/tcp_lp.c
+TECHNOLOGIC SYSTEMS TS-5500 MACHINE SUPPORT
+M: Savoir-faire Linux Inc. <ts-kernel@savoirfairelinux.com>
+S: Maintained
+F: arch/x86/platform/ts5500/*
+
TEGRA SUPPORT
M: Colin Cross <ccross@android.com>
M: Olof Johansson <olof@lixom.net>
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index efb4294..5204f62 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -2090,6 +2090,8 @@ config ALIX
Note: You have to set alix.force=1 for boards with Award BIOS.
+source "arch/x86/platform/ts5500/Kconfig"
+
endif # X86_32
config AMD_NB
diff --git a/arch/x86/platform/Makefile b/arch/x86/platform/Makefile
index 8d87439..2d17fd3 100644
--- a/arch/x86/platform/Makefile
+++ b/arch/x86/platform/Makefile
@@ -9,3 +9,4 @@ obj-y += scx200/
obj-y += sfi/
obj-y += visws/
obj-y += uv/
+obj-y += ts5500/
diff --git a/arch/x86/platform/ts5500/Kconfig b/arch/x86/platform/ts5500/Kconfig
new file mode 100644
index 0000000..929b617
--- /dev/null
+++ b/arch/x86/platform/ts5500/Kconfig
@@ -0,0 +1,8 @@
+config TS5500
+ bool "Technologic Systems TS-5500 Single Board Computer support"
+ depends on MELAN
+ select CHECK_SIGNATURE
+ help
+ Add support for the Technologic Systems TS-5500 platform.
+
+ If you have a TS-5500, say Y here.
diff --git a/arch/x86/platform/ts5500/Makefile b/arch/x86/platform/ts5500/Makefile
new file mode 100644
index 0000000..0a689a7
--- /dev/null
+++ b/arch/x86/platform/ts5500/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_TS5500) += ts5500.o
diff --git a/arch/x86/platform/ts5500/ts5500.c b/arch/x86/platform/ts5500/ts5500.c
new file mode 100644
index 0000000..693ce3a
--- /dev/null
+++ b/arch/x86/platform/ts5500/ts5500.c
@@ -0,0 +1,302 @@
+/*
+ * Technologic Systems TS-5500 board - SBC info layer
+ *
+ * Copyright (c) 2010-2012 Savoir-faire Linux Inc.
+ * Alexandre Savard <alexandre.savard@savoirfairelinux.com>
+ * Jonas Fonseca <jonas.fonseca@savoirfairelinux.com>
+ * Vivien Didelot <vivien.didelot@savoirfairelinux.com>
+ *
+ * Portions originate from ts_sbcinfo.c (c) Technologic Systems
+ * Liberty Young <liberty@embeddedx86.com>
+ *
+ * These functions add sysfs platform entries to display information about
+ * the Technologic Systems TS-5500 Single Board Computer (SBC).
+ *
+ * For further information about sysfs entries, see
+ * Documentation/ABI/testing/sysfs-platform-ts5500
+ */
+
+#include <linux/module.h>
+#include <linux/ioport.h>
+#include <linux/platform_device.h>
+#include <linux/kernel.h>
+#include <linux/mutex.h>
+#include <linux/io.h>
+#include <linux/slab.h>
+#include <asm/processor.h>
+#include "ts5500.h"
+
+/* Hardware info for pre-detection */
+#define AMD_ELAN_FAMILY 4
+#define AMD_ELAN_SC520 9
+
+/* Product code register */
+#define TS5500_PRODUCT_CODE_REG 0x74
+#define TS5500_PRODUCT_CODE 0x60 /* TS-5500 product code */
+
+/**
+ * struct ts5500_sbc - TS-5500 SBC main structure
+ * @lock: Read/Write mutex.
+ * @board_id: Board name.
+ * @sram: Check SRAM option.
+ * @rs485: Check RS-485 option.
+ * @adc: Check Analogic/Digital converter option.
+ * @ereset: Check External Reset option.
+ * @itr: Check Industrial Temperature Range option.
+ * @jumpers: States of jumpers 1-7.
+ */
+struct ts5500_sbc {
+ struct mutex lock;
+ int board_id;
+ bool sram;
+ bool rs485;
+ bool adc;
+ bool ereset;
+ bool itr;
+ u8 jumpers;
+};
+
+/* Current platform */
+struct ts5500_sbc *ts5500;
+
+/**
+ * ts5500_pre_detect_hw() - check for TS-5500 specific hardware
+ */
+static __init int ts5500_pre_detect_hw(void)
+{
+ /* Check for AMD ElanSC520 Microcontroller */
+ if (cpu_info.x86_vendor != X86_VENDOR_AMD ||
+ cpu_info.x86 != AMD_ELAN_FAMILY ||
+ cpu_info.x86_model != AMD_ELAN_SC520)
+ return -ENODEV;
+
+ return 0;
+}
+
+/* BIOS signatures */
+static struct {
+ const unsigned char *string;
+ const ssize_t offset;
+} signatures[] __initdata = {
+ {"TS-5x00 AMD Elan", 0xb14}
+};
+
+/**
+ * ts5500_bios_signature() - find board signature in BIOS shadow RAM.
+ */
+static __init int ts5500_bios_signature(void)
+{
+ void __iomem *bios = ioremap(0xF0000, 0x10000);
+ int i, ret = 0;
+
+ for (i = 0; i < ARRAY_SIZE(signatures); i++)
+ if (check_signature(bios + signatures[i].offset,
+ signatures[i].string,
+ strlen(signatures[i].string)))
+ goto found;
+ else
+ pr_notice("Technologic Systems BIOS signature "
+ "'%s' not found at offset %zd\n",
+ signatures[i].string, signatures[i].offset);
+ ret = -ENODEV;
+found:
+ iounmap(bios);
+ return ret;
+}
+
+/**
+ * ts5500_detect_config() - detect the TS board
+ * @sbc: Structure where to store the detected board's details.
+ */
+static int ts5500_detect_config(struct ts5500_sbc *sbc)
+{
+ u8 tmp;
+ int ret = 0;
+
+ if (!request_region(TS5500_PRODUCT_CODE_REG, 4, "ts5500"))
+ return -EBUSY;
+
+ mutex_lock(&ts5500->lock);
+ tmp = inb(TS5500_PRODUCT_CODE_REG);
+ if (tmp != TS5500_PRODUCT_CODE) {
+ pr_err("This platform is not a TS-5500 (found ID 0x%x)\n", tmp);
+ ret = -ENODEV;
+ goto error;
+ }
+ sbc->board_id = tmp;
+
+ tmp = inb(TS5500_SRAM_RS485_ADC_REG);
+ ts5500->sram = !!(tmp & TS5500_SRAM_OPT);
+ ts5500->rs485 = !!(tmp & TS5500_RS485_OPT);
+ ts5500->adc = !!(tmp & TS5500_ADC_OPT);
+
+ tmp = inb(TS5500_ERESET_ITR_REG);
+ ts5500->ereset = !!(tmp & TS5500_ERESET_OPT);
+ ts5500->itr = !!(tmp & TS5500_ITR_OPT);
+
+ tmp = inb(TS5500_LED_JMPRS_REG);
+ sbc->jumpers = tmp & 0xFE; /* All bits except the first (LED) */
+
+error:
+ mutex_unlock(&ts5500->lock);
+ release_region(TS5500_PRODUCT_CODE_REG, 4);
+ return ret;
+}
+
+#define TS5500_IS_JP_SET(sbc, jmp) (!!(sbc->jumpers & TS5500_JP##jmp))
+
+static ssize_t ts5500_show_id(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct ts5500_sbc *sbc = dev_get_drvdata(dev);
+
+ return sprintf(buf, "0x%x\n", sbc->board_id);
+}
+
+static ssize_t ts5500_show_sram(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct ts5500_sbc *sbc = dev_get_drvdata(dev);
+
+ return sprintf(buf, "%d\n", sbc->sram);
+}
+
+static ssize_t ts5500_show_rs485(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct ts5500_sbc *sbc = dev_get_drvdata(dev);
+
+ return sprintf(buf, "%d\n", sbc->rs485);
+}
+
+static ssize_t ts5500_show_adc(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct ts5500_sbc *sbc = dev_get_drvdata(dev);
+
+ return sprintf(buf, "%d\n", sbc->adc);
+}
+
+static ssize_t ts5500_show_ereset(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct ts5500_sbc *sbc = dev_get_drvdata(dev);
+
+ return sprintf(buf, "%d\n", sbc->ereset);
+}
+
+static ssize_t ts5500_show_itr(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct ts5500_sbc *sbc = dev_get_drvdata(dev);
+
+ return sprintf(buf, "%d\n", sbc->itr);
+}
+
+#define TS5500_SHOW_JP(jp) \
+ static ssize_t ts5500_show_jp##jp(struct device *dev, \
+ struct device_attribute *attr,\
+ char *buf) \
+ { \
+ struct ts5500_sbc *sbc = dev_get_drvdata(dev); \
+ return sprintf(buf, "%d\n", TS5500_IS_JP_SET(sbc, jp)); \
+ }
+
+TS5500_SHOW_JP(1)
+TS5500_SHOW_JP(2)
+TS5500_SHOW_JP(3)
+TS5500_SHOW_JP(4)
+TS5500_SHOW_JP(5)
+TS5500_SHOW_JP(6)
+
+static DEVICE_ATTR(id, S_IRUGO, ts5500_show_id, NULL);
+static DEVICE_ATTR(sram, S_IRUGO, ts5500_show_sram, NULL);
+static DEVICE_ATTR(rs485, S_IRUGO, ts5500_show_rs485, NULL);
+static DEVICE_ATTR(adc, S_IRUGO, ts5500_show_adc, NULL);
+static DEVICE_ATTR(ereset, S_IRUGO, ts5500_show_ereset, NULL);
+static DEVICE_ATTR(itr, S_IRUGO, ts5500_show_itr, NULL);
+static DEVICE_ATTR(jp1, S_IRUGO, ts5500_show_jp1, NULL);
+static DEVICE_ATTR(jp2, S_IRUGO, ts5500_show_jp2, NULL);
+static DEVICE_ATTR(jp3, S_IRUGO, ts5500_show_jp3, NULL);
+static DEVICE_ATTR(jp4, S_IRUGO, ts5500_show_jp4, NULL);
+static DEVICE_ATTR(jp5, S_IRUGO, ts5500_show_jp5, NULL);
+static DEVICE_ATTR(jp6, S_IRUGO, ts5500_show_jp6, NULL);
+
+static struct attribute *ts5500_attributes[] = {
+ &dev_attr_id.attr,
+ &dev_attr_sram.attr,
+ &dev_attr_rs485.attr,
+ &dev_attr_adc.attr,
+ &dev_attr_ereset.attr,
+ &dev_attr_itr.attr,
+ &dev_attr_jp1.attr,
+ &dev_attr_jp2.attr,
+ &dev_attr_jp3.attr,
+ &dev_attr_jp4.attr,
+ &dev_attr_jp5.attr,
+ &dev_attr_jp6.attr,
+ NULL
+};
+
+static const struct attribute_group ts5500_attr_group = {
+ .attrs = ts5500_attributes
+};
+
+static int __init ts5500_init(void)
+{
+ int ret;
+ struct platform_device *pdev;
+
+ /*
+ * There is no DMI available, or PCI bridge subvendor info,
+ * only the BIOS provides a 16-bit identification call.
+ * It is safer to check for a TS-5500 specific hardware
+ * such as the processor, then find a signature in the BIOS.
+ */
+ ret = ts5500_pre_detect_hw();
+ if (ret)
+ return ret;
+
+ ret = ts5500_bios_signature();
+ if (ret)
+ return ret;
+
+ ts5500 = kzalloc(sizeof(struct ts5500_sbc), GFP_KERNEL);
+ if (!ts5500)
+ return -ENOMEM;
+ mutex_init(&ts5500->lock);
+
+ ret = ts5500_detect_config(ts5500);
+ if (ret)
+ goto release_mem;
+
+ pdev = platform_device_register_simple("ts5500", -1, NULL, 0);
+ if (IS_ERR(pdev)) {
+ ret = PTR_ERR(pdev);
+ goto release_mem;
+ }
+ platform_set_drvdata(pdev, ts5500);
+
+ ret = sysfs_create_group(&pdev->dev.kobj,
+ &ts5500_attr_group);
+ if (ret)
+ goto release_pdev;
+
+ return 0;
+
+release_pdev:
+ platform_device_unregister(pdev);
+release_mem:
+ kfree(ts5500);
+
+ return ret;
+}
+postcore_initcall(ts5500_init);
+
+MODULE_AUTHOR("Jonas Fonseca <jonas.fonseca@savoirfairelinux.com>");
+MODULE_AUTHOR("Alexandre Savard <alexandre.savard@savoirfairelinux.com>");
+MODULE_AUTHOR("Vivien Didelot <vivien.didelot@savoirfairelinux.com>");
+MODULE_DESCRIPTION("Technologic Systems TS-5500 Board's platform driver");
+MODULE_LICENSE("GPL");
diff --git a/arch/x86/platform/ts5500/ts5500.h b/arch/x86/platform/ts5500/ts5500.h
new file mode 100644
index 0000000..f4f1bc4
--- /dev/null
+++ b/arch/x86/platform/ts5500/ts5500.h
@@ -0,0 +1,35 @@
+/*
+ * Technologic Systems TS-5500 board
+ *
+ * Copyright (c) 2010-2012 Savoir-faire Linux Inc.
+ * Vivien Didelot <vivien.didelot@savoirfairelinux.com>
+ */
+
+#ifndef _TS5500_H
+#define _TS5500_H
+
+/* SRAM/RS-485/ADC options, and RS-485 RTS/Automatic RS-485 flags register */
+#define TS5500_SRAM_RS485_ADC_REG 0x75
+#define TS5500_SRAM_OPT 0x01 /* SRAM option */
+#define TS5500_RS485_OPT 0x02 /* RS-485 option */
+#define TS5500_ADC_OPT 0x04 /* A/D converter option */
+#define TS5500_RS485_RTS_MASK 0x40 /* RTS for RS-485 */
+#define TS5500_RS485_AUTO_MASK 0x80 /* Automatic RS-485 */
+
+/* External Reset/Industrial Temperature Range options register */
+#define TS5500_ERESET_ITR_REG 0x76
+#define TS5500_ERESET_OPT 0x01 /* External Reset option */
+#define TS5500_ITR_OPT 0x02 /* Indust. Temp. Range option */
+
+/* LED/Jumpers register */
+#define TS5500_LED_JMPRS_REG 0x77
+#define TS5500_LED_MASK 0x01 /* LED flag */
+#define TS5500_JP1 0x02 /* Automatic CMOS */
+#define TS5500_JP2 0x04 /* Enable Serial Console */
+#define TS5500_JP3 0x08 /* Write Enable Drive A */
+#define TS5500_JP4 0x10 /* Fast Console (115K baud) */
+#define TS5500_JP5 0x20 /* User Jumper */
+#define TS5500_JP6 0x40 /* Console on COM1 (req. JP2) */
+#define TS5500_JP7 0x80 /* Undocumented (Unused) */
+
+#endif
--
1.7.6.5
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v4 2/4] x86/platform: (TS-5500) add GPIO support
2012-01-18 23:52 [PATCH v4 0/4] Support for the TS-5500 platform Vivien Didelot
2012-01-18 23:52 ` [PATCH v4 1/4] x86/platform: (TS-5500) add platform base support Vivien Didelot
@ 2012-01-18 23:52 ` Vivien Didelot
2012-01-18 23:52 ` [PATCH v4 3/4] x86/platform: (TS-5500) add LED support Vivien Didelot
2012-01-18 23:52 ` [PATCH v4 4/4] x86/platform: (TS-5500) add ADC support Vivien Didelot
3 siblings, 0 replies; 18+ messages in thread
From: Vivien Didelot @ 2012-01-18 23:52 UTC (permalink / raw)
To: x86
Cc: Jerome Oufella, Ingo Molnar, Thomas Gleixner, H. Peter Anvin,
linux-kernel, Vivien Didelot
From: Jerome Oufella <jerome.oufella@savoirfairelinux.com>
Signed-off-by: Jerome Oufella <jerome.oufella@savoirfairelinux.com>
Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
arch/x86/platform/ts5500/Kconfig | 7 +
arch/x86/platform/ts5500/Makefile | 1 +
arch/x86/platform/ts5500/ts5500_gpio.c | 479 ++++++++++++++++++++++++++++++++
arch/x86/platform/ts5500/ts5500_gpio.h | 60 ++++
4 files changed, 547 insertions(+), 0 deletions(-)
create mode 100644 arch/x86/platform/ts5500/ts5500_gpio.c
create mode 100644 arch/x86/platform/ts5500/ts5500_gpio.h
diff --git a/arch/x86/platform/ts5500/Kconfig b/arch/x86/platform/ts5500/Kconfig
index 929b617..62095c9 100644
--- a/arch/x86/platform/ts5500/Kconfig
+++ b/arch/x86/platform/ts5500/Kconfig
@@ -6,3 +6,10 @@ config TS5500
Add support for the Technologic Systems TS-5500 platform.
If you have a TS-5500, say Y here.
+
+config TS5500_GPIO
+ tristate "TS-5500 GPIO support"
+ depends on TS5500 && GENERIC_GPIO
+ default y
+ help
+ This enables support for the DIO headers for GPIO usage.
diff --git a/arch/x86/platform/ts5500/Makefile b/arch/x86/platform/ts5500/Makefile
index 0a689a7..71c1398 100644
--- a/arch/x86/platform/ts5500/Makefile
+++ b/arch/x86/platform/ts5500/Makefile
@@ -1 +1,2 @@
obj-$(CONFIG_TS5500) += ts5500.o
+obj-$(CONFIG_TS5500_GPIO) += ts5500_gpio.o
diff --git a/arch/x86/platform/ts5500/ts5500_gpio.c b/arch/x86/platform/ts5500/ts5500_gpio.c
new file mode 100644
index 0000000..46dadc4
--- /dev/null
+++ b/arch/x86/platform/ts5500/ts5500_gpio.c
@@ -0,0 +1,479 @@
+/*
+ * GPIO (DIO) driver for Technologic Systems TS-5500
+ *
+ * Copyright (c) 2010-2012 Savoir-faire Linux Inc.
+ * Jerome Oufella <jerome.oufella@savoirfairelinux.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * TS-5500 board has 38 GPIOs referred to as DIOs in the product's literature.
+ */
+
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/gpio.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/io.h>
+#include "ts5500_gpio.h"
+
+static void port_bit_set(u8 addr, int bit)
+{
+ u8 var;
+ var = inb(addr);
+ var |= (1 << bit);
+ outb(var, addr);
+}
+
+static void port_bit_clear(u8 addr, int bit)
+{
+ u8 var;
+ var = inb(addr);
+ var &= ~(1 << bit);
+ outb(var, addr);
+}
+
+/* "DIO" line to IO port mapping table for line's value */
+static const unsigned long line_to_port_map[] = {
+ 0x7B, 0x7B, 0x7B, 0x7B, 0x7B, 0x7B, 0x7B, 0x7B, /* DIO1_0~7 */
+ 0x7C, 0x7C, 0x7C, 0x7C, 0x7C, 0x7C, /* DIO1_8~13 */
+ 0x7E, 0x7E, 0x7E, 0x7E, 0x7E, 0x7E, 0x7E, 0x7E, /* DIO2_0~7 */
+ 0x7F, 0x7F, 0x7F, 0x7F, 0x7F, 0x7F, /* DIO2_8~13 */
+ 0x72, 0x72, 0x72, 0x72, 0x72, 0x72, 0x72, 0x72, /* LCD_0~7 */
+ 0x73, 0x73, 0x73 /* LCD_EN, LCD_RS, LCD_WR */
+};
+
+/* "DIO" line to IO port's bit map for line's value */
+static const int line_to_bit_map[] = {
+ 0, 1, 2, 3, 4, 5, 6, 7, /* DIO1_0~7 */
+ 0, 1, 2, 3, 4, 5, /* DIO1_8~13 */
+ 0, 1, 2, 3, 4, 5, 6, 7, /* DIO2_0~7 */
+ 0, 1, 2, 3, 4, 5, /* DIO2_8~13 */
+ 0, 1, 2, 3, 4, 5, 6, 7, /* LCD_0~7 */
+ 0, 7, 6 /* LCD_EN, LCD_RS, LCD_WR */
+};
+
+/* "DIO" line's direction control mapping table */
+static const unsigned long line_to_dir_map[] = {
+ 0x7A, 0x7A, 0x7A, 0x7A, 0x7A, 0x7A, 0x7A, 0x7A, /* DIO1_0~7 */
+ 0x7A, 0x7A, 0x7A, 0x7A, 0, 0, /* DIO1_8~13 */
+ 0x7D, 0x7D, 0x7D, 0x7D, 0x7D, 0x7D, 0x7D, 0x7D, /* DIO2_0~7 */
+ 0x7D, 0x7D, 0x7D, 0x7D, 0, 0, /* DIO2_8~13 */
+ 0x7D, 0x7D, 0x7D, 0x7D, 0x7D, 0x7D, 0x7D, 0x7D, /* LCD_0~7 */
+ 0, 0, 0 /* LCD_EN, LCD_RS, LCD_WR */
+};
+
+/* "DIO" line's direction control bit-mapping table */
+static const int line_to_dir_bit_map[] = {
+ 0, 0, 0, 0, 1, 1, 1, 1, /* DIO1_0~7 */
+ 5, 5, 5, 5, -1, -1, /* DIO1_8~13 */
+ 0, 0, 0, 0, 1, 1, 1, 1, /* DIO2_0~7 */
+ 5, 5, 5, 5, -1, -1, /* DIO2_8~13 */
+ 2, 2, 2, 2, 3, 3, 3, 3, /* LCD_0~7 */
+ -1, -1, -1 /* LCD_EN, LCD_RS, LCD_WR */
+};
+
+/* This array is used to track requests for our GPIO lines */
+static int requested_gpios[TS5500_LCD_WR + 1];
+
+static int dio1_irq = 1;
+module_param(dio1_irq, int, 0644);
+MODULE_PARM_DESC(dio1_irq,
+ "Enable usage of IRQ7 for any DIO1 line (default 1).");
+
+static int dio2_irq = 0;
+module_param(dio2_irq, int, 0644);
+MODULE_PARM_DESC(dio2_irq,
+ "Enable usage of IRQ6 for any DIO2 line (default 0).");
+
+static int lcd_irq = 0;
+module_param(lcd_irq, int, 0644);
+MODULE_PARM_DESC(lcd_irq, "Enable usage of IRQ1 for any LCD line (default 0).");
+
+static int use_lcdio = 0;
+module_param(use_lcdio, int, 0644);
+MODULE_PARM_DESC(use_lcdio, "Enable usage of the LCD header for DIO operation"
+ " (default 0).");
+
+/**
+ * struct ts5500_drvdata - Driver data
+ * @master: Device.
+ * @gpio_chip: GPIO chip.
+ * @gpio_lock: Read/Write Mutex.
+ */
+struct ts5500_drvdata {
+ struct device *master;
+ struct gpio_chip gpio_chip;
+ struct mutex gpio_lock;
+};
+
+static int ts5500_gpio_request(struct gpio_chip *chip, unsigned offset)
+{
+ struct ts5500_drvdata *drvdata;
+
+ drvdata = container_of(chip, struct ts5500_drvdata, gpio_chip);
+
+ mutex_lock(&drvdata->gpio_lock);
+ if (requested_gpios[offset]) {
+ mutex_unlock(&drvdata->gpio_lock);
+ return -EBUSY;
+ }
+ requested_gpios[offset] = 1;
+ mutex_unlock(&drvdata->gpio_lock);
+
+ return 0;
+}
+
+static void ts5500_gpio_free(struct gpio_chip *chip, unsigned offset)
+{
+ struct ts5500_drvdata *drvdata;
+
+ drvdata = container_of(chip, struct ts5500_drvdata, gpio_chip);
+
+ mutex_lock(&drvdata->gpio_lock);
+ requested_gpios[offset] = 0;
+ mutex_unlock(&drvdata->gpio_lock);
+}
+
+static int ts5500_gpio_get(struct gpio_chip *chip, unsigned offset)
+{
+ unsigned long ioaddr;
+ u8 byte;
+ int bitno;
+ struct ts5500_drvdata *drvdata;
+
+ drvdata = container_of(chip, struct ts5500_drvdata, gpio_chip);
+
+ /* Some lines are output-only and cannot be read */
+ if (offset == TS5500_LCD_EN || offset > chip->ngpio)
+ return -ENXIO;
+
+ ioaddr = line_to_port_map[offset];
+ bitno = line_to_bit_map[offset];
+ byte = inb(ioaddr);
+
+ return (byte >> bitno) & 0x1;
+}
+
+static void ts5500_gpio_set(struct gpio_chip *chip, unsigned offset, int val)
+{
+ int bitno;
+ unsigned long ioaddr;
+ struct ts5500_drvdata *drvdata;
+
+ drvdata = container_of(chip, struct ts5500_drvdata, gpio_chip);
+
+ /* Some lines just can't be set */
+ switch (offset) {
+ case TS5500_DIO1_12:
+ case TS5500_DIO1_13:
+ case TS5500_DIO2_13:
+ case TS5500_LCD_RS:
+ case TS5500_LCD_WR:
+ return;
+ default:
+ if (offset > chip->ngpio)
+ return;
+ break;
+ }
+
+ /* Get io port and bit for 'offset' */
+ ioaddr = line_to_port_map[offset];
+ bitno = line_to_bit_map[offset];
+
+ mutex_lock(&drvdata->gpio_lock);
+ if (val == 0)
+ port_bit_clear(ioaddr, bitno);
+ else
+ port_bit_set(ioaddr, bitno);
+ mutex_unlock(&drvdata->gpio_lock);
+}
+
+static int ts5500_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
+{
+ /* Only a few lines are IRQ-Capable */
+ switch (offset) {
+ case TS5500_DIO1_13:
+ return TS5500_DIO1_13_IRQ;
+ case TS5500_DIO2_13:
+ return TS5500_DIO2_13_IRQ;
+ case TS5500_LCD_RS:
+ return TS5500_LCD_RS_IRQ;
+ default:
+ break;
+ }
+
+ /*
+ * Handle the case where the user bridged the IRQ line with another
+ * DIO line from the same header.
+ */
+ if (dio1_irq && offset >= TS5500_DIO1_0 && offset < TS5500_DIO1_13)
+ return TS5500_DIO1_13_IRQ;
+
+ if (dio2_irq && offset >= TS5500_DIO2_0 && offset < TS5500_DIO2_13)
+ return TS5500_DIO2_13_IRQ;
+
+ if (lcd_irq && offset >= TS5500_LCD_0 && offset <= TS5500_LCD_WR)
+ return TS5500_LCD_RS_IRQ;
+
+ return -ENXIO;
+}
+
+static int ts5500_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
+{
+ unsigned long dir_reg;
+ int dir_bit;
+ struct ts5500_drvdata *drvdata;
+
+ drvdata = container_of(chip, struct ts5500_drvdata, gpio_chip);
+
+ /* Some lines cannot be set as inputs */
+ switch (offset) {
+ case TS5500_LCD_EN:
+ return -ENXIO;
+ default:
+ if (offset > chip->ngpio)
+ return -ENXIO;
+ break;
+ }
+
+ dir_reg = line_to_dir_map[offset];
+ dir_bit = line_to_dir_bit_map[offset];
+
+ mutex_lock(&drvdata->gpio_lock);
+ port_bit_clear(dir_reg, dir_bit);
+ mutex_unlock(&drvdata->gpio_lock);
+
+ return 0;
+}
+
+static int ts5500_gpio_direction_output(struct gpio_chip *chip, unsigned offset,
+ int val)
+{
+ unsigned long dir_reg, ioaddr;
+ int dir_bit, bitno;
+ struct ts5500_drvdata *drvdata;
+
+ drvdata = container_of(chip, struct ts5500_drvdata, gpio_chip);
+
+ /* Some lines cannot be set as outputs */
+ switch (offset) {
+ case TS5500_DIO1_12:
+ case TS5500_DIO1_13:
+ case TS5500_DIO2_13:
+ case TS5500_LCD_RS:
+ case TS5500_LCD_WR:
+ return -ENXIO;
+ default:
+ if (offset > chip->ngpio)
+ return -ENXIO;
+ break;
+ }
+
+ /* Get direction and value registers infos */
+ dir_reg = line_to_dir_map[offset];
+ dir_bit = line_to_dir_bit_map[offset];
+ ioaddr = line_to_port_map[offset];
+ bitno = line_to_bit_map[offset];
+
+ mutex_lock(&drvdata->gpio_lock);
+ if (val == 0)
+ port_bit_clear(ioaddr, bitno); /* Set initial line value */
+ else
+ port_bit_set(ioaddr, bitno);
+ port_bit_set(dir_reg, dir_bit); /* Set output direction for line */
+
+ /*
+ * Confirm initial line output value
+ * (might have been changed by input)
+ */
+ if (val == 0)
+ port_bit_clear(ioaddr, bitno);
+ else
+ port_bit_set(ioaddr, bitno);
+ mutex_unlock(&drvdata->gpio_lock);
+
+ return 0;
+}
+
+static int __devinit ts5500_gpio_probe(struct platform_device *pdev)
+{
+ struct ts5500_drvdata *drvdata;
+ struct gpio_chip *chip;
+ int ret;
+
+ if (pdev == NULL) {
+ dev_err(&pdev->dev, "Platform device not available!\n");
+ return -ENODEV;
+ }
+
+ /* Request DIO1 */
+ if (!request_region(0x7A, 3, "ts5500-gpio-DIO1")) {
+ dev_err(&pdev->dev, "Cannot request I/O port 0x7A-7C\n");
+ goto err_req_dio1;
+ }
+
+ /* Request DIO2 */
+ if (!request_region(0x7D, 3, "ts5500-gpio-DIO2")) {
+ dev_err(&pdev->dev, "Cannot request I/O port 0x7D-7F\n");
+ goto err_req_dio2;
+ }
+
+ /* Request LCDIO if wanted */
+ if (use_lcdio && !request_region(0x72, 2, "ts5500-gpio-LCD")) {
+ dev_err(&pdev->dev, "Cannot request I/O port 0x72-73\n");
+ goto err_req_lcdio;
+ }
+
+ /* Setup the gpio_chip structure */
+ drvdata = kzalloc(sizeof(struct ts5500_drvdata), GFP_KERNEL);
+ if (drvdata == NULL)
+ goto err_alloc_dev;
+
+ memset(requested_gpios, 0, sizeof(requested_gpios));
+ mutex_init(&drvdata->gpio_lock);
+
+ drvdata->master = pdev->dev.parent;
+ chip = &drvdata->gpio_chip;
+ chip->request = ts5500_gpio_request;
+ chip->free = ts5500_gpio_free;
+ chip->to_irq = ts5500_gpio_to_irq;
+ chip->direction_input = ts5500_gpio_direction_input;
+ chip->direction_output = ts5500_gpio_direction_output;
+ chip->get = ts5500_gpio_get;
+ chip->set = ts5500_gpio_set;
+ chip->can_sleep = 0;
+ chip->base = TS5500_DIO1_0;
+ chip->label = pdev->name;
+ chip->ngpio = (use_lcdio ? TS5500_LCD_WR + 1 : TS5500_DIO2_13 + 1);
+
+ /* Enable IRQ generation */
+ mutex_lock(&drvdata->gpio_lock);
+ port_bit_set(0x7A, 7); /* DIO1_13 on IRQ7 */
+ port_bit_set(0x7D, 7); /* DIO2_13 on IRQ6 */
+ if (use_lcdio) {
+ port_bit_clear(0x7D, 4); /* Enable LCD header usage as DIO */
+ port_bit_set(0x7D, 6); /* LCD_RS on IRQ1 */
+ }
+ mutex_unlock(&drvdata->gpio_lock);
+
+ /* Register chip */
+ ret = gpiochip_add(&drvdata->gpio_chip);
+ if (ret)
+ goto err_gpiochip_add;
+
+ platform_set_drvdata(pdev, drvdata);
+
+ return 0;
+
+err_gpiochip_add:
+ dev_err(&pdev->dev, "Failed to register the gpio chip.\n");
+ kfree(drvdata);
+
+err_alloc_dev:
+ if (use_lcdio)
+ release_region(0x72, 2); /* Release LCD's region */
+
+err_req_lcdio:
+ release_region(0x7D, 3); /* Release DIO2's region */
+
+err_req_dio2:
+ release_region(0x7A, 3); /* Release DIO1's region */
+
+err_req_dio1:
+ ret = -EBUSY;
+
+ return ret;
+}
+
+/* Callback for releasing resources */
+static void ts5500_gpio_device_release(struct device *dev)
+{
+ /* noop */
+}
+
+static struct platform_device ts5500_gpio_device = {
+ .name = "ts5500_gpio",
+ .id = -1,
+ .dev = {
+ .release = ts5500_gpio_device_release,
+ }
+};
+
+static int __devexit ts5500_gpio_remove(struct platform_device *pdev)
+{
+ struct ts5500_drvdata *drvdata;
+ int ret, i;
+
+ drvdata = platform_get_drvdata(pdev);
+
+ /* Release GPIO lines */
+ for (i = 0; i < ARRAY_SIZE(requested_gpios); i++) {
+ if (requested_gpios[i])
+ gpio_free(i);
+ }
+
+ mutex_lock(&drvdata->gpio_lock);
+ /* Disable IRQs generation */
+ port_bit_clear(0x7A, 7);
+ port_bit_clear(0x7D, 7);
+ if (use_lcdio)
+ port_bit_clear(0x7D, 6);
+
+ /* Release IO regions */
+ release_region(0x7A, 3);
+ release_region(0x7D, 3);
+ if (use_lcdio)
+ release_region(0x72, 2);
+ mutex_unlock(&drvdata->gpio_lock);
+
+ ret = gpiochip_remove(&drvdata->gpio_chip);
+ if (ret)
+ dev_err(&pdev->dev, "Failed to remove the gpio chip\n");
+
+ kfree(drvdata);
+ return ret;
+}
+
+static struct platform_driver ts5500_gpio_driver = {
+ .driver = {
+ .name = "ts5500_gpio",
+ .owner = THIS_MODULE,
+ },
+ .probe = ts5500_gpio_probe,
+ .remove = __devexit_p(ts5500_gpio_remove)
+};
+
+static int __init ts5500_gpio_init(void)
+{
+ int ret;
+
+ ret = platform_driver_register(&ts5500_gpio_driver);
+ if (ret)
+ goto error_out;
+
+ ret = platform_device_register(&ts5500_gpio_device);
+ if (ret)
+ goto error_device_register;
+
+ return 0;
+
+error_device_register:
+ platform_driver_unregister(&ts5500_gpio_driver);
+error_out:
+ return ret;
+}
+module_init(ts5500_gpio_init);
+
+static void __exit ts5500_gpio_exit(void)
+{
+ platform_driver_unregister(&ts5500_gpio_driver);
+ platform_device_unregister(&ts5500_gpio_device);
+}
+module_exit(ts5500_gpio_exit);
+
+MODULE_AUTHOR("Jerome Oufella <jerome.oufella@savoirfairelinux.com>");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Technologic Systems TS-5500, GPIO/DIO driver");
diff --git a/arch/x86/platform/ts5500/ts5500_gpio.h b/arch/x86/platform/ts5500/ts5500_gpio.h
new file mode 100644
index 0000000..eecd4fc
--- /dev/null
+++ b/arch/x86/platform/ts5500/ts5500_gpio.h
@@ -0,0 +1,60 @@
+/*
+ * GPIO (DIO) driver for Technologic Systems TS-5500
+ *
+ * Copyright (c) 2010-2012 Savoir-faire Linux Inc.
+ * Jerome Oufella <jerome.oufella@savoirfairelinux.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef _TS5500_GPIO_H
+#define _TS5500_GPIO_H
+
+#define TS5500_DIO1_0 0
+#define TS5500_DIO1_1 1
+#define TS5500_DIO1_2 2
+#define TS5500_DIO1_3 3
+#define TS5500_DIO1_4 4
+#define TS5500_DIO1_5 5
+#define TS5500_DIO1_6 6
+#define TS5500_DIO1_7 7
+#define TS5500_DIO1_8 8
+#define TS5500_DIO1_9 9
+#define TS5500_DIO1_10 10
+#define TS5500_DIO1_11 11
+#define TS5500_DIO1_12 12
+#define TS5500_DIO1_13 13
+#define TS5500_DIO2_0 14
+#define TS5500_DIO2_1 15
+#define TS5500_DIO2_2 16
+#define TS5500_DIO2_3 17
+#define TS5500_DIO2_4 18
+#define TS5500_DIO2_5 19
+#define TS5500_DIO2_6 20
+#define TS5500_DIO2_7 21
+#define TS5500_DIO2_8 22
+#define TS5500_DIO2_9 23
+#define TS5500_DIO2_10 24
+#define TS5500_DIO2_11 25
+/* #define TS5500_DIO2_12 - Keep commented out as it simply doesn't exist. */
+#define TS5500_DIO2_13 26
+#define TS5500_LCD_0 27
+#define TS5500_LCD_1 28
+#define TS5500_LCD_2 29
+#define TS5500_LCD_3 30
+#define TS5500_LCD_4 31
+#define TS5500_LCD_5 32
+#define TS5500_LCD_6 33
+#define TS5500_LCD_7 34
+#define TS5500_LCD_EN 35
+#define TS5500_LCD_RS 36
+#define TS5500_LCD_WR 37
+
+/* Lines that can trigger IRQs */
+#define TS5500_DIO1_13_IRQ 7
+#define TS5500_DIO2_13_IRQ 6
+#define TS5500_LCD_RS_IRQ 1
+
+#endif /* _TS5500_GPIO_H */
--
1.7.6.5
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v4 3/4] x86/platform: (TS-5500) add LED support
2012-01-18 23:52 [PATCH v4 0/4] Support for the TS-5500 platform Vivien Didelot
2012-01-18 23:52 ` [PATCH v4 1/4] x86/platform: (TS-5500) add platform base support Vivien Didelot
2012-01-18 23:52 ` [PATCH v4 2/4] x86/platform: (TS-5500) add GPIO support Vivien Didelot
@ 2012-01-18 23:52 ` Vivien Didelot
2012-01-18 23:52 ` [PATCH v4 4/4] x86/platform: (TS-5500) add ADC support Vivien Didelot
3 siblings, 0 replies; 18+ messages in thread
From: Vivien Didelot @ 2012-01-18 23:52 UTC (permalink / raw)
To: x86
Cc: Jonas Fonseca, Ingo Molnar, Thomas Gleixner, H. Peter Anvin,
linux-kernel, Vivien Didelot
From: Jonas Fonseca <jonas.fonseca@savoirfairelinux.com>
Signed-off-by: Jonas Fonseca <jonas.fonseca@savoirfairelinux.com>
Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
arch/x86/platform/ts5500/Kconfig | 7 ++
arch/x86/platform/ts5500/Makefile | 1 +
arch/x86/platform/ts5500/ts5500_led.c | 180 +++++++++++++++++++++++++++++++++
3 files changed, 188 insertions(+), 0 deletions(-)
create mode 100644 arch/x86/platform/ts5500/ts5500_led.c
diff --git a/arch/x86/platform/ts5500/Kconfig b/arch/x86/platform/ts5500/Kconfig
index 62095c9..76f777f 100644
--- a/arch/x86/platform/ts5500/Kconfig
+++ b/arch/x86/platform/ts5500/Kconfig
@@ -13,3 +13,10 @@ config TS5500_GPIO
default y
help
This enables support for the DIO headers for GPIO usage.
+
+config TS5500_LED
+ tristate "TS-5500 LED Support"
+ depends on TS5500 && LEDS_CLASS
+ default y
+ help
+ This option enables support for the on-chip LED.
diff --git a/arch/x86/platform/ts5500/Makefile b/arch/x86/platform/ts5500/Makefile
index 71c1398..88eccc9 100644
--- a/arch/x86/platform/ts5500/Makefile
+++ b/arch/x86/platform/ts5500/Makefile
@@ -1,2 +1,3 @@
obj-$(CONFIG_TS5500) += ts5500.o
obj-$(CONFIG_TS5500_GPIO) += ts5500_gpio.o
+obj-$(CONFIG_TS5500_LED) += ts5500_led.o
diff --git a/arch/x86/platform/ts5500/ts5500_led.c b/arch/x86/platform/ts5500/ts5500_led.c
new file mode 100644
index 0000000..1c78ec2
--- /dev/null
+++ b/arch/x86/platform/ts5500/ts5500_led.c
@@ -0,0 +1,180 @@
+/*
+ * Technologic Systems TS-5500 boards - LED driver
+ *
+ * Copyright (c) 2010-2012 Savoir-faire Linux Inc.
+ * Jonas Fonseca <jonas.fonseca@savoirfairelinux.com>
+ *
+ * Portions Copyright (c) 2008 Compulab, Ltd.
+ * Mike Rapoport <mike@compulab.co.il>
+ *
+ * Portions Copyright (c) 2006-2008 Marvell International Ltd.
+ * Eric Miao <eric.miao@marvell.com>
+ *
+ * Based on drivers/leds/leds-da903x.c from linux-2.6.32.8.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/io.h>
+#include <linux/leds.h>
+#include "ts5500.h"
+
+/**
+ * struct ts5500_led - LED structure
+ * @cdev: LED class device structure.
+ * @ioaddr: LED I/O address.
+ */
+struct ts5500_led {
+ struct led_classdev cdev;
+ int ioaddr;
+ int bit;
+};
+
+static void ts5500_led_set(struct led_classdev *led_cdev,
+ enum led_brightness value)
+{
+ struct ts5500_led *led = container_of(led_cdev, struct ts5500_led,
+ cdev);
+ outb(!!value, led->ioaddr);
+}
+
+static struct led_info ts5500_led_info = {
+ .name = "ts5500_led",
+ .default_trigger = "ts5500_led",
+ .flags = TS5500_LED_MASK
+};
+
+static struct led_platform_data ts5500_led_platform_data = {
+ .num_leds = 1,
+ .leds = &ts5500_led_info
+};
+
+static struct resource ts5500_led_resources[] = {
+ {
+ .name = "ts5500_led",
+ .start = TS5500_LED_JMPRS_REG,
+ .end = TS5500_LED_JMPRS_REG,
+ .flags = IORESOURCE_IO
+ }
+};
+
+static void ts5500_led_release(struct device *dev)
+{
+ /* noop */
+}
+
+static struct platform_device ts5500_led_device = {
+ .name = "ts5500_led",
+ .resource = ts5500_led_resources,
+ .num_resources = ARRAY_SIZE(ts5500_led_resources),
+ .id = -1,
+ .dev = {
+ .platform_data = &ts5500_led_platform_data,
+ .release = ts5500_led_release
+ }
+};
+
+static int __devinit ts5500_led_probe(struct platform_device *pdev)
+{
+ struct led_platform_data *pdata = pdev->dev.platform_data;
+ struct ts5500_led *led;
+ struct resource *res;
+ int ret;
+
+ if (pdata == NULL || !pdata->num_leds) {
+ dev_err(&pdev->dev, "No platform data available\n");
+ return -ENODEV;
+ }
+
+ res = platform_get_resource(pdev, IORESOURCE_IO, 0);
+ if (!res) {
+ dev_err(&pdev->dev, "Failed to get I/O resource\n");
+ return -EBUSY;
+ }
+
+ led = kzalloc(sizeof(struct ts5500_led), GFP_KERNEL);
+ if (led == NULL) {
+ dev_err(&pdev->dev, "Failed to alloc memory for LED device\n");
+ return -ENOMEM;
+ }
+
+ led->cdev.name = pdata->leds[0].name;
+ led->cdev.default_trigger = pdata->leds[0].default_trigger;
+ led->cdev.brightness_set = ts5500_led_set;
+ led->cdev.brightness = LED_OFF;
+
+ led->ioaddr = res->start;
+ led->bit = pdata->leds[0].flags;
+
+ ret = led_classdev_register(pdev->dev.parent, &led->cdev);
+ if (ret) {
+ dev_err(&pdev->dev, "Failed to register LED\n");
+ goto err;
+ }
+
+ platform_set_drvdata(pdev, led);
+ return 0;
+
+err:
+ kfree(led);
+ return ret;
+}
+
+static int __devexit ts5500_led_remove(struct platform_device *pdev)
+{
+ struct ts5500_led *led = platform_get_drvdata(pdev);
+
+ platform_set_drvdata(pdev, NULL);
+ led_classdev_unregister(&led->cdev);
+ kfree(led);
+
+ return 0;
+}
+
+static struct platform_driver ts5500_led_driver = {
+ .driver = {
+ .name = "ts5500_led",
+ .owner = THIS_MODULE
+ },
+ .probe = ts5500_led_probe,
+ .remove = __devexit_p(ts5500_led_remove)
+};
+
+static int __init ts5500_led_init(void)
+{
+ int ret;
+
+ ret = platform_driver_register(&ts5500_led_driver);
+ if (ret)
+ goto error_out;
+
+ ret = platform_device_register(&ts5500_led_device);
+ if (ret)
+ goto error_device_register;
+
+ return 0;
+
+error_device_register:
+ platform_driver_unregister(&ts5500_led_driver);
+error_out:
+ return ret;
+}
+module_init(ts5500_led_init);
+
+static void __exit ts5500_led_exit(void)
+{
+ platform_driver_unregister(&ts5500_led_driver);
+ platform_device_unregister(&ts5500_led_device);
+}
+module_exit(ts5500_led_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Jonas Fonseca <jonas.fonseca@savoirfairelinux.com>");
+MODULE_DESCRIPTION("LED driver for Technologic Systems TS-5500");
--
1.7.6.5
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v4 4/4] x86/platform: (TS-5500) add ADC support
2012-01-18 23:52 [PATCH v4 0/4] Support for the TS-5500 platform Vivien Didelot
` (2 preceding siblings ...)
2012-01-18 23:52 ` [PATCH v4 3/4] x86/platform: (TS-5500) add LED support Vivien Didelot
@ 2012-01-18 23:52 ` Vivien Didelot
2012-01-19 2:55 ` Guenter Roeck
2012-01-20 23:43 ` [PATCH] x86/platform: (TS-5500) revised ADC driver Vivien Didelot
3 siblings, 2 replies; 18+ messages in thread
From: Vivien Didelot @ 2012-01-18 23:52 UTC (permalink / raw)
To: x86
Cc: Jonas Fonseca, Ingo Molnar, Thomas Gleixner, H. Peter Anvin,
linux-kernel, Vivien Didelot
From: Jonas Fonseca <jonas.fonseca@savoirfairelinux.com>
Signed-off-by: Jonas Fonseca <jonas.fonseca@savoirfairelinux.com>
Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
arch/x86/platform/ts5500/Kconfig | 7 +
arch/x86/platform/ts5500/Makefile | 1 +
arch/x86/platform/ts5500/ts5500_adc.c | 478 +++++++++++++++++++++++++++++++++
3 files changed, 486 insertions(+), 0 deletions(-)
create mode 100644 arch/x86/platform/ts5500/ts5500_adc.c
diff --git a/arch/x86/platform/ts5500/Kconfig b/arch/x86/platform/ts5500/Kconfig
index 76f777f..f1a5f1d 100644
--- a/arch/x86/platform/ts5500/Kconfig
+++ b/arch/x86/platform/ts5500/Kconfig
@@ -20,3 +20,10 @@ config TS5500_LED
default y
help
This option enables support for the on-chip LED.
+
+config TS5500_ADC
+ tristate "TS-5500 ADC"
+ depends on TS5500 && HWMON=y
+ default y
+ help
+ Support for the A/D converter on Technologic Systems TS-5500 SBCs.
diff --git a/arch/x86/platform/ts5500/Makefile b/arch/x86/platform/ts5500/Makefile
index 88eccc9..dcf46d8 100644
--- a/arch/x86/platform/ts5500/Makefile
+++ b/arch/x86/platform/ts5500/Makefile
@@ -1,3 +1,4 @@
obj-$(CONFIG_TS5500) += ts5500.o
obj-$(CONFIG_TS5500_GPIO) += ts5500_gpio.o
obj-$(CONFIG_TS5500_LED) += ts5500_led.o
+obj-$(CONFIG_TS5500_ADC) += ts5500_adc.o
diff --git a/arch/x86/platform/ts5500/ts5500_adc.c b/arch/x86/platform/ts5500/ts5500_adc.c
new file mode 100644
index 0000000..fc4560f
--- /dev/null
+++ b/arch/x86/platform/ts5500/ts5500_adc.c
@@ -0,0 +1,478 @@
+/*
+ * Technologic Systems TS-5500 boards - Mapped MAX197 ADC driver
+ *
+ * Copyright (c) 2010-2012 Savoir-faire Linux Inc.
+ * Jonas Fonseca <jonas.fonseca@savoirfairelinux.com>
+ *
+ * Portions Copyright (C) 2008 Marc Pignat <marc.pignat@hevs.ch>
+ *
+ * The driver uses direct access for communication with the ADC.
+ * Should work unchanged with the MAX199 chip.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ *
+ * The TS-5500 uses a CPLD to abstract the interface to a MAX197.
+ */
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/device.h>
+#include <linux/delay.h>
+#include <linux/platform_device.h>
+#include <linux/err.h>
+#include <linux/sysfs.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
+#include <linux/io.h>
+#include "ts5500.h"
+
+#define TS5500_ADC_CTRL_REG 0x195 /* Conversion state register */
+#define TS5500_ADC_INIT_LSB_REG 0x196 /* Init conv. / LSB register */
+#define TS5500_ADC_MSB_REG 0x197 /* MSB register */
+/*
+ * Control bits of A/D command
+ * bits 0-2: selected channel (0 - 7)
+ * bits 3: uni/bipolar (0 = unipolar (ie 0 to +5V))
+ * (1 = bipolar (ie -5 to +5V))
+ * bit 4: selected range (0 = 5v range, 1 = 10V range)
+ * bit 5-7: padded zero (unused)
+ */
+
+#define TS5500_ADC_CHANNELS_MAX 8 /* 0 to 7 channels on device */
+
+#define TS5500_ADC_BIPOLAR 0x08
+#define TS5500_ADC_UNIPOLAR 0x00
+#define TS5500_ADC_RANGE_5V 0x00 /* 0 to 5V range */
+#define TS5500_ADC_RANGE_10V 0x10 /* 0 to 10V range */
+
+#define TS5500_ADC_READ_DELAY 15 /* usec */
+#define TS5500_ADC_READ_BUSY_MASK 0x01
+#define TS5500_ADC_NAME "MAX197 (8 channels)"
+
+/**
+ * struct ts5500_adc_platform_data
+ * @name: Name of the device.
+ * @ioaddr: I/O address containing:
+ * .data: Data register for conversion reading.
+ * .ctrl: A/D Control Register (bit 0 = 0 when
+ * conversion completed).
+ * @read: Information about conversion reading, with:
+ * .delay: Delay before next conversion.
+ * .busy_mask: Control register bit 0 equals 1 means
+ * conversion is not completed yet.
+ * @ctrl: Data tables addressable by [polarity][range].
+ * @ranges: Ranges.
+ * .min: Min value.
+ * .max: Max value.
+ * @scale: Polarity/Range coefficients to scale raw conversion reading.
+ */
+struct ts5500_adc_platform_data {
+ const char *name;
+ struct {
+ int data;
+ int ctrl;
+ } ioaddr;
+ struct {
+ u8 delay;
+ u8 busy_mask;
+ } read;
+ u8 ctrl[2][2];
+ struct {
+ int min[2][2];
+ int max[2][2];
+ } ranges;
+ int scale[2][2];
+};
+
+#define ts5500_adc_test_bit(bit, map) (test_bit(bit, map) != 0)
+
+/**
+ * struct ts5500_adc_chip
+ * @hwmon_dev: The hwmon device.
+ * @lock: Read/Write mutex.
+ * @spec: The mapped MAX197 platform data.
+ * @polarity: bitmap for polarity.
+ * @range: bitmap for range.
+ */
+struct ts5500_adc_chip {
+ struct device *hwmon_dev;
+ struct mutex lock;
+ struct ts5500_adc_platform_data spec;
+ DECLARE_BITMAP(polarity, TS5500_ADC_CHANNELS_MAX);
+ DECLARE_BITMAP(range, TS5500_ADC_CHANNELS_MAX);
+};
+
+static s32 ts5500_adc_scale(struct ts5500_adc_chip *chip, s16 raw,
+ int polarity, int range)
+{
+ s32 scaled = raw;
+
+ scaled *= chip->spec.scale[polarity][range];
+ scaled /= 10000;
+
+ return scaled;
+}
+
+static int ts5500_adc_range(struct ts5500_adc_chip *chip, int is_min,
+ int polarity, int range)
+{
+ if (is_min)
+ return chip->spec.ranges.min[polarity][range];
+ return chip->spec.ranges.max[polarity][range];
+}
+
+static int ts5500_adc_strtol(const char *buf, long *value, int range1,
+ int range2)
+{
+ if (strict_strtol(buf, 10, value))
+ return -EINVAL;
+
+ if (range1 < range2)
+ *value = SENSORS_LIMIT(*value, range1, range2);
+ else
+ *value = SENSORS_LIMIT(*value, range2, range1);
+
+ return 0;
+}
+
+static struct ts5500_adc_chip *ts5500_adc_get_drvdata(struct device *dev)
+{
+ return platform_get_drvdata(to_platform_device(dev));
+}
+
+/**
+ * ts5500_adc_show_range() - Display range on user output
+ *
+ * Function called on read access on
+ * /sys/devices/platform/ts5500-adc/in{0,1,2,3,4,5,6,7}_{min,max}
+ */
+static ssize_t ts5500_adc_show_range(struct device *dev,
+ struct device_attribute *devattr, char *buf)
+{
+ struct ts5500_adc_chip *chip = ts5500_adc_get_drvdata(dev);
+ struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
+ int is_min = attr->nr != 0;
+ int polarity, range;
+
+ if (mutex_lock_interruptible(&chip->lock))
+ return -ERESTARTSYS;
+
+ polarity = ts5500_adc_test_bit(attr->index, chip->polarity);
+ range = ts5500_adc_test_bit(attr->index, chip->range);
+
+ mutex_unlock(&chip->lock);
+
+ return sprintf(buf, "%d\n",
+ ts5500_adc_range(chip, is_min, polarity, range));
+}
+
+/**
+ * ts5500_adc_store_range() - Write range from user input
+ *
+ * Function called on write access on
+ * /sys/devices/platform/ts5500-adc/in{0,1,2,3,4,5,6,7}_{min,max}
+ */
+static ssize_t ts5500_adc_store_range(struct device *dev,
+ struct device_attribute *devattr,
+ const char *buf, size_t count)
+{
+ struct ts5500_adc_chip *chip = ts5500_adc_get_drvdata(dev);
+ struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
+ int is_min = attr->nr != 0;
+ int range1 = ts5500_adc_range(chip, is_min, 0, 0);
+ int range2 = ts5500_adc_range(chip, is_min, 1, 1);
+ long value;
+
+ if (ts5500_adc_strtol(buf, &value, range1, range2))
+ return -EINVAL;
+
+ if (mutex_lock_interruptible(&chip->lock))
+ return -ERESTARTSYS;
+
+ if (abs(value) > 5000)
+ set_bit(attr->index, chip->range);
+ else
+ clear_bit(attr->index, chip->range);
+
+ if (is_min) {
+ if (value < 0)
+ set_bit(attr->index, chip->polarity);
+ else
+ clear_bit(attr->index, chip->polarity);
+ }
+
+ mutex_unlock(&chip->lock);
+
+ return count;
+}
+
+/**
+ * ts5500_adc_show_input() - Show channel input
+ *
+ * Function called on read access on
+ * /sys/devices/platform/ts5500-adc/in{0,1,2,3,4,5,6,7}_input
+ */
+static ssize_t ts5500_adc_show_input(struct device *dev,
+ struct device_attribute *devattr,
+ char *buf)
+{
+ struct ts5500_adc_chip *chip = ts5500_adc_get_drvdata(dev);
+ struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+ int polarity, range;
+ int ret;
+ u8 command;
+
+ if (mutex_lock_interruptible(&chip->lock))
+ return -ERESTARTSYS;
+
+ polarity = ts5500_adc_test_bit(attr->index, chip->polarity);
+ range = ts5500_adc_test_bit(attr->index, chip->range);
+
+ command = attr->index | chip->spec.ctrl[polarity][range];
+
+ outb(command, chip->spec.ioaddr.data);
+
+ udelay(chip->spec.read.delay);
+ ret = inb(chip->spec.ioaddr.ctrl);
+
+ if (ret & chip->spec.read.busy_mask) {
+ dev_err(dev, "device not ready (ret=0x0%x, try=%d)\n", ret,
+ range);
+ ret = -EIO;
+ } else {
+ /* LSB of conversion is at 0x196 and MSB is at 0x197 */
+ u8 lsb = inb(chip->spec.ioaddr.data);
+ u8 msb = inb(chip->spec.ioaddr.data + 1);
+ s16 raw = (msb << 8) | lsb;
+ s32 scaled = ts5500_adc_scale(chip, raw, polarity, range);
+
+ ret = sprintf(buf, "%d\n", scaled);
+ }
+
+ mutex_unlock(&chip->lock);
+ return ret;
+}
+
+static ssize_t ts5500_adc_show_name(struct device *dev,
+ struct device_attribute *devattr, char *buf)
+{
+ return sprintf(buf, "%s\n", ts5500_adc_get_drvdata(dev)->spec.name);
+}
+
+#define TS5500_ADC_HWMON_CHANNEL(chan) \
+ SENSOR_DEVICE_ATTR(in##chan##_input, S_IRUGO, \
+ ts5500_adc_show_input, NULL, chan); \
+ SENSOR_DEVICE_ATTR_2(in##chan##_max, S_IRUGO | S_IWUSR, \
+ ts5500_adc_show_range, \
+ ts5500_adc_store_range, 0, chan); \
+ SENSOR_DEVICE_ATTR_2(in##chan##_min, S_IRUGO | S_IWUSR, \
+ ts5500_adc_show_range, \
+ ts5500_adc_store_range, 1, chan) \
+
+#define TS5500_ADC_SYSFS_CHANNEL(chan) \
+ &sensor_dev_attr_in##chan##_input.dev_attr.attr, \
+ &sensor_dev_attr_in##chan##_max.dev_attr.attr, \
+ &sensor_dev_attr_in##chan##_min.dev_attr.attr
+
+static DEVICE_ATTR(name, S_IRUGO, ts5500_adc_show_name, NULL);
+
+static TS5500_ADC_HWMON_CHANNEL(0);
+static TS5500_ADC_HWMON_CHANNEL(1);
+static TS5500_ADC_HWMON_CHANNEL(2);
+static TS5500_ADC_HWMON_CHANNEL(3);
+static TS5500_ADC_HWMON_CHANNEL(4);
+static TS5500_ADC_HWMON_CHANNEL(5);
+static TS5500_ADC_HWMON_CHANNEL(6);
+static TS5500_ADC_HWMON_CHANNEL(7);
+
+static const struct attribute_group ts5500_adc_sysfs_group = {
+ .attrs = (struct attribute *[]) {
+ &dev_attr_name.attr,
+ TS5500_ADC_SYSFS_CHANNEL(0),
+ TS5500_ADC_SYSFS_CHANNEL(1),
+ TS5500_ADC_SYSFS_CHANNEL(2),
+ TS5500_ADC_SYSFS_CHANNEL(3),
+ TS5500_ADC_SYSFS_CHANNEL(4),
+ TS5500_ADC_SYSFS_CHANNEL(5),
+ TS5500_ADC_SYSFS_CHANNEL(6),
+ TS5500_ADC_SYSFS_CHANNEL(7),
+ NULL
+ }
+};
+
+static int __devinit ts5500_adc_probe(struct platform_device *pdev)
+{
+ struct ts5500_adc_platform_data *pdata = pdev->dev.platform_data;
+ struct ts5500_adc_chip *chip;
+ int ret;
+
+ if (pdata == NULL)
+ return -ENODEV;
+
+ chip = kzalloc(sizeof *chip, GFP_KERNEL);
+ if (!chip)
+ return -ENOMEM;
+
+ chip->spec = *pdata;
+
+ mutex_init(&chip->lock);
+ mutex_lock(&chip->lock);
+
+ ret = sysfs_create_group(&pdev->dev.kobj, &ts5500_adc_sysfs_group);
+ if (ret) {
+ dev_err(&pdev->dev, "sysfs_create_group failed.\n");
+ goto error_unlock_and_free;
+ }
+
+ chip->hwmon_dev = hwmon_device_register(&pdev->dev);
+ if (IS_ERR(chip->hwmon_dev)) {
+ dev_err(&pdev->dev, "hwmon_device_register failed.\n");
+ ret = PTR_ERR(chip->hwmon_dev);
+ goto error_unregister_device;
+ }
+
+ platform_set_drvdata(pdev, chip);
+ mutex_unlock(&chip->lock);
+ return 0;
+
+error_unregister_device:
+ sysfs_remove_group(&pdev->dev.kobj, &ts5500_adc_sysfs_group);
+
+error_unlock_and_free:
+ mutex_unlock(&chip->lock);
+ kfree(chip);
+ return ret;
+}
+
+static void ts5500_adc_release(struct device *dev)
+{
+ /* noop */
+}
+
+static struct resource ts5500_adc_resources[] = {
+ {
+ .name = "ts5500_adc" "-data",
+ .start = TS5500_ADC_INIT_LSB_REG,
+ .end = TS5500_ADC_MSB_REG,
+ .flags = IORESOURCE_IO,
+ },
+ {
+ .name = "ts5500_adc" "-ctrl",
+ .start = TS5500_ADC_CTRL_REG,
+ .end = TS5500_ADC_CTRL_REG,
+ .flags = IORESOURCE_IO,
+ }
+};
+
+static struct ts5500_adc_platform_data ts5500_adc_platform_data = {
+ .name = TS5500_ADC_NAME,
+ .ioaddr = {
+ .data = TS5500_ADC_INIT_LSB_REG,
+ .ctrl = TS5500_ADC_CTRL_REG,
+ },
+ .read = {
+ .delay = TS5500_ADC_READ_DELAY,
+ .busy_mask = TS5500_ADC_READ_BUSY_MASK,
+ },
+ .ctrl = {
+ { TS5500_ADC_UNIPOLAR | TS5500_ADC_RANGE_5V,
+ TS5500_ADC_UNIPOLAR | TS5500_ADC_RANGE_10V },
+ { TS5500_ADC_BIPOLAR | TS5500_ADC_RANGE_5V,
+ TS5500_ADC_BIPOLAR | TS5500_ADC_RANGE_10V },
+ },
+ .ranges = {
+ .min = {
+ { 0, 0 },
+ { -5000, -10000 },
+ },
+ .max = {
+ { 5000, 10000 },
+ { 5000, 10000 },
+ },
+ },
+ .scale = {
+ { 12207, 24414 },
+ { 24414, 48828 },
+ },
+};
+
+static struct platform_device ts5500_adc_device = {
+ .name = "ts5500_adc",
+ .id = -1,
+ .resource = ts5500_adc_resources,
+ .num_resources = ARRAY_SIZE(ts5500_adc_resources),
+ .dev = {
+ .platform_data = &ts5500_adc_platform_data,
+ .release = ts5500_adc_release,
+ },
+};
+
+static int __devexit ts5500_adc_remove(struct platform_device *pdev)
+{
+ struct ts5500_adc_chip *chip = platform_get_drvdata(pdev);
+
+ mutex_lock(&chip->lock);
+ hwmon_device_unregister(chip->hwmon_dev);
+ sysfs_remove_group(&pdev->dev.kobj, &ts5500_adc_sysfs_group);
+ platform_set_drvdata(pdev, NULL);
+ mutex_unlock(&chip->lock);
+ kfree(chip);
+
+ return 0;
+}
+
+static struct platform_driver ts5500_adc_driver = {
+ .driver = {
+ .name = "ts5500_adc",
+ .owner = THIS_MODULE,
+ },
+ .probe = ts5500_adc_probe,
+ .remove = __devexit_p(ts5500_adc_remove)
+};
+
+static int __init ts5500_adc_init(void)
+{
+ int ret;
+
+ ret = platform_driver_register(&ts5500_adc_driver);
+ if (ret)
+ goto error_out;
+
+ ret = platform_device_register(&ts5500_adc_device);
+ if (ret)
+ goto error_device_register;
+
+ return 0;
+
+error_device_register:
+ platform_driver_unregister(&ts5500_adc_driver);
+error_out:
+ return ret;
+}
+module_init(ts5500_adc_init);
+
+static void __exit ts5500_adc_exit(void)
+{
+ platform_driver_unregister(&ts5500_adc_driver);
+ platform_device_unregister(&ts5500_adc_device);
+}
+module_exit(ts5500_adc_exit);
+
+MODULE_DESCRIPTION("TS-5500 mapped MAX197 ADC device driver");
+MODULE_AUTHOR("Jonas Fonseca <jonas.fonseca@savoirfairelinux.com>");
+MODULE_LICENSE("GPL");
--
1.7.6.5
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v4 4/4] x86/platform: (TS-5500) add ADC support
2012-01-18 23:52 ` [PATCH v4 4/4] x86/platform: (TS-5500) add ADC support Vivien Didelot
@ 2012-01-19 2:55 ` Guenter Roeck
2012-01-19 2:57 ` Guenter Roeck
2012-01-20 23:41 ` Vivien Didelot
2012-01-20 23:43 ` [PATCH] x86/platform: (TS-5500) revised ADC driver Vivien Didelot
1 sibling, 2 replies; 18+ messages in thread
From: Guenter Roeck @ 2012-01-19 2:55 UTC (permalink / raw)
To: Vivien Didelot
Cc: x86, Jonas Fonseca, Ingo Molnar, Thomas Gleixner, H. Peter Anvin,
linux-kernel
On Wed, Jan 18, 2012 at 06:52:11PM -0500, Vivien Didelot wrote:
> From: Jonas Fonseca <jonas.fonseca@savoirfairelinux.com>
>
> Signed-off-by: Jonas Fonseca <jonas.fonseca@savoirfairelinux.com>
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> ---
> arch/x86/platform/ts5500/Kconfig | 7 +
> arch/x86/platform/ts5500/Makefile | 1 +
> arch/x86/platform/ts5500/ts5500_adc.c | 478 +++++++++++++++++++++++++++++++++
> 3 files changed, 486 insertions(+), 0 deletions(-)
> create mode 100644 arch/x86/platform/ts5500/ts5500_adc.c
>
I can not comment about the other drivers, but this is a hwmon driver, and thus should
reside in drivers/hwmon and be reviewed and handled on the hwmon mailing list (copied).
Couple of additional comments below; just some things I noticed, not a complete review.
> diff --git a/arch/x86/platform/ts5500/Kconfig b/arch/x86/platform/ts5500/Kconfig
> index 76f777f..f1a5f1d 100644
> --- a/arch/x86/platform/ts5500/Kconfig
> +++ b/arch/x86/platform/ts5500/Kconfig
> @@ -20,3 +20,10 @@ config TS5500_LED
> default y
> help
> This option enables support for the on-chip LED.
> +
> +config TS5500_ADC
> + tristate "TS-5500 ADC"
> + depends on TS5500 && HWMON=y
> + default y
> + help
> + Support for the A/D converter on Technologic Systems TS-5500 SBCs.
> diff --git a/arch/x86/platform/ts5500/Makefile b/arch/x86/platform/ts5500/Makefile
> index 88eccc9..dcf46d8 100644
> --- a/arch/x86/platform/ts5500/Makefile
> +++ b/arch/x86/platform/ts5500/Makefile
> @@ -1,3 +1,4 @@
> obj-$(CONFIG_TS5500) += ts5500.o
> obj-$(CONFIG_TS5500_GPIO) += ts5500_gpio.o
> obj-$(CONFIG_TS5500_LED) += ts5500_led.o
> +obj-$(CONFIG_TS5500_ADC) += ts5500_adc.o
> diff --git a/arch/x86/platform/ts5500/ts5500_adc.c b/arch/x86/platform/ts5500/ts5500_adc.c
> new file mode 100644
> index 0000000..fc4560f
> --- /dev/null
> +++ b/arch/x86/platform/ts5500/ts5500_adc.c
> @@ -0,0 +1,478 @@
> +/*
> + * Technologic Systems TS-5500 boards - Mapped MAX197 ADC driver
> + *
> + * Copyright (c) 2010-2012 Savoir-faire Linux Inc.
> + * Jonas Fonseca <jonas.fonseca@savoirfairelinux.com>
> + *
> + * Portions Copyright (C) 2008 Marc Pignat <marc.pignat@hevs.ch>
> + *
> + * The driver uses direct access for communication with the ADC.
> + * Should work unchanged with the MAX199 chip.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> + *
> + * The TS-5500 uses a CPLD to abstract the interface to a MAX197.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/device.h>
> +#include <linux/delay.h>
> +#include <linux/platform_device.h>
> +#include <linux/err.h>
> +#include <linux/sysfs.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/mutex.h>
> +#include <linux/slab.h>
> +#include <linux/io.h>
> +#include "ts5500.h"
> +
> +#define TS5500_ADC_CTRL_REG 0x195 /* Conversion state register */
> +#define TS5500_ADC_INIT_LSB_REG 0x196 /* Init conv. / LSB register */
> +#define TS5500_ADC_MSB_REG 0x197 /* MSB register */
> +/*
> + * Control bits of A/D command
> + * bits 0-2: selected channel (0 - 7)
> + * bits 3: uni/bipolar (0 = unipolar (ie 0 to +5V))
> + * (1 = bipolar (ie -5 to +5V))
> + * bit 4: selected range (0 = 5v range, 1 = 10V range)
> + * bit 5-7: padded zero (unused)
> + */
> +
> +#define TS5500_ADC_CHANNELS_MAX 8 /* 0 to 7 channels on device */
> +
> +#define TS5500_ADC_BIPOLAR 0x08
> +#define TS5500_ADC_UNIPOLAR 0x00
> +#define TS5500_ADC_RANGE_5V 0x00 /* 0 to 5V range */
> +#define TS5500_ADC_RANGE_10V 0x10 /* 0 to 10V range */
> +
> +#define TS5500_ADC_READ_DELAY 15 /* usec */
> +#define TS5500_ADC_READ_BUSY_MASK 0x01
> +#define TS5500_ADC_NAME "MAX197 (8 channels)"
> +
> +/**
> + * struct ts5500_adc_platform_data
> + * @name: Name of the device.
> + * @ioaddr: I/O address containing:
> + * .data: Data register for conversion reading.
> + * .ctrl: A/D Control Register (bit 0 = 0 when
> + * conversion completed).
> + * @read: Information about conversion reading, with:
> + * .delay: Delay before next conversion.
> + * .busy_mask: Control register bit 0 equals 1 means
> + * conversion is not completed yet.
> + * @ctrl: Data tables addressable by [polarity][range].
> + * @ranges: Ranges.
> + * .min: Min value.
> + * .max: Max value.
> + * @scale: Polarity/Range coefficients to scale raw conversion reading.
> + */
> +struct ts5500_adc_platform_data {
> + const char *name;
> + struct {
> + int data;
> + int ctrl;
> + } ioaddr;
> + struct {
> + u8 delay;
> + u8 busy_mask;
> + } read;
> + u8 ctrl[2][2];
const ?
> + struct {
> + int min[2][2];
> + int max[2][2];
Should those be const ?
> + } ranges;
> + int scale[2][2];
const ?
> +};
> +
I am a bit lost about this structure and its use. It appears as if you expect that
there will be other uses besides the one defined here (otherwise the variables would
not make much sense and you could use defines), yet it is all defined in this file
and thus static. Please elaborate.
> +#define ts5500_adc_test_bit(bit, map) (test_bit(bit, map) != 0)
> +
Why "!= 0" ? Isn't that implied ? And then why the define to start with ?
> +/**
> + * struct ts5500_adc_chip
> + * @hwmon_dev: The hwmon device.
> + * @lock: Read/Write mutex.
> + * @spec: The mapped MAX197 platform data.
> + * @polarity: bitmap for polarity.
> + * @range: bitmap for range.
> + */
> +struct ts5500_adc_chip {
> + struct device *hwmon_dev;
> + struct mutex lock;
> + struct ts5500_adc_platform_data spec;
> + DECLARE_BITMAP(polarity, TS5500_ADC_CHANNELS_MAX);
> + DECLARE_BITMAP(range, TS5500_ADC_CHANNELS_MAX);
> +};
> +
> +static s32 ts5500_adc_scale(struct ts5500_adc_chip *chip, s16 raw,
> + int polarity, int range)
> +{
> + s32 scaled = raw;
> +
> + scaled *= chip->spec.scale[polarity][range];
> + scaled /= 10000;
> +
> + return scaled;
> +}
> +
> +static int ts5500_adc_range(struct ts5500_adc_chip *chip, int is_min,
> + int polarity, int range)
> +{
> + if (is_min)
> + return chip->spec.ranges.min[polarity][range];
> + return chip->spec.ranges.max[polarity][range];
> +}
> +
> +static int ts5500_adc_strtol(const char *buf, long *value, int range1,
> + int range2)
> +{
> + if (strict_strtol(buf, 10, value))
checkpatch warning.
> + return -EINVAL;
> +
> + if (range1 < range2)
> + *value = SENSORS_LIMIT(*value, range1, range2);
> + else
> + *value = SENSORS_LIMIT(*value, range2, range1);
> +
> + return 0;
> +}
This function is called exactly once. Why not just embed it in the calling code ?
> +
> +static struct ts5500_adc_chip *ts5500_adc_get_drvdata(struct device *dev)
> +{
> + return platform_get_drvdata(to_platform_device(dev));
> +}
> +
> +/**
> + * ts5500_adc_show_range() - Display range on user output
> + *
> + * Function called on read access on
> + * /sys/devices/platform/ts5500-adc/in{0,1,2,3,4,5,6,7}_{min,max}
> + */
> +static ssize_t ts5500_adc_show_range(struct device *dev,
> + struct device_attribute *devattr, char *buf)
> +{
> + struct ts5500_adc_chip *chip = ts5500_adc_get_drvdata(dev);
> + struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
> + int is_min = attr->nr != 0;
> + int polarity, range;
> +
> + if (mutex_lock_interruptible(&chip->lock))
> + return -ERESTARTSYS;
> +
> + polarity = ts5500_adc_test_bit(attr->index, chip->polarity);
> + range = ts5500_adc_test_bit(attr->index, chip->range);
> +
> + mutex_unlock(&chip->lock);
> +
> + return sprintf(buf, "%d\n",
> + ts5500_adc_range(chip, is_min, polarity, range));
> +}
> +
> +/**
> + * ts5500_adc_store_range() - Write range from user input
> + *
> + * Function called on write access on
> + * /sys/devices/platform/ts5500-adc/in{0,1,2,3,4,5,6,7}_{min,max}
> + */
> +static ssize_t ts5500_adc_store_range(struct device *dev,
> + struct device_attribute *devattr,
> + const char *buf, size_t count)
> +{
> + struct ts5500_adc_chip *chip = ts5500_adc_get_drvdata(dev);
> + struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
> + int is_min = attr->nr != 0;
> + int range1 = ts5500_adc_range(chip, is_min, 0, 0);
> + int range2 = ts5500_adc_range(chip, is_min, 1, 1);
> + long value;
> +
> + if (ts5500_adc_strtol(buf, &value, range1, range2))
> + return -EINVAL;
> +
> + if (mutex_lock_interruptible(&chip->lock))
> + return -ERESTARTSYS;
> +
> + if (abs(value) > 5000)
> + set_bit(attr->index, chip->range);
> + else
> + clear_bit(attr->index, chip->range);
> +
> + if (is_min) {
> + if (value < 0)
> + set_bit(attr->index, chip->polarity);
> + else
> + clear_bit(attr->index, chip->polarity);
> + }
> +
> + mutex_unlock(&chip->lock);
> +
> + return count;
> +}
> +
> +/**
> + * ts5500_adc_show_input() - Show channel input
> + *
> + * Function called on read access on
> + * /sys/devices/platform/ts5500-adc/in{0,1,2,3,4,5,6,7}_input
> + */
> +static ssize_t ts5500_adc_show_input(struct device *dev,
> + struct device_attribute *devattr,
> + char *buf)
> +{
> + struct ts5500_adc_chip *chip = ts5500_adc_get_drvdata(dev);
> + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> + int polarity, range;
> + int ret;
> + u8 command;
> +
> + if (mutex_lock_interruptible(&chip->lock))
> + return -ERESTARTSYS;
> +
> + polarity = ts5500_adc_test_bit(attr->index, chip->polarity);
> + range = ts5500_adc_test_bit(attr->index, chip->range);
> +
> + command = attr->index | chip->spec.ctrl[polarity][range];
> +
> + outb(command, chip->spec.ioaddr.data);
> +
> + udelay(chip->spec.read.delay);
> + ret = inb(chip->spec.ioaddr.ctrl);
> +
> + if (ret & chip->spec.read.busy_mask) {
> + dev_err(dev, "device not ready (ret=0x0%x, try=%d)\n", ret,
> + range);
What does "try=" have to do with the displayed "range" ?
> + ret = -EIO;
> + } else {
> + /* LSB of conversion is at 0x196 and MSB is at 0x197 */
> + u8 lsb = inb(chip->spec.ioaddr.data);
> + u8 msb = inb(chip->spec.ioaddr.data + 1);
> + s16 raw = (msb << 8) | lsb;
> + s32 scaled = ts5500_adc_scale(chip, raw, polarity, range);
> +
> + ret = sprintf(buf, "%d\n", scaled);
> + }
> +
> + mutex_unlock(&chip->lock);
> + return ret;
> +}
> +
> +static ssize_t ts5500_adc_show_name(struct device *dev,
> + struct device_attribute *devattr, char *buf)
> +{
> + return sprintf(buf, "%s\n", ts5500_adc_get_drvdata(dev)->spec.name);
> +}
> +
> +#define TS5500_ADC_HWMON_CHANNEL(chan) \
> + SENSOR_DEVICE_ATTR(in##chan##_input, S_IRUGO, \
> + ts5500_adc_show_input, NULL, chan); \
> + SENSOR_DEVICE_ATTR_2(in##chan##_max, S_IRUGO | S_IWUSR, \
> + ts5500_adc_show_range, \
> + ts5500_adc_store_range, 0, chan); \
> + SENSOR_DEVICE_ATTR_2(in##chan##_min, S_IRUGO | S_IWUSR, \
> + ts5500_adc_show_range, \
> + ts5500_adc_store_range, 1, chan) \
> +
> +#define TS5500_ADC_SYSFS_CHANNEL(chan) \
> + &sensor_dev_attr_in##chan##_input.dev_attr.attr, \
> + &sensor_dev_attr_in##chan##_max.dev_attr.attr, \
> + &sensor_dev_attr_in##chan##_min.dev_attr.attr
> +
> +static DEVICE_ATTR(name, S_IRUGO, ts5500_adc_show_name, NULL);
> +
> +static TS5500_ADC_HWMON_CHANNEL(0);
> +static TS5500_ADC_HWMON_CHANNEL(1);
> +static TS5500_ADC_HWMON_CHANNEL(2);
> +static TS5500_ADC_HWMON_CHANNEL(3);
> +static TS5500_ADC_HWMON_CHANNEL(4);
> +static TS5500_ADC_HWMON_CHANNEL(5);
> +static TS5500_ADC_HWMON_CHANNEL(6);
> +static TS5500_ADC_HWMON_CHANNEL(7);
> +
Looks good at first glance, but unless I misunderstand something, each define generates
three structures, yet only the first of those is declared static, the others are global.
> +static const struct attribute_group ts5500_adc_sysfs_group = {
> + .attrs = (struct attribute *[]) {
checkpatch error.
> + &dev_attr_name.attr,
> + TS5500_ADC_SYSFS_CHANNEL(0),
> + TS5500_ADC_SYSFS_CHANNEL(1),
> + TS5500_ADC_SYSFS_CHANNEL(2),
> + TS5500_ADC_SYSFS_CHANNEL(3),
> + TS5500_ADC_SYSFS_CHANNEL(4),
> + TS5500_ADC_SYSFS_CHANNEL(5),
> + TS5500_ADC_SYSFS_CHANNEL(6),
> + TS5500_ADC_SYSFS_CHANNEL(7),
> + NULL
> + }
> +};
> +
> +static int __devinit ts5500_adc_probe(struct platform_device *pdev)
> +{
> + struct ts5500_adc_platform_data *pdata = pdev->dev.platform_data;
> + struct ts5500_adc_chip *chip;
> + int ret;
> +
> + if (pdata == NULL)
> + return -ENODEV;
> +
> + chip = kzalloc(sizeof *chip, GFP_KERNEL);
> + if (!chip)
> + return -ENOMEM;
> +
> + chip->spec = *pdata;
> +
> + mutex_init(&chip->lock);
> + mutex_lock(&chip->lock);
Probe function does not need a lock if you rearrange the code below.
> +
> + ret = sysfs_create_group(&pdev->dev.kobj, &ts5500_adc_sysfs_group);
> + if (ret) {
> + dev_err(&pdev->dev, "sysfs_create_group failed.\n");
> + goto error_unlock_and_free;
> + }
> +
> + chip->hwmon_dev = hwmon_device_register(&pdev->dev);
> + if (IS_ERR(chip->hwmon_dev)) {
> + dev_err(&pdev->dev, "hwmon_device_register failed.\n");
> + ret = PTR_ERR(chip->hwmon_dev);
> + goto error_unregister_device;
> + }
> +
> + platform_set_drvdata(pdev, chip);
Set this before creating sysfs entries.
> + mutex_unlock(&chip->lock);
> + return 0;
> +
> +error_unregister_device:
> + sysfs_remove_group(&pdev->dev.kobj, &ts5500_adc_sysfs_group);
> +
> +error_unlock_and_free:
> + mutex_unlock(&chip->lock);
> + kfree(chip);
> + return ret;
> +}
> +
> +static void ts5500_adc_release(struct device *dev)
> +{
> + /* noop */
> +}
Is this really needed ? Just wondering.
> +
> +static struct resource ts5500_adc_resources[] = {
> + {
> + .name = "ts5500_adc" "-data",
> + .start = TS5500_ADC_INIT_LSB_REG,
> + .end = TS5500_ADC_MSB_REG,
> + .flags = IORESOURCE_IO,
> + },
> + {
> + .name = "ts5500_adc" "-ctrl",
> + .start = TS5500_ADC_CTRL_REG,
> + .end = TS5500_ADC_CTRL_REG,
> + .flags = IORESOURCE_IO,
> + }
> +};
> +
> +static struct ts5500_adc_platform_data ts5500_adc_platform_data = {
> + .name = TS5500_ADC_NAME,
> + .ioaddr = {
> + .data = TS5500_ADC_INIT_LSB_REG,
> + .ctrl = TS5500_ADC_CTRL_REG,
> + },
> + .read = {
> + .delay = TS5500_ADC_READ_DELAY,
> + .busy_mask = TS5500_ADC_READ_BUSY_MASK,
> + },
> + .ctrl = {
> + { TS5500_ADC_UNIPOLAR | TS5500_ADC_RANGE_5V,
> + TS5500_ADC_UNIPOLAR | TS5500_ADC_RANGE_10V },
> + { TS5500_ADC_BIPOLAR | TS5500_ADC_RANGE_5V,
> + TS5500_ADC_BIPOLAR | TS5500_ADC_RANGE_10V },
> + },
> + .ranges = {
> + .min = {
> + { 0, 0 },
> + { -5000, -10000 },
> + },
> + .max = {
> + { 5000, 10000 },
> + { 5000, 10000 },
> + },
> + },
> + .scale = {
> + { 12207, 24414 },
> + { 24414, 48828 },
> + },
> +};
> +
> +static struct platform_device ts5500_adc_device = {
> + .name = "ts5500_adc",
> + .id = -1,
> + .resource = ts5500_adc_resources,
> + .num_resources = ARRAY_SIZE(ts5500_adc_resources),
> + .dev = {
> + .platform_data = &ts5500_adc_platform_data,
> + .release = ts5500_adc_release,
> + },
> +};
> +
Usually all the above would be in a platform file.
> +static int __devexit ts5500_adc_remove(struct platform_device *pdev)
> +{
> + struct ts5500_adc_chip *chip = platform_get_drvdata(pdev);
> +
> + mutex_lock(&chip->lock);
> + hwmon_device_unregister(chip->hwmon_dev);
> + sysfs_remove_group(&pdev->dev.kobj, &ts5500_adc_sysfs_group);
> + platform_set_drvdata(pdev, NULL);
> + mutex_unlock(&chip->lock);
Lock not needed here.
> + kfree(chip);
> +
> + return 0;
> +}
> +
> +static struct platform_driver ts5500_adc_driver = {
> + .driver = {
> + .name = "ts5500_adc",
> + .owner = THIS_MODULE,
> + },
> + .probe = ts5500_adc_probe,
> + .remove = __devexit_p(ts5500_adc_remove)
> +};
> +
> +static int __init ts5500_adc_init(void)
> +{
> + int ret;
> +
> + ret = platform_driver_register(&ts5500_adc_driver);
> + if (ret)
> + goto error_out;
> +
> + ret = platform_device_register(&ts5500_adc_device);
> + if (ret)
> + goto error_device_register;
> +
> + return 0;
> +
> +error_device_register:
> + platform_driver_unregister(&ts5500_adc_driver);
> +error_out:
> + return ret;
> +}
> +module_init(ts5500_adc_init);
> +
> +static void __exit ts5500_adc_exit(void)
> +{
> + platform_driver_unregister(&ts5500_adc_driver);
> + platform_device_unregister(&ts5500_adc_device);
> +}
> +module_exit(ts5500_adc_exit);
> +
> +MODULE_DESCRIPTION("TS-5500 mapped MAX197 ADC device driver");
> +MODULE_AUTHOR("Jonas Fonseca <jonas.fonseca@savoirfairelinux.com>");
> +MODULE_LICENSE("GPL");
> --
> 1.7.6.5
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 4/4] x86/platform: (TS-5500) add ADC support
2012-01-19 2:55 ` Guenter Roeck
@ 2012-01-19 2:57 ` Guenter Roeck
2012-01-20 23:41 ` Vivien Didelot
1 sibling, 0 replies; 18+ messages in thread
From: Guenter Roeck @ 2012-01-19 2:57 UTC (permalink / raw)
To: Vivien Didelot
Cc: x86, Jonas Fonseca, Ingo Molnar, Thomas Gleixner, H. Peter Anvin,
linux-kernel, lm-sensors
On Wed, Jan 18, 2012 at 06:55:49PM -0800, Guenter Roeck wrote:
> On Wed, Jan 18, 2012 at 06:52:11PM -0500, Vivien Didelot wrote:
> > From: Jonas Fonseca <jonas.fonseca@savoirfairelinux.com>
> >
> > Signed-off-by: Jonas Fonseca <jonas.fonseca@savoirfairelinux.com>
> > Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> > ---
> > arch/x86/platform/ts5500/Kconfig | 7 +
> > arch/x86/platform/ts5500/Makefile | 1 +
> > arch/x86/platform/ts5500/ts5500_adc.c | 478 +++++++++++++++++++++++++++++++++
> > 3 files changed, 486 insertions(+), 0 deletions(-)
> > create mode 100644 arch/x86/platform/ts5500/ts5500_adc.c
> >
> I can not comment about the other drivers, but this is a hwmon driver, and thus should
> reside in drivers/hwmon and be reviewed and handled on the hwmon mailing list (copied).
>
Now I forgot that myself. Copying lm-sensors now.
> Couple of additional comments below; just some things I noticed, not a complete review.
>
> > diff --git a/arch/x86/platform/ts5500/Kconfig b/arch/x86/platform/ts5500/Kconfig
> > index 76f777f..f1a5f1d 100644
> > --- a/arch/x86/platform/ts5500/Kconfig
> > +++ b/arch/x86/platform/ts5500/Kconfig
> > @@ -20,3 +20,10 @@ config TS5500_LED
> > default y
> > help
> > This option enables support for the on-chip LED.
> > +
> > +config TS5500_ADC
> > + tristate "TS-5500 ADC"
> > + depends on TS5500 && HWMON=y
> > + default y
> > + help
> > + Support for the A/D converter on Technologic Systems TS-5500 SBCs.
> > diff --git a/arch/x86/platform/ts5500/Makefile b/arch/x86/platform/ts5500/Makefile
> > index 88eccc9..dcf46d8 100644
> > --- a/arch/x86/platform/ts5500/Makefile
> > +++ b/arch/x86/platform/ts5500/Makefile
> > @@ -1,3 +1,4 @@
> > obj-$(CONFIG_TS5500) += ts5500.o
> > obj-$(CONFIG_TS5500_GPIO) += ts5500_gpio.o
> > obj-$(CONFIG_TS5500_LED) += ts5500_led.o
> > +obj-$(CONFIG_TS5500_ADC) += ts5500_adc.o
> > diff --git a/arch/x86/platform/ts5500/ts5500_adc.c b/arch/x86/platform/ts5500/ts5500_adc.c
> > new file mode 100644
> > index 0000000..fc4560f
> > --- /dev/null
> > +++ b/arch/x86/platform/ts5500/ts5500_adc.c
> > @@ -0,0 +1,478 @@
> > +/*
> > + * Technologic Systems TS-5500 boards - Mapped MAX197 ADC driver
> > + *
> > + * Copyright (c) 2010-2012 Savoir-faire Linux Inc.
> > + * Jonas Fonseca <jonas.fonseca@savoirfairelinux.com>
> > + *
> > + * Portions Copyright (C) 2008 Marc Pignat <marc.pignat@hevs.ch>
> > + *
> > + * The driver uses direct access for communication with the ADC.
> > + * Should work unchanged with the MAX199 chip.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> > + *
> > + * The TS-5500 uses a CPLD to abstract the interface to a MAX197.
> > + */
> > +
> > +#include <linux/init.h>
> > +#include <linux/module.h>
> > +#include <linux/kernel.h>
> > +#include <linux/device.h>
> > +#include <linux/delay.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/err.h>
> > +#include <linux/sysfs.h>
> > +#include <linux/hwmon.h>
> > +#include <linux/hwmon-sysfs.h>
> > +#include <linux/mutex.h>
> > +#include <linux/slab.h>
> > +#include <linux/io.h>
> > +#include "ts5500.h"
> > +
> > +#define TS5500_ADC_CTRL_REG 0x195 /* Conversion state register */
> > +#define TS5500_ADC_INIT_LSB_REG 0x196 /* Init conv. / LSB register */
> > +#define TS5500_ADC_MSB_REG 0x197 /* MSB register */
> > +/*
> > + * Control bits of A/D command
> > + * bits 0-2: selected channel (0 - 7)
> > + * bits 3: uni/bipolar (0 = unipolar (ie 0 to +5V))
> > + * (1 = bipolar (ie -5 to +5V))
> > + * bit 4: selected range (0 = 5v range, 1 = 10V range)
> > + * bit 5-7: padded zero (unused)
> > + */
> > +
> > +#define TS5500_ADC_CHANNELS_MAX 8 /* 0 to 7 channels on device */
> > +
> > +#define TS5500_ADC_BIPOLAR 0x08
> > +#define TS5500_ADC_UNIPOLAR 0x00
> > +#define TS5500_ADC_RANGE_5V 0x00 /* 0 to 5V range */
> > +#define TS5500_ADC_RANGE_10V 0x10 /* 0 to 10V range */
> > +
> > +#define TS5500_ADC_READ_DELAY 15 /* usec */
> > +#define TS5500_ADC_READ_BUSY_MASK 0x01
> > +#define TS5500_ADC_NAME "MAX197 (8 channels)"
> > +
> > +/**
> > + * struct ts5500_adc_platform_data
> > + * @name: Name of the device.
> > + * @ioaddr: I/O address containing:
> > + * .data: Data register for conversion reading.
> > + * .ctrl: A/D Control Register (bit 0 = 0 when
> > + * conversion completed).
> > + * @read: Information about conversion reading, with:
> > + * .delay: Delay before next conversion.
> > + * .busy_mask: Control register bit 0 equals 1 means
> > + * conversion is not completed yet.
> > + * @ctrl: Data tables addressable by [polarity][range].
> > + * @ranges: Ranges.
> > + * .min: Min value.
> > + * .max: Max value.
> > + * @scale: Polarity/Range coefficients to scale raw conversion reading.
> > + */
> > +struct ts5500_adc_platform_data {
> > + const char *name;
> > + struct {
> > + int data;
> > + int ctrl;
> > + } ioaddr;
> > + struct {
> > + u8 delay;
> > + u8 busy_mask;
> > + } read;
> > + u8 ctrl[2][2];
>
> const ?
>
> > + struct {
> > + int min[2][2];
> > + int max[2][2];
>
> Should those be const ?
>
> > + } ranges;
> > + int scale[2][2];
>
> const ?
>
> > +};
> > +
>
> I am a bit lost about this structure and its use. It appears as if you expect that
> there will be other uses besides the one defined here (otherwise the variables would
> not make much sense and you could use defines), yet it is all defined in this file
> and thus static. Please elaborate.
>
> > +#define ts5500_adc_test_bit(bit, map) (test_bit(bit, map) != 0)
> > +
> Why "!= 0" ? Isn't that implied ? And then why the define to start with ?
>
> > +/**
> > + * struct ts5500_adc_chip
> > + * @hwmon_dev: The hwmon device.
> > + * @lock: Read/Write mutex.
> > + * @spec: The mapped MAX197 platform data.
> > + * @polarity: bitmap for polarity.
> > + * @range: bitmap for range.
> > + */
> > +struct ts5500_adc_chip {
> > + struct device *hwmon_dev;
> > + struct mutex lock;
> > + struct ts5500_adc_platform_data spec;
> > + DECLARE_BITMAP(polarity, TS5500_ADC_CHANNELS_MAX);
> > + DECLARE_BITMAP(range, TS5500_ADC_CHANNELS_MAX);
> > +};
> > +
> > +static s32 ts5500_adc_scale(struct ts5500_adc_chip *chip, s16 raw,
> > + int polarity, int range)
> > +{
> > + s32 scaled = raw;
> > +
> > + scaled *= chip->spec.scale[polarity][range];
> > + scaled /= 10000;
> > +
> > + return scaled;
> > +}
> > +
> > +static int ts5500_adc_range(struct ts5500_adc_chip *chip, int is_min,
> > + int polarity, int range)
> > +{
> > + if (is_min)
> > + return chip->spec.ranges.min[polarity][range];
> > + return chip->spec.ranges.max[polarity][range];
> > +}
> > +
> > +static int ts5500_adc_strtol(const char *buf, long *value, int range1,
> > + int range2)
> > +{
> > + if (strict_strtol(buf, 10, value))
>
> checkpatch warning.
>
> > + return -EINVAL;
> > +
> > + if (range1 < range2)
> > + *value = SENSORS_LIMIT(*value, range1, range2);
> > + else
> > + *value = SENSORS_LIMIT(*value, range2, range1);
> > +
> > + return 0;
> > +}
> This function is called exactly once. Why not just embed it in the calling code ?
>
> > +
> > +static struct ts5500_adc_chip *ts5500_adc_get_drvdata(struct device *dev)
> > +{
> > + return platform_get_drvdata(to_platform_device(dev));
> > +}
> > +
> > +/**
> > + * ts5500_adc_show_range() - Display range on user output
> > + *
> > + * Function called on read access on
> > + * /sys/devices/platform/ts5500-adc/in{0,1,2,3,4,5,6,7}_{min,max}
> > + */
> > +static ssize_t ts5500_adc_show_range(struct device *dev,
> > + struct device_attribute *devattr, char *buf)
> > +{
> > + struct ts5500_adc_chip *chip = ts5500_adc_get_drvdata(dev);
> > + struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
> > + int is_min = attr->nr != 0;
> > + int polarity, range;
> > +
> > + if (mutex_lock_interruptible(&chip->lock))
> > + return -ERESTARTSYS;
> > +
> > + polarity = ts5500_adc_test_bit(attr->index, chip->polarity);
> > + range = ts5500_adc_test_bit(attr->index, chip->range);
> > +
> > + mutex_unlock(&chip->lock);
> > +
> > + return sprintf(buf, "%d\n",
> > + ts5500_adc_range(chip, is_min, polarity, range));
> > +}
> > +
> > +/**
> > + * ts5500_adc_store_range() - Write range from user input
> > + *
> > + * Function called on write access on
> > + * /sys/devices/platform/ts5500-adc/in{0,1,2,3,4,5,6,7}_{min,max}
> > + */
> > +static ssize_t ts5500_adc_store_range(struct device *dev,
> > + struct device_attribute *devattr,
> > + const char *buf, size_t count)
> > +{
> > + struct ts5500_adc_chip *chip = ts5500_adc_get_drvdata(dev);
> > + struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
> > + int is_min = attr->nr != 0;
> > + int range1 = ts5500_adc_range(chip, is_min, 0, 0);
> > + int range2 = ts5500_adc_range(chip, is_min, 1, 1);
> > + long value;
> > +
> > + if (ts5500_adc_strtol(buf, &value, range1, range2))
> > + return -EINVAL;
> > +
> > + if (mutex_lock_interruptible(&chip->lock))
> > + return -ERESTARTSYS;
> > +
> > + if (abs(value) > 5000)
> > + set_bit(attr->index, chip->range);
> > + else
> > + clear_bit(attr->index, chip->range);
> > +
> > + if (is_min) {
> > + if (value < 0)
> > + set_bit(attr->index, chip->polarity);
> > + else
> > + clear_bit(attr->index, chip->polarity);
> > + }
> > +
> > + mutex_unlock(&chip->lock);
> > +
> > + return count;
> > +}
> > +
> > +/**
> > + * ts5500_adc_show_input() - Show channel input
> > + *
> > + * Function called on read access on
> > + * /sys/devices/platform/ts5500-adc/in{0,1,2,3,4,5,6,7}_input
> > + */
> > +static ssize_t ts5500_adc_show_input(struct device *dev,
> > + struct device_attribute *devattr,
> > + char *buf)
> > +{
> > + struct ts5500_adc_chip *chip = ts5500_adc_get_drvdata(dev);
> > + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> > + int polarity, range;
> > + int ret;
> > + u8 command;
> > +
> > + if (mutex_lock_interruptible(&chip->lock))
> > + return -ERESTARTSYS;
> > +
> > + polarity = ts5500_adc_test_bit(attr->index, chip->polarity);
> > + range = ts5500_adc_test_bit(attr->index, chip->range);
> > +
> > + command = attr->index | chip->spec.ctrl[polarity][range];
> > +
> > + outb(command, chip->spec.ioaddr.data);
> > +
> > + udelay(chip->spec.read.delay);
> > + ret = inb(chip->spec.ioaddr.ctrl);
> > +
> > + if (ret & chip->spec.read.busy_mask) {
> > + dev_err(dev, "device not ready (ret=0x0%x, try=%d)\n", ret,
> > + range);
>
> What does "try=" have to do with the displayed "range" ?
>
> > + ret = -EIO;
> > + } else {
> > + /* LSB of conversion is at 0x196 and MSB is at 0x197 */
> > + u8 lsb = inb(chip->spec.ioaddr.data);
> > + u8 msb = inb(chip->spec.ioaddr.data + 1);
> > + s16 raw = (msb << 8) | lsb;
> > + s32 scaled = ts5500_adc_scale(chip, raw, polarity, range);
> > +
> > + ret = sprintf(buf, "%d\n", scaled);
> > + }
> > +
> > + mutex_unlock(&chip->lock);
> > + return ret;
> > +}
> > +
> > +static ssize_t ts5500_adc_show_name(struct device *dev,
> > + struct device_attribute *devattr, char *buf)
> > +{
> > + return sprintf(buf, "%s\n", ts5500_adc_get_drvdata(dev)->spec.name);
> > +}
> > +
> > +#define TS5500_ADC_HWMON_CHANNEL(chan) \
> > + SENSOR_DEVICE_ATTR(in##chan##_input, S_IRUGO, \
> > + ts5500_adc_show_input, NULL, chan); \
> > + SENSOR_DEVICE_ATTR_2(in##chan##_max, S_IRUGO | S_IWUSR, \
> > + ts5500_adc_show_range, \
> > + ts5500_adc_store_range, 0, chan); \
> > + SENSOR_DEVICE_ATTR_2(in##chan##_min, S_IRUGO | S_IWUSR, \
> > + ts5500_adc_show_range, \
> > + ts5500_adc_store_range, 1, chan) \
> > +
> > +#define TS5500_ADC_SYSFS_CHANNEL(chan) \
> > + &sensor_dev_attr_in##chan##_input.dev_attr.attr, \
> > + &sensor_dev_attr_in##chan##_max.dev_attr.attr, \
> > + &sensor_dev_attr_in##chan##_min.dev_attr.attr
> > +
> > +static DEVICE_ATTR(name, S_IRUGO, ts5500_adc_show_name, NULL);
> > +
> > +static TS5500_ADC_HWMON_CHANNEL(0);
> > +static TS5500_ADC_HWMON_CHANNEL(1);
> > +static TS5500_ADC_HWMON_CHANNEL(2);
> > +static TS5500_ADC_HWMON_CHANNEL(3);
> > +static TS5500_ADC_HWMON_CHANNEL(4);
> > +static TS5500_ADC_HWMON_CHANNEL(5);
> > +static TS5500_ADC_HWMON_CHANNEL(6);
> > +static TS5500_ADC_HWMON_CHANNEL(7);
> > +
> Looks good at first glance, but unless I misunderstand something, each define generates
> three structures, yet only the first of those is declared static, the others are global.
>
> > +static const struct attribute_group ts5500_adc_sysfs_group = {
> > + .attrs = (struct attribute *[]) {
>
> checkpatch error.
>
> > + &dev_attr_name.attr,
> > + TS5500_ADC_SYSFS_CHANNEL(0),
> > + TS5500_ADC_SYSFS_CHANNEL(1),
> > + TS5500_ADC_SYSFS_CHANNEL(2),
> > + TS5500_ADC_SYSFS_CHANNEL(3),
> > + TS5500_ADC_SYSFS_CHANNEL(4),
> > + TS5500_ADC_SYSFS_CHANNEL(5),
> > + TS5500_ADC_SYSFS_CHANNEL(6),
> > + TS5500_ADC_SYSFS_CHANNEL(7),
> > + NULL
> > + }
> > +};
> > +
> > +static int __devinit ts5500_adc_probe(struct platform_device *pdev)
> > +{
> > + struct ts5500_adc_platform_data *pdata = pdev->dev.platform_data;
> > + struct ts5500_adc_chip *chip;
> > + int ret;
> > +
> > + if (pdata == NULL)
> > + return -ENODEV;
> > +
> > + chip = kzalloc(sizeof *chip, GFP_KERNEL);
> > + if (!chip)
> > + return -ENOMEM;
> > +
> > + chip->spec = *pdata;
> > +
> > + mutex_init(&chip->lock);
> > + mutex_lock(&chip->lock);
>
> Probe function does not need a lock if you rearrange the code below.
>
> > +
> > + ret = sysfs_create_group(&pdev->dev.kobj, &ts5500_adc_sysfs_group);
> > + if (ret) {
> > + dev_err(&pdev->dev, "sysfs_create_group failed.\n");
> > + goto error_unlock_and_free;
> > + }
> > +
> > + chip->hwmon_dev = hwmon_device_register(&pdev->dev);
> > + if (IS_ERR(chip->hwmon_dev)) {
> > + dev_err(&pdev->dev, "hwmon_device_register failed.\n");
> > + ret = PTR_ERR(chip->hwmon_dev);
> > + goto error_unregister_device;
> > + }
> > +
> > + platform_set_drvdata(pdev, chip);
>
> Set this before creating sysfs entries.
>
> > + mutex_unlock(&chip->lock);
> > + return 0;
> > +
> > +error_unregister_device:
> > + sysfs_remove_group(&pdev->dev.kobj, &ts5500_adc_sysfs_group);
> > +
> > +error_unlock_and_free:
> > + mutex_unlock(&chip->lock);
> > + kfree(chip);
> > + return ret;
> > +}
> > +
> > +static void ts5500_adc_release(struct device *dev)
> > +{
> > + /* noop */
> > +}
>
> Is this really needed ? Just wondering.
>
> > +
> > +static struct resource ts5500_adc_resources[] = {
> > + {
> > + .name = "ts5500_adc" "-data",
> > + .start = TS5500_ADC_INIT_LSB_REG,
> > + .end = TS5500_ADC_MSB_REG,
> > + .flags = IORESOURCE_IO,
> > + },
> > + {
> > + .name = "ts5500_adc" "-ctrl",
> > + .start = TS5500_ADC_CTRL_REG,
> > + .end = TS5500_ADC_CTRL_REG,
> > + .flags = IORESOURCE_IO,
> > + }
> > +};
> > +
> > +static struct ts5500_adc_platform_data ts5500_adc_platform_data = {
> > + .name = TS5500_ADC_NAME,
> > + .ioaddr = {
> > + .data = TS5500_ADC_INIT_LSB_REG,
> > + .ctrl = TS5500_ADC_CTRL_REG,
> > + },
> > + .read = {
> > + .delay = TS5500_ADC_READ_DELAY,
> > + .busy_mask = TS5500_ADC_READ_BUSY_MASK,
> > + },
> > + .ctrl = {
> > + { TS5500_ADC_UNIPOLAR | TS5500_ADC_RANGE_5V,
> > + TS5500_ADC_UNIPOLAR | TS5500_ADC_RANGE_10V },
> > + { TS5500_ADC_BIPOLAR | TS5500_ADC_RANGE_5V,
> > + TS5500_ADC_BIPOLAR | TS5500_ADC_RANGE_10V },
> > + },
> > + .ranges = {
> > + .min = {
> > + { 0, 0 },
> > + { -5000, -10000 },
> > + },
> > + .max = {
> > + { 5000, 10000 },
> > + { 5000, 10000 },
> > + },
> > + },
> > + .scale = {
> > + { 12207, 24414 },
> > + { 24414, 48828 },
> > + },
> > +};
> > +
> > +static struct platform_device ts5500_adc_device = {
> > + .name = "ts5500_adc",
> > + .id = -1,
> > + .resource = ts5500_adc_resources,
> > + .num_resources = ARRAY_SIZE(ts5500_adc_resources),
> > + .dev = {
> > + .platform_data = &ts5500_adc_platform_data,
> > + .release = ts5500_adc_release,
> > + },
> > +};
> > +
> Usually all the above would be in a platform file.
>
> > +static int __devexit ts5500_adc_remove(struct platform_device *pdev)
> > +{
> > + struct ts5500_adc_chip *chip = platform_get_drvdata(pdev);
> > +
> > + mutex_lock(&chip->lock);
> > + hwmon_device_unregister(chip->hwmon_dev);
> > + sysfs_remove_group(&pdev->dev.kobj, &ts5500_adc_sysfs_group);
> > + platform_set_drvdata(pdev, NULL);
> > + mutex_unlock(&chip->lock);
>
> Lock not needed here.
>
> > + kfree(chip);
> > +
> > + return 0;
> > +}
> > +
> > +static struct platform_driver ts5500_adc_driver = {
> > + .driver = {
> > + .name = "ts5500_adc",
> > + .owner = THIS_MODULE,
> > + },
> > + .probe = ts5500_adc_probe,
> > + .remove = __devexit_p(ts5500_adc_remove)
> > +};
> > +
> > +static int __init ts5500_adc_init(void)
> > +{
> > + int ret;
> > +
> > + ret = platform_driver_register(&ts5500_adc_driver);
> > + if (ret)
> > + goto error_out;
> > +
> > + ret = platform_device_register(&ts5500_adc_device);
> > + if (ret)
> > + goto error_device_register;
> > +
> > + return 0;
> > +
> > +error_device_register:
> > + platform_driver_unregister(&ts5500_adc_driver);
> > +error_out:
> > + return ret;
> > +}
> > +module_init(ts5500_adc_init);
> > +
> > +static void __exit ts5500_adc_exit(void)
> > +{
> > + platform_driver_unregister(&ts5500_adc_driver);
> > + platform_device_unregister(&ts5500_adc_device);
> > +}
> > +module_exit(ts5500_adc_exit);
> > +
> > +MODULE_DESCRIPTION("TS-5500 mapped MAX197 ADC device driver");
> > +MODULE_AUTHOR("Jonas Fonseca <jonas.fonseca@savoirfairelinux.com>");
> > +MODULE_LICENSE("GPL");
> > --
> > 1.7.6.5
> >
> >
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 4/4] x86/platform: (TS-5500) add ADC support
2012-01-19 2:55 ` Guenter Roeck
2012-01-19 2:57 ` Guenter Roeck
@ 2012-01-20 23:41 ` Vivien Didelot
2012-01-21 0:09 ` Guenter Roeck
1 sibling, 1 reply; 18+ messages in thread
From: Vivien Didelot @ 2012-01-20 23:41 UTC (permalink / raw)
To: Guenter Roeck
Cc: x86, Jonas Fonseca, Ingo Molnar, Thomas Gleixner, H. Peter Anvin,
linux-kernel
Hi Guenter,
Thanks for the code review.
On Wed, 18 Jan 2012 18:55:49
-0800, Guenter Roeck <guenter.roeck@ericsson.com> wrote:
> On Wed, Jan 18, 2012 at 06:52:11PM -0500, Vivien Didelot wrote:
> > From: Jonas Fonseca <jonas.fonseca@savoirfairelinux.com>
> >
> > Signed-off-by: Jonas Fonseca <jonas.fonseca@savoirfairelinux.com>
> > Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> > ---
> > arch/x86/platform/ts5500/Kconfig | 7 +
> > arch/x86/platform/ts5500/Makefile | 1 +
> > arch/x86/platform/ts5500/ts5500_adc.c | 478
> > +++++++++++++++++++++++++++++++++ 3 files changed, 486
> > insertions(+), 0 deletions(-) create mode 100644
> > arch/x86/platform/ts5500/ts5500_adc.c
> >
> I can not comment about the other drivers, but this is a hwmon
> driver, and thus should reside in drivers/hwmon and be reviewed and
> handled on the hwmon mailing list (copied).
The ADC is a MAX197, but it is totally abstracted by the TS-5500 CPLD.
The device (and its driver) is then really specific to the TS-5500
platform (like the other devices). So the driver should be in its
platform directory.
>
> Couple of additional comments below; just some things I noticed, not
> a complete review.
>
> > diff --git a/arch/x86/platform/ts5500/Kconfig
> > b/arch/x86/platform/ts5500/Kconfig index 76f777f..f1a5f1d 100644
> > --- a/arch/x86/platform/ts5500/Kconfig
> > +++ b/arch/x86/platform/ts5500/Kconfig
> > @@ -20,3 +20,10 @@ config TS5500_LED
> > default y
> > help
> > This option enables support for the on-chip LED.
> > +
> > +config TS5500_ADC
> > + tristate "TS-5500 ADC"
> > + depends on TS5500 && HWMON=y
> > + default y
> > + help
> > + Support for the A/D converter on Technologic Systems TS-5500
> > SBCs. diff --git a/arch/x86/platform/ts5500/Makefile
> > b/arch/x86/platform/ts5500/Makefile index 88eccc9..dcf46d8 100644
> > --- a/arch/x86/platform/ts5500/Makefile
> > +++ b/arch/x86/platform/ts5500/Makefile
> > @@ -1,3 +1,4 @@
> > obj-$(CONFIG_TS5500) += ts5500.o
> > obj-$(CONFIG_TS5500_GPIO) += ts5500_gpio.o
> > obj-$(CONFIG_TS5500_LED) += ts5500_led.o
> > +obj-$(CONFIG_TS5500_ADC) += ts5500_adc.o
> > diff --git a/arch/x86/platform/ts5500/ts5500_adc.c
> > b/arch/x86/platform/ts5500/ts5500_adc.c new file mode 100644
> > index 0000000..fc4560f
> > --- /dev/null
> > +++ b/arch/x86/platform/ts5500/ts5500_adc.c
> > @@ -0,0 +1,478 @@
> > +/*
> > + * Technologic Systems TS-5500 boards - Mapped MAX197 ADC driver
> > + *
> > + * Copyright (c) 2010-2012 Savoir-faire Linux Inc.
> > + * Jonas Fonseca <jonas.fonseca@savoirfairelinux.com>
> > + *
> > + * Portions Copyright (C) 2008 Marc Pignat <marc.pignat@hevs.ch>
> > + *
> > + * The driver uses direct access for communication with the ADC.
> > + * Should work unchanged with the MAX199 chip.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > modify
> > + * it under the terms of the GNU General Public License as
> > published by
> > + * the Free Software Foundation; either version 2 of the License,
> > or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public
> > License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> > + *
> > + * The TS-5500 uses a CPLD to abstract the interface to a MAX197.
> > + */
> > +
> > +#include <linux/init.h>
> > +#include <linux/module.h>
> > +#include <linux/kernel.h>
> > +#include <linux/device.h>
> > +#include <linux/delay.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/err.h>
> > +#include <linux/sysfs.h>
> > +#include <linux/hwmon.h>
> > +#include <linux/hwmon-sysfs.h>
> > +#include <linux/mutex.h>
> > +#include <linux/slab.h>
> > +#include <linux/io.h>
> > +#include "ts5500.h"
> > +
> > +#define TS5500_ADC_CTRL_REG 0x195 /*
> > Conversion state register */ +#define
> > TS5500_ADC_INIT_LSB_REG 0x196 /* Init conv. /
> > LSB register */ +#define TS5500_ADC_MSB_REG
> > 0x197 /* MSB register */ +/*
> > + * Control bits of A/D command
> > + * bits 0-2: selected channel (0 - 7)
> > + * bits 3: uni/bipolar (0 = unipolar (ie 0 to +5V))
> > + * (1 = bipolar (ie -5 to +5V))
> > + * bit 4: selected range (0 = 5v range, 1 = 10V range)
> > + * bit 5-7: padded zero (unused)
> > + */
> > +
> > +#define TS5500_ADC_CHANNELS_MAX 8 /* 0 to 7
> > channels on device */ +
> > +#define TS5500_ADC_BIPOLAR 0x08
> > +#define TS5500_ADC_UNIPOLAR 0x00
> > +#define TS5500_ADC_RANGE_5V 0x00 /* 0 to 5V
> > range */ +#define TS5500_ADC_RANGE_10V
> > 0x10 /* 0 to 10V range */ +
> > +#define TS5500_ADC_READ_DELAY 15 /* usec */
> > +#define TS5500_ADC_READ_BUSY_MASK 0x01
> > +#define TS5500_ADC_NAME "MAX197 (8
> > channels)" +
> > +/**
> > + * struct ts5500_adc_platform_data
> > + * @name: Name of the device.
> > + * @ioaddr: I/O address containing:
> > + * .data: Data register for
> > conversion reading.
> > + * .ctrl: A/D Control Register (bit
> > 0 = 0 when
> > + * conversion completed).
> > + * @read: Information about conversion reading, with:
> > + * .delay: Delay before next
> > conversion.
> > + * .busy_mask: Control register bit 0 equals
> > 1 means
> > + * conversion is not completed yet.
> > + * @ctrl: Data tables addressable by [polarity][range].
> > + * @ranges: Ranges.
> > + * .min: Min value.
> > + * .max: Max value.
> > + * @scale: Polarity/Range coefficients to scale raw
> > conversion reading.
> > + */
> > +struct ts5500_adc_platform_data {
> > + const char *name;
> > + struct {
> > + int data;
> > + int ctrl;
> > + } ioaddr;
> > + struct {
> > + u8 delay;
> > + u8 busy_mask;
> > + } read;
> > + u8 ctrl[2][2];
>
> const ?
>
> > + struct {
> > + int min[2][2];
> > + int max[2][2];
>
> Should those be const ?
>
> > + } ranges;
> > + int scale[2][2];
>
> const ?
>
> > +};
> > +
>
> I am a bit lost about this structure and its use. It appears as if
> you expect that there will be other uses besides the one defined here
> (otherwise the variables would not make much sense and you could use
> defines), yet it is all defined in this file and thus static. Please
> elaborate.
You're right. I removed this structure and used define instead.
>
> > +#define ts5500_adc_test_bit(bit, map) (test_bit(bit,
> > map) != 0) +
> Why "!= 0" ? Isn't that implied ? And then why the define to start
> with ?
Dropped. I replaced ts5500_adc_test_bit() call by !!test_bit().
>
> > +/**
> > + * struct ts5500_adc_chip
> > + * @hwmon_dev: The hwmon device.
> > + * @lock: Read/Write mutex.
> > + * @spec: The mapped MAX197 platform data.
> > + * @polarity: bitmap for polarity.
> > + * @range: bitmap for range.
> > + */
> > +struct ts5500_adc_chip {
> > + struct device *hwmon_dev;
> > + struct mutex lock;
> > + struct ts5500_adc_platform_data spec;
> > + DECLARE_BITMAP(polarity, TS5500_ADC_CHANNELS_MAX);
> > + DECLARE_BITMAP(range, TS5500_ADC_CHANNELS_MAX);
> > +};
> > +
> > +static s32 ts5500_adc_scale(struct ts5500_adc_chip *chip, s16 raw,
> > + int polarity, int range)
> > +{
> > + s32 scaled = raw;
> > +
> > + scaled *= chip->spec.scale[polarity][range];
> > + scaled /= 10000;
> > +
> > + return scaled;
> > +}
> > +
> > +static int ts5500_adc_range(struct ts5500_adc_chip *chip, int
> > is_min,
> > + int polarity, int range)
> > +{
> > + if (is_min)
> > + return chip->spec.ranges.min[polarity][range];
> > + return chip->spec.ranges.max[polarity][range];
> > +}
> > +
> > +static int ts5500_adc_strtol(const char *buf, long *value, int
> > range1,
> > + int range2)
> > +{
> > + if (strict_strtol(buf, 10, value))
>
> checkpatch warning.
>
> > + return -EINVAL;
> > +
> > + if (range1 < range2)
> > + *value = SENSORS_LIMIT(*value, range1, range2);
> > + else
> > + *value = SENSORS_LIMIT(*value, range2, range1);
> > +
> > + return 0;
> > +}
> This function is called exactly once. Why not just embed it in the
> calling code ?
>
> > +
> > +static struct ts5500_adc_chip *ts5500_adc_get_drvdata(struct
> > device *dev) +{
> > + return platform_get_drvdata(to_platform_device(dev));
> > +}
> > +
> > +/**
> > + * ts5500_adc_show_range() - Display range on user output
> > + *
> > + * Function called on read access on
> > + * /sys/devices/platform/ts5500-adc/in{0,1,2,3,4,5,6,7}_{min,max}
> > + */
> > +static ssize_t ts5500_adc_show_range(struct device *dev,
> > + struct device_attribute *devattr,
> > char *buf) +{
> > + struct ts5500_adc_chip *chip = ts5500_adc_get_drvdata(dev);
> > + struct sensor_device_attribute_2 *attr =
> > to_sensor_dev_attr_2(devattr);
> > + int is_min = attr->nr != 0;
> > + int polarity, range;
> > +
> > + if (mutex_lock_interruptible(&chip->lock))
> > + return -ERESTARTSYS;
> > +
> > + polarity = ts5500_adc_test_bit(attr->index,
> > chip->polarity);
> > + range = ts5500_adc_test_bit(attr->index, chip->range);
> > +
> > + mutex_unlock(&chip->lock);
> > +
> > + return sprintf(buf, "%d\n",
> > + ts5500_adc_range(chip, is_min, polarity,
> > range)); +}
> > +
> > +/**
> > + * ts5500_adc_store_range() - Write range from user input
> > + *
> > + * Function called on write access on
> > + * /sys/devices/platform/ts5500-adc/in{0,1,2,3,4,5,6,7}_{min,max}
> > + */
> > +static ssize_t ts5500_adc_store_range(struct device *dev,
> > + struct device_attribute *devattr,
> > + const char *buf, size_t count)
> > +{
> > + struct ts5500_adc_chip *chip = ts5500_adc_get_drvdata(dev);
> > + struct sensor_device_attribute_2 *attr =
> > to_sensor_dev_attr_2(devattr);
> > + int is_min = attr->nr != 0;
> > + int range1 = ts5500_adc_range(chip, is_min, 0, 0);
> > + int range2 = ts5500_adc_range(chip, is_min, 1, 1);
> > + long value;
> > +
> > + if (ts5500_adc_strtol(buf, &value, range1, range2))
> > + return -EINVAL;
> > +
> > + if (mutex_lock_interruptible(&chip->lock))
> > + return -ERESTARTSYS;
> > +
> > + if (abs(value) > 5000)
> > + set_bit(attr->index, chip->range);
> > + else
> > + clear_bit(attr->index, chip->range);
> > +
> > + if (is_min) {
> > + if (value < 0)
> > + set_bit(attr->index, chip->polarity);
> > + else
> > + clear_bit(attr->index, chip->polarity);
> > + }
> > +
> > + mutex_unlock(&chip->lock);
> > +
> > + return count;
> > +}
> > +
> > +/**
> > + * ts5500_adc_show_input() - Show channel input
> > + *
> > + * Function called on read access on
> > + * /sys/devices/platform/ts5500-adc/in{0,1,2,3,4,5,6,7}_input
> > + */
> > +static ssize_t ts5500_adc_show_input(struct device *dev,
> > + struct device_attribute
> > *devattr,
> > + char *buf)
> > +{
> > + struct ts5500_adc_chip *chip = ts5500_adc_get_drvdata(dev);
> > + struct sensor_device_attribute *attr =
> > to_sensor_dev_attr(devattr);
> > + int polarity, range;
> > + int ret;
> > + u8 command;
> > +
> > + if (mutex_lock_interruptible(&chip->lock))
> > + return -ERESTARTSYS;
> > +
> > + polarity = ts5500_adc_test_bit(attr->index,
> > chip->polarity);
> > + range = ts5500_adc_test_bit(attr->index, chip->range);
> > +
> > + command = attr->index | chip->spec.ctrl[polarity][range];
> > +
> > + outb(command, chip->spec.ioaddr.data);
> > +
> > + udelay(chip->spec.read.delay);
> > + ret = inb(chip->spec.ioaddr.ctrl);
> > +
> > + if (ret & chip->spec.read.busy_mask) {
> > + dev_err(dev, "device not ready (ret=0x0%x,
> > try=%d)\n", ret,
> > + range);
>
> What does "try=" have to do with the displayed "range" ?
>
> > + ret = -EIO;
> > + } else {
> > + /* LSB of conversion is at 0x196 and MSB is at
> > 0x197 */
> > + u8 lsb = inb(chip->spec.ioaddr.data);
> > + u8 msb = inb(chip->spec.ioaddr.data + 1);
> > + s16 raw = (msb << 8) | lsb;
> > + s32 scaled = ts5500_adc_scale(chip, raw, polarity,
> > range); +
> > + ret = sprintf(buf, "%d\n", scaled);
> > + }
> > +
> > + mutex_unlock(&chip->lock);
> > + return ret;
> > +}
> > +
> > +static ssize_t ts5500_adc_show_name(struct device *dev,
> > + struct device_attribute *devattr, char *buf)
> > +{
> > + return sprintf(buf, "%s\n",
> > ts5500_adc_get_drvdata(dev)->spec.name); +}
> > +
> > +#define
> > TS5500_ADC_HWMON_CHANNEL(chan) \
> > + SENSOR_DEVICE_ATTR(in##chan##_input,
> > S_IRUGO, \
> > + ts5500_adc_show_input, NULL,
> > chan); \
> > + SENSOR_DEVICE_ATTR_2(in##chan##_max, S_IRUGO |
> > S_IWUSR, \
> > + ts5500_adc_show_range,
> > \
> > + ts5500_adc_store_range, 0,
> > chan); \
> > + SENSOR_DEVICE_ATTR_2(in##chan##_min, S_IRUGO |
> > S_IWUSR, \
> > + ts5500_adc_show_range,
> > \
> > + ts5500_adc_store_range, 1,
> > chan) \ +
> > +#define
> > TS5500_ADC_SYSFS_CHANNEL(chan) \
> > + &sensor_dev_attr_in##chan##_input.dev_attr.attr, \
> > +
> > &sensor_dev_attr_in##chan##_max.dev_attr.attr, \
> > + &sensor_dev_attr_in##chan##_min.dev_attr.attr
> > +
> > +static DEVICE_ATTR(name, S_IRUGO, ts5500_adc_show_name, NULL);
> > +
> > +static TS5500_ADC_HWMON_CHANNEL(0);
> > +static TS5500_ADC_HWMON_CHANNEL(1);
> > +static TS5500_ADC_HWMON_CHANNEL(2);
> > +static TS5500_ADC_HWMON_CHANNEL(3);
> > +static TS5500_ADC_HWMON_CHANNEL(4);
> > +static TS5500_ADC_HWMON_CHANNEL(5);
> > +static TS5500_ADC_HWMON_CHANNEL(6);
> > +static TS5500_ADC_HWMON_CHANNEL(7);
> > +
> Looks good at first glance, but unless I misunderstand something,
> each define generates three structures, yet only the first of those
> is declared static, the others are global.
Good point, now everything's declared as static.
>
> > +static const struct attribute_group ts5500_adc_sysfs_group = {
> > + .attrs = (struct attribute *[]) {
>
> checkpatch error.
This is a bit tricky. checkpatch thinks that the asterisk is a
multiplication, so it is complaining about coding style.
>
> > + &dev_attr_name.attr,
> > + TS5500_ADC_SYSFS_CHANNEL(0),
> > + TS5500_ADC_SYSFS_CHANNEL(1),
> > + TS5500_ADC_SYSFS_CHANNEL(2),
> > + TS5500_ADC_SYSFS_CHANNEL(3),
> > + TS5500_ADC_SYSFS_CHANNEL(4),
> > + TS5500_ADC_SYSFS_CHANNEL(5),
> > + TS5500_ADC_SYSFS_CHANNEL(6),
> > + TS5500_ADC_SYSFS_CHANNEL(7),
> > + NULL
> > + }
> > +};
> > +
> > +static int __devinit ts5500_adc_probe(struct platform_device *pdev)
> > +{
> > + struct ts5500_adc_platform_data *pdata =
> > pdev->dev.platform_data;
> > + struct ts5500_adc_chip *chip;
> > + int ret;
> > +
> > + if (pdata == NULL)
> > + return -ENODEV;
> > +
> > + chip = kzalloc(sizeof *chip, GFP_KERNEL);
> > + if (!chip)
> > + return -ENOMEM;
> > +
> > + chip->spec = *pdata;
> > +
> > + mutex_init(&chip->lock);
> > + mutex_lock(&chip->lock);
>
> Probe function does not need a lock if you rearrange the code below.
>
> > +
> > + ret = sysfs_create_group(&pdev->dev.kobj,
> > &ts5500_adc_sysfs_group);
> > + if (ret) {
> > + dev_err(&pdev->dev, "sysfs_create_group
> > failed.\n");
> > + goto error_unlock_and_free;
> > + }
> > +
> > + chip->hwmon_dev = hwmon_device_register(&pdev->dev);
> > + if (IS_ERR(chip->hwmon_dev)) {
> > + dev_err(&pdev->dev, "hwmon_device_register
> > failed.\n");
> > + ret = PTR_ERR(chip->hwmon_dev);
> > + goto error_unregister_device;
> > + }
> > +
> > + platform_set_drvdata(pdev, chip);
>
> Set this before creating sysfs entries.
Thanks for the trick.
>
> > + mutex_unlock(&chip->lock);
> > + return 0;
> > +
> > +error_unregister_device:
> > + sysfs_remove_group(&pdev->dev.kobj,
> > &ts5500_adc_sysfs_group); +
> > +error_unlock_and_free:
> > + mutex_unlock(&chip->lock);
> > + kfree(chip);
> > + return ret;
> > +}
> > +
> > +static void ts5500_adc_release(struct device *dev)
> > +{
> > + /* noop */
> > +}
>
> Is this really needed ? Just wondering.
Yes it is, otherwise you get a "Device 'ts5500_adc' does not have a
release() function, it is broken and must be fixed." kernel error.
>
> > +
> > +static struct resource ts5500_adc_resources[] = {
> > + {
> > + .name = "ts5500_adc" "-data",
> > + .start = TS5500_ADC_INIT_LSB_REG,
> > + .end = TS5500_ADC_MSB_REG,
> > + .flags = IORESOURCE_IO,
> > + },
> > + {
> > + .name = "ts5500_adc" "-ctrl",
> > + .start = TS5500_ADC_CTRL_REG,
> > + .end = TS5500_ADC_CTRL_REG,
> > + .flags = IORESOURCE_IO,
> > + }
> > +};
> > +
> > +static struct ts5500_adc_platform_data ts5500_adc_platform_data = {
> > + .name = TS5500_ADC_NAME,
> > + .ioaddr = {
> > + .data = TS5500_ADC_INIT_LSB_REG,
> > + .ctrl = TS5500_ADC_CTRL_REG,
> > + },
> > + .read = {
> > + .delay = TS5500_ADC_READ_DELAY,
> > + .busy_mask = TS5500_ADC_READ_BUSY_MASK,
> > + },
> > + .ctrl = {
> > + { TS5500_ADC_UNIPOLAR | TS5500_ADC_RANGE_5V,
> > + TS5500_ADC_UNIPOLAR | TS5500_ADC_RANGE_10V },
> > + { TS5500_ADC_BIPOLAR | TS5500_ADC_RANGE_5V,
> > + TS5500_ADC_BIPOLAR | TS5500_ADC_RANGE_10V },
> > + },
> > + .ranges = {
> > + .min = {
> > + { 0, 0 },
> > + { -5000, -10000 },
> > + },
> > + .max = {
> > + { 5000, 10000 },
> > + { 5000, 10000 },
> > + },
> > + },
> > + .scale = {
> > + { 12207, 24414 },
> > + { 24414, 48828 },
> > + },
> > +};
> > +
> > +static struct platform_device ts5500_adc_device = {
> > + .name = "ts5500_adc",
> > + .id = -1,
> > + .resource = ts5500_adc_resources,
> > + .num_resources = ARRAY_SIZE(ts5500_adc_resources),
> > + .dev = {
> > + .platform_data = &ts5500_adc_platform_data,
> > + .release = ts5500_adc_release,
> > + },
> > +};
> > +
> Usually all the above would be in a platform file.
>
> > +static int __devexit ts5500_adc_remove(struct platform_device
> > *pdev) +{
> > + struct ts5500_adc_chip *chip = platform_get_drvdata(pdev);
> > +
> > + mutex_lock(&chip->lock);
> > + hwmon_device_unregister(chip->hwmon_dev);
> > + sysfs_remove_group(&pdev->dev.kobj,
> > &ts5500_adc_sysfs_group);
> > + platform_set_drvdata(pdev, NULL);
> > + mutex_unlock(&chip->lock);
>
> Lock not needed here.
>
> > + kfree(chip);
> > +
> > + return 0;
> > +}
> > +
> > +static struct platform_driver ts5500_adc_driver = {
> > + .driver = {
> > + .name = "ts5500_adc",
> > + .owner = THIS_MODULE,
> > + },
> > + .probe = ts5500_adc_probe,
> > + .remove = __devexit_p(ts5500_adc_remove)
> > +};
> > +
> > +static int __init ts5500_adc_init(void)
> > +{
> > + int ret;
> > +
> > + ret = platform_driver_register(&ts5500_adc_driver);
> > + if (ret)
> > + goto error_out;
> > +
> > + ret = platform_device_register(&ts5500_adc_device);
> > + if (ret)
> > + goto error_device_register;
> > +
> > + return 0;
> > +
> > +error_device_register:
> > + platform_driver_unregister(&ts5500_adc_driver);
> > +error_out:
> > + return ret;
> > +}
> > +module_init(ts5500_adc_init);
> > +
> > +static void __exit ts5500_adc_exit(void)
> > +{
> > + platform_driver_unregister(&ts5500_adc_driver);
> > + platform_device_unregister(&ts5500_adc_device);
> > +}
> > +module_exit(ts5500_adc_exit);
> > +
> > +MODULE_DESCRIPTION("TS-5500 mapped MAX197 ADC device driver");
> > +MODULE_AUTHOR("Jonas Fonseca
> > <jonas.fonseca@savoirfairelinux.com>"); +MODULE_LICENSE("GPL");
> > --
> > 1.7.6.5
> >
> >
A patch for this review will follow.
Regards,
Vivien.
--
Vivien Didelot
Savoir-faire Linux Inc.
Tel: (514) 276-5468 #149
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH] x86/platform: (TS-5500) revised ADC driver
2012-01-18 23:52 ` [PATCH v4 4/4] x86/platform: (TS-5500) add ADC support Vivien Didelot
2012-01-19 2:55 ` Guenter Roeck
@ 2012-01-20 23:43 ` Vivien Didelot
2012-01-21 4:46 ` Guenter Roeck
1 sibling, 1 reply; 18+ messages in thread
From: Vivien Didelot @ 2012-01-20 23:43 UTC (permalink / raw)
To: x86
Cc: Vivien Didelot, Ingo Molnar, Thomas Gleixner, H. Peter Anvin,
linux-kernel, Guenter Roeck
Update of the ADC driver according to the Guenter Roeck's review.
Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
arch/x86/platform/ts5500/ts5500_adc.c | 228 +++++++++++++--------------------
1 files changed, 89 insertions(+), 139 deletions(-)
diff --git a/arch/x86/platform/ts5500/ts5500_adc.c b/arch/x86/platform/ts5500/ts5500_adc.c
index fc4560f..da6decf 100644
--- a/arch/x86/platform/ts5500/ts5500_adc.c
+++ b/arch/x86/platform/ts5500/ts5500_adc.c
@@ -62,57 +62,62 @@
#define TS5500_ADC_READ_DELAY 15 /* usec */
#define TS5500_ADC_READ_BUSY_MASK 0x01
-#define TS5500_ADC_NAME "MAX197 (8 channels)"
+#define TS5500_ADC_NAME "TS-5500 mapped MAX197 (8 channels)"
-/**
- * struct ts5500_adc_platform_data
- * @name: Name of the device.
- * @ioaddr: I/O address containing:
- * .data: Data register for conversion reading.
- * .ctrl: A/D Control Register (bit 0 = 0 when
- * conversion completed).
- * @read: Information about conversion reading, with:
- * .delay: Delay before next conversion.
- * .busy_mask: Control register bit 0 equals 1 means
- * conversion is not completed yet.
- * @ctrl: Data tables addressable by [polarity][range].
- * @ranges: Ranges.
- * .min: Min value.
- * .max: Max value.
- * @scale: Polarity/Range coefficients to scale raw conversion reading.
- */
-struct ts5500_adc_platform_data {
- const char *name;
- struct {
- int data;
- int ctrl;
- } ioaddr;
- struct {
- u8 delay;
- u8 busy_mask;
- } read;
- u8 ctrl[2][2];
- struct {
- int min[2][2];
- int max[2][2];
- } ranges;
- int scale[2][2];
+/* Data tables addressable by [polarity][range] */
+static const u8 ts5500_adc_ctrl[2][2] = {
+ {
+ TS5500_ADC_UNIPOLAR | TS5500_ADC_RANGE_5V,
+ TS5500_ADC_UNIPOLAR | TS5500_ADC_RANGE_10V
+ },
+ {
+ TS5500_ADC_BIPOLAR | TS5500_ADC_RANGE_5V,
+ TS5500_ADC_BIPOLAR | TS5500_ADC_RANGE_10V
+ }
+};
+
+/* Min/Max ranges */
+#define TS5500_ADC_RANGES_MIN_UNIPOLAR_5V 0
+#define TS5500_ADC_RANGES_MIN_UNIPOLAR_10V 0
+#define TS5500_ADC_RANGES_MIN_BIPOLAR_5V -5000
+#define TS5500_ADC_RANGES_MIN_BIPOLAR_10V -10000
+
+static const int ts5500_adc_ranges_min[2][2] = {
+ { TS5500_ADC_RANGES_MIN_UNIPOLAR_5V, TS5500_ADC_RANGES_MIN_UNIPOLAR_10V },
+ { TS5500_ADC_RANGES_MIN_BIPOLAR_5V, TS5500_ADC_RANGES_MIN_BIPOLAR_10V }
+};
+
+#define TS5500_ADC_RANGES_MAX_UNIPOLAR_5V 5000
+#define TS5500_ADC_RANGES_MAX_UNIPOLAR_10V 10000
+#define TS5500_ADC_RANGES_MAX_BIPOLAR_5V 5000
+#define TS5500_ADC_RANGES_MAX_BIPOLAR_10V 10000
+
+static const int ts5500_adc_ranges_max[2][2] = {
+ { TS5500_ADC_RANGES_MAX_UNIPOLAR_5V, TS5500_ADC_RANGES_MAX_UNIPOLAR_10V },
+ { TS5500_ADC_RANGES_MAX_BIPOLAR_5V, TS5500_ADC_RANGES_MAX_BIPOLAR_10V }
};
-#define ts5500_adc_test_bit(bit, map) (test_bit(bit, map) != 0)
+/* Polarity/Range coefficients to scale raw conversion reading */
+#define TS5500_ADC_SCALE_UNIPOLAR_5V 12207
+#define TS5500_ADC_SCALE_UNIPOLAR_10V 24414
+#define TS5500_ADC_SCALE_BIPOLAR_5V 24414
+#define TS5500_ADC_SCALE_BIPOLAR_10V 48828
+
+static const int ts5500_adc_scales[2][2] = {
+ { TS5500_ADC_SCALE_UNIPOLAR_5V, TS5500_ADC_SCALE_UNIPOLAR_10V },
+ { TS5500_ADC_SCALE_BIPOLAR_5V, TS5500_ADC_SCALE_BIPOLAR_10V }
+};
/**
* struct ts5500_adc_chip
* @hwmon_dev: The hwmon device.
* @lock: Read/Write mutex.
- * @spec: The mapped MAX197 platform data.
* @polarity: bitmap for polarity.
* @range: bitmap for range.
*/
struct ts5500_adc_chip {
struct device *hwmon_dev;
struct mutex lock;
- struct ts5500_adc_platform_data spec;
DECLARE_BITMAP(polarity, TS5500_ADC_CHANNELS_MAX);
DECLARE_BITMAP(range, TS5500_ADC_CHANNELS_MAX);
};
@@ -122,7 +127,7 @@ static s32 ts5500_adc_scale(struct ts5500_adc_chip *chip, s16 raw,
{
s32 scaled = raw;
- scaled *= chip->spec.scale[polarity][range];
+ scaled *= ts5500_adc_scales[polarity][range];
scaled /= 10000;
return scaled;
@@ -132,22 +137,8 @@ static int ts5500_adc_range(struct ts5500_adc_chip *chip, int is_min,
int polarity, int range)
{
if (is_min)
- return chip->spec.ranges.min[polarity][range];
- return chip->spec.ranges.max[polarity][range];
-}
-
-static int ts5500_adc_strtol(const char *buf, long *value, int range1,
- int range2)
-{
- if (strict_strtol(buf, 10, value))
- return -EINVAL;
-
- if (range1 < range2)
- *value = SENSORS_LIMIT(*value, range1, range2);
- else
- *value = SENSORS_LIMIT(*value, range2, range1);
-
- return 0;
+ return ts5500_adc_ranges_min[polarity][range];
+ return ts5500_adc_ranges_max[polarity][range];
}
static struct ts5500_adc_chip *ts5500_adc_get_drvdata(struct device *dev)
@@ -166,14 +157,14 @@ static ssize_t ts5500_adc_show_range(struct device *dev,
{
struct ts5500_adc_chip *chip = ts5500_adc_get_drvdata(dev);
struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
- int is_min = attr->nr != 0;
+ int is_min = !!attr->nr;
int polarity, range;
if (mutex_lock_interruptible(&chip->lock))
return -ERESTARTSYS;
- polarity = ts5500_adc_test_bit(attr->index, chip->polarity);
- range = ts5500_adc_test_bit(attr->index, chip->range);
+ polarity = !!test_bit(attr->index, chip->polarity);
+ range = !!test_bit(attr->index, chip->range);
mutex_unlock(&chip->lock);
@@ -198,9 +189,14 @@ static ssize_t ts5500_adc_store_range(struct device *dev,
int range2 = ts5500_adc_range(chip, is_min, 1, 1);
long value;
- if (ts5500_adc_strtol(buf, &value, range1, range2))
+ if (kstrtol(buf, 10, &value))
return -EINVAL;
+ if (range1 < range2)
+ value = SENSORS_LIMIT(value, range1, range2);
+ else
+ value = SENSORS_LIMIT(value, range2, range1);
+
if (mutex_lock_interruptible(&chip->lock))
return -ERESTARTSYS;
@@ -240,24 +236,23 @@ static ssize_t ts5500_adc_show_input(struct device *dev,
if (mutex_lock_interruptible(&chip->lock))
return -ERESTARTSYS;
- polarity = ts5500_adc_test_bit(attr->index, chip->polarity);
- range = ts5500_adc_test_bit(attr->index, chip->range);
+ polarity = !!test_bit(attr->index, chip->polarity);
+ range = !!test_bit(attr->index, chip->range);
- command = attr->index | chip->spec.ctrl[polarity][range];
+ command = attr->index | ts5500_adc_ctrl[polarity][range];
- outb(command, chip->spec.ioaddr.data);
+ outb(command, TS5500_ADC_INIT_LSB_REG);
- udelay(chip->spec.read.delay);
- ret = inb(chip->spec.ioaddr.ctrl);
+ udelay(TS5500_ADC_READ_DELAY);
+ ret = inb(TS5500_ADC_CTRL_REG);
- if (ret & chip->spec.read.busy_mask) {
- dev_err(dev, "device not ready (ret=0x0%x, try=%d)\n", ret,
- range);
+ if (ret & TS5500_ADC_READ_BUSY_MASK) {
+ dev_err(dev, "device not ready\n");
ret = -EIO;
} else {
/* LSB of conversion is at 0x196 and MSB is at 0x197 */
- u8 lsb = inb(chip->spec.ioaddr.data);
- u8 msb = inb(chip->spec.ioaddr.data + 1);
+ u8 lsb = inb(TS5500_ADC_INIT_LSB_REG);
+ u8 msb = inb(TS5500_ADC_INIT_LSB_REG + 1);
s16 raw = (msb << 8) | lsb;
s32 scaled = ts5500_adc_scale(chip, raw, polarity, range);
@@ -271,18 +266,18 @@ static ssize_t ts5500_adc_show_input(struct device *dev,
static ssize_t ts5500_adc_show_name(struct device *dev,
struct device_attribute *devattr, char *buf)
{
- return sprintf(buf, "%s\n", ts5500_adc_get_drvdata(dev)->spec.name);
+ return sprintf(buf, "%s\n", TS5500_ADC_NAME);
}
-#define TS5500_ADC_HWMON_CHANNEL(chan) \
- SENSOR_DEVICE_ATTR(in##chan##_input, S_IRUGO, \
- ts5500_adc_show_input, NULL, chan); \
- SENSOR_DEVICE_ATTR_2(in##chan##_max, S_IRUGO | S_IWUSR, \
- ts5500_adc_show_range, \
- ts5500_adc_store_range, 0, chan); \
- SENSOR_DEVICE_ATTR_2(in##chan##_min, S_IRUGO | S_IWUSR, \
- ts5500_adc_show_range, \
- ts5500_adc_store_range, 1, chan) \
+#define TS5500_ADC_HWMON_CHANNEL(chan) \
+ static SENSOR_DEVICE_ATTR(in##chan##_input, S_IRUGO, \
+ ts5500_adc_show_input, NULL, chan); \
+ static SENSOR_DEVICE_ATTR_2(in##chan##_max, S_IRUGO | S_IWUSR, \
+ ts5500_adc_show_range, \
+ ts5500_adc_store_range, 0, chan); \
+ static SENSOR_DEVICE_ATTR_2(in##chan##_min, S_IRUGO | S_IWUSR, \
+ ts5500_adc_show_range, \
+ ts5500_adc_store_range, 1, chan)
#define TS5500_ADC_SYSFS_CHANNEL(chan) \
&sensor_dev_attr_in##chan##_input.dev_attr.attr, \
@@ -291,14 +286,14 @@ static ssize_t ts5500_adc_show_name(struct device *dev,
static DEVICE_ATTR(name, S_IRUGO, ts5500_adc_show_name, NULL);
-static TS5500_ADC_HWMON_CHANNEL(0);
-static TS5500_ADC_HWMON_CHANNEL(1);
-static TS5500_ADC_HWMON_CHANNEL(2);
-static TS5500_ADC_HWMON_CHANNEL(3);
-static TS5500_ADC_HWMON_CHANNEL(4);
-static TS5500_ADC_HWMON_CHANNEL(5);
-static TS5500_ADC_HWMON_CHANNEL(6);
-static TS5500_ADC_HWMON_CHANNEL(7);
+TS5500_ADC_HWMON_CHANNEL(0);
+TS5500_ADC_HWMON_CHANNEL(1);
+TS5500_ADC_HWMON_CHANNEL(2);
+TS5500_ADC_HWMON_CHANNEL(3);
+TS5500_ADC_HWMON_CHANNEL(4);
+TS5500_ADC_HWMON_CHANNEL(5);
+TS5500_ADC_HWMON_CHANNEL(6);
+TS5500_ADC_HWMON_CHANNEL(7);
static const struct attribute_group ts5500_adc_sysfs_group = {
.attrs = (struct attribute *[]) {
@@ -317,44 +312,34 @@ static const struct attribute_group ts5500_adc_sysfs_group = {
static int __devinit ts5500_adc_probe(struct platform_device *pdev)
{
- struct ts5500_adc_platform_data *pdata = pdev->dev.platform_data;
struct ts5500_adc_chip *chip;
int ret;
- if (pdata == NULL)
- return -ENODEV;
-
chip = kzalloc(sizeof *chip, GFP_KERNEL);
if (!chip)
return -ENOMEM;
- chip->spec = *pdata;
-
mutex_init(&chip->lock);
- mutex_lock(&chip->lock);
+ platform_set_drvdata(pdev, chip);
ret = sysfs_create_group(&pdev->dev.kobj, &ts5500_adc_sysfs_group);
if (ret) {
- dev_err(&pdev->dev, "sysfs_create_group failed.\n");
- goto error_unlock_and_free;
+ dev_err(&pdev->dev, "sysfs_create_group failed\n");
+ goto error_free;
}
chip->hwmon_dev = hwmon_device_register(&pdev->dev);
if (IS_ERR(chip->hwmon_dev)) {
- dev_err(&pdev->dev, "hwmon_device_register failed.\n");
+ dev_err(&pdev->dev, "hwmon_device_register failed\n");
ret = PTR_ERR(chip->hwmon_dev);
goto error_unregister_device;
}
- platform_set_drvdata(pdev, chip);
- mutex_unlock(&chip->lock);
return 0;
error_unregister_device:
sysfs_remove_group(&pdev->dev.kobj, &ts5500_adc_sysfs_group);
-
-error_unlock_and_free:
- mutex_unlock(&chip->lock);
+error_free:
kfree(chip);
return ret;
}
@@ -379,58 +364,23 @@ static struct resource ts5500_adc_resources[] = {
}
};
-static struct ts5500_adc_platform_data ts5500_adc_platform_data = {
- .name = TS5500_ADC_NAME,
- .ioaddr = {
- .data = TS5500_ADC_INIT_LSB_REG,
- .ctrl = TS5500_ADC_CTRL_REG,
- },
- .read = {
- .delay = TS5500_ADC_READ_DELAY,
- .busy_mask = TS5500_ADC_READ_BUSY_MASK,
- },
- .ctrl = {
- { TS5500_ADC_UNIPOLAR | TS5500_ADC_RANGE_5V,
- TS5500_ADC_UNIPOLAR | TS5500_ADC_RANGE_10V },
- { TS5500_ADC_BIPOLAR | TS5500_ADC_RANGE_5V,
- TS5500_ADC_BIPOLAR | TS5500_ADC_RANGE_10V },
- },
- .ranges = {
- .min = {
- { 0, 0 },
- { -5000, -10000 },
- },
- .max = {
- { 5000, 10000 },
- { 5000, 10000 },
- },
- },
- .scale = {
- { 12207, 24414 },
- { 24414, 48828 },
- },
-};
-
static struct platform_device ts5500_adc_device = {
.name = "ts5500_adc",
.id = -1,
.resource = ts5500_adc_resources,
.num_resources = ARRAY_SIZE(ts5500_adc_resources),
.dev = {
- .platform_data = &ts5500_adc_platform_data,
- .release = ts5500_adc_release,
- },
+ .release = ts5500_adc_release
+ }
};
static int __devexit ts5500_adc_remove(struct platform_device *pdev)
{
struct ts5500_adc_chip *chip = platform_get_drvdata(pdev);
- mutex_lock(&chip->lock);
hwmon_device_unregister(chip->hwmon_dev);
sysfs_remove_group(&pdev->dev.kobj, &ts5500_adc_sysfs_group);
platform_set_drvdata(pdev, NULL);
- mutex_unlock(&chip->lock);
kfree(chip);
return 0;
--
1.7.6.5
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v4 4/4] x86/platform: (TS-5500) add ADC support
2012-01-20 23:41 ` Vivien Didelot
@ 2012-01-21 0:09 ` Guenter Roeck
0 siblings, 0 replies; 18+ messages in thread
From: Guenter Roeck @ 2012-01-21 0:09 UTC (permalink / raw)
To: Vivien Didelot
Cc: x86, Jonas Fonseca, Ingo Molnar, Thomas Gleixner, H. Peter Anvin,
linux-kernel, lm-sensors
Vivien,
On Fri, Jan 20, 2012 at 06:41:50PM -0500, Vivien Didelot wrote:
> Hi Guenter,
>
> Thanks for the code review.
>
> On Wed, 18 Jan 2012 18:55:49
> -0800, Guenter Roeck <guenter.roeck@ericsson.com> wrote:
>
> > On Wed, Jan 18, 2012 at 06:52:11PM -0500, Vivien Didelot wrote:
> > > From: Jonas Fonseca <jonas.fonseca@savoirfairelinux.com>
> > >
> > > Signed-off-by: Jonas Fonseca <jonas.fonseca@savoirfairelinux.com>
> > > Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> > > ---
> > > arch/x86/platform/ts5500/Kconfig | 7 +
> > > arch/x86/platform/ts5500/Makefile | 1 +
> > > arch/x86/platform/ts5500/ts5500_adc.c | 478
> > > +++++++++++++++++++++++++++++++++ 3 files changed, 486
> > > insertions(+), 0 deletions(-) create mode 100644
> > > arch/x86/platform/ts5500/ts5500_adc.c
> > >
> > I can not comment about the other drivers, but this is a hwmon
> > driver, and thus should reside in drivers/hwmon and be reviewed and
> > handled on the hwmon mailing list (copied).
>
> The ADC is a MAX197, but it is totally abstracted by the TS-5500 CPLD.
> The device (and its driver) is then really specific to the TS-5500
> platform (like the other devices). So the driver should be in its
> platform directory.
>
I disagree. There are lots of other platform specific hwmon drivers in the hwmon directory.
If there is a new upstream policy to move such drivers into platform specific directories,
please point me to it (or someone else point me to it, please). I don't think that would
be supportable, though, since subsystem maintainers will hardly ever know about such drivers,
and a lot of bad code could get in. It was more or less accidential that I noticed this one.
In addition to that, it would be an easy way to bypass the review process and sneak in drivers
in unrelated directories. We would then be stuck having to support drivers we never
had a chance to review - and just saying "sorry, not my problem" doesn't seem to be the
right way either.
> >
> > Couple of additional comments below; just some things I noticed, not
> > a complete review.
> >
> > > diff --git a/arch/x86/platform/ts5500/Kconfig
> > > b/arch/x86/platform/ts5500/Kconfig index 76f777f..f1a5f1d 100644
> > > --- a/arch/x86/platform/ts5500/Kconfig
> > > +++ b/arch/x86/platform/ts5500/Kconfig
> > > @@ -20,3 +20,10 @@ config TS5500_LED
> > > default y
> > > help
> > > This option enables support for the on-chip LED.
> > > +
> > > +config TS5500_ADC
> > > + tristate "TS-5500 ADC"
> > > + depends on TS5500 && HWMON=y
> > > + default y
> > > + help
> > > + Support for the A/D converter on Technologic Systems TS-5500
> > > SBCs. diff --git a/arch/x86/platform/ts5500/Makefile
> > > b/arch/x86/platform/ts5500/Makefile index 88eccc9..dcf46d8 100644
> > > --- a/arch/x86/platform/ts5500/Makefile
> > > +++ b/arch/x86/platform/ts5500/Makefile
> > > @@ -1,3 +1,4 @@
> > > obj-$(CONFIG_TS5500) += ts5500.o
> > > obj-$(CONFIG_TS5500_GPIO) += ts5500_gpio.o
> > > obj-$(CONFIG_TS5500_LED) += ts5500_led.o
> > > +obj-$(CONFIG_TS5500_ADC) += ts5500_adc.o
> > > diff --git a/arch/x86/platform/ts5500/ts5500_adc.c
> > > b/arch/x86/platform/ts5500/ts5500_adc.c new file mode 100644
> > > index 0000000..fc4560f
> > > --- /dev/null
> > > +++ b/arch/x86/platform/ts5500/ts5500_adc.c
> > > @@ -0,0 +1,478 @@
> > > +/*
> > > + * Technologic Systems TS-5500 boards - Mapped MAX197 ADC driver
> > > + *
> > > + * Copyright (c) 2010-2012 Savoir-faire Linux Inc.
> > > + * Jonas Fonseca <jonas.fonseca@savoirfairelinux.com>
> > > + *
> > > + * Portions Copyright (C) 2008 Marc Pignat <marc.pignat@hevs.ch>
> > > + *
> > > + * The driver uses direct access for communication with the ADC.
> > > + * Should work unchanged with the MAX199 chip.
> > > + *
> > > + * This program is free software; you can redistribute it and/or
> > > modify
> > > + * it under the terms of the GNU General Public License as
> > > published by
> > > + * the Free Software Foundation; either version 2 of the License,
> > > or
> > > + * (at your option) any later version.
> > > + *
> > > + * This program is distributed in the hope that it will be useful,
> > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > > + * GNU General Public License for more details.
> > > + *
> > > + * You should have received a copy of the GNU General Public
> > > License
> > > + * along with this program; if not, write to the Free Software
> > > + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> > > + *
> > > + * The TS-5500 uses a CPLD to abstract the interface to a MAX197.
> > > + */
> > > +
> > > +#include <linux/init.h>
> > > +#include <linux/module.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/device.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/err.h>
> > > +#include <linux/sysfs.h>
> > > +#include <linux/hwmon.h>
> > > +#include <linux/hwmon-sysfs.h>
> > > +#include <linux/mutex.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/io.h>
> > > +#include "ts5500.h"
> > > +
> > > +#define TS5500_ADC_CTRL_REG 0x195 /*
> > > Conversion state register */ +#define
> > > TS5500_ADC_INIT_LSB_REG 0x196 /* Init conv. /
> > > LSB register */ +#define TS5500_ADC_MSB_REG
> > > 0x197 /* MSB register */ +/*
> > > + * Control bits of A/D command
> > > + * bits 0-2: selected channel (0 - 7)
> > > + * bits 3: uni/bipolar (0 = unipolar (ie 0 to +5V))
> > > + * (1 = bipolar (ie -5 to +5V))
> > > + * bit 4: selected range (0 = 5v range, 1 = 10V range)
> > > + * bit 5-7: padded zero (unused)
> > > + */
> > > +
> > > +#define TS5500_ADC_CHANNELS_MAX 8 /* 0 to 7
> > > channels on device */ +
> > > +#define TS5500_ADC_BIPOLAR 0x08
> > > +#define TS5500_ADC_UNIPOLAR 0x00
> > > +#define TS5500_ADC_RANGE_5V 0x00 /* 0 to 5V
> > > range */ +#define TS5500_ADC_RANGE_10V
> > > 0x10 /* 0 to 10V range */ +
> > > +#define TS5500_ADC_READ_DELAY 15 /* usec */
> > > +#define TS5500_ADC_READ_BUSY_MASK 0x01
> > > +#define TS5500_ADC_NAME "MAX197 (8
> > > channels)" +
> > > +/**
> > > + * struct ts5500_adc_platform_data
> > > + * @name: Name of the device.
> > > + * @ioaddr: I/O address containing:
> > > + * .data: Data register for
> > > conversion reading.
> > > + * .ctrl: A/D Control Register (bit
> > > 0 = 0 when
> > > + * conversion completed).
> > > + * @read: Information about conversion reading, with:
> > > + * .delay: Delay before next
> > > conversion.
> > > + * .busy_mask: Control register bit 0 equals
> > > 1 means
> > > + * conversion is not completed yet.
> > > + * @ctrl: Data tables addressable by [polarity][range].
> > > + * @ranges: Ranges.
> > > + * .min: Min value.
> > > + * .max: Max value.
> > > + * @scale: Polarity/Range coefficients to scale raw
> > > conversion reading.
> > > + */
> > > +struct ts5500_adc_platform_data {
> > > + const char *name;
> > > + struct {
> > > + int data;
> > > + int ctrl;
> > > + } ioaddr;
> > > + struct {
> > > + u8 delay;
> > > + u8 busy_mask;
> > > + } read;
> > > + u8 ctrl[2][2];
> >
> > const ?
> >
> > > + struct {
> > > + int min[2][2];
> > > + int max[2][2];
> >
> > Should those be const ?
> >
> > > + } ranges;
> > > + int scale[2][2];
> >
> > const ?
> >
> > > +};
> > > +
> >
> > I am a bit lost about this structure and its use. It appears as if
> > you expect that there will be other uses besides the one defined here
> > (otherwise the variables would not make much sense and you could use
> > defines), yet it is all defined in this file and thus static. Please
> > elaborate.
>
> You're right. I removed this structure and used define instead.
>
> >
> > > +#define ts5500_adc_test_bit(bit, map) (test_bit(bit,
> > > map) != 0) +
> > Why "!= 0" ? Isn't that implied ? And then why the define to start
> > with ?
>
> Dropped. I replaced ts5500_adc_test_bit() call by !!test_bit().
>
Tricky, tricky. You want a bool which you then use to index an array, meaning you use
the bool as integer. And a few implementations unfortunately do not return a bool
but an int.
Maybe it would be better if you were to declare range and polarity as bool wherever used.
At least then it would be more obvious that this is what you are doing.
> >
> > > +/**
> > > + * struct ts5500_adc_chip
> > > + * @hwmon_dev: The hwmon device.
> > > + * @lock: Read/Write mutex.
> > > + * @spec: The mapped MAX197 platform data.
> > > + * @polarity: bitmap for polarity.
> > > + * @range: bitmap for range.
> > > + */
> > > +struct ts5500_adc_chip {
> > > + struct device *hwmon_dev;
> > > + struct mutex lock;
> > > + struct ts5500_adc_platform_data spec;
> > > + DECLARE_BITMAP(polarity, TS5500_ADC_CHANNELS_MAX);
> > > + DECLARE_BITMAP(range, TS5500_ADC_CHANNELS_MAX);
> > > +};
> > > +
> > > +static s32 ts5500_adc_scale(struct ts5500_adc_chip *chip, s16 raw,
> > > + int polarity, int range)
> > > +{
> > > + s32 scaled = raw;
> > > +
> > > + scaled *= chip->spec.scale[polarity][range];
> > > + scaled /= 10000;
> > > +
> > > + return scaled;
> > > +}
> > > +
> > > +static int ts5500_adc_range(struct ts5500_adc_chip *chip, int
> > > is_min,
> > > + int polarity, int range)
> > > +{
> > > + if (is_min)
> > > + return chip->spec.ranges.min[polarity][range];
> > > + return chip->spec.ranges.max[polarity][range];
> > > +}
> > > +
> > > +static int ts5500_adc_strtol(const char *buf, long *value, int
> > > range1,
> > > + int range2)
> > > +{
> > > + if (strict_strtol(buf, 10, value))
> >
> > checkpatch warning.
> >
> > > + return -EINVAL;
> > > +
> > > + if (range1 < range2)
> > > + *value = SENSORS_LIMIT(*value, range1, range2);
> > > + else
> > > + *value = SENSORS_LIMIT(*value, range2, range1);
> > > +
> > > + return 0;
> > > +}
> > This function is called exactly once. Why not just embed it in the
> > calling code ?
> >
> > > +
> > > +static struct ts5500_adc_chip *ts5500_adc_get_drvdata(struct
> > > device *dev) +{
> > > + return platform_get_drvdata(to_platform_device(dev));
> > > +}
> > > +
> > > +/**
> > > + * ts5500_adc_show_range() - Display range on user output
> > > + *
> > > + * Function called on read access on
> > > + * /sys/devices/platform/ts5500-adc/in{0,1,2,3,4,5,6,7}_{min,max}
> > > + */
> > > +static ssize_t ts5500_adc_show_range(struct device *dev,
> > > + struct device_attribute *devattr,
> > > char *buf) +{
> > > + struct ts5500_adc_chip *chip = ts5500_adc_get_drvdata(dev);
> > > + struct sensor_device_attribute_2 *attr =
> > > to_sensor_dev_attr_2(devattr);
> > > + int is_min = attr->nr != 0;
> > > + int polarity, range;
> > > +
> > > + if (mutex_lock_interruptible(&chip->lock))
> > > + return -ERESTARTSYS;
> > > +
> > > + polarity = ts5500_adc_test_bit(attr->index,
> > > chip->polarity);
> > > + range = ts5500_adc_test_bit(attr->index, chip->range);
> > > +
> > > + mutex_unlock(&chip->lock);
> > > +
> > > + return sprintf(buf, "%d\n",
> > > + ts5500_adc_range(chip, is_min, polarity,
> > > range)); +}
> > > +
> > > +/**
> > > + * ts5500_adc_store_range() - Write range from user input
> > > + *
> > > + * Function called on write access on
> > > + * /sys/devices/platform/ts5500-adc/in{0,1,2,3,4,5,6,7}_{min,max}
> > > + */
> > > +static ssize_t ts5500_adc_store_range(struct device *dev,
> > > + struct device_attribute *devattr,
> > > + const char *buf, size_t count)
> > > +{
> > > + struct ts5500_adc_chip *chip = ts5500_adc_get_drvdata(dev);
> > > + struct sensor_device_attribute_2 *attr =
> > > to_sensor_dev_attr_2(devattr);
> > > + int is_min = attr->nr != 0;
> > > + int range1 = ts5500_adc_range(chip, is_min, 0, 0);
> > > + int range2 = ts5500_adc_range(chip, is_min, 1, 1);
> > > + long value;
> > > +
> > > + if (ts5500_adc_strtol(buf, &value, range1, range2))
> > > + return -EINVAL;
> > > +
> > > + if (mutex_lock_interruptible(&chip->lock))
> > > + return -ERESTARTSYS;
> > > +
> > > + if (abs(value) > 5000)
> > > + set_bit(attr->index, chip->range);
> > > + else
> > > + clear_bit(attr->index, chip->range);
> > > +
> > > + if (is_min) {
> > > + if (value < 0)
> > > + set_bit(attr->index, chip->polarity);
> > > + else
> > > + clear_bit(attr->index, chip->polarity);
> > > + }
> > > +
> > > + mutex_unlock(&chip->lock);
> > > +
> > > + return count;
> > > +}
> > > +
> > > +/**
> > > + * ts5500_adc_show_input() - Show channel input
> > > + *
> > > + * Function called on read access on
> > > + * /sys/devices/platform/ts5500-adc/in{0,1,2,3,4,5,6,7}_input
> > > + */
> > > +static ssize_t ts5500_adc_show_input(struct device *dev,
> > > + struct device_attribute
> > > *devattr,
> > > + char *buf)
> > > +{
> > > + struct ts5500_adc_chip *chip = ts5500_adc_get_drvdata(dev);
> > > + struct sensor_device_attribute *attr =
> > > to_sensor_dev_attr(devattr);
> > > + int polarity, range;
> > > + int ret;
> > > + u8 command;
> > > +
> > > + if (mutex_lock_interruptible(&chip->lock))
> > > + return -ERESTARTSYS;
> > > +
> > > + polarity = ts5500_adc_test_bit(attr->index,
> > > chip->polarity);
> > > + range = ts5500_adc_test_bit(attr->index, chip->range);
> > > +
> > > + command = attr->index | chip->spec.ctrl[polarity][range];
> > > +
> > > + outb(command, chip->spec.ioaddr.data);
> > > +
> > > + udelay(chip->spec.read.delay);
> > > + ret = inb(chip->spec.ioaddr.ctrl);
> > > +
> > > + if (ret & chip->spec.read.busy_mask) {
> > > + dev_err(dev, "device not ready (ret=0x0%x,
> > > try=%d)\n", ret,
> > > + range);
> >
> > What does "try=" have to do with the displayed "range" ?
> >
> > > + ret = -EIO;
> > > + } else {
> > > + /* LSB of conversion is at 0x196 and MSB is at
> > > 0x197 */
> > > + u8 lsb = inb(chip->spec.ioaddr.data);
> > > + u8 msb = inb(chip->spec.ioaddr.data + 1);
> > > + s16 raw = (msb << 8) | lsb;
> > > + s32 scaled = ts5500_adc_scale(chip, raw, polarity,
> > > range); +
> > > + ret = sprintf(buf, "%d\n", scaled);
> > > + }
> > > +
> > > + mutex_unlock(&chip->lock);
> > > + return ret;
> > > +}
> > > +
> > > +static ssize_t ts5500_adc_show_name(struct device *dev,
> > > + struct device_attribute *devattr, char *buf)
> > > +{
> > > + return sprintf(buf, "%s\n",
> > > ts5500_adc_get_drvdata(dev)->spec.name); +}
> > > +
> > > +#define
> > > TS5500_ADC_HWMON_CHANNEL(chan) \
> > > + SENSOR_DEVICE_ATTR(in##chan##_input,
> > > S_IRUGO, \
> > > + ts5500_adc_show_input, NULL,
> > > chan); \
> > > + SENSOR_DEVICE_ATTR_2(in##chan##_max, S_IRUGO |
> > > S_IWUSR, \
> > > + ts5500_adc_show_range,
> > > \
> > > + ts5500_adc_store_range, 0,
> > > chan); \
> > > + SENSOR_DEVICE_ATTR_2(in##chan##_min, S_IRUGO |
> > > S_IWUSR, \
> > > + ts5500_adc_show_range,
> > > \
> > > + ts5500_adc_store_range, 1,
> > > chan) \ +
> > > +#define
> > > TS5500_ADC_SYSFS_CHANNEL(chan) \
> > > + &sensor_dev_attr_in##chan##_input.dev_attr.attr, \
> > > +
> > > &sensor_dev_attr_in##chan##_max.dev_attr.attr, \
> > > + &sensor_dev_attr_in##chan##_min.dev_attr.attr
> > > +
> > > +static DEVICE_ATTR(name, S_IRUGO, ts5500_adc_show_name, NULL);
> > > +
> > > +static TS5500_ADC_HWMON_CHANNEL(0);
> > > +static TS5500_ADC_HWMON_CHANNEL(1);
> > > +static TS5500_ADC_HWMON_CHANNEL(2);
> > > +static TS5500_ADC_HWMON_CHANNEL(3);
> > > +static TS5500_ADC_HWMON_CHANNEL(4);
> > > +static TS5500_ADC_HWMON_CHANNEL(5);
> > > +static TS5500_ADC_HWMON_CHANNEL(6);
> > > +static TS5500_ADC_HWMON_CHANNEL(7);
> > > +
> > Looks good at first glance, but unless I misunderstand something,
> > each define generates three structures, yet only the first of those
> > is declared static, the others are global.
>
> Good point, now everything's declared as static.
>
> >
> > > +static const struct attribute_group ts5500_adc_sysfs_group = {
> > > + .attrs = (struct attribute *[]) {
> >
> > checkpatch error.
>
> This is a bit tricky. checkpatch thinks that the asterisk is a
> multiplication, so it is complaining about coding style.
>
Hmm. YOu are right ... and by changing it you would actually add a coding style error.
Maybe just add a note in the version change log that this is a false positive.
> >
> > > + &dev_attr_name.attr,
> > > + TS5500_ADC_SYSFS_CHANNEL(0),
> > > + TS5500_ADC_SYSFS_CHANNEL(1),
> > > + TS5500_ADC_SYSFS_CHANNEL(2),
> > > + TS5500_ADC_SYSFS_CHANNEL(3),
> > > + TS5500_ADC_SYSFS_CHANNEL(4),
> > > + TS5500_ADC_SYSFS_CHANNEL(5),
> > > + TS5500_ADC_SYSFS_CHANNEL(6),
> > > + TS5500_ADC_SYSFS_CHANNEL(7),
> > > + NULL
> > > + }
> > > +};
> > > +
> > > +static int __devinit ts5500_adc_probe(struct platform_device *pdev)
> > > +{
> > > + struct ts5500_adc_platform_data *pdata =
> > > pdev->dev.platform_data;
> > > + struct ts5500_adc_chip *chip;
> > > + int ret;
> > > +
> > > + if (pdata == NULL)
> > > + return -ENODEV;
> > > +
> > > + chip = kzalloc(sizeof *chip, GFP_KERNEL);
> > > + if (!chip)
> > > + return -ENOMEM;
> > > +
> > > + chip->spec = *pdata;
> > > +
> > > + mutex_init(&chip->lock);
> > > + mutex_lock(&chip->lock);
> >
> > Probe function does not need a lock if you rearrange the code below.
> >
> > > +
> > > + ret = sysfs_create_group(&pdev->dev.kobj,
> > > &ts5500_adc_sysfs_group);
> > > + if (ret) {
> > > + dev_err(&pdev->dev, "sysfs_create_group
> > > failed.\n");
> > > + goto error_unlock_and_free;
> > > + }
> > > +
> > > + chip->hwmon_dev = hwmon_device_register(&pdev->dev);
> > > + if (IS_ERR(chip->hwmon_dev)) {
> > > + dev_err(&pdev->dev, "hwmon_device_register
> > > failed.\n");
> > > + ret = PTR_ERR(chip->hwmon_dev);
> > > + goto error_unregister_device;
> > > + }
> > > +
> > > + platform_set_drvdata(pdev, chip);
> >
> > Set this before creating sysfs entries.
>
> Thanks for the trick.
>
> >
> > > + mutex_unlock(&chip->lock);
> > > + return 0;
> > > +
> > > +error_unregister_device:
> > > + sysfs_remove_group(&pdev->dev.kobj,
> > > &ts5500_adc_sysfs_group); +
> > > +error_unlock_and_free:
> > > + mutex_unlock(&chip->lock);
> > > + kfree(chip);
> > > + return ret;
> > > +}
> > > +
> > > +static void ts5500_adc_release(struct device *dev)
> > > +{
> > > + /* noop */
> > > +}
> >
> > Is this really needed ? Just wondering.
>
> Yes it is, otherwise you get a "Device 'ts5500_adc' does not have a
> release() function, it is broken and must be fixed." kernel error.
>
> >
> > > +
> > > +static struct resource ts5500_adc_resources[] = {
> > > + {
> > > + .name = "ts5500_adc" "-data",
> > > + .start = TS5500_ADC_INIT_LSB_REG,
> > > + .end = TS5500_ADC_MSB_REG,
> > > + .flags = IORESOURCE_IO,
> > > + },
> > > + {
> > > + .name = "ts5500_adc" "-ctrl",
> > > + .start = TS5500_ADC_CTRL_REG,
> > > + .end = TS5500_ADC_CTRL_REG,
> > > + .flags = IORESOURCE_IO,
> > > + }
> > > +};
> > > +
> > > +static struct ts5500_adc_platform_data ts5500_adc_platform_data = {
> > > + .name = TS5500_ADC_NAME,
> > > + .ioaddr = {
> > > + .data = TS5500_ADC_INIT_LSB_REG,
> > > + .ctrl = TS5500_ADC_CTRL_REG,
> > > + },
> > > + .read = {
> > > + .delay = TS5500_ADC_READ_DELAY,
> > > + .busy_mask = TS5500_ADC_READ_BUSY_MASK,
> > > + },
> > > + .ctrl = {
> > > + { TS5500_ADC_UNIPOLAR | TS5500_ADC_RANGE_5V,
> > > + TS5500_ADC_UNIPOLAR | TS5500_ADC_RANGE_10V },
> > > + { TS5500_ADC_BIPOLAR | TS5500_ADC_RANGE_5V,
> > > + TS5500_ADC_BIPOLAR | TS5500_ADC_RANGE_10V },
> > > + },
> > > + .ranges = {
> > > + .min = {
> > > + { 0, 0 },
> > > + { -5000, -10000 },
> > > + },
> > > + .max = {
> > > + { 5000, 10000 },
> > > + { 5000, 10000 },
> > > + },
> > > + },
> > > + .scale = {
> > > + { 12207, 24414 },
> > > + { 24414, 48828 },
> > > + },
> > > +};
> > > +
> > > +static struct platform_device ts5500_adc_device = {
> > > + .name = "ts5500_adc",
> > > + .id = -1,
> > > + .resource = ts5500_adc_resources,
> > > + .num_resources = ARRAY_SIZE(ts5500_adc_resources),
> > > + .dev = {
> > > + .platform_data = &ts5500_adc_platform_data,
> > > + .release = ts5500_adc_release,
> > > + },
> > > +};
> > > +
> > Usually all the above would be in a platform file.
> >
> > > +static int __devexit ts5500_adc_remove(struct platform_device
> > > *pdev) +{
> > > + struct ts5500_adc_chip *chip = platform_get_drvdata(pdev);
> > > +
> > > + mutex_lock(&chip->lock);
> > > + hwmon_device_unregister(chip->hwmon_dev);
> > > + sysfs_remove_group(&pdev->dev.kobj,
> > > &ts5500_adc_sysfs_group);
> > > + platform_set_drvdata(pdev, NULL);
> > > + mutex_unlock(&chip->lock);
> >
> > Lock not needed here.
> >
> > > + kfree(chip);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static struct platform_driver ts5500_adc_driver = {
> > > + .driver = {
> > > + .name = "ts5500_adc",
> > > + .owner = THIS_MODULE,
> > > + },
> > > + .probe = ts5500_adc_probe,
> > > + .remove = __devexit_p(ts5500_adc_remove)
> > > +};
> > > +
> > > +static int __init ts5500_adc_init(void)
> > > +{
> > > + int ret;
> > > +
> > > + ret = platform_driver_register(&ts5500_adc_driver);
> > > + if (ret)
> > > + goto error_out;
> > > +
> > > + ret = platform_device_register(&ts5500_adc_device);
> > > + if (ret)
> > > + goto error_device_register;
> > > +
> > > + return 0;
> > > +
> > > +error_device_register:
> > > + platform_driver_unregister(&ts5500_adc_driver);
> > > +error_out:
> > > + return ret;
> > > +}
> > > +module_init(ts5500_adc_init);
> > > +
> > > +static void __exit ts5500_adc_exit(void)
> > > +{
> > > + platform_driver_unregister(&ts5500_adc_driver);
> > > + platform_device_unregister(&ts5500_adc_device);
> > > +}
> > > +module_exit(ts5500_adc_exit);
> > > +
> > > +MODULE_DESCRIPTION("TS-5500 mapped MAX197 ADC device driver");
> > > +MODULE_AUTHOR("Jonas Fonseca
> > > <jonas.fonseca@savoirfairelinux.com>"); +MODULE_LICENSE("GPL");
> > > --
> > > 1.7.6.5
> > >
> > >
>
> A patch for this review will follow.
>
> Regards,
>
> Vivien.
>
>
> --
> Vivien Didelot
> Savoir-faire Linux Inc.
> Tel: (514) 276-5468 #149
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] x86/platform: (TS-5500) revised ADC driver
2012-01-20 23:43 ` [PATCH] x86/platform: (TS-5500) revised ADC driver Vivien Didelot
@ 2012-01-21 4:46 ` Guenter Roeck
2012-01-23 1:36 ` Vivien Didelot
0 siblings, 1 reply; 18+ messages in thread
From: Guenter Roeck @ 2012-01-21 4:46 UTC (permalink / raw)
To: Vivien Didelot
Cc: x86, Ingo Molnar, Thomas Gleixner, H. Peter Anvin, linux-kernel
On Fri, Jan 20, 2012 at 06:43:22PM -0500, Vivien Didelot wrote:
> Update of the ADC driver according to the Guenter Roeck's review.
>
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> ---
> arch/x86/platform/ts5500/ts5500_adc.c | 228 +++++++++++++--------------------
> 1 files changed, 89 insertions(+), 139 deletions(-)
>
> diff --git a/arch/x86/platform/ts5500/ts5500_adc.c b/arch/x86/platform/ts5500/ts5500_adc.c
> index fc4560f..da6decf 100644
> --- a/arch/x86/platform/ts5500/ts5500_adc.c
> +++ b/arch/x86/platform/ts5500/ts5500_adc.c
> @@ -62,57 +62,62 @@
>
Hi Vivien,
This isn't really a revised driver ... it is a patch on top of the previous version.
Regarding the location, I'd really like to know from the powers-that-be if
arch/x86/platform/ts5500/
or
drivers/platform/x86
or
drivers/hwmon
would be the appropriate location for a driver like this. As mentioned before,
my strong preference is drivers/hwmon, but I would like to hear from others.
Also, I am not sure if the current approach is appropriate to start with.
Looking at the datasheet as well as into existing kernel code, it appears quite likely
that some kind of more or less generic MAX197 driver exists somewhere. The existence
of is_max197_installed() - without any calling code - is a strong indication that
this is the case, as well as the "static" platform data in your original patch.
It might be more appropriate to take this more or less generic driver, move it to
drivers/hwmon, and provide - for example through platform data - a means
to read from and write to the chip on a per-platform basis, ie with per-platform
access functions.
Thanks,
Guenter
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] x86/platform: (TS-5500) revised ADC driver
2012-01-21 4:46 ` Guenter Roeck
@ 2012-01-23 1:36 ` Vivien Didelot
2012-01-23 4:36 ` Guenter Roeck
0 siblings, 1 reply; 18+ messages in thread
From: Vivien Didelot @ 2012-01-23 1:36 UTC (permalink / raw)
To: Guenter Roeck
Cc: x86, Ingo Molnar, Thomas Gleixner, H. Peter Anvin, linux-kernel
Hi Guenter,
On Fri, 20 Jan 2012 20:46:31 -0800,
Guenter Roeck <guenter.roeck@ericsson.com> wrote:
> On Fri, Jan 20, 2012 at 06:43:22PM -0500, Vivien Didelot wrote:
> > Update of the ADC driver according to the Guenter Roeck's review.
> >
> > Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> > ---
> > arch/x86/platform/ts5500/ts5500_adc.c | 228
> > +++++++++++++-------------------- 1 files changed, 89
> > insertions(+), 139 deletions(-)
> >
> > diff --git a/arch/x86/platform/ts5500/ts5500_adc.c
> > b/arch/x86/platform/ts5500/ts5500_adc.c index fc4560f..da6decf
> > 100644 --- a/arch/x86/platform/ts5500/ts5500_adc.c
> > +++ b/arch/x86/platform/ts5500/ts5500_adc.c
> > @@ -62,57 +62,62 @@
> >
> Hi Vivien,
>
> This isn't really a revised driver ... it is a patch on top of the
> previous version.
True, sorry for the mistake.
> Regarding the location, I'd really like to know from the
> powers-that-be if arch/x86/platform/ts5500/
> or
> drivers/platform/x86
> or
> drivers/hwmon
>
> would be the appropriate location for a driver like this. As
> mentioned before, my strong preference is drivers/hwmon, but I would
> like to hear from others.
We should either split every driver into corresponding subdirectories,
or put everything in a common platform directory. My first RFC patches
set has every driver separated. As they are really specific to the
platform, people seem to agree with grouping them, mainly because they
won't be shared. I changed that in the following patches sets, and X86
maintainers seemed to be ok with that.
I'm ok with both solutions, but we should all agree on one.
Maybe we should have other maintainers view on this?
> Also, I am not sure if the current approach is appropriate to start
> with. Looking at the datasheet as well as into existing kernel code,
> it appears quite likely that some kind of more or less generic MAX197
> driver exists somewhere. The existence of is_max197_installed() -
> without any calling code - is a strong indication that this is the
> case, as well as the "static" platform data in your original patch.
> It might be more appropriate to take this more or less generic
> driver, move it to drivers/hwmon, and provide - for example through
> platform data - a means to read from and write to the chip on a
> per-platform basis, ie with per-platform access functions.
You're right, it should be possible to create a generic max197 driver
and provide read/write functions through platform data. But we don't
have a max197 right now... So it can stay as a compact TS-5500 ADC
driver for the moment, and maybe we will split later. What do you think?
Thanks,
-Vivien
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] x86/platform: (TS-5500) revised ADC driver
2012-01-23 1:36 ` Vivien Didelot
@ 2012-01-23 4:36 ` Guenter Roeck
2012-01-23 20:54 ` Vivien Didelot
0 siblings, 1 reply; 18+ messages in thread
From: Guenter Roeck @ 2012-01-23 4:36 UTC (permalink / raw)
To: Vivien Didelot
Cc: x86, Ingo Molnar, Thomas Gleixner, H. Peter Anvin, linux-kernel
On Sun, Jan 22, 2012 at 08:36:19PM -0500, Vivien Didelot wrote:
> Hi Guenter,
>
> On Fri, 20 Jan 2012 20:46:31 -0800,
> Guenter Roeck <guenter.roeck@ericsson.com> wrote:
>
> > On Fri, Jan 20, 2012 at 06:43:22PM -0500, Vivien Didelot wrote:
> > > Update of the ADC driver according to the Guenter Roeck's review.
> > >
> > > Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> > > ---
> > > arch/x86/platform/ts5500/ts5500_adc.c | 228
> > > +++++++++++++-------------------- 1 files changed, 89
> > > insertions(+), 139 deletions(-)
> > >
> > > diff --git a/arch/x86/platform/ts5500/ts5500_adc.c
> > > b/arch/x86/platform/ts5500/ts5500_adc.c index fc4560f..da6decf
> > > 100644 --- a/arch/x86/platform/ts5500/ts5500_adc.c
> > > +++ b/arch/x86/platform/ts5500/ts5500_adc.c
> > > @@ -62,57 +62,62 @@
> > >
> > Hi Vivien,
> >
> > This isn't really a revised driver ... it is a patch on top of the
> > previous version.
>
> True, sorry for the mistake.
>
> > Regarding the location, I'd really like to know from the
> > powers-that-be if arch/x86/platform/ts5500/
> > or
> > drivers/platform/x86
> > or
> > drivers/hwmon
> >
> > would be the appropriate location for a driver like this. As
> > mentioned before, my strong preference is drivers/hwmon, but I would
> > like to hear from others.
>
> We should either split every driver into corresponding subdirectories,
> or put everything in a common platform directory. My first RFC patches
> set has every driver separated. As they are really specific to the
> platform, people seem to agree with grouping them, mainly because they
> won't be shared. I changed that in the following patches sets, and X86
> maintainers seemed to be ok with that.
>
> I'm ok with both solutions, but we should all agree on one.
> Maybe we should have other maintainers view on this?
>
That is what I had asked for. I thought the whole point of per-module
directories was to have all drivers there. If that is no longer true,
fine with me; who am I to argue about something like that.
I'd just like to know.
> > Also, I am not sure if the current approach is appropriate to start
> > with. Looking at the datasheet as well as into existing kernel code,
> > it appears quite likely that some kind of more or less generic MAX197
> > driver exists somewhere. The existence of is_max197_installed() -
> > without any calling code - is a strong indication that this is the
> > case, as well as the "static" platform data in your original patch.
> > It might be more appropriate to take this more or less generic
> > driver, move it to drivers/hwmon, and provide - for example through
> > platform data - a means to read from and write to the chip on a
> > per-platform basis, ie with per-platform access functions.
>
> You're right, it should be possible to create a generic max197 driver
> and provide read/write functions through platform data. But we don't
> have a max197 right now... So it can stay as a compact TS-5500 ADC
> driver for the moment, and maybe we will split later. What do you think?
>
I am lost. If you don't have a TS-5500 with max197, how do you test
the driver ?
I had another look into the MAX197 and TS-5500 data sheets. In my opinion,
a generic MAX197 driver in drivers/hwmon combined with a platform
driver in the current location would be the way to go. That driver would
then also work for the other TS-5x00 systems. All you need is a single
chip access function in the platform code, since the chip is always accessed
with a write followed by a read.
If you don't have a system with max197 available to test the driver, you can
not test the current driver either, and there is nothing you can do anyway,
and you can as well wait with the adc code until you have one.
Thanks,
Guenter
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] x86/platform: (TS-5500) revised ADC driver
2012-01-23 4:36 ` Guenter Roeck
@ 2012-01-23 20:54 ` Vivien Didelot
2012-01-23 22:35 ` Vivien Didelot
0 siblings, 1 reply; 18+ messages in thread
From: Vivien Didelot @ 2012-01-23 20:54 UTC (permalink / raw)
To: Guenter Roeck
Cc: x86, Ingo Molnar, Thomas Gleixner, H. Peter Anvin, linux-kernel
On Sun, 22 Jan 2012 20:36:46 -0800,
Guenter Roeck <guenter.roeck@ericsson.com> wrote:
> > > Regarding the location, I'd really like to know from the
> > > powers-that-be if arch/x86/platform/ts5500/
> > > or
> > > drivers/platform/x86
> > > or
> > > drivers/hwmon
> > >
> > > would be the appropriate location for a driver like this. As
> > > mentioned before, my strong preference is drivers/hwmon, but I
> > > would like to hear from others.
> >
> > We should either split every driver into corresponding
> > subdirectories, or put everything in a common platform directory.
> > My first RFC patches set has every driver separated. As they are
> > really specific to the platform, people seem to agree with grouping
> > them, mainly because they won't be shared. I changed that in the
> > following patches sets, and X86 maintainers seemed to be ok with
> > that.
> >
> > I'm ok with both solutions, but we should all agree on one.
> > Maybe we should have other maintainers view on this?
> >
> That is what I had asked for. I thought the whole point of per-module
> directories was to have all drivers there. If that is no longer true,
> fine with me; who am I to argue about something like that.
> I'd just like to know.
>
> > > Also, I am not sure if the current approach is appropriate to
> > > start with. Looking at the datasheet as well as into existing
> > > kernel code, it appears quite likely that some kind of more or
> > > less generic MAX197 driver exists somewhere. The existence of
> > > is_max197_installed() - without any calling code - is a strong
> > > indication that this is the case, as well as the "static"
> > > platform data in your original patch. It might be more
> > > appropriate to take this more or less generic driver, move it to
> > > drivers/hwmon, and provide - for example through platform data -
> > > a means to read from and write to the chip on a per-platform
> > > basis, ie with per-platform access functions.
> >
> > You're right, it should be possible to create a generic max197
> > driver and provide read/write functions through platform data. But
> > we don't have a max197 right now... So it can stay as a compact
> > TS-5500 ADC driver for the moment, and maybe we will split later.
> > What do you think?
> >
> I am lost. If you don't have a TS-5500 with max197, how do you test
> the driver ?
Sorry, I wasn't clear. I meant the only max197 I have is the one
behind the TS-5500 CPLD, I don't have any others to test independently.
> I had another look into the MAX197 and TS-5500 data sheets. In my
> opinion, a generic MAX197 driver in drivers/hwmon combined with a
> platform driver in the current location would be the way to go. That
> driver would then also work for the other TS-5x00 systems. All you
> need is a single chip access function in the platform code, since the
> chip is always accessed with a write followed by a read.
I took a deeper look at the datasheets, and you're right, a MAX197
driver seems to be a good choice. However, there are a number of
differences between a direct usage of a MAX197 and the TS-5500 mapped
MAX197.
To start a conversion of a channel for a given range and polarity, it
consists on both sides of a u8 outb() call on pins 7-14 (i.e. bits
D7-D0). To be notified when the result is ready, we can either set an
IRQ on INT pin (falling edge), or poll it.
Then on the MAX197, you read the pins 7-14, set pin HBEN to 1, and
read the same pins again to get the 4 remaining bits. On the TS-5500,
only polling is available, and the 12 bits are mapped on 2 registers.
I propose to write a max197 driver with default read and write
functions. A platform_data will be used to specify the base address
(pins 7-14), and eventually a custom read function pointer, which will
be used instead of the default one if it is different of NULL.
What do you think?
I will write a max197 driver with default read and write functions. A
platform_data will be used to specify the base address (pins 7-14), and
eventually a custom read function pointer, which will be used instead
of the default, if it is not NULL.
What do you think?
--
Vivien Didelot
Savoir-faire Linux Inc.
Tel: (514) 276-5468 #149
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] x86/platform: (TS-5500) revised ADC driver
2012-01-23 20:54 ` Vivien Didelot
@ 2012-01-23 22:35 ` Vivien Didelot
2012-01-24 0:41 ` Guenter Roeck
0 siblings, 1 reply; 18+ messages in thread
From: Vivien Didelot @ 2012-01-23 22:35 UTC (permalink / raw)
To: khali
Cc: Vivien Didelot, Guenter Roeck, x86, Ingo Molnar, Thomas Gleixner,
H. Peter Anvin, linux-kernel, lm-sensors
On Mon, 23 Jan 2012 15:54:08 -0500,
Vivien Didelot <vivien.didelot@savoirfairelinux.com> wrote:
> On Sun, 22 Jan 2012 20:36:46 -0800,
> Guenter Roeck <guenter.roeck@ericsson.com> wrote:
> > > > Regarding the location, I'd really like to know from the
> > > > powers-that-be if arch/x86/platform/ts5500/
> > > > or
> > > > drivers/platform/x86
> > > > or
> > > > drivers/hwmon
> > > >
> > > > would be the appropriate location for a driver like this. As
> > > > mentioned before, my strong preference is drivers/hwmon, but I
> > > > would like to hear from others.
> > >
> > > We should either split every driver into corresponding
> > > subdirectories, or put everything in a common platform directory.
> > > My first RFC patches set has every driver separated. As they are
> > > really specific to the platform, people seem to agree with
> > > grouping them, mainly because they won't be shared. I changed
> > > that in the following patches sets, and X86 maintainers seemed to
> > > be ok with that.
> > >
> > > I'm ok with both solutions, but we should all agree on one.
> > > Maybe we should have other maintainers view on this?
> > >
> > That is what I had asked for. I thought the whole point of
> > per-module directories was to have all drivers there. If that is no
> > longer true, fine with me; who am I to argue about something like
> > that. I'd just like to know.
> >
> > > > Also, I am not sure if the current approach is appropriate to
> > > > start with. Looking at the datasheet as well as into existing
> > > > kernel code, it appears quite likely that some kind of more or
> > > > less generic MAX197 driver exists somewhere. The existence of
> > > > is_max197_installed() - without any calling code - is a strong
> > > > indication that this is the case, as well as the "static"
> > > > platform data in your original patch. It might be more
> > > > appropriate to take this more or less generic driver, move it to
> > > > drivers/hwmon, and provide - for example through platform data -
> > > > a means to read from and write to the chip on a per-platform
> > > > basis, ie with per-platform access functions.
> > >
> > > You're right, it should be possible to create a generic max197
> > > driver and provide read/write functions through platform data. But
> > > we don't have a max197 right now... So it can stay as a compact
> > > TS-5500 ADC driver for the moment, and maybe we will split later.
> > > What do you think?
> > >
> > I am lost. If you don't have a TS-5500 with max197, how do you test
> > the driver ?
>
> Sorry, I wasn't clear. I meant the only max197 I have is the one
> behind the TS-5500 CPLD, I don't have any others to test
> independently.
>
> > I had another look into the MAX197 and TS-5500 data sheets. In my
> > opinion, a generic MAX197 driver in drivers/hwmon combined with a
> > platform driver in the current location would be the way to go. That
> > driver would then also work for the other TS-5x00 systems. All you
> > need is a single chip access function in the platform code, since
> > the chip is always accessed with a write followed by a read.
>
> I took a deeper look at the datasheets, and you're right, a MAX197
> driver seems to be a good choice. However, there are a number of
> differences between a direct usage of a MAX197 and the TS-5500 mapped
> MAX197.
>
> To start a conversion of a channel for a given range and polarity, it
> consists on both sides of a u8 outb() call on pins 7-14 (i.e. bits
> D7-D0). To be notified when the result is ready, we can either set an
> IRQ on INT pin (falling edge), or poll it.
> Then on the MAX197, you read the pins 7-14, set pin HBEN to 1, and
> read the same pins again to get the 4 remaining bits. On the TS-5500,
> only polling is available, and the 12 bits are mapped on 2 registers.
>
> I propose to write a max197 driver with default read and write
> functions. A platform_data will be used to specify the base address
> (pins 7-14), and eventually a custom read function pointer, which will
> be used instead of the default one if it is different of NULL.
>
> What do you think?
>
> I will write a max197 driver with default read and write functions. A
> platform_data will be used to specify the base address (pins 7-14),
> and eventually a custom read function pointer, which will be used
> instead of the default, if it is not NULL.
>
> What do you think?
Sorry for the duplicate :)
BTW, I've added Jean Delvare and the lm-sensors mailing list in Cc, in
case they have an opinion on this.
Thanks,
--
Vivien Didelot
Savoir-faire Linux Inc.
Tel: (514) 276-5468 #149
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] x86/platform: (TS-5500) revised ADC driver
2012-01-23 22:35 ` Vivien Didelot
@ 2012-01-24 0:41 ` Guenter Roeck
2012-01-24 20:16 ` Vivien Didelot
0 siblings, 1 reply; 18+ messages in thread
From: Guenter Roeck @ 2012-01-24 0:41 UTC (permalink / raw)
To: Vivien Didelot
Cc: khali, x86, Ingo Molnar, Thomas Gleixner, H. Peter Anvin,
linux-kernel, lm-sensors
On Mon, Jan 23, 2012 at 05:35:51PM -0500, Vivien Didelot wrote:
> On Mon, 23 Jan 2012 15:54:08 -0500,
> Vivien Didelot <vivien.didelot@savoirfairelinux.com> wrote:
>
> > On Sun, 22 Jan 2012 20:36:46 -0800,
> > Guenter Roeck <guenter.roeck@ericsson.com> wrote:
> > > > > Regarding the location, I'd really like to know from the
> > > > > powers-that-be if arch/x86/platform/ts5500/
> > > > > or
> > > > > drivers/platform/x86
> > > > > or
> > > > > drivers/hwmon
> > > > >
> > > > > would be the appropriate location for a driver like this. As
> > > > > mentioned before, my strong preference is drivers/hwmon, but I
> > > > > would like to hear from others.
> > > >
> > > > We should either split every driver into corresponding
> > > > subdirectories, or put everything in a common platform directory.
> > > > My first RFC patches set has every driver separated. As they are
> > > > really specific to the platform, people seem to agree with
> > > > grouping them, mainly because they won't be shared. I changed
> > > > that in the following patches sets, and X86 maintainers seemed to
> > > > be ok with that.
> > > >
> > > > I'm ok with both solutions, but we should all agree on one.
> > > > Maybe we should have other maintainers view on this?
> > > >
> > > That is what I had asked for. I thought the whole point of
> > > per-module directories was to have all drivers there. If that is no
> > > longer true, fine with me; who am I to argue about something like
> > > that. I'd just like to know.
> > >
> > > > > Also, I am not sure if the current approach is appropriate to
> > > > > start with. Looking at the datasheet as well as into existing
> > > > > kernel code, it appears quite likely that some kind of more or
> > > > > less generic MAX197 driver exists somewhere. The existence of
> > > > > is_max197_installed() - without any calling code - is a strong
> > > > > indication that this is the case, as well as the "static"
> > > > > platform data in your original patch. It might be more
> > > > > appropriate to take this more or less generic driver, move it to
> > > > > drivers/hwmon, and provide - for example through platform data -
> > > > > a means to read from and write to the chip on a per-platform
> > > > > basis, ie with per-platform access functions.
> > > >
> > > > You're right, it should be possible to create a generic max197
> > > > driver and provide read/write functions through platform data. But
> > > > we don't have a max197 right now... So it can stay as a compact
> > > > TS-5500 ADC driver for the moment, and maybe we will split later.
> > > > What do you think?
> > > >
> > > I am lost. If you don't have a TS-5500 with max197, how do you test
> > > the driver ?
> >
> > Sorry, I wasn't clear. I meant the only max197 I have is the one
> > behind the TS-5500 CPLD, I don't have any others to test
> > independently.
> >
> > > I had another look into the MAX197 and TS-5500 data sheets. In my
> > > opinion, a generic MAX197 driver in drivers/hwmon combined with a
> > > platform driver in the current location would be the way to go. That
> > > driver would then also work for the other TS-5x00 systems. All you
> > > need is a single chip access function in the platform code, since
> > > the chip is always accessed with a write followed by a read.
> >
> > I took a deeper look at the datasheets, and you're right, a MAX197
> > driver seems to be a good choice. However, there are a number of
> > differences between a direct usage of a MAX197 and the TS-5500 mapped
> > MAX197.
> >
> > To start a conversion of a channel for a given range and polarity, it
> > consists on both sides of a u8 outb() call on pins 7-14 (i.e. bits
> > D7-D0). To be notified when the result is ready, we can either set an
> > IRQ on INT pin (falling edge), or poll it.
> > Then on the MAX197, you read the pins 7-14, set pin HBEN to 1, and
> > read the same pins again to get the 4 remaining bits. On the TS-5500,
> > only polling is available, and the 12 bits are mapped on 2 registers.
> >
> > I propose to write a max197 driver with default read and write
> > functions. A platform_data will be used to specify the base address
> > (pins 7-14), and eventually a custom read function pointer, which will
> > be used instead of the default one if it is different of NULL.
> >
> > What do you think?
> >
> > I will write a max197 driver with default read and write functions. A
> > platform_data will be used to specify the base address (pins 7-14),
> > and eventually a custom read function pointer, which will be used
> > instead of the default, if it is not NULL.
> >
> > What do you think?
>
Sounds like a plan to me.
> Sorry for the duplicate :)
>
> BTW, I've added Jean Delvare and the lm-sensors mailing list in Cc, in
> case they have an opinion on this.
>
Thanks,
Guenter
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] x86/platform: (TS-5500) revised ADC driver
2012-01-24 0:41 ` Guenter Roeck
@ 2012-01-24 20:16 ` Vivien Didelot
2012-01-24 21:20 ` Guenter Roeck
0 siblings, 1 reply; 18+ messages in thread
From: Vivien Didelot @ 2012-01-24 20:16 UTC (permalink / raw)
To: Guenter Roeck
Cc: khali, x86, Ingo Molnar, Thomas Gleixner, H. Peter Anvin,
linux-kernel, lm-sensors
Le Mon, 23 Jan 2012 16:41:38 -0800,
Guenter Roeck <guenter.roeck@ericsson.com> a écrit :
> On Mon, Jan 23, 2012 at 05:35:51PM -0500, Vivien Didelot wrote:
> > On Mon, 23 Jan 2012 15:54:08 -0500,
> > Vivien Didelot <vivien.didelot@savoirfairelinux.com> wrote:
> >
> > > On Sun, 22 Jan 2012 20:36:46 -0800,
> > > Guenter Roeck <guenter.roeck@ericsson.com> wrote:
> > > > > > Regarding the location, I'd really like to know from the
> > > > > > powers-that-be if arch/x86/platform/ts5500/
> > > > > > or
> > > > > > drivers/platform/x86
> > > > > > or
> > > > > > drivers/hwmon
> > > > > >
> > > > > > would be the appropriate location for a driver like this. As
> > > > > > mentioned before, my strong preference is drivers/hwmon,
> > > > > > but I would like to hear from others.
> > > > >
> > > > > We should either split every driver into corresponding
> > > > > subdirectories, or put everything in a common platform
> > > > > directory. My first RFC patches set has every driver
> > > > > separated. As they are really specific to the platform,
> > > > > people seem to agree with grouping them, mainly because they
> > > > > won't be shared. I changed that in the following patches
> > > > > sets, and X86 maintainers seemed to be ok with that.
> > > > >
> > > > > I'm ok with both solutions, but we should all agree on one.
> > > > > Maybe we should have other maintainers view on this?
> > > > >
> > > > That is what I had asked for. I thought the whole point of
> > > > per-module directories was to have all drivers there. If that
> > > > is no longer true, fine with me; who am I to argue about
> > > > something like that. I'd just like to know.
> > > >
> > > > > > Also, I am not sure if the current approach is appropriate
> > > > > > to start with. Looking at the datasheet as well as into
> > > > > > existing kernel code, it appears quite likely that some
> > > > > > kind of more or less generic MAX197 driver exists
> > > > > > somewhere. The existence of is_max197_installed() - without
> > > > > > any calling code - is a strong indication that this is the
> > > > > > case, as well as the "static" platform data in your
> > > > > > original patch. It might be more appropriate to take this
> > > > > > more or less generic driver, move it to drivers/hwmon, and
> > > > > > provide - for example through platform data - a means to
> > > > > > read from and write to the chip on a per-platform basis, ie
> > > > > > with per-platform access functions.
> > > > >
> > > > > You're right, it should be possible to create a generic max197
> > > > > driver and provide read/write functions through platform
> > > > > data. But we don't have a max197 right now... So it can stay
> > > > > as a compact TS-5500 ADC driver for the moment, and maybe we
> > > > > will split later. What do you think?
> > > > >
> > > > I am lost. If you don't have a TS-5500 with max197, how do you
> > > > test the driver ?
> > >
> > > Sorry, I wasn't clear. I meant the only max197 I have is the one
> > > behind the TS-5500 CPLD, I don't have any others to test
> > > independently.
> > >
> > > > I had another look into the MAX197 and TS-5500 data sheets. In
> > > > my opinion, a generic MAX197 driver in drivers/hwmon combined
> > > > with a platform driver in the current location would be the way
> > > > to go. That driver would then also work for the other TS-5x00
> > > > systems. All you need is a single chip access function in the
> > > > platform code, since the chip is always accessed with a write
> > > > followed by a read.
> > >
> > > I took a deeper look at the datasheets, and you're right, a MAX197
> > > driver seems to be a good choice. However, there are a number of
> > > differences between a direct usage of a MAX197 and the TS-5500
> > > mapped MAX197.
> > >
> > > To start a conversion of a channel for a given range and
> > > polarity, it consists on both sides of a u8 outb() call on pins
> > > 7-14 (i.e. bits D7-D0). To be notified when the result is ready,
> > > we can either set an IRQ on INT pin (falling edge), or poll it.
> > > Then on the MAX197, you read the pins 7-14, set pin HBEN to 1, and
> > > read the same pins again to get the 4 remaining bits. On the
> > > TS-5500, only polling is available, and the 12 bits are mapped on
> > > 2 registers.
> > >
> > > I propose to write a max197 driver with default read and write
> > > functions. A platform_data will be used to specify the base
> > > address (pins 7-14), and eventually a custom read function
> > > pointer, which will be used instead of the default one if it is
> > > different of NULL.
> > >
> > > What do you think?
> > >
> > > I will write a max197 driver with default read and write
> > > functions. A platform_data will be used to specify the base
> > > address (pins 7-14), and eventually a custom read function
> > > pointer, which will be used instead of the default, if it is not
> > > NULL.
> > >
> > > What do you think?
> >
> Sounds like a plan to me.
>
> > Sorry for the duplicate :)
> >
> > BTW, I've added Jean Delvare and the lm-sensors mailing list in Cc,
> > in case they have an opinion on this.
> >
>
> Thanks,
> Guenter
Hi Guenter,
There's another disadvantage with the generic driver.
Usually, the MAX197 is memory mapped, but on the TS-5500, the CPLD is
port mapped, so even the write function would be different.
It sounds like the amount of reusable lines of code is not that
significant.
Do you think it is still worthwhile?
Thanks,
Vivien
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] x86/platform: (TS-5500) revised ADC driver
2012-01-24 20:16 ` Vivien Didelot
@ 2012-01-24 21:20 ` Guenter Roeck
0 siblings, 0 replies; 18+ messages in thread
From: Guenter Roeck @ 2012-01-24 21:20 UTC (permalink / raw)
To: Vivien Didelot
Cc: khali, x86, Ingo Molnar, Thomas Gleixner, H. Peter Anvin,
linux-kernel, lm-sensors
On Tue, 2012-01-24 at 15:16 -0500, Vivien Didelot wrote:
> Le Mon, 23 Jan 2012 16:41:38 -0800,
> Guenter Roeck <guenter.roeck@ericsson.com> a écrit :
>
> > On Mon, Jan 23, 2012 at 05:35:51PM -0500, Vivien Didelot wrote:
> > > On Mon, 23 Jan 2012 15:54:08 -0500,
> > > Vivien Didelot <vivien.didelot@savoirfairelinux.com> wrote:
> > >
> > > > On Sun, 22 Jan 2012 20:36:46 -0800,
> > > > Guenter Roeck <guenter.roeck@ericsson.com> wrote:
> > > > > > > Regarding the location, I'd really like to know from the
> > > > > > > powers-that-be if arch/x86/platform/ts5500/
> > > > > > > or
> > > > > > > drivers/platform/x86
> > > > > > > or
> > > > > > > drivers/hwmon
> > > > > > >
> > > > > > > would be the appropriate location for a driver like this. As
> > > > > > > mentioned before, my strong preference is drivers/hwmon,
> > > > > > > but I would like to hear from others.
> > > > > >
> > > > > > We should either split every driver into corresponding
> > > > > > subdirectories, or put everything in a common platform
> > > > > > directory. My first RFC patches set has every driver
> > > > > > separated. As they are really specific to the platform,
> > > > > > people seem to agree with grouping them, mainly because they
> > > > > > won't be shared. I changed that in the following patches
> > > > > > sets, and X86 maintainers seemed to be ok with that.
> > > > > >
> > > > > > I'm ok with both solutions, but we should all agree on one.
> > > > > > Maybe we should have other maintainers view on this?
> > > > > >
> > > > > That is what I had asked for. I thought the whole point of
> > > > > per-module directories was to have all drivers there. If that
> > > > > is no longer true, fine with me; who am I to argue about
> > > > > something like that. I'd just like to know.
> > > > >
> > > > > > > Also, I am not sure if the current approach is appropriate
> > > > > > > to start with. Looking at the datasheet as well as into
> > > > > > > existing kernel code, it appears quite likely that some
> > > > > > > kind of more or less generic MAX197 driver exists
> > > > > > > somewhere. The existence of is_max197_installed() - without
> > > > > > > any calling code - is a strong indication that this is the
> > > > > > > case, as well as the "static" platform data in your
> > > > > > > original patch. It might be more appropriate to take this
> > > > > > > more or less generic driver, move it to drivers/hwmon, and
> > > > > > > provide - for example through platform data - a means to
> > > > > > > read from and write to the chip on a per-platform basis, ie
> > > > > > > with per-platform access functions.
> > > > > >
> > > > > > You're right, it should be possible to create a generic max197
> > > > > > driver and provide read/write functions through platform
> > > > > > data. But we don't have a max197 right now... So it can stay
> > > > > > as a compact TS-5500 ADC driver for the moment, and maybe we
> > > > > > will split later. What do you think?
> > > > > >
> > > > > I am lost. If you don't have a TS-5500 with max197, how do you
> > > > > test the driver ?
> > > >
> > > > Sorry, I wasn't clear. I meant the only max197 I have is the one
> > > > behind the TS-5500 CPLD, I don't have any others to test
> > > > independently.
> > > >
> > > > > I had another look into the MAX197 and TS-5500 data sheets. In
> > > > > my opinion, a generic MAX197 driver in drivers/hwmon combined
> > > > > with a platform driver in the current location would be the way
> > > > > to go. That driver would then also work for the other TS-5x00
> > > > > systems. All you need is a single chip access function in the
> > > > > platform code, since the chip is always accessed with a write
> > > > > followed by a read.
> > > >
> > > > I took a deeper look at the datasheets, and you're right, a MAX197
> > > > driver seems to be a good choice. However, there are a number of
> > > > differences between a direct usage of a MAX197 and the TS-5500
> > > > mapped MAX197.
> > > >
> > > > To start a conversion of a channel for a given range and
> > > > polarity, it consists on both sides of a u8 outb() call on pins
> > > > 7-14 (i.e. bits D7-D0). To be notified when the result is ready,
> > > > we can either set an IRQ on INT pin (falling edge), or poll it.
> > > > Then on the MAX197, you read the pins 7-14, set pin HBEN to 1, and
> > > > read the same pins again to get the 4 remaining bits. On the
> > > > TS-5500, only polling is available, and the 12 bits are mapped on
> > > > 2 registers.
> > > >
> > > > I propose to write a max197 driver with default read and write
> > > > functions. A platform_data will be used to specify the base
> > > > address (pins 7-14), and eventually a custom read function
> > > > pointer, which will be used instead of the default one if it is
> > > > different of NULL.
> > > >
> > > > What do you think?
> > > >
> > > > I will write a max197 driver with default read and write
> > > > functions. A platform_data will be used to specify the base
> > > > address (pins 7-14), and eventually a custom read function
> > > > pointer, which will be used instead of the default, if it is not
> > > > NULL.
> > > >
> > > > What do you think?
> > >
> > Sounds like a plan to me.
> >
> > > Sorry for the duplicate :)
> > >
> > > BTW, I've added Jean Delvare and the lm-sensors mailing list in Cc,
> > > in case they have an opinion on this.
> > >
> >
> > Thanks,
> > Guenter
>
> Hi Guenter,
>
> There's another disadvantage with the generic driver.
> Usually, the MAX197 is memory mapped, but on the TS-5500, the CPLD is
> port mapped, so even the write function would be different.
> It sounds like the amount of reusable lines of code is not that
> significant.
>
> Do you think it is still worthwhile?
Points to a platform specific access function, as I had suggested
earlier. The access function is just a small part of the driver; the
entire hwmon/sysfs code is generic. Yes, I think it is worthwhile.
Guenter
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2012-01-24 21:24 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-01-18 23:52 [PATCH v4 0/4] Support for the TS-5500 platform Vivien Didelot
2012-01-18 23:52 ` [PATCH v4 1/4] x86/platform: (TS-5500) add platform base support Vivien Didelot
2012-01-18 23:52 ` [PATCH v4 2/4] x86/platform: (TS-5500) add GPIO support Vivien Didelot
2012-01-18 23:52 ` [PATCH v4 3/4] x86/platform: (TS-5500) add LED support Vivien Didelot
2012-01-18 23:52 ` [PATCH v4 4/4] x86/platform: (TS-5500) add ADC support Vivien Didelot
2012-01-19 2:55 ` Guenter Roeck
2012-01-19 2:57 ` Guenter Roeck
2012-01-20 23:41 ` Vivien Didelot
2012-01-21 0:09 ` Guenter Roeck
2012-01-20 23:43 ` [PATCH] x86/platform: (TS-5500) revised ADC driver Vivien Didelot
2012-01-21 4:46 ` Guenter Roeck
2012-01-23 1:36 ` Vivien Didelot
2012-01-23 4:36 ` Guenter Roeck
2012-01-23 20:54 ` Vivien Didelot
2012-01-23 22:35 ` Vivien Didelot
2012-01-24 0:41 ` Guenter Roeck
2012-01-24 20:16 ` Vivien Didelot
2012-01-24 21:20 ` Guenter Roeck
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).