Devicetree
 help / color / mirror / Atom feed
* [PATCH v2 2/5] Input: mpr121 - remove unused field in struct mpr121_touchkey
From: Akinobu Mita @ 2017-01-15 13:15 UTC (permalink / raw)
  To: linux-input-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA
  Cc: Akinobu Mita, Dmitry Torokhov
In-Reply-To: <1484486144-27947-1-git-send-email-akinobu.mita-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Remove unused key_val field in struct mpr121_touchkey.

Cc: Dmitry Torokhov <dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Signed-off-by: Akinobu Mita <akinobu.mita-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
* No changes from v1

 drivers/input/keyboard/mpr121_touchkey.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/input/keyboard/mpr121_touchkey.c b/drivers/input/keyboard/mpr121_touchkey.c
index a3fd6e5..90be99d 100644
--- a/drivers/input/keyboard/mpr121_touchkey.c
+++ b/drivers/input/keyboard/mpr121_touchkey.c
@@ -59,7 +59,6 @@
 struct mpr121_touchkey {
 	struct i2c_client	*client;
 	struct input_dev	*input_dev;
-	unsigned int		key_val;
 	unsigned int		statusbits;
 	unsigned int		keycount;
 	u16			keycodes[MPR121_MAX_KEY_COUNT];
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related

* [PATCH v2 3/5] Input: mpr121 - set missing event capability
From: Akinobu Mita @ 2017-01-15 13:15 UTC (permalink / raw)
  To: linux-input-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA
  Cc: Akinobu Mita, Dmitry Torokhov
In-Reply-To: <1484486144-27947-1-git-send-email-akinobu.mita-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

This driver reports misc scan input events on the sensor's status
register changes.  But the event capability for them was not set in the
device initialization, so these events were ignored.

This change adds the missing event capability.

Cc: Dmitry Torokhov <dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Signed-off-by: Akinobu Mita <akinobu.mita-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
* No changes from v1

 drivers/input/keyboard/mpr121_touchkey.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/input/keyboard/mpr121_touchkey.c b/drivers/input/keyboard/mpr121_touchkey.c
index 90be99d..2558c60 100644
--- a/drivers/input/keyboard/mpr121_touchkey.c
+++ b/drivers/input/keyboard/mpr121_touchkey.c
@@ -230,6 +230,7 @@ static int mpr_touchkey_probe(struct i2c_client *client,
 	input_dev->id.bustype = BUS_I2C;
 	input_dev->dev.parent = &client->dev;
 	input_dev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_REP);
+	input_set_capability(input_dev, EV_MSC, MSC_SCAN);
 
 	input_dev->keycode = mpr121->keycodes;
 	input_dev->keycodesize = sizeof(mpr121->keycodes[0]);
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related

* [PATCH v2 4/5] Input: mpr121 - handle multiple bits change of status register
From: Akinobu Mita @ 2017-01-15 13:15 UTC (permalink / raw)
  To: linux-input-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA
  Cc: Akinobu Mita, Dmitry Torokhov
In-Reply-To: <1484486144-27947-1-git-send-email-akinobu.mita-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

This driver reports input events on their interrupts which are triggered
by the sensor's status register changes.  But only single bit change is
reported in the interrupt handler.  So if there are multiple bits are
changed at almost the same time, other press or release events are ignored.

This fixes it by detecting all changed bits in the status register.

Cc: Dmitry Torokhov <dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Signed-off-by: Akinobu Mita <akinobu.mita-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
* Use for_each_set_bit() to search changed bit

 drivers/input/keyboard/mpr121_touchkey.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/input/keyboard/mpr121_touchkey.c b/drivers/input/keyboard/mpr121_touchkey.c
index 2558c60..a0210ae 100644
--- a/drivers/input/keyboard/mpr121_touchkey.c
+++ b/drivers/input/keyboard/mpr121_touchkey.c
@@ -86,7 +86,8 @@ static irqreturn_t mpr_touchkey_interrupt(int irq, void *dev_id)
 	struct mpr121_touchkey *mpr121 = dev_id;
 	struct i2c_client *client = mpr121->client;
 	struct input_dev *input = mpr121->input_dev;
-	unsigned int key_num, key_val, pressed;
+	unsigned long bit_changed;
+	unsigned int key_num;
 	int reg;
 
 	reg = i2c_smbus_read_byte_data(client, ELE_TOUCH_STATUS_1_ADDR);
@@ -104,18 +105,22 @@ static irqreturn_t mpr_touchkey_interrupt(int irq, void *dev_id)
 
 	reg &= TOUCH_STATUS_MASK;
 	/* use old press bit to figure out which bit changed */
-	key_num = ffs(reg ^ mpr121->statusbits) - 1;
-	pressed = reg & (1 << key_num);
+	bit_changed = reg ^ mpr121->statusbits;
 	mpr121->statusbits = reg;
+	for_each_set_bit(key_num, &bit_changed, mpr121->keycount) {
+		unsigned int key_val, pressed;
 
-	key_val = mpr121->keycodes[key_num];
+		pressed = reg & (1 << key_num);
+		key_val = mpr121->keycodes[key_num];
 
-	input_event(input, EV_MSC, MSC_SCAN, key_num);
-	input_report_key(input, key_val, pressed);
-	input_sync(input);
+		input_event(input, EV_MSC, MSC_SCAN, key_num);
+		input_report_key(input, key_val, pressed);
+
+		dev_dbg(&client->dev, "key %d %d %s\n", key_num, key_val,
+			pressed ? "pressed" : "released");
 
-	dev_dbg(&client->dev, "key %d %d %s\n", key_num, key_val,
-		pressed ? "pressed" : "released");
+	}
+	input_sync(input);
 
 out:
 	return IRQ_HANDLED;
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related

* [PATCH v2 5/5] Input: mpr121 - switch to device tree probe
From: Akinobu Mita @ 2017-01-15 13:15 UTC (permalink / raw)
  To: linux-input-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA
  Cc: Akinobu Mita, Dmitry Torokhov, Rob Herring
In-Reply-To: <1484486144-27947-1-git-send-email-akinobu.mita-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

This driver currently only supports legacy platform data probe.  This
change adds device tree support and gets rid of platform data probe code
since no one is actually using mpr121 platform data in the mainline.

The device tree property parsing code is based on the work of
atmel_captouch driver.

Cc: Dmitry Torokhov <dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Signed-off-by: Akinobu Mita <akinobu.mita-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
* Newly added patch from v2

 .../devicetree/bindings/input/mpr121-touchkey.txt  |  30 ++++++
 drivers/input/keyboard/mpr121_touchkey.c           | 110 +++++++++++++++------
 include/linux/i2c/mpr121_touchkey.h                |  20 ----
 3 files changed, 110 insertions(+), 50 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/input/mpr121-touchkey.txt
 delete mode 100644 include/linux/i2c/mpr121_touchkey.h

diff --git a/Documentation/devicetree/bindings/input/mpr121-touchkey.txt b/Documentation/devicetree/bindings/input/mpr121-touchkey.txt
new file mode 100644
index 0000000..b7c61ee
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/mpr121-touchkey.txt
@@ -0,0 +1,30 @@
+* Freescale MPR121 Controllor
+
+Required Properties:
+- compatible:		Should be "fsl,mpr121-touchkey"
+- reg:			The I2C slave address of the device.
+- interrupts:		The interrupt number to the cpu.
+- vdd-supply:		Phandle to the Vdd power supply.
+- linux,keycodes:	Specifies an array of numeric keycode values to
+			be used for reporting button presses. The array can
+			contain up to 12 entries.
+
+Optional Properties:
+- wakeup-source:	Use any event on keypad as wakeup event.
+- autorepeat:		Enable autorepeat feature.
+
+Example:
+
+#include "dt-bindings/input/input.h"
+
+	touchkey: mpr121@5a {
+		compatible = "fsl,mpr121-touchkey";
+		reg = <0x5a>;
+		interrupt-parent = <&gpio1>;
+		interrupts = <28 2>;
+		autorepeat;
+		vdd-supply = <&ldo4_reg>;
+		linux,keycodes = <KEY_0>, <KEY_1>, <KEY_2>, <KEY_3>,
+				<KEY_4> <KEY_5>, <KEY_6>, <KEY_7>,
+				<KEY_8>, <KEY_9>, <KEY_A>, <KEY_B>;
+	};
diff --git a/drivers/input/keyboard/mpr121_touchkey.c b/drivers/input/keyboard/mpr121_touchkey.c
index a0210ae..ebb401f 100644
--- a/drivers/input/keyboard/mpr121_touchkey.c
+++ b/drivers/input/keyboard/mpr121_touchkey.c
@@ -19,7 +19,7 @@
 #include <linux/delay.h>
 #include <linux/bitops.h>
 #include <linux/interrupt.h>
-#include <linux/i2c/mpr121_touchkey.h>
+#include <linux/regulator/consumer.h>
 
 /* Register definitions */
 #define ELE_TOUCH_STATUS_0_ADDR	0x0
@@ -61,7 +61,7 @@ struct mpr121_touchkey {
 	struct input_dev	*input_dev;
 	unsigned int		statusbits;
 	unsigned int		keycount;
-	u16			keycodes[MPR121_MAX_KEY_COUNT];
+	u32			keycodes[MPR121_MAX_KEY_COUNT];
 };
 
 struct mpr121_init_register {
@@ -81,6 +81,42 @@ static const struct mpr121_init_register init_reg_table[] = {
 	{ AUTO_CONFIG_CTRL_ADDR, 0x0b },
 };
 
+static void mpr121_vdd_supply_disable(void *data)
+{
+	struct regulator *vdd_supply = data;
+
+	regulator_disable(vdd_supply);
+}
+
+static struct regulator *mpr121_vdd_supply_init(struct device *dev)
+{
+	struct regulator *vdd_supply;
+	int err;
+
+	vdd_supply = devm_regulator_get(dev, "vdd");
+	if (IS_ERR(vdd_supply)) {
+		dev_err(dev, "failed to get vdd regulator: %ld\n",
+			PTR_ERR(vdd_supply));
+		return vdd_supply;
+	}
+
+	err = regulator_enable(vdd_supply);
+	if (err) {
+		dev_err(dev, "failed to enable vdd regulator: %d\n", err);
+		return ERR_PTR(err);
+	}
+
+	err = devm_add_action(dev, mpr121_vdd_supply_disable, vdd_supply);
+	if (err) {
+		regulator_disable(vdd_supply);
+		dev_err(dev, "failed to add disable regulator action: %d\n",
+			err);
+		return ERR_PTR(err);
+	}
+
+	return vdd_supply;
+}
+
 static irqreturn_t mpr_touchkey_interrupt(int irq, void *dev_id)
 {
 	struct mpr121_touchkey *mpr121 = dev_id;
@@ -126,9 +162,8 @@ static irqreturn_t mpr_touchkey_interrupt(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-static int mpr121_phys_init(const struct mpr121_platform_data *pdata,
-				      struct mpr121_touchkey *mpr121,
-				      struct i2c_client *client)
+static int mpr121_phys_init(struct mpr121_touchkey *mpr121,
+			    struct i2c_client *client, int vdd_uv)
 {
 	const struct mpr121_init_register *reg;
 	unsigned char usl, lsl, tl, eleconf;
@@ -158,9 +193,9 @@ static int mpr121_phys_init(const struct mpr121_platform_data *pdata,
 	/*
 	 * Capacitance on sensing input varies and needs to be compensated.
 	 * The internal MPR121-auto-configuration can do this if it's
-	 * registers are set properly (based on pdata->vdd_uv).
+	 * registers are set properly (based on vdd_uv).
 	 */
-	vdd = pdata->vdd_uv / 1000;
+	vdd = vdd_uv / 1000;
 	usl = ((vdd - 700) * 256) / vdd;
 	lsl = (usl * 65) / 100;
 	tl = (usl * 90) / 100;
@@ -191,27 +226,19 @@ static int mpr121_phys_init(const struct mpr121_platform_data *pdata,
 static int mpr_touchkey_probe(struct i2c_client *client,
 			      const struct i2c_device_id *id)
 {
-	const struct mpr121_platform_data *pdata =
-			dev_get_platdata(&client->dev);
+	struct device *dev = &client->dev;
+	struct regulator *vdd_supply;
+	int vdd_uv;
 	struct mpr121_touchkey *mpr121;
 	struct input_dev *input_dev;
 	int error;
 	int i;
 
-	if (!pdata) {
-		dev_err(&client->dev, "no platform data defined\n");
-		return -EINVAL;
-	}
+	vdd_supply = mpr121_vdd_supply_init(dev);
+	if (IS_ERR(vdd_supply))
+		return PTR_ERR(vdd_supply);
 
-	if (!pdata->keymap || !pdata->keymap_size) {
-		dev_err(&client->dev, "missing keymap data\n");
-		return -EINVAL;
-	}
-
-	if (pdata->keymap_size > MPR121_MAX_KEY_COUNT) {
-		dev_err(&client->dev, "too many keys defined\n");
-		return -EINVAL;
-	}
+	vdd_uv = regulator_get_voltage(vdd_supply);
 
 	if (!client->irq) {
 		dev_err(&client->dev, "irq number should not be zero\n");
@@ -229,24 +256,37 @@ static int mpr_touchkey_probe(struct i2c_client *client,
 
 	mpr121->client = client;
 	mpr121->input_dev = input_dev;
-	mpr121->keycount = pdata->keymap_size;
+	mpr121->keycount = device_property_read_u32_array(dev, "linux,keycodes",
+							NULL, 0);
+	if (mpr121->keycount > MPR121_MAX_KEY_COUNT) {
+		dev_err(dev, "too many keys defined\n");
+		return -EINVAL;
+	}
+
+	error = device_property_read_u32_array(dev, "linux,keycodes",
+						mpr121->keycodes,
+						mpr121->keycount);
+	if (error) {
+		dev_err(dev,
+			"failed to read linux,keycode property: %d\n", error);
+		return error;
+	}
 
 	input_dev->name = "Freescale MPR121 Touchkey";
 	input_dev->id.bustype = BUS_I2C;
 	input_dev->dev.parent = &client->dev;
-	input_dev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_REP);
+	if (device_property_present(dev, "autorepeat"))
+		__set_bit(EV_REP, input_dev->evbit);
 	input_set_capability(input_dev, EV_MSC, MSC_SCAN);
 
 	input_dev->keycode = mpr121->keycodes;
 	input_dev->keycodesize = sizeof(mpr121->keycodes[0]);
 	input_dev->keycodemax = mpr121->keycount;
 
-	for (i = 0; i < pdata->keymap_size; i++) {
-		input_set_capability(input_dev, EV_KEY, pdata->keymap[i]);
-		mpr121->keycodes[i] = pdata->keymap[i];
-	}
+	for (i = 0; i < mpr121->keycount; i++)
+		input_set_capability(input_dev, EV_KEY, mpr121->keycodes[i]);
 
-	error = mpr121_phys_init(pdata, mpr121, client);
+	error = mpr121_phys_init(mpr121, client, vdd_uv);
 	if (error) {
 		dev_err(&client->dev, "Failed to init register\n");
 		return error;
@@ -266,7 +306,8 @@ static int mpr_touchkey_probe(struct i2c_client *client,
 		return error;
 
 	i2c_set_clientdata(client, mpr121);
-	device_init_wakeup(&client->dev, pdata->wakeup);
+	device_init_wakeup(dev,
+			device_property_read_bool(dev, "wakeup-source"));
 
 	return 0;
 }
@@ -305,10 +346,19 @@ static const struct i2c_device_id mpr121_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, mpr121_id);
 
+#ifdef CONFIG_OF
+static const struct of_device_id mpr121_touchkey_dt_match_table[] = {
+	{ .compatible = "fsl,mpr121-touchkey" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, mpr121_touchkey_dt_match_table);
+#endif
+
 static struct i2c_driver mpr_touchkey_driver = {
 	.driver = {
 		.name	= "mpr121",
 		.pm	= &mpr121_touchkey_pm_ops,
+		.of_match_table = of_match_ptr(mpr121_touchkey_dt_match_table),
 	},
 	.id_table	= mpr121_id,
 	.probe		= mpr_touchkey_probe,
diff --git a/include/linux/i2c/mpr121_touchkey.h b/include/linux/i2c/mpr121_touchkey.h
deleted file mode 100644
index f0bcc38..0000000
--- a/include/linux/i2c/mpr121_touchkey.h
+++ /dev/null
@@ -1,20 +0,0 @@
-/* Header file for Freescale MPR121 Capacitive Touch Sensor */
-
-#ifndef _MPR121_TOUCHKEY_H
-#define _MPR121_TOUCHKEY_H
-
-/**
- * struct mpr121_platform_data - platform data for mpr121 sensor
- * @keymap: pointer to array of KEY_* values representing keymap
- * @keymap_size: size of the keymap
- * @wakeup: configure the button as a wake-up source
- * @vdd_uv: VDD voltage in uV
- */
-struct mpr121_platform_data {
-	const unsigned short *keymap;
-	unsigned int keymap_size;
-	bool wakeup;
-	int vdd_uv;
-};
-
-#endif /* _MPR121_TOUCHKEY_H */
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related

* [PATCH] pcie: ti: Provide patch to force GEN1 PCIe operation
From: Lukasz Majewski @ 2017-01-15 13:19 UTC (permalink / raw)
  To: Kishon Vijay Abraham I, Bjorn Helgaas
  Cc: Rob Herring, Mark Rutland, Jingoo Han, Joao Pinto,
	linux-omap-u79uwXL29TY76Z2rM5mHXA,
	linux-pci-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, Lukasz Majewski

Some devices (due to e.g. bad PCIe signal integrity) require to run
with forced GEN1 speed on PCIe bus.

This patch changes the speed explicitly on dra7 based devices when
proper device tree attribute is defined for the PCIe controller.

Signed-off-by: Lukasz Majewski <lukma-ynQEQJNshbs@public.gmane.org>
---

Patch applies on newest origin/master
SHA1: f4d3935e4f4884ba80561db5549394afb8eef8f7

Tested at AM5728

---
 Documentation/devicetree/bindings/pci/ti-pci.txt |  1 +
 drivers/pci/host/pci-dra7xx.c                    | 23 +++++++++++++++++++++++
 drivers/pci/host/pcie-designware.h               |  1 +
 3 files changed, 25 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/ti-pci.txt b/Documentation/devicetree/bindings/pci/ti-pci.txt
index 60e2516..9f97409 100644
--- a/Documentation/devicetree/bindings/pci/ti-pci.txt
+++ b/Documentation/devicetree/bindings/pci/ti-pci.txt
@@ -25,6 +25,7 @@ PCIe Designware Controller
 
 Optional Property:
  - gpios : Should be added if a gpio line is required to drive PERST# line
+ - to,pcie-is-gen1: Indicates that forced gen1 port operation is needed.
 
 Example:
 axi {
diff --git a/drivers/pci/host/pci-dra7xx.c b/drivers/pci/host/pci-dra7xx.c
index 9595fad..eec5fae 100644
--- a/drivers/pci/host/pci-dra7xx.c
+++ b/drivers/pci/host/pci-dra7xx.c
@@ -63,6 +63,13 @@
 #define	LINK_UP						BIT(16)
 #define	DRA7XX_CPU_TO_BUS_ADDR				0x0FFFFFFF
 
+#define         PCIECTRL_EP_DBICS_LNK_CAP                       0x007C
+#define         MAX_LINK_SPEEDS_MASK				GENMASK(3, 0)
+#define         MAX_LINK_SPEEDS_GEN1                            BIT(0)
+
+#define         PCIECTRL_PL_WIDTH_SPEED_CTL                     0x080C
+#define         CFG_DIRECTED_SPEED_CHANGE                       BIT(17)
+
 struct dra7xx_pcie {
 	struct pcie_port	pp;
 	void __iomem		*base;		/* DT ti_conf */
@@ -270,6 +277,7 @@ static int __init dra7xx_add_pcie_port(struct dra7xx_pcie *dra7xx,
 	struct pcie_port *pp = &dra7xx->pp;
 	struct device *dev = pp->dev;
 	struct resource *res;
+	u32 val;
 
 	pp->irq = platform_get_irq(pdev, 1);
 	if (pp->irq < 0) {
@@ -296,6 +304,18 @@ static int __init dra7xx_add_pcie_port(struct dra7xx_pcie *dra7xx,
 	if (!pp->dbi_base)
 		return -ENOMEM;
 
+	if (pp->is_gen1) {
+		dev_info(dev, "GEN1 forced\n");
+
+		val = readl(pp->dbi_base + PCIECTRL_EP_DBICS_LNK_CAP);
+		set_mask_bits(&val, MAX_LINK_SPEEDS_MASK, MAX_LINK_SPEEDS_GEN1);
+		writel(val, pp->dbi_base + PCIECTRL_EP_DBICS_LNK_CAP);
+
+		val = readl(pp->dbi_base + PCIECTRL_PL_WIDTH_SPEED_CTL);
+		val &= ~CFG_DIRECTED_SPEED_CHANGE;
+		writel(val, pp->dbi_base + PCIECTRL_PL_WIDTH_SPEED_CTL);
+	}
+
 	ret = dw_pcie_host_init(pp);
 	if (ret) {
 		dev_err(dev, "failed to initialize host\n");
@@ -404,6 +424,9 @@ static int __init dra7xx_pcie_probe(struct platform_device *pdev)
 		goto err_gpio;
 	}
 
+	if (of_property_read_bool(np, "ti,pcie-is-gen1"))
+		pp->is_gen1 = true;
+
 	reg = dra7xx_pcie_readl(dra7xx, PCIECTRL_DRA7XX_CONF_DEVICE_CMD);
 	reg &= ~LTSSM_EN;
 	dra7xx_pcie_writel(dra7xx, PCIECTRL_DRA7XX_CONF_DEVICE_CMD, reg);
diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h
index a567ea2..2fb0b18 100644
--- a/drivers/pci/host/pcie-designware.h
+++ b/drivers/pci/host/pcie-designware.h
@@ -50,6 +50,7 @@ struct pcie_port {
 	struct irq_domain	*irq_domain;
 	unsigned long		msi_data;
 	u8			iatu_unroll_enabled;
+	u8                      is_gen1;
 	DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_IRQS);
 };
 
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related

* Re: [PATCH 1/3] DT/bindings: Add bindings for TI ADS7950 A/DC chips
From: Jonathan Cameron @ 2017-01-15 13:52 UTC (permalink / raw)
  To: David Lechner, devicetree, linux-iio
  Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
	Rob Herring, Mark Rutland, linux-kernel
In-Reply-To: <662a48e1-39fc-4fb7-a3b9-b671d2e40029@lechnology.com>

On 14/01/17 18:00, David Lechner wrote:
> On 01/14/2017 06:53 AM, Jonathan Cameron wrote:
>> On 11/01/17 17:52, David Lechner wrote:
>>> This adds device tree bindings for the TI ADS7950 family of A/DC chips.
>>>
>>> Signed-off-by: David Lechner <david@lechnology.com>
>> This is in of itself good, but we may need to have some deprecated
>> elements to continue supporting what was implicitly happening with
>> the missnaming so as to avoid accidentally breaking someone's device
>> tree.
>>
> 
> As I mentioned in my cover letter, this driver, as far as I can tell,
> only exists in your testing branch, so I find it highly unlikely that
> we would be breaking anyone. It is not in mainline and it is not even
> in linux-next.

Oops, I not only have a memory like a goldfish, I clearly didn't read your
cover letter either.

Applied to the togreg branch of iio.git and pushed out as testing.

The driver will hit next before this does but will get into mainline
in the same ultimate pull request unless something weird happens so that's
all fine.

Jonathan
>> Jonathan
>>> ---
>>>  .../devicetree/bindings/iio/adc/ti-ads7950.txt     | 23 ++++++++++++++++++++++
>>>  1 file changed, 23 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/iio/adc/ti-ads7950.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/iio/adc/ti-ads7950.txt b/Documentation/devicetree/bindings/iio/adc/ti-ads7950.txt
>>> new file mode 100644
>>> index 0000000..e77a6f7
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/iio/adc/ti-ads7950.txt
>>> @@ -0,0 +1,23 @@
>>> +* Texas Instruments ADS7950 family of A/DC chips
>>> +
>>> +Required properties:
>>> + - compatible: Must be one of "ti,ads7950", "ti,ads7951", "ti,ads7952",
>>> +   "ti,ads7953", "ti,ads7954", "ti,ads7955", "ti,ads7956", "ti,ads7957",
>>> +   "ti,ads7958", "ti,ads7959", "ti,ads7960", or "ti,ads7961"
>>> + - reg: SPI chip select number for the device
>>> + - #io-channel-cells: Must be 1 as per ../iio-bindings.txt
>>> + - vref-supply: phandle to a regulator node that supplies the 2.5V or 5V
>>> +   reference voltage
>>> +
>>> +Recommended properties:
>>> + - spi-max-frequency: Definition as per
>>> +        Documentation/devicetree/bindings/spi/spi-bus.txt
>>> +
>>> +Example:
>>> +adc@0 {
>>> +    compatible = "ti,ads7957";
>>> +    reg = <0>;
>>> +    #io-channel-cells = <1>;
>>> +    vref-supply = <&refin_supply>;
>>> +    spi-max-frequency = <10000000>;
>>> +};
>>>
>>
> 
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH 2/3] iio: adc: ti-ads7950: Drop "ti-" prefix from module name
From: Jonathan Cameron @ 2017-01-15 13:54 UTC (permalink / raw)
  To: David Lechner, devicetree, linux-iio
  Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
	Rob Herring, Mark Rutland, linux-kernel
In-Reply-To: <83b36ce2-6b54-b5ee-6ad3-1f90d26f5e52@lechnology.com>

On 14/01/17 18:07, David Lechner wrote:
> On 01/14/2017 06:49 AM, Jonathan Cameron wrote:
>> On 11/01/17 17:52, David Lechner wrote:
>>> This drops the "ti-" prefix from the module name. It makes the module name
>>> consistent with other iio ti-ads* drivers and it makes the driver work
>>> with device tree (the spi subsystem drops the "ti," prefix when matching
>>> compatible strings from device tree).
>>>
>>> Tested working on LEGO MINDSTORMS EV3 with the following device tree node:
>>>
>>>     adc@3 {
>>>         compatible = "ti,ads7957";
>>>         reg = <3>;
>>>         #io-channel-cells = <1>;
>>>         spi-max-frequency = <10000000>;
>>>         vref-supply = <&adc_ref>;
>>>     };
>>>
>>> Signed-off-by: David Lechner <david@lechnology.com>
>> What worries me here is that we might break existing setups.  I agree
>> we should have gotten this 'right' in the first place, but can we fix
>> it now.  Not so sure. We'd be better off perhaps adding an of_device_id
>> table with the write entries for device tree.
> 

> As far as I can tell, this driver only exists in your testing branch.
> Does that really mean that it is too late to get it right?
Gah!  I have a memory like a goldfish.

Excellent point - we can and should do this asap.
Applied to the togreg branch of iio.git and pushed out as testing for
the autobuilders to play with it.

Thanks,

Jonathan


>>> ---
>>>  drivers/iio/adc/ti-ads7950.c | 26 +++++++++++++-------------
>>>  1 file changed, 13 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c
>>> index 0330361..b587fa6 100644
>>> --- a/drivers/iio/adc/ti-ads7950.c
>>> +++ b/drivers/iio/adc/ti-ads7950.c
>>> @@ -459,25 +459,25 @@ static int ti_ads7950_remove(struct spi_device *spi)
>>>  }
>>>
>>>  static const struct spi_device_id ti_ads7950_id[] = {
>>> -    {"ti-ads7950", TI_ADS7950},
>>> -    {"ti-ads7951", TI_ADS7951},
>>> -    {"ti-ads7952", TI_ADS7952},
>>> -    {"ti-ads7953", TI_ADS7953},
>>> -    {"ti-ads7954", TI_ADS7954},
>>> -    {"ti-ads7955", TI_ADS7955},
>>> -    {"ti-ads7956", TI_ADS7956},
>>> -    {"ti-ads7957", TI_ADS7957},
>>> -    {"ti-ads7958", TI_ADS7958},
>>> -    {"ti-ads7959", TI_ADS7959},
>>> -    {"ti-ads7960", TI_ADS7960},
>>> -    {"ti-ads7961", TI_ADS7961},
>>> +    { "ads7950", TI_ADS7950 },
>>> +    { "ads7951", TI_ADS7951 },
>>> +    { "ads7952", TI_ADS7952 },
>>> +    { "ads7953", TI_ADS7953 },
>>> +    { "ads7954", TI_ADS7954 },
>>> +    { "ads7955", TI_ADS7955 },
>>> +    { "ads7956", TI_ADS7956 },
>>> +    { "ads7957", TI_ADS7957 },
>>> +    { "ads7958", TI_ADS7958 },
>>> +    { "ads7959", TI_ADS7959 },
>>> +    { "ads7960", TI_ADS7960 },
>>> +    { "ads7961", TI_ADS7961 },
>>>      { }
>>>  };
>>>  MODULE_DEVICE_TABLE(spi, ti_ads7950_id);
>>>
>>>  static struct spi_driver ti_ads7950_driver = {
>>>      .driver = {
>>> -        .name    = "ti-ads7950",
>>> +        .name    = "ads7950",
>>>      },
>>>      .probe        = ti_ads7950_probe,
>>>      .remove        = ti_ads7950_remove,
>>>
>>
> 
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH 3/3] iio: adc: ti-ads7950: Change regulator matching string to "vref"
From: Jonathan Cameron @ 2017-01-15 13:55 UTC (permalink / raw)
  To: David Lechner, devicetree, linux-iio
  Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
	Rob Herring, Mark Rutland, linux-kernel
In-Reply-To: <e17acc8c-01b9-284c-a321-5135de80c2b7@lechnology.com>

On 14/01/17 18:09, David Lechner wrote:
> On 01/14/2017 06:52 AM, Jonathan Cameron wrote:
>> On 11/01/17 17:52, David Lechner wrote:
>>> This changes the reference voltage regulator matching string from "refin"
>>> to "vref". This is to be consistent with other A/DC chips that also use
>>> "vref-supply" in their device tree bindings.
>>>
>>> Signed-off-by: David Lechner <david@lechnology.com>
>>> ---
>>>  drivers/iio/adc/ti-ads7950.c | 6 +++---
>> Again, we missed this before and it would have been nice to have
>> had it as vref (which is matches the datasheet).  The question
>> becomes how do we handle this going forward with no risk of breaking
>> existing device trees.  We may have to do an optional get on one
>> name and then a non optional on the second. It's ugly, but
>> would fix this up in a 'safe' way.
>>
> 
> Again, I don't think it is too late since this driver only exists in the iio/testing branch.
Applied to the togreg branch of iio.git and pushed out as testing for
the autobuilders to play with it.

Thanks,

Jonathan
> 
>> Jonathan
>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c
>>> index b587fa6..16a0663 100644
>>> --- a/drivers/iio/adc/ti-ads7950.c
>>> +++ b/drivers/iio/adc/ti-ads7950.c
>>> @@ -411,15 +411,15 @@ static int ti_ads7950_probe(struct spi_device *spi)
>>>      spi_message_init_with_transfers(&st->scan_single_msg,
>>>                      st->scan_single_xfer, 3);
>>>
>>> -    st->reg = devm_regulator_get(&spi->dev, "refin");
>>> +    st->reg = devm_regulator_get(&spi->dev, "vref");
>>>      if (IS_ERR(st->reg)) {
>>> -        dev_err(&spi->dev, "Failed get get regulator \"refin\"\n");
>>> +        dev_err(&spi->dev, "Failed get get regulator \"vref\"\n");
>>>          return PTR_ERR(st->reg);
>>>      }
>>>
>>>      ret = regulator_enable(st->reg);
>>>      if (ret) {
>>> -        dev_err(&spi->dev, "Failed to enable regulator \"refin\"\n");
>>> +        dev_err(&spi->dev, "Failed to enable regulator \"vref\"\n");
>>>          return ret;
>>>      }
>>>
>>>
>>
> 
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH 0/3] ti,ads7950 device tree bindings
From: Jonathan Cameron @ 2017-01-15 13:58 UTC (permalink / raw)
  To: David Lechner, devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-iio-u79uwXL29TY76Z2rM5mHXA
  Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
	Rob Herring, Mark Rutland, linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1484157171-15571-1-git-send-email-david-nq/r/kbU++upp/zk7JDF2g@public.gmane.org>

On 11/01/17 17:52, David Lechner wrote:
> This series adds device tree bindings for the TI ADS7950 family of A/DC chips.
> The series includes the bindings documentation and some fixes to the iio driver
> to make it work with the device tree bindings.
> 
> FYI, the ads7950 driver has not made it into mainline yet, so no worries about
> breaking anyone with these changes.
> 
And here's the bit I failed to read!

As an extra point, could you confirm what /sys/bus/iio/iio\:deviceX/name for this
one reads?  I have a feeling this is another case of what Lars has been pointing
out in other drivers this morning.  That name should be the device part number..

Thanks,

Jonathan
> David Lechner (3):
>   DT/bindings: Add bindings for TI ADS7950 A/DC chips
>   iio: adc: ti-ads7950: Drop "ti-" prefix from module name
>   iio: adc: ti-ads7950: Change regulator matching string to "vref"
> 
>  .../devicetree/bindings/iio/adc/ti-ads7950.txt     | 23 ++++++++++++++++
>  drivers/iio/adc/ti-ads7950.c                       | 32 +++++++++++-----------
>  2 files changed, 39 insertions(+), 16 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/ti-ads7950.txt
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v4 0/2] iio: adc: Add Maxim MAX11100 driver
From: jacopo mondi @ 2017-01-15 14:13 UTC (permalink / raw)
  To: Jacopo Mondi, wsa+renesas-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/,
	magnus.damm-Re5JQEeQqe8AvxtiuMwx3w, jic23-DgEjT+Ai2ygdnm+yROfE0A,
	knaack.h-Mmb7MZpHnFY, lars-Qo5EllUWu/uELgA04lAiVw,
	pmeerw-jW+XmwGofnusTnJN9+BGXg, marek.vasut-Re5JQEeQqe8AvxtiuMwx3w,
	geert-Td1EMuHUCqxL1ZNQvxDV9g, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
	mark.rutland-5wv7dgnIgG8
  Cc: linux-iio-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-renesas-soc-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1484301038-16386-1-git-send-email-jacopo+renesas-AW8dsiIh9cEdnm+yROfE0A@public.gmane.org>

Hi Jonathan,
    thanks for review,

On 13/01/2017 10:50, Jacopo Mondi wrote:
> Hello,
>    sending out v4 splitting device tree bindings documentation and actual ADC
> driver.
> No changes in driver code since v3.
>
> Same question for iio maintainers here:
> I would like to have clarified the measure unit returned by read_raw().
> Currently (value_raw * value_scale) return the ADC input value in mV.
> While testing the patch I've been questioned if that should not actually
> be in uV. This is easily achievable making _scale return a value in uV.
> I have found no mention of this in the ABI documentation as it speaks of
> generic voltage.
> Can we have a final word on this?

I see you have reviewed the driver without complaining for the 
read_raw() measure unit, so I assume this replies to the above question 
as well...

Thanks
    j


>
> Thanks Marek for having tested this.
>
> v1 -> v2:
>     - incorporated pmeerw's review comments
>     - retrieve vref from dts and use that to convert read_raw result
>       to mV
>     - add device tree bindings documentation
>
> v2 -> v3:
>     - add _SCALE bit of read_raw function and change _RAW bit accordingly
>     - call regulator_get_voltage when accessing the _SCALE part of read_raw
>       and not during probe
>     - add back remove function as regulator has to be disabled when detaching
>       the module. Do not use devm_ version of iio_register/unregister functions
>       anymore but do unregister in the remove.
>     - remove mutex as access to SPI bus is protected by SPI core. Thanks marex
>
> v3 -> v4:
>     - split device tree binding documentation and actual ADC driver
>     - add "reg" to the list of required properties and use a better
>       namimg for the adc device node in bindings documentation as suggested
>       by Geert.
>
> Jacopo Mondi (2):
>   iio: adc: Add Maxim MAX11100 driver
>   dt-bindings: iio: document MAX11100 ADC
>
>  .../devicetree/bindings/iio/adc/max11100.txt       |  19 +++
>  drivers/iio/adc/Kconfig                            |   9 +
>  drivers/iio/adc/Makefile                           |   1 +
>  drivers/iio/adc/max11100.c                         | 187 +++++++++++++++++++++
>  4 files changed, 216 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/max11100.txt
>  create mode 100644 drivers/iio/adc/max11100.c
>

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v3] iio: max5481: Add support for Maxim digital potentiometers
From: Jonathan Cameron @ 2017-01-15 14:27 UTC (permalink / raw)
  To: Slawomir Stepien, linux-iio-u79uwXL29TY76Z2rM5mHXA
  Cc: matthew.weber-lFk7bPDcGtkY5TsXZYaR1UEOCMrvLtNR,
	maury.anderson-lFk7bPDcGtkY5TsXZYaR1UEOCMrvLtNR,
	devicetree-u79uwXL29TY76Z2rM5mHXA, Mark Rutland, Rob Herring
In-Reply-To: <20170114212139.GA3418-SwUeJysX96B82hYKe6nXyg@public.gmane.org>

On 14/01/17 21:21, Slawomir Stepien wrote:
> From: Matt Weber <matthew.weber-lFk7bPDcGtkY5TsXZYaR1UEOCMrvLtNR@public.gmane.org>
> 
> Add implementation for Maxim Integrated 5481, 5482, 5483,
> and 5484 digital potentiometer devices.
> 
> Datasheet:
> http://datasheets.maximintegrated.com/en/ds/MAX5481-MAX5484.pdf
> 
> Signed-off-by: Maury Anderson <maury.anderson-lFk7bPDcGtkY5TsXZYaR1UEOCMrvLtNR@public.gmane.org>
> Signed-off-by: Matthew Weber <matthew.weber-lFk7bPDcGtkY5TsXZYaR1UEOCMrvLtNR@public.gmane.org>
> Signed-off-by: Slawomir Stepien <sst-IjDXvh/HVVUAvxtiuMwx3w@public.gmane.org>
A few more bits an pieces.

Thanks,

Jonathan
> ---
> 
> This is my resubmission of this patch after original authors decided not to
> pursuit it inclusion into kernel.
> 
> Tested using signal analyzer.
> 
> Changes since v2:
> * spi write buffer moved to struct max5481_data and ____cacheline_aligned.
> * max5481_write_cmd now uses pointer to struct max5481_data as a first argument.
> 
> Changes since v1:
> * removed not needed '``' and 'c' chars
> * includes are now sorted
> * added coma to last item in enum max5481_variant
> * removed maxpos from struct max5481_cfg
> * max5481_CHANNEL is no MAX5481_CHANNEL and it does not have 'ch' argument
> * max5481_write_cmd is now based around switch
> * removed not needed cast in max5481_write_cmd
> * wpier state is saved after iio_device_unregister
> * changed names in spi_device_id and acpi_device_id to be equal to names in of_device_id
> 
> ---
>  .../bindings/iio/potentiometer/max5481.txt         |  23 +++
>  drivers/iio/potentiometer/Kconfig                  |  11 ++
>  drivers/iio/potentiometer/Makefile                 |   1 +
>  drivers/iio/potentiometer/max5481.c                | 216 +++++++++++++++++++++
>  4 files changed, 251 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/potentiometer/max5481.txt
>  create mode 100644 drivers/iio/potentiometer/max5481.c
> 
> diff --git a/Documentation/devicetree/bindings/iio/potentiometer/max5481.txt b/Documentation/devicetree/bindings/iio/potentiometer/max5481.txt
> new file mode 100644
> index 000000000000..6a91b106e076
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/potentiometer/max5481.txt
> @@ -0,0 +1,23 @@
> +* Maxim Linear-Taper Digital Potentiometer MAX5481-MAX5484
> +
> +The node for this driver must be a child node of a SPI controller, hence
> +all mandatory properties described in
> +
> +        Documentation/devicetree/bindings/spi/spi-bus.txt
> +
> +must be specified.
> +
> +Required properties:
> +	- compatible:  	Must be one of the following, depending on the
> +			model:
> +			"maxim,max5481"
> +			"maxim,max5482"
> +			"maxim,max5483"
> +			"maxim,max5484"
> +
> +Example:
> +max548x: max548x@0 {
> +	compatible = "maxim,max5482";
> +	spi-max-frequency = <7000000>;
> +	reg = <0>; /* chip-select */
> +};
> diff --git a/drivers/iio/potentiometer/Kconfig b/drivers/iio/potentiometer/Kconfig
> index 2e9da1cf3297..8bf282510be6 100644
> --- a/drivers/iio/potentiometer/Kconfig
> +++ b/drivers/iio/potentiometer/Kconfig
> @@ -15,6 +15,17 @@ config DS1803
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called ds1803.
>  
> +config MAX5481
> +        tristate "Maxim MAX5481-MAX5484 Digital Potentiometer driver"
> +        depends on SPI
> +        help
> +          Say yes here to build support for the Maxim
> +          MAX5481, MAX5482, MAX5483, MAX5484 digital potentiometer
> +          chips.
> +
> +          To compile this driver as a module, choose M here: the
> +          module will be called max5481.
> +
>  config MAX5487
>          tristate "Maxim MAX5487/MAX5488/MAX5489 Digital Potentiometer driver"
>          depends on SPI
> diff --git a/drivers/iio/potentiometer/Makefile b/drivers/iio/potentiometer/Makefile
> index 8adb58f38c0b..2260d40e0936 100644
> --- a/drivers/iio/potentiometer/Makefile
> +++ b/drivers/iio/potentiometer/Makefile
> @@ -4,6 +4,7 @@
>  
>  # When adding new entries keep the list in alphabetical order
>  obj-$(CONFIG_DS1803) += ds1803.o
> +obj-$(CONFIG_MAX5481) += max5481.o
>  obj-$(CONFIG_MAX5487) += max5487.o
>  obj-$(CONFIG_MCP4131) += mcp4131.o
>  obj-$(CONFIG_MCP4531) += mcp4531.o
> diff --git a/drivers/iio/potentiometer/max5481.c b/drivers/iio/potentiometer/max5481.c
> new file mode 100644
> index 000000000000..559f1635be0a
> --- /dev/null
> +++ b/drivers/iio/potentiometer/max5481.c
> @@ -0,0 +1,216 @@
> +/*
> + * Maxim Integrated MAX5481-MAX5484 digital potentiometer driver
> + * Copyright 2016 Rockwell Collins
> + *
> + * Datasheet:
> + * http://datasheets.maximintegrated.com/en/ds/MAX5481-MAX5484.pdf
> + *
> + * 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/acpi.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/spi/spi.h>
> +
> +/* write wiper reg */
> +#define MAX5481_WRITE_WIPER (0 << 4)
> +/* copy wiper reg to NV reg */
> +#define MAX5481_COPY_AB_TO_NV (2 << 4)
> +/* copy NV reg to wiper reg */
> +#define MAX5481_COPY_NV_TO_AB (3 << 4)
> +
> +#define MAX5481_MAX_POS    1023
> +
> +enum max5481_variant {
> +	max5481,
> +	max5482,
> +	max5483,
> +	max5484,
> +};
> +
> +struct max5481_cfg {
> +	int kohms;
> +};
> +
> +static const struct max5481_cfg max5481_cfg[] = {
> +	[max5481] = { .kohms =  10, },
> +	[max5482] = { .kohms =  50, },
> +	[max5483] = { .kohms =  10, },
> +	[max5484] = { .kohms =  50, },
> +};
> +
> +struct max5481_data {
> +	struct spi_device *spi;
> +	const struct max5481_cfg *cfg;
> +	u8 msg[3] ____cacheline_aligned;
> +};
> +
> +#define MAX5481_CHANNEL {					\
> +	.type = IIO_RESISTANCE,					\
> +	.indexed = 1,						\
> +	.output = 1,						\
> +	.channel = 0,						\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
> +}
> +
> +static const struct iio_chan_spec max5481_channels[] = {
> +	MAX5481_CHANNEL,
> +};
> +
> +static int max5481_write_cmd(struct max5481_data *data, u8 cmd, u16 val)
> +{
> +	struct spi_device *spi = data->spi;
> +
> +	data->msg[0] = cmd;
> +
> +	switch (cmd) {
> +	case MAX5481_WRITE_WIPER:
> +		data->msg[1] = val >> 2;
> +		data->msg[2] = (val & 0x3) << 6;
> +		return spi_write(spi, data->msg, ARRAY_SIZE(data->msg));
array_size will give you the number of elements in the array. Here that
is fine, but inconsistent with the use of sizeof(data->msg[0]) below.
> +
> +	case MAX5481_COPY_AB_TO_NV:
> +	case MAX5481_COPY_NV_TO_AB:
> +		return spi_write(spi, data->msg, sizeof(data->msg[0]));
> +
> +	default:
> +		return -EIO;
> +	}
> +}
> +
> +static int max5481_read_raw(struct iio_dev *indio_dev,
> +		struct iio_chan_spec const *chan,
> +		int *val, int *val2, long mask)
> +{
> +	struct max5481_data *data = iio_priv(indio_dev);
> +
> +	if (mask != IIO_CHAN_INFO_SCALE)
> +		return -EINVAL;
> +
> +	*val = 1000 * data->cfg->kohms;
> +	*val2 = MAX5481_MAX_POS;
> +
> +	return IIO_VAL_FRACTIONAL;
> +}
> +
> +static int max5481_write_raw(struct iio_dev *indio_dev,
> +		struct iio_chan_spec const *chan,
> +		int val, int val2, long mask)
> +{
> +	struct max5481_data *data = iio_priv(indio_dev);
> +
> +	if (mask != IIO_CHAN_INFO_RAW)
> +		return -EINVAL;
> +
> +	if (val < 0 || val > MAX5481_MAX_POS)
> +		return -EINVAL;
> +
> +	return max5481_write_cmd(data, MAX5481_WRITE_WIPER, val);
> +}
> +
> +static const struct iio_info max5481_info = {
> +	.read_raw = max5481_read_raw,
> +	.write_raw = max5481_write_raw,
> +	.driver_module = THIS_MODULE,
> +};
> +
> +static int max5481_probe(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev;
> +	struct max5481_data *data;
> +	const struct spi_device_id *id = spi_get_device_id(spi);
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	dev_set_drvdata(&spi->dev, indio_dev);
> +	data = iio_priv(indio_dev);
> +
> +	data->spi = spi;
To use the of data you have below this will need different handling,
specifically the use of of_match_device to get access to the data.
See something like adc/max1363 for how to do this.

> +	data->cfg = &max5481_cfg[id->driver_data];
> +
> +	indio_dev->name = id->name;
> +	indio_dev->dev.parent = &spi->dev;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	/* variant specific configuration */
> +	indio_dev->info = &max5481_info;
> +	indio_dev->channels = max5481_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(max5481_channels);
> +
> +	/* restore wiper from NV */
> +	ret = max5481_write_cmd(data, MAX5481_COPY_NV_TO_AB, 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	return iio_device_register(indio_dev);
> +}
> +
> +static int max5481_remove(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(&spi->dev);
> +	struct max5481_data *data = iio_priv(indio_dev);
> +
> +	iio_device_unregister(indio_dev);
> +
> +	/* save wiper reg to NV reg */
> +	return max5481_write_cmd(data, MAX5481_COPY_AB_TO_NV, 0);
> +}
> +
> +static const struct spi_device_id max5481_id_table[] = {
> +	{ "max5481", max5481 },
> +	{ "max5482", max5482 },
> +	{ "max5483", max5483 },
> +	{ "max5484", max5484 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(spi, max5481_id_table);
> +
> +#if defined(CONFIG_OF)
> +static const struct of_device_id max5481_match[] = {
> +	{ .compatible = "maxim,max5481", .data = &max5481_cfg[max5481] },
> +	{ .compatible = "maxim,max5482", .data = &max5481_cfg[max5482] },
> +	{ .compatible = "maxim,max5483", .data = &max5481_cfg[max5483] },
> +	{ .compatible = "maxim,max5484", .data = &max5481_cfg[max5484] },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, max5481_match);
> +#endif
> +
> +#if defined(CONFIG_ACPI)
> +static const struct acpi_device_id max5481_acpi_match[] = {
> +	{ "max5481", max5481 },
> +	{ "max5482", max5482 },
> +	{ "max5483", max5483 },
> +	{ "max5484", max5484 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(acpi, max5481_acpi_match);
> +#endif
> +
> +static struct spi_driver max5481_driver = {
> +	.driver = {
> +		.name  = "max5481",
> +		.owner = THIS_MODULE,
> +		.of_match_table = of_match_ptr(max5481_match),
> +		.acpi_match_table = ACPI_PTR(max5481_acpi_match),
> +	},
> +	.probe = max5481_probe,
> +	.remove = max5481_remove,
> +	.id_table = max5481_id_table,
> +};
> +
> +module_spi_driver(max5481_driver);
> +
> +MODULE_AUTHOR("Maury Anderson <maury.anderson-lFk7bPDcGtkY5TsXZYaR1UEOCMrvLtNR@public.gmane.org>");
> +MODULE_DESCRIPTION("max5481 SPI driver");
> +MODULE_LICENSE("GPL v2");
> 
1

^ permalink raw reply

* Re: [PATCH v4 0/2] iio: adc: Add Maxim MAX11100 driver
From: Jonathan Cameron @ 2017-01-15 14:31 UTC (permalink / raw)
  To: jacopo mondi, Jacopo Mondi,
	wsa+renesas-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/,
	magnus.damm-Re5JQEeQqe8AvxtiuMwx3w, knaack.h-Mmb7MZpHnFY,
	lars-Qo5EllUWu/uELgA04lAiVw, pmeerw-jW+XmwGofnusTnJN9+BGXg,
	marek.vasut-Re5JQEeQqe8AvxtiuMwx3w, geert-Td1EMuHUCqxL1ZNQvxDV9g,
	robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8
  Cc: linux-iio-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-renesas-soc-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <502b12a0-6011-c0eb-06b4-a40245d1d8db-AW8dsiIh9cEdnm+yROfE0A@public.gmane.org>

On 15/01/17 14:13, jacopo mondi wrote:
> Hi Jonathan,
>    thanks for review,
> 
> On 13/01/2017 10:50, Jacopo Mondi wrote:
>> Hello,
>>    sending out v4 splitting device tree bindings documentation and actual ADC
>> driver.
>> No changes in driver code since v3.
>>
>> Same question for iio maintainers here:
>> I would like to have clarified the measure unit returned by read_raw().
>> Currently (value_raw * value_scale) return the ADC input value in mV.
Good.
>> While testing the patch I've been questioned if that should not actually
>> be in uV. This is easily achievable making _scale return a value in uV.
>> I have found no mention of this in the ABI documentation as it speaks of
>> generic voltage. Can we have a final word on this?
> 
> I see you have reviewed the driver without complaining for the
> read_raw() measure unit, so I assume this replies to the above
> question as well...
> 
The units are specified in Documentation/ABI/testing/sysfs-bus-iio.

> What:		/sys/bus/iio/devices/iio:deviceX/in_voltageY_raw
> What:		/sys/bus/iio/devices/iio:deviceX/in_voltageY_supply_raw
> What:		/sys/bus/iio/devices/iio:deviceX/in_voltageY_i_raw
> What:		/sys/bus/iio/devices/iio:deviceX/in_voltageY_q_raw
> KernelVersion:	2.6.35
> Contact:	linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Description:
> 		Raw (unscaled no bias removal etc.) voltage measurement from
> 		channel Y. In special cases where the channel does not
> 		correspond to externally available input one of the named
> 		versions may be used. The number must always be specified and
> 		unique to allow association with event codes. Units after
> 		application of scale and offset are millivolts.

So millivolts.  This comes from a, perhaps ill judged, decision to match
hwmon units were we could.

Jonathan

> Thanks j
> 
>>
>> Thanks Marek for having tested this.
>>
>> v1 -> v2:
>>     - incorporated pmeerw's review comments
>>     - retrieve vref from dts and use that to convert read_raw result
>>       to mV
>>     - add device tree bindings documentation
>>
>> v2 -> v3:
>>     - add _SCALE bit of read_raw function and change _RAW bit accordingly
>>     - call regulator_get_voltage when accessing the _SCALE part of read_raw
>>       and not during probe
>>     - add back remove function as regulator has to be disabled when detaching
>>       the module. Do not use devm_ version of iio_register/unregister functions
>>       anymore but do unregister in the remove.
>>     - remove mutex as access to SPI bus is protected by SPI core. Thanks marex
>>
>> v3 -> v4:
>>     - split device tree binding documentation and actual ADC driver
>>     - add "reg" to the list of required properties and use a better
>>       namimg for the adc device node in bindings documentation as suggested
>>       by Geert.
>>
>> Jacopo Mondi (2):
>>   iio: adc: Add Maxim MAX11100 driver
>>   dt-bindings: iio: document MAX11100 ADC
>>
>>  .../devicetree/bindings/iio/adc/max11100.txt       |  19 +++
>>  drivers/iio/adc/Kconfig                            |   9 +
>>  drivers/iio/adc/Makefile                           |   1 +
>>  drivers/iio/adc/max11100.c                         | 187 +++++++++++++++++++++
>>  4 files changed, 216 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/iio/adc/max11100.txt
>>  create mode 100644 drivers/iio/adc/max11100.c
>>
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v4 0/2] iio: adc: Add Maxim MAX11100 driver
From: jacopo mondi @ 2017-01-15 14:41 UTC (permalink / raw)
  To: Jonathan Cameron, Jacopo Mondi,
	wsa+renesas-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/,
	magnus.damm-Re5JQEeQqe8AvxtiuMwx3w, knaack.h-Mmb7MZpHnFY,
	lars-Qo5EllUWu/uELgA04lAiVw, pmeerw-jW+XmwGofnusTnJN9+BGXg,
	marek.vasut-Re5JQEeQqe8AvxtiuMwx3w, geert-Td1EMuHUCqxL1ZNQvxDV9g,
	robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8
  Cc: linux-iio-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-renesas-soc-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <a4fe9cd7-4854-d916-ae30-50a7668cec4e-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

Hi Jonathan,

On 15/01/2017 15:31, Jonathan Cameron wrote:
> On 15/01/17 14:13, jacopo mondi wrote:
>> Hi Jonathan,
>>    thanks for review,
>>
>> On 13/01/2017 10:50, Jacopo Mondi wrote:
>>> Hello,
>>>    sending out v4 splitting device tree bindings documentation and actual ADC
>>> driver.
>>> No changes in driver code since v3.
>>>
>>> Same question for iio maintainers here:
>>> I would like to have clarified the measure unit returned by read_raw().
>>> Currently (value_raw * value_scale) return the ADC input value in mV.
> Good.
>>> While testing the patch I've been questioned if that should not actually
>>> be in uV. This is easily achievable making _scale return a value in uV.
>>> I have found no mention of this in the ABI documentation as it speaks of
>>> generic voltage. Can we have a final word on this?
>>
>> I see you have reviewed the driver without complaining for the
>> read_raw() measure unit, so I assume this replies to the above
>> question as well...
>>
> The units are specified in Documentation/ABI/testing/sysfs-bus-iio.
>
>> What:		/sys/bus/iio/devices/iio:deviceX/in_voltageY_raw
>> What:		/sys/bus/iio/devices/iio:deviceX/in_voltageY_supply_raw
>> What:		/sys/bus/iio/devices/iio:deviceX/in_voltageY_i_raw
>> What:		/sys/bus/iio/devices/iio:deviceX/in_voltageY_q_raw
>> KernelVersion:	2.6.35
>> Contact:	linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> Description:
>> 		Raw (unscaled no bias removal etc.) voltage measurement from
>> 		channel Y. In special cases where the channel does not
>> 		correspond to externally available input one of the named
>> 		versions may be used. The number must always be specified and
>> 		unique to allow association with event codes. Units after
>> 		application of scale and offset are millivolts.
>
> So millivolts.  This comes from a, perhaps ill judged, decision to match
> hwmon units were we could.
>

Thank you! The "generic voltage" I said is mentioned in the ABI 
documentation is in _scale attributes description.
I assume then the measure unit of the value returned by _scale is not 
relevant as long as the _raw one with scale applied  returns millivolts.

Thanks, as soon as I have feedbacks on DTS binding docs, I'll send v5 
and we can hopefully close this one ;)

Thanks
    j




> Jonathan
>
>> Thanks j
>>
>>>
>>> Thanks Marek for having tested this.
>>>
>>> v1 -> v2:
>>>     - incorporated pmeerw's review comments
>>>     - retrieve vref from dts and use that to convert read_raw result
>>>       to mV
>>>     - add device tree bindings documentation
>>>
>>> v2 -> v3:
>>>     - add _SCALE bit of read_raw function and change _RAW bit accordingly
>>>     - call regulator_get_voltage when accessing the _SCALE part of read_raw
>>>       and not during probe
>>>     - add back remove function as regulator has to be disabled when detaching
>>>       the module. Do not use devm_ version of iio_register/unregister functions
>>>       anymore but do unregister in the remove.
>>>     - remove mutex as access to SPI bus is protected by SPI core. Thanks marex
>>>
>>> v3 -> v4:
>>>     - split device tree binding documentation and actual ADC driver
>>>     - add "reg" to the list of required properties and use a better
>>>       namimg for the adc device node in bindings documentation as suggested
>>>       by Geert.
>>>
>>> Jacopo Mondi (2):
>>>   iio: adc: Add Maxim MAX11100 driver
>>>   dt-bindings: iio: document MAX11100 ADC
>>>
>>>  .../devicetree/bindings/iio/adc/max11100.txt       |  19 +++
>>>  drivers/iio/adc/Kconfig                            |   9 +
>>>  drivers/iio/adc/Makefile                           |   1 +
>>>  drivers/iio/adc/max11100.c                         | 187 +++++++++++++++++++++
>>>  4 files changed, 216 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/iio/adc/max11100.txt
>>>  create mode 100644 drivers/iio/adc/max11100.c
>>>
>>
>

^ permalink raw reply

* Re: [PATCH v4] ARM64: dts: meson-gx: Add reserved memory zone and usable memory range
From: Andreas Färber @ 2017-01-15 14:43 UTC (permalink / raw)
  To: Kevin Hilman, Neil Armstrong
  Cc: devicetree-u79uwXL29TY76Z2rM5mHXA, xypron.glpk-Mmb7MZpHnFY,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, carlo-KA+7E9HrN00dnm+yROfE0A,
	linux-amlogic-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <m21sw66828.fsf-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>

Am 13.01.2017 um 21:03 schrieb Kevin Hilman:
> Neil Armstrong <narmstrong-rdvid1DuHRBWk0Htik3J/w@public.gmane.org> writes:
> 
>> The Amlogic Meson GXBB/GXL/GXM secure monitor uses part of the memory space,
>> this patch adds this reserved zone and redefines the usable memory range.
>>
>> The memory node is also moved from the dtsi files into the proper dts files
>> to handle variants memory sizes.
>>
>> This patch also fixes the memory sizes for the following platforms :
>> - gxl-s905x-p212 : 1GiB instead of 2GiB, a proper 2GiB dts should be pushed
>> - gxm-s912-q201 : 1GiB instead of 2GiB, a proper 2GiB dts should be pushed
>> - gxl-s905d-p231 : 1GiB instead of 2GiB, a proper 2GiB dts should be pushed
>> - gxl-nexbox-a95x : 1GiB instead of 2GiB, a proper 2GiB dts should be pushed
>>
>> Signed-off-by: Neil Armstrong <narmstrong-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
> 
> Queued for v4.10-rc.

What is the motivation for this change? I have a local U-Boot patch to
detect the amount of memory available as done downstream, but U-Boot
only updates the reg property that you seem to be abandoning here...

So for devices that come in multiple RAM configurations - like R-Box Pro
- this would require separate .dts files now! This looks very wrong to
me, especially since I am not aware of other platforms doing the same.
Instead, there's memory reservations for top and bottom done in U-Boot
for reg, plus reserved-memory nodes for anything in the middle.

Another thing to consider is that uEFI boot (bootefi) handles memory
reservation differently yet again, on the bootloader level. I have had
that working fine on Odroid-C2 and Vega S95.

So if there's no bug this is fixing (none mentioned in commit message) I
strongly object to this patch.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v4 0/2] iio: adc: Add Maxim MAX11100 driver
From: Jonathan Cameron @ 2017-01-15 14:45 UTC (permalink / raw)
  To: jacopo mondi, Jacopo Mondi,
	wsa+renesas-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/,
	magnus.damm-Re5JQEeQqe8AvxtiuMwx3w, knaack.h-Mmb7MZpHnFY,
	lars-Qo5EllUWu/uELgA04lAiVw, pmeerw-jW+XmwGofnusTnJN9+BGXg,
	marek.vasut-Re5JQEeQqe8AvxtiuMwx3w, geert-Td1EMuHUCqxL1ZNQvxDV9g,
	robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8
  Cc: linux-iio-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-renesas-soc-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <a1866192-dd3b-7ea8-bb03-1bbbf0678226-AW8dsiIh9cEdnm+yROfE0A@public.gmane.org>

On 15/01/17 14:41, jacopo mondi wrote:
> Hi Jonathan,
> 
> On 15/01/2017 15:31, Jonathan Cameron wrote:
>> On 15/01/17 14:13, jacopo mondi wrote:
>>> Hi Jonathan,
>>>    thanks for review,
>>>
>>> On 13/01/2017 10:50, Jacopo Mondi wrote:
>>>> Hello,
>>>>    sending out v4 splitting device tree bindings documentation and actual ADC
>>>> driver.
>>>> No changes in driver code since v3.
>>>>
>>>> Same question for iio maintainers here:
>>>> I would like to have clarified the measure unit returned by read_raw().
>>>> Currently (value_raw * value_scale) return the ADC input value in mV.
>> Good.
>>>> While testing the patch I've been questioned if that should not actually
>>>> be in uV. This is easily achievable making _scale return a value in uV.
>>>> I have found no mention of this in the ABI documentation as it speaks of
>>>> generic voltage. Can we have a final word on this?
>>>
>>> I see you have reviewed the driver without complaining for the
>>> read_raw() measure unit, so I assume this replies to the above
>>> question as well...
>>>
>> The units are specified in Documentation/ABI/testing/sysfs-bus-iio.
>>
>>> What:        /sys/bus/iio/devices/iio:deviceX/in_voltageY_raw
>>> What:        /sys/bus/iio/devices/iio:deviceX/in_voltageY_supply_raw
>>> What:        /sys/bus/iio/devices/iio:deviceX/in_voltageY_i_raw
>>> What:        /sys/bus/iio/devices/iio:deviceX/in_voltageY_q_raw
>>> KernelVersion:    2.6.35
>>> Contact:    linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>>> Description:
>>>         Raw (unscaled no bias removal etc.) voltage measurement from
>>>         channel Y. In special cases where the channel does not
>>>         correspond to externally available input one of the named
>>>         versions may be used. The number must always be specified and
>>>         unique to allow association with event codes. Units after
>>>         application of scale and offset are millivolts.
>>
>> So millivolts.  This comes from a, perhaps ill judged, decision to match
>> hwmon units were we could.
>>
> 
> Thank you! The "generic voltage" I said is mentioned in the ABI
> documentation is in _scale attributes description. I assume then the
> measure unit of the value returned by _scale is not relevant as long
> as the _raw one with scale applied returns millivolts.
Yes.  Scale is inherently a ratio really rather than a quantity with units
at all.  It just happens to correspond to the voltage represented by 1LSB.
> 
> Thanks, as soon as I have feedbacks on DTS binding docs, I'll send v5
> and we can hopefully close this one ;)
Cool.  Got a while in this cycle yet, but I know it's always nice to get
patches out of your personal queue!

Jonathan
> 
> Thanks j
> 
> 
> 
>> Jonathan
>>
>>> Thanks j
>>>
>>>>
>>>> Thanks Marek for having tested this.
>>>>
>>>> v1 -> v2:
>>>>     - incorporated pmeerw's review comments
>>>>     - retrieve vref from dts and use that to convert read_raw result
>>>>       to mV
>>>>     - add device tree bindings documentation
>>>>
>>>> v2 -> v3:
>>>>     - add _SCALE bit of read_raw function and change _RAW bit accordingly
>>>>     - call regulator_get_voltage when accessing the _SCALE part of read_raw
>>>>       and not during probe
>>>>     - add back remove function as regulator has to be disabled when detaching
>>>>       the module. Do not use devm_ version of iio_register/unregister functions
>>>>       anymore but do unregister in the remove.
>>>>     - remove mutex as access to SPI bus is protected by SPI core. Thanks marex
>>>>
>>>> v3 -> v4:
>>>>     - split device tree binding documentation and actual ADC driver
>>>>     - add "reg" to the list of required properties and use a better
>>>>       namimg for the adc device node in bindings documentation as suggested
>>>>       by Geert.
>>>>
>>>> Jacopo Mondi (2):
>>>>   iio: adc: Add Maxim MAX11100 driver
>>>>   dt-bindings: iio: document MAX11100 ADC
>>>>
>>>>  .../devicetree/bindings/iio/adc/max11100.txt       |  19 +++
>>>>  drivers/iio/adc/Kconfig                            |   9 +
>>>>  drivers/iio/adc/Makefile                           |   1 +
>>>>  drivers/iio/adc/max11100.c                         | 187 +++++++++++++++++++++
>>>>  4 files changed, 216 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/iio/adc/max11100.txt
>>>>  create mode 100644 drivers/iio/adc/max11100.c
>>>>
>>>
>>
> 
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v4 0/2] iio: adc: Add Maxim MAX11100 driver
From: Marek Vasut @ 2017-01-15 15:41 UTC (permalink / raw)
  To: Jonathan Cameron, jacopo mondi, Jacopo Mondi,
	wsa+renesas-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/,
	magnus.damm-Re5JQEeQqe8AvxtiuMwx3w, knaack.h-Mmb7MZpHnFY,
	lars-Qo5EllUWu/uELgA04lAiVw, pmeerw-jW+XmwGofnusTnJN9+BGXg,
	geert-Td1EMuHUCqxL1ZNQvxDV9g, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
	mark.rutland-5wv7dgnIgG8
  Cc: linux-iio-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-renesas-soc-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <a4fe9cd7-4854-d916-ae30-50a7668cec4e-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

On 01/15/2017 03:31 PM, Jonathan Cameron wrote:
> On 15/01/17 14:13, jacopo mondi wrote:
>> Hi Jonathan,
>>    thanks for review,
>>
>> On 13/01/2017 10:50, Jacopo Mondi wrote:
>>> Hello,
>>>    sending out v4 splitting device tree bindings documentation and actual ADC
>>> driver.
>>> No changes in driver code since v3.
>>>
>>> Same question for iio maintainers here:
>>> I would like to have clarified the measure unit returned by read_raw().
>>> Currently (value_raw * value_scale) return the ADC input value in mV.
> Good.
>>> While testing the patch I've been questioned if that should not actually
>>> be in uV. This is easily achievable making _scale return a value in uV.
>>> I have found no mention of this in the ABI documentation as it speaks of
>>> generic voltage. Can we have a final word on this?
>>
>> I see you have reviewed the driver without complaining for the
>> read_raw() measure unit, so I assume this replies to the above
>> question as well...
>>
> The units are specified in Documentation/ABI/testing/sysfs-bus-iio.
> 
>> What:		/sys/bus/iio/devices/iio:deviceX/in_voltageY_raw
>> What:		/sys/bus/iio/devices/iio:deviceX/in_voltageY_supply_raw
>> What:		/sys/bus/iio/devices/iio:deviceX/in_voltageY_i_raw
>> What:		/sys/bus/iio/devices/iio:deviceX/in_voltageY_q_raw
>> KernelVersion:	2.6.35
>> Contact:	linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> Description:
>> 		Raw (unscaled no bias removal etc.) voltage measurement from
>> 		channel Y. In special cases where the channel does not
>> 		correspond to externally available input one of the named
>> 		versions may be used. The number must always be specified and
>> 		unique to allow association with event codes. Units after
>> 		application of scale and offset are millivolts.
> 
> So millivolts.  This comes from a, perhaps ill judged, decision to match
> hwmon units were we could.

Fun, regulator framework returns values in uV to add to the chaos :)


-- 
Best regards,
Marek Vasut

^ permalink raw reply

* Re: [RFT PATCH] ARM64: dts: meson-gxbb: Add reserved memory zone and usable memory range
From: Andreas Färber @ 2017-01-15 15:44 UTC (permalink / raw)
  To: Heinrich Schuchardt
  Cc: Neil Armstrong, khilman, carlo, linux-amlogic, linux-kernel,
	linux-arm-kernel, devicetree
In-Reply-To: <90d5b5a3-6dd8-aab2-8701-112a10e3364c@gmx.de>

Am 23.12.2016 um 10:42 schrieb Heinrich Schuchardt:
> it really makes a difference if we write
> 
>  	memory@0 {
>  		device_type = "memory";
>  		linux,usable-memory = <0x0 0x1000000 0x0 0x7f000000>;
>  	};
> 
> or
> 
>  	memory@0 {
>  		device_type = "memory";
>  		reg = <0x0 0x1000000 0x0 0x7f000000>;
>  	};
> 
> The second version leads to failure of the Odroid C2.
> 
> When I looked at /sys/firmware/fdt I saw this difference:
> 
> --- fails
> +++ works
> 
>         memory@0 {
> -               device_type = "memory";
>                 reg = <0x0 0x0 0x0 0x78000000>;
> +               device_type = "memory";
> +               linux,usable-memory = <0x0 0x1000000 0x0 0x7f000000>;
>         };
> 
> I found the following sentence in the NXP forum:
> In case you want to overwrite the memory usage passed from u-boot, you
> can use "linux,usable-memory".
> https://community.nxp.com/thread/382284

The Odroid-C2 is in mainline U-Boot. Please submit a patch to U-Boot
instead of forcing the creation of unnecessary new .dts files onto
everyone due to hardcoded linux,usable-memory properties. In fact, it
already reserves 0x1000000, so it seems you are merely using an older
U-Boot.

http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/mach-meson/board.c;h=f159cbf849f75ab046e6f3a025bbc97c0bcfd59d;hb=HEAD#l39

I would bet that the upper limit is unrelated here.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

^ permalink raw reply

* Dropping device tree pinmux nodes for GPIO usage (Was: [PATCH 3/5] arm64: dts: sun50i: add MMC nodes)
From: Rask Ingemann Lambertsen @ 2017-01-15 15:59 UTC (permalink / raw)
  To: Chen-Yu Tsai
  Cc: Mark Rutland, devicetree, Ulf Hansson, Andre Przywara,
	linux-mmc@vger.kernel.org, linux-kernel, Hans De Goede,
	linux-sunxi, Rob Herring, Icenowy Zheng, Maxime Ripard,
	linux-arm-kernel
In-Reply-To: <CAGb2v65GJFTnQZtWHCS7CAtA+Dry94o77aakPLr938BpQksLog@mail.gmail.com>

On Tue, Jan 03, 2017 at 10:52:12AM +0800, Chen-Yu Tsai wrote:
> On Tue, Jan 3, 2017 at 7:03 AM, Andre Przywara <andre.przywara@arm.com> wrote:
> > +
> > +                       mmc0_default_cd_pin: mmc0_cd_pin@0 {
> > +                               pins = "PF6";
> > +                               function = "gpio_in";
> > +                               bias-pull-up;
> > +                       };
> 
> We are starting to drop pinmux nodes for gpio usage.

How do we get the equivalent of bias-pull-up/down and drive-strength if we
run across a pin that needs it?

-- 
Rask Ingemann Lambertsen

^ permalink raw reply

* Re: [PATCH linux v2 5/6] hwmon: occ: Add hwmon implementation for the P8 OCC
From: Guenter Roeck @ 2017-01-15 18:15 UTC (permalink / raw)
  To: eajames.ibm
  Cc: devicetree, jdelvare, corbet, linux-doc, linux-hwmon, linux-i2c,
	linux-kernel, mark.rutland, robh+dt, wsa, andrew, joel, benh,
	Edward A. James
In-Reply-To: <1484158237-10014-6-git-send-email-eajames.ibm@gmail.com>

On 01/11/2017 10:10 AM, eajames.ibm@gmail.com wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
>
> Add code to tie the hwmon sysfs code and the POWER8 OCC code together, as
> well as probe the entire driver from the I2C bus. I2C is the communication
> method between the BMC and the P8 OCC.
>
> Signed-off-by: Edward A. James <eajames@us.ibm.com>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>  Documentation/devicetree/bindings/hwmon/occ.txt |  13 +++
>  drivers/hwmon/occ/Kconfig                       |  14 +++
>  drivers/hwmon/occ/Makefile                      |   1 +
>  drivers/hwmon/occ/p8_occ_i2c.c                  | 123 ++++++++++++++++++++++++
>  4 files changed, 151 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/occ.txt
>  create mode 100644 drivers/hwmon/occ/p8_occ_i2c.c
>
> diff --git a/Documentation/devicetree/bindings/hwmon/occ.txt b/Documentation/devicetree/bindings/hwmon/occ.txt
> new file mode 100644
> index 0000000..b0d2b36
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/occ.txt
> @@ -0,0 +1,13 @@
> +HWMON I2C driver for IBM POWER CPU OCC (On Chip Controller)
> +
> +Required properties:
> + - compatible: must be "ibm,p8-occ-i2c"
> + - reg: physical address
> +
> +Example:
> +i2c3: i2c-bus@100 {
> +	occ@50 {
> +		compatible = "ibm,p8-occ-i2c";
> +		reg = <0x50>;
> +	};
> +};
> diff --git a/drivers/hwmon/occ/Kconfig b/drivers/hwmon/occ/Kconfig
> index cdb64a7..3a5188f 100644
> --- a/drivers/hwmon/occ/Kconfig
> +++ b/drivers/hwmon/occ/Kconfig
> @@ -13,3 +13,17 @@ menuconfig SENSORS_PPC_OCC
>
>  	  This driver can also be built as a module. If so, the module
>  	  will be called occ.
> +
> +if SENSORS_PPC_OCC
> +
> +config SENSORS_PPC_OCC_P8_I2C
> +	tristate "POWER8 OCC hwmon support"
> +	depends on I2C
> +	help
> +	 Provide a hwmon sysfs interface for the POWER8 On-Chip Controller,
> +	 exposing temperature, frequency and power measurements.
> +
> +	 This driver can also be built as a module. If so, the module will be
> +	 called p8-occ-i2c.
> +
> +endif
> diff --git a/drivers/hwmon/occ/Makefile b/drivers/hwmon/occ/Makefile
> index a6881f9..9294b58 100644
> --- a/drivers/hwmon/occ/Makefile
> +++ b/drivers/hwmon/occ/Makefile
> @@ -1 +1,2 @@
>  obj-$(CONFIG_SENSORS_PPC_OCC) += occ.o occ_sysfs.o
> +obj-$(CONFIG_SENSORS_PPC_OCC_P8_I2C) += occ_scom_i2c.o occ_p8.o p8_occ_i2c.o
> diff --git a/drivers/hwmon/occ/p8_occ_i2c.c b/drivers/hwmon/occ/p8_occ_i2c.c
> new file mode 100644
> index 0000000..4515c68
> --- /dev/null
> +++ b/drivers/hwmon/occ/p8_occ_i2c.c
> @@ -0,0 +1,123 @@
> +/*
> + * p8_occ_i2c.c - hwmon OCC driver
> + *
> + * This file contains the i2c layer for accessing the P8 OCC over i2c bus.
> + *
> + * Copyright 2016 IBM Corp.
> + *
> + * 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.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/i2c.h>
> +#include <linux/err.h>
> +#include <linux/of.h>
> +#include <linux/kernel.h>
> +#include <linux/device.h>

For all patches: alphabetic order, please.

> +
> +#include "scom.h"
> +#include "occ_scom_i2c.h"
> +#include "occ_p8.h"
> +#include "occ_sysfs.h"
> +
> +#define P8_OCC_I2C_NAME	"p8-occ-i2c"
> +
> +int p8_i2c_getscom(void *bus, u32 address, u64 *data)
> +{
> +	/* P8 i2c slave requires address to be shifted by 1 */
> +	address = address << 1;
> +
> +	return occ_i2c_getscom(bus, address, data);
> +}
> +
> +int p8_i2c_putscom(void *bus, u32 address, u32 data0, u32 data1)
> +{
> +	/* P8 i2c slave requires address to be shifted by 1 */
> +	address = address << 1;
> +
> +	return occ_i2c_putscom(bus, address, data0, data1);
> +}
> +
> +static struct occ_bus_ops p8_bus_ops = {
> +	.getscom = p8_i2c_getscom,
> +	.putscom = p8_i2c_putscom,
> +};
> +
> +static int p8_occ_probe(struct i2c_client *client,
> +			const struct i2c_device_id *id)
> +{
> +	struct occ *occ;
> +	struct occ_sysfs *hwmon;
> +	const u32 *sensor_hwmon_configs = p8_get_sensor_hwmon_configs();
> +
> +	occ = p8_occ_start(&client->dev, client, &p8_bus_ops);
> +	if (IS_ERR(occ))
> +		return PTR_ERR(occ);
> +
> +	hwmon = occ_sysfs_start(&client->dev, occ, sensor_hwmon_configs,
> +				P8_OCC_I2C_NAME);
> +	if (IS_ERR(hwmon))
> +		return PTR_ERR(hwmon);
> +
> +	i2c_set_clientdata(client, occ);

This maps to
	dev_set_drvdata(&client->dev, occ);
and code in occ_sysfs_start() does the same, writing a reference to the hwmon device.

> +
> +	return 0;
> +}
> +
> +static int p8_occ_remove(struct i2c_client *client)
> +{
> +	struct occ *occ = i2c_get_clientdata(client);
> +
> +	return p8_occ_stop(occ);
> +}
> +
> +/* used by old-style board info. */
> +static const struct i2c_device_id occ_ids[] = {
> +	{ P8_OCC_I2C_NAME, 0 },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, occ_ids);
> +
> +/* used by device table */
> +static const struct of_device_id occ_of_match[] = {
> +	{ .compatible = "ibm,p8-occ-i2c" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, occ_of_match);
> +
> +/*
> + * i2c-core uses i2c-detect() to detect device in below address list.
> + * If exists, address will be assigned to client.
> + * It is also possible to read address from device table.
> + */
> +static const unsigned short normal_i2c[] = {0x50, 0x51, I2C_CLIENT_END };
> +
> +static struct i2c_driver p8_occ_driver = {
> +	.class = I2C_CLASS_HWMON,
> +	.driver = {
> +		.name = P8_OCC_I2C_NAME,
> +		.pm = NULL,
> +		.of_match_table = occ_of_match,
> +	},
> +	.probe = p8_occ_probe,
> +	.remove = p8_occ_remove,
> +	.id_table = occ_ids,
> +	.address_list = normal_i2c,
> +};
> +
> +module_i2c_driver(p8_occ_driver);
> +
> +MODULE_AUTHOR("Eddie James <eajames@us.ibm.com>");
> +MODULE_DESCRIPTION("BMC P8 OCC hwmon driver");
> +MODULE_LICENSE("GPL");
>


^ permalink raw reply

* Re: [PATCH linux v2 2/6] hwmon: occ: Add sysfs interface
From: Guenter Roeck @ 2017-01-15 18:18 UTC (permalink / raw)
  To: eajames.ibm
  Cc: devicetree, jdelvare, corbet, linux-doc, linux-hwmon, linux-i2c,
	linux-kernel, mark.rutland, robh+dt, wsa, andrew, joel, benh,
	Edward A. James
In-Reply-To: <1484158237-10014-3-git-send-email-eajames.ibm@gmail.com>

On 01/11/2017 10:10 AM, eajames.ibm@gmail.com wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
>
> Add a generic mechanism to expose the sensors provided by the OCC in
> sysfs.
>
> Signed-off-by: Edward A. James <eajames@us.ibm.com>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>  Documentation/hwmon/occ       |  62 ++++++++++
>  drivers/hwmon/occ/Makefile    |   2 +-
>  drivers/hwmon/occ/occ_sysfs.c | 274 ++++++++++++++++++++++++++++++++++++++++++
>  drivers/hwmon/occ/occ_sysfs.h |  44 +++++++
>  4 files changed, 381 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/hwmon/occ/occ_sysfs.c
>  create mode 100644 drivers/hwmon/occ/occ_sysfs.h
>
> diff --git a/Documentation/hwmon/occ b/Documentation/hwmon/occ
> index 79d1642..d0bdf06 100644
> --- a/Documentation/hwmon/occ
> +++ b/Documentation/hwmon/occ
> @@ -25,6 +25,68 @@ Currently, all versions of the OCC support four types of sensor data: power,
>  temperature, frequency, and "caps," which indicate limits and thresholds used
>  internally on the OCC.
>
> +sysfs Entries
> +-------------
> +
> +The OCC driver uses the hwmon sysfs framework to provide data to userspace.
> +
> +The driver exports a number of sysfs files for each type of sensor. The
> +sensor-specific files vary depending on the processor type, though many of the
> +attributes are common for both the POWER8 and POWER9.
> +
> +The hwmon interface cannot define every type of sensor that may be used.
> +Therefore, the frequency sensor on the OCC uses the "input" type sensor defined
> +by the hwmon interface, rather than defining a new type of custom sensor.
> +
> +Below are detailed the names and meaning of each sensor file for both types of
> +processors. All sensors are read-only unless otherwise specified. <x> indicates
> +the hwmon index. sensor id indicates the unique internal OCC identifer. Please
> +see the POWER OCC specification for details on all these sensor values.
> +
> +frequency:
> +	all processors:
> +		in<x>_input - frequency value
> +		in<x>_label - sensor id
> +temperature:
> +	POWER8:
> +		temp<x>_input - temperature value
> +		temp<x>_label - sensor id
> +	POWER9 (in addition to above):
> +		temp<x>_type - FRU type
> +
> +power:
> +	POWER8:
> +		power<x>_input - power value
> +		power<x>_label - sensor id
> +		power<x>_average - accumulator
> +		power<x>_average_interval - update tag (number of samples in
> +			accumulator)
> +	POWER9:
> +		power<x>_input - power value
> +		power<x>_label - sensor id
> +		power<x>_average_min - accumulator[0]
> +		power<x>_average_max - accumulator[1] (64 bits total)
> +		power<x>_average_interval - update tag
> +		power<x>_reset_history - (function_id | (apss_channel << 8)
> +
> +caps:
> +	POWER8:
> +		power<x>_cap - current powercap
> +		power<x>_cap_max - max powercap
> +		power<x>_cap_min - min powercap
> +		power<x>_max - normal powercap
> +		power<x>_alarm - user powercap, r/w
> +	POWER9:
> +		power<x>_cap_alarm - user powercap source
> +
> +The driver also provides two sysfs entries through hwmon to better
> +control the driver and monitor the master OCC. Though there may be multiple
> +OCCs present on the system, these two files are only present for the "master"
> +OCC.
> +	name - read the name of the driver
> +	update_interval - read or write the minimum interval for polling the
> +		OCC.
> +
>  BMC - Host Communications
>  -------------------------
>
> diff --git a/drivers/hwmon/occ/Makefile b/drivers/hwmon/occ/Makefile
> index 93cb52f..a6881f9 100644
> --- a/drivers/hwmon/occ/Makefile
> +++ b/drivers/hwmon/occ/Makefile
> @@ -1 +1 @@
> -obj-$(CONFIG_SENSORS_PPC_OCC) += occ.o
> +obj-$(CONFIG_SENSORS_PPC_OCC) += occ.o occ_sysfs.o
> diff --git a/drivers/hwmon/occ/occ_sysfs.c b/drivers/hwmon/occ/occ_sysfs.c
> new file mode 100644
> index 0000000..e846b0c
> --- /dev/null
> +++ b/drivers/hwmon/occ/occ_sysfs.c
> @@ -0,0 +1,274 @@
> +/*
> + * occ_sysfs.c - OCC sysfs interface
> + *
> + * This file contains the methods and data structures for implementing the OCC
> + * hwmon sysfs entries.
> + *
> + * Copyright 2016 IBM Corp.
> + *
> + * 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.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/jiffies.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/err.h>
> +#include <linux/mutex.h>
> +#include <linux/delay.h>
> +#include <linux/kernel.h>
> +#include <linux/device.h>
> +
Again, for all patches: alphabetic order, please.

> +#include "occ.h"
> +#include "occ_sysfs.h"
> +
> +#define RESP_RETURN_CMD_INVAL	0x13
> +
> +static int occ_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> +			  u32 attr, int channel, long *val)
> +{
> +	int rc = 0;
> +	struct occ_sysfs *driver = dev_get_drvdata(dev);
> +	struct occ *occ = driver->occ;
> +
> +	switch (type) {
> +	case hwmon_in:
> +		rc = occ_get_sensor_value(occ, FREQ, channel, attr, val);
> +		break;
> +	case hwmon_temp:
> +		rc = occ_get_sensor_value(occ, TEMP, channel, attr, val);
> +		break;
> +	case hwmon_power:
> +		rc = occ_get_sensor_value(occ, POWER, channel, attr, val);
> +		break;
> +	default:
> +		rc = -EOPNOTSUPP;
> +	}
> +
> +	return rc;
> +}
> +
> +static int occ_hwmon_read_string(struct device *dev,
> +				 enum hwmon_sensor_types type, u32 attr,
> +				 int channel, char **str)
> +{
> +	int rc;
> +	unsigned long val = 0;
> +
> +	if (!((type == hwmon_in && attr == hwmon_in_label) ||
> +	    (type == hwmon_temp && attr == hwmon_temp_label) ||
> +	    (type == hwmon_power && attr == hwmon_power_label)))
> +		return -EOPNOTSUPP;
> +
> +	rc = occ_hwmon_read(dev, type, attr, channel, &val);
> +	if (rc < 0)
> +		return rc;
> +
> +	rc = snprintf(*str, PAGE_SIZE - 1, "%ld", val);
> +	if (rc > 0)
> +		rc = 0;
> +
> +	return rc;
> +}
> +
> +static int occ_hwmon_write(struct device *dev, enum hwmon_sensor_types type,
> +			   u32 attr, int channel, long val)
> +{
> +	int rc = 0;
> +	struct occ_sysfs *driver = dev_get_drvdata(dev);
> +
> +	if (type == hwmon_chip && attr == hwmon_chip_update_interval) {
> +		occ_set_update_interval(driver->occ, val);
> +		return 0;
> +	} else if (type == hwmon_power && attr == hwmon_power_alarm) {
> +		rc = occ_set_user_powercap(driver->occ, val);
> +		if (rc) {
> +			if (rc == RESP_RETURN_CMD_INVAL) {
> +				dev_err(dev,
> +					"set invalid powercap value: %ld\n",
> +					val);
> +				return -EINVAL;
> +			}
> +
> +			dev_err(dev, "set user powercap failed: 0x:%x\n", rc);
> +			return rc;
> +		}
> +
> +		driver->user_powercap = val;
> +
> +		return rc;
> +	}
> +
> +	return -EOPNOTSUPP;
> +}
> +
> +static umode_t occ_is_visible(const void *data, enum hwmon_sensor_types type,
> +			      u32 attr, int channel)
> +{
> +	const struct occ_sysfs *driver = data;
> +
> +	switch (type) {
> +	case hwmon_chip:
> +		if (attr == hwmon_chip_update_interval)
> +			return S_IRUGO | S_IWUSR;
> +		break;
> +	case hwmon_in:
> +		if (BIT(attr) & driver->sensor_hwmon_configs[0])
> +			return S_IRUGO;
> +		break;
> +	case hwmon_temp:
> +		if (BIT(attr) & driver->sensor_hwmon_configs[1])
> +			return S_IRUGO;
> +		break;
> +	case hwmon_power:
> +		/* user power limit */
> +		if (attr == hwmon_power_alarm)
> +			return S_IRUGO | S_IWUSR;
> +		else if ((BIT(attr) & driver->sensor_hwmon_configs[2]) ||
> +			 (BIT(attr) & driver->sensor_hwmon_configs[3]))
> +			return S_IRUGO;
> +		break;
> +	default:
> +		return 0;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct hwmon_ops occ_hwmon_ops = {
> +	.is_visible = occ_is_visible,
> +	.read = occ_hwmon_read,
> +	.read_string = occ_hwmon_read_string,
> +	.write = occ_hwmon_write,
> +};
> +
> +static const u32 occ_chip_config[] = {
> +	HWMON_C_UPDATE_INTERVAL,
> +	0
> +};
> +
> +static const struct hwmon_channel_info occ_chip = {
> +	.type = hwmon_chip,
> +	.config = occ_chip_config
> +};
> +
> +static const enum hwmon_sensor_types occ_sensor_types[MAX_OCC_SENSOR_TYPE] = {
> +	hwmon_in,
> +	hwmon_temp,
> +	hwmon_power,
> +	hwmon_power
> +};
> +
> +struct occ_sysfs *occ_sysfs_start(struct device *dev, struct occ *occ,
> +				  const u32 *sensor_hwmon_configs,
> +				  const char *name)
> +{
> +	bool master_occ = false;
> +	int rc, i, j, sensor_num, index = 0, id;
> +	char *brk;
> +	struct occ_blocks *resp = NULL;
> +	u32 *sensor_config;
> +	struct occ_sysfs *hwmon = devm_kzalloc(dev, sizeof(struct occ_sysfs),
> +					       GFP_KERNEL);
> +	if (!hwmon)
> +		return ERR_PTR(-ENOMEM);
> +
> +	/* need space for null-termination and occ chip */
> +	hwmon->occ_sensors =
> +		devm_kzalloc(dev, sizeof(struct hwmon_channel_info *) *
> +			     (MAX_OCC_SENSOR_TYPE + 2), GFP_KERNEL);
> +	if (!hwmon->occ_sensors)
> +		return ERR_PTR(-ENOMEM);
> +
> +	hwmon->occ = occ;
> +	hwmon->sensor_hwmon_configs = (u32 *)sensor_hwmon_configs;
> +	hwmon->occ_info.ops = &occ_hwmon_ops;
> +	hwmon->occ_info.info =
> +		(const struct hwmon_channel_info **)hwmon->occ_sensors;
> +
> +	dev_set_drvdata(dev, hwmon);
> +
Overwritten in the calling code. It is actually not needed here because dev_get_drvdata()
in the access functions gets the value from the hwmon device, and that is set with
the 3rd parameter of devm_hwmon_device_register_with_info().

> +	occ_get_response_blocks(occ, &resp);
> +
> +	for (i = 0; i < MAX_OCC_SENSOR_TYPE; ++i)
> +		resp->sensor_block_id[i] = -1;
> +
> +	/* read sensor data from occ */
> +	rc = occ_update_device(occ);
> +	if (rc) {
> +		dev_err(dev, "cannot get occ sensor data: %d\n", rc);
> +		return ERR_PTR(rc);
> +	}
> +	if (!resp->blocks)
> +		return ERR_PTR(-ENOMEM);
> +
> +	master_occ = resp->sensor_block_id[CAPS] >= 0;
> +
> +	for (i = 0; i < MAX_OCC_SENSOR_TYPE; i++) {
> +		id = resp->sensor_block_id[i];
> +		if (id < 0)
> +			continue;
> +
> +		sensor_num = resp->blocks[id].header.sensor_num;
> +		/* need null-termination */
> +		sensor_config = devm_kzalloc(dev,
> +					     sizeof(u32) * (sensor_num + 1),
> +					     GFP_KERNEL);
> +		if (!sensor_config)
> +			return ERR_PTR(-ENOMEM);
> +
> +		for (j = 0; j < sensor_num; j++)
> +			sensor_config[j] = sensor_hwmon_configs[i];
> +
> +		hwmon->occ_sensors[index] =
> +			devm_kzalloc(dev, sizeof(struct hwmon_channel_info),
> +				     GFP_KERNEL);
> +		if (!hwmon->occ_sensors[index])
> +			return ERR_PTR(-ENOMEM);
> +
> +		hwmon->occ_sensors[index]->type = occ_sensor_types[i];
> +		hwmon->occ_sensors[index]->config = sensor_config;
> +		index++;
> +	}
> +
> +	/* only need one of these for any number of occs */
> +	if (master_occ)
> +		hwmon->occ_sensors[index] =
> +			(struct hwmon_channel_info *)&occ_chip;
> +
> +	/* search for bad chars */
> +	strncpy(hwmon->hwmon_name, name, OCC_HWMON_NAME_LENGTH);
> +	brk = strpbrk(hwmon->hwmon_name, "-* \t\n");
> +	while (brk) {
> +		*brk = '_';
> +		brk = strpbrk(brk,  "-* \t\n");
> +	}
> +
> +	hwmon->dev = devm_hwmon_device_register_with_info(dev,
> +							  hwmon->hwmon_name,
> +							  hwmon,
> +							  &hwmon->occ_info,
> +							  NULL);
> +	if (IS_ERR(hwmon->dev)) {
> +		dev_err(dev, "cannot register hwmon device %s: %ld\n",
> +			hwmon->hwmon_name, PTR_ERR(hwmon->dev));
> +		return ERR_PTR(PTR_ERR(hwmon->dev));
> +	}
> +
> +	return hwmon;
> +}
> +EXPORT_SYMBOL(occ_sysfs_start);
> +
> +MODULE_AUTHOR("Eddie James <eajames@us.ibm.com>");
> +MODULE_DESCRIPTION("OCC sysfs driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/hwmon/occ/occ_sysfs.h b/drivers/hwmon/occ/occ_sysfs.h
> new file mode 100644
> index 0000000..7de92e7
> --- /dev/null
> +++ b/drivers/hwmon/occ/occ_sysfs.h
> @@ -0,0 +1,44 @@
> +/*
> + * occ_sysfs.h - OCC sysfs interface
> + *
> + * This file contains the data structures and function prototypes for the OCC
> + * hwmon sysfs entries.
> + *
> + * Copyright 2016 IBM Corp.
> + *
> + * 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.
> + */
> +
> +#ifndef __OCC_SYSFS_H__
> +#define __OCC_SYSFS_H__
> +
> +#include <linux/hwmon.h>
> +
> +struct occ;
> +struct device;
> +
> +#define OCC_HWMON_NAME_LENGTH	32
> +
> +struct occ_sysfs {
> +	struct device *dev;
> +	struct occ *occ;
> +
> +	char hwmon_name[OCC_HWMON_NAME_LENGTH + 1];
> +	u32 *sensor_hwmon_configs;
> +	struct hwmon_channel_info **occ_sensors;
> +	struct hwmon_chip_info occ_info;
> +	u16 user_powercap;
> +};
> +
> +struct occ_sysfs *occ_sysfs_start(struct device *dev, struct occ *occ,
> +				  const u32 *sensor_hwmon_configs,
> +				  const char *name);
> +#endif /* __OCC_SYSFS_H__ */
>


^ permalink raw reply

* Re: [PATCH linux v2 5/6] hwmon: occ: Add hwmon implementation for the P8 OCC
From: Guenter Roeck @ 2017-01-15 18:34 UTC (permalink / raw)
  To: eajames.ibm-Re5JQEeQqe8AvxtiuMwx3w
  Cc: devicetree-u79uwXL29TY76Z2rM5mHXA, jdelvare-IBi9RG/b67k,
	corbet-T1hC0tSOHrs, linux-doc-u79uwXL29TY76Z2rM5mHXA,
	linux-hwmon-u79uwXL29TY76Z2rM5mHXA,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, mark.rutland-5wv7dgnIgG8,
	robh+dt-DgEjT+Ai2ygdnm+yROfE0A, wsa-z923LK4zBo2bacvFa/9K2g,
	andrew-zrmu5oMJ5Fs, joel-U3u1mxZcP9KHXe+LvDLADg,
	benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r, Edward A. James
In-Reply-To: <1484158237-10014-6-git-send-email-eajames.ibm-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

On 01/11/2017 10:10 AM, eajames.ibm-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
> From: "Edward A. James" <eajames-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
>
> Add code to tie the hwmon sysfs code and the POWER8 OCC code together, as
> well as probe the entire driver from the I2C bus. I2C is the communication
> method between the BMC and the P8 OCC.
>
> Signed-off-by: Edward A. James <eajames-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Andrew Jeffery <andrew-zrmu5oMJ5Fs@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/hwmon/occ.txt |  13 +++
>  drivers/hwmon/occ/Kconfig                       |  14 +++
>  drivers/hwmon/occ/Makefile                      |   1 +
>  drivers/hwmon/occ/p8_occ_i2c.c                  | 123 ++++++++++++++++++++++++
>  4 files changed, 151 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/occ.txt
>  create mode 100644 drivers/hwmon/occ/p8_occ_i2c.c
>
> diff --git a/Documentation/devicetree/bindings/hwmon/occ.txt b/Documentation/devicetree/bindings/hwmon/occ.txt
> new file mode 100644
> index 0000000..b0d2b36
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/occ.txt
> @@ -0,0 +1,13 @@
> +HWMON I2C driver for IBM POWER CPU OCC (On Chip Controller)
> +
> +Required properties:
> + - compatible: must be "ibm,p8-occ-i2c"
> + - reg: physical address
> +
> +Example:
> +i2c3: i2c-bus@100 {
> +	occ@50 {
> +		compatible = "ibm,p8-occ-i2c";
> +		reg = <0x50>;
> +	};
> +};
> diff --git a/drivers/hwmon/occ/Kconfig b/drivers/hwmon/occ/Kconfig
> index cdb64a7..3a5188f 100644
> --- a/drivers/hwmon/occ/Kconfig
> +++ b/drivers/hwmon/occ/Kconfig
> @@ -13,3 +13,17 @@ menuconfig SENSORS_PPC_OCC
>
>  	  This driver can also be built as a module. If so, the module
>  	  will be called occ.
> +
> +if SENSORS_PPC_OCC
> +
> +config SENSORS_PPC_OCC_P8_I2C
> +	tristate "POWER8 OCC hwmon support"
> +	depends on I2C
> +	help
> +	 Provide a hwmon sysfs interface for the POWER8 On-Chip Controller,
> +	 exposing temperature, frequency and power measurements.
> +
> +	 This driver can also be built as a module. If so, the module will be
> +	 called p8-occ-i2c.
> +
> +endif
> diff --git a/drivers/hwmon/occ/Makefile b/drivers/hwmon/occ/Makefile
> index a6881f9..9294b58 100644
> --- a/drivers/hwmon/occ/Makefile
> +++ b/drivers/hwmon/occ/Makefile
> @@ -1 +1,2 @@
>  obj-$(CONFIG_SENSORS_PPC_OCC) += occ.o occ_sysfs.o
> +obj-$(CONFIG_SENSORS_PPC_OCC_P8_I2C) += occ_scom_i2c.o occ_p8.o p8_occ_i2c.o
> diff --git a/drivers/hwmon/occ/p8_occ_i2c.c b/drivers/hwmon/occ/p8_occ_i2c.c
> new file mode 100644
> index 0000000..4515c68
> --- /dev/null
> +++ b/drivers/hwmon/occ/p8_occ_i2c.c
> @@ -0,0 +1,123 @@
> +/*
> + * p8_occ_i2c.c - hwmon OCC driver
> + *
> + * This file contains the i2c layer for accessing the P8 OCC over i2c bus.
> + *
> + * Copyright 2016 IBM Corp.
> + *
> + * 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.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/i2c.h>
> +#include <linux/err.h>
> +#include <linux/of.h>
> +#include <linux/kernel.h>
> +#include <linux/device.h>
> +
> +#include "scom.h"
> +#include "occ_scom_i2c.h"
> +#include "occ_p8.h"
> +#include "occ_sysfs.h"
> +
> +#define P8_OCC_I2C_NAME	"p8-occ-i2c"
> +
> +int p8_i2c_getscom(void *bus, u32 address, u64 *data)
> +{
> +	/* P8 i2c slave requires address to be shifted by 1 */
> +	address = address << 1;
> +
> +	return occ_i2c_getscom(bus, address, data);
> +}
> +
> +int p8_i2c_putscom(void *bus, u32 address, u32 data0, u32 data1)
> +{
> +	/* P8 i2c slave requires address to be shifted by 1 */
> +	address = address << 1;
> +
> +	return occ_i2c_putscom(bus, address, data0, data1);
> +}
> +
> +static struct occ_bus_ops p8_bus_ops = {
> +	.getscom = p8_i2c_getscom,
> +	.putscom = p8_i2c_putscom,
> +};
> +
> +static int p8_occ_probe(struct i2c_client *client,
> +			const struct i2c_device_id *id)
> +{
> +	struct occ *occ;
> +	struct occ_sysfs *hwmon;
> +	const u32 *sensor_hwmon_configs = p8_get_sensor_hwmon_configs();
> +
> +	occ = p8_occ_start(&client->dev, client, &p8_bus_ops);
> +	if (IS_ERR(occ))
> +		return PTR_ERR(occ);
> +
> +	hwmon = occ_sysfs_start(&client->dev, occ, sensor_hwmon_configs,
> +				P8_OCC_I2C_NAME);
> +	if (IS_ERR(hwmon))
> +		return PTR_ERR(hwmon);
> +
> +	i2c_set_clientdata(client, occ);
> +
You might do the set_clientdata() call before the registration, and use
return PTR_ERR_OR_ZERO(hwmon). You'll need to drop the call to dev_set_drvdata()
in occ_sysfs_start() for that to work.

> +	return 0;
> +}
> +
> +static int p8_occ_remove(struct i2c_client *client)
> +{
> +	struct occ *occ = i2c_get_clientdata(client);
> +
> +	return p8_occ_stop(occ);

That stops occ first, then removes the hwmon driver registration. It also leaves a hole:
if occ_sysfs_start() failed, occ_stop() won't be called.

But wait ... all occ_stop does is to call devm_kfree(), which is wrong, because it
should never be necessary to call it, but even more so since the data structure
is still in use after the call to p8_occ_stop(). Just drop that function.

> +}
> +
> +/* used by old-style board info. */
> +static const struct i2c_device_id occ_ids[] = {
> +	{ P8_OCC_I2C_NAME, 0 },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, occ_ids);
> +
> +/* used by device table */
> +static const struct of_device_id occ_of_match[] = {
> +	{ .compatible = "ibm,p8-occ-i2c" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, occ_of_match);
> +
> +/*
> + * i2c-core uses i2c-detect() to detect device in below address list.
> + * If exists, address will be assigned to client.
> + * It is also possible to read address from device table.
> + */
> +static const unsigned short normal_i2c[] = {0x50, 0x51, I2C_CLIENT_END };
> +
... but only if there is a detect function, and trying to detect any chip
on the common eeprom addresses is not a good idea anyway (it would be really
easy to spoof). Please drop.

> +static struct i2c_driver p8_occ_driver = {
> +	.class = I2C_CLASS_HWMON,
> +	.driver = {
> +		.name = P8_OCC_I2C_NAME,
> +		.pm = NULL,
> +		.of_match_table = occ_of_match,
> +	},
> +	.probe = p8_occ_probe,
> +	.remove = p8_occ_remove,
> +	.id_table = occ_ids,
> +	.address_list = normal_i2c,
> +};
> +
> +module_i2c_driver(p8_occ_driver);
> +
> +MODULE_AUTHOR("Eddie James <eajames-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>");
> +MODULE_DESCRIPTION("BMC P8 OCC hwmon driver");
> +MODULE_LICENSE("GPL");
>

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH linux v2 0/6] drivers: hwmon: Add On-Chip Controller driver
From: Guenter Roeck @ 2017-01-15 18:35 UTC (permalink / raw)
  To: eajames.ibm
  Cc: devicetree, jdelvare, corbet, linux-doc, linux-hwmon, linux-i2c,
	linux-kernel, mark.rutland, robh+dt, wsa, andrew, joel, benh,
	Edward A. James
In-Reply-To: <1484158237-10014-1-git-send-email-eajames.ibm@gmail.com>

On 01/11/2017 10:10 AM, eajames.ibm@gmail.com wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
>
> This patchset adds a hwmon driver to support the OCC (On-Chip Controller)
> on the IBM POWER8 and POWER9 processors, from a BMC (Baseboard Management
> Controller). The OCC is an embedded processor that provides real time
> power and thermal monitoring.
>
> The driver provides an interface on a BMC to poll OCC sensor data, set
> user power caps, and perform some basic OCC error handling. It interfaces
> with userspace through hwmon.
>
> The driver is currently functional only for the OCC on POWER8 chips.
> Communicating with the POWER9 OCC requries FSI support.
>

Overall looks pretty good. There is the 0day hiccup (was that solved ?) and
a few comments I made separately. It might make sense to add someone from IBM
as maintainer.

Thanks,
Guenter

> Edward A. James (6):
>   hwmon: Add core On-Chip Controller support for POWER CPUs
>   hwmon: occ: Add sysfs interface
>   hwmon: occ: Add I2C transport implementation for SCOM operations
>   hwmon: occ: Add callbacks for parsing P8 OCC datastructures
>   hwmon: occ: Add hwmon implementation for the P8 OCC
>   hwmon: occ: Add callbacks for parsing P9 OCC datastructures
>
>  Documentation/devicetree/bindings/hwmon/occ.txt |  13 +
>  Documentation/hwmon/occ                         | 114 +++++
>  drivers/hwmon/Kconfig                           |   2 +
>  drivers/hwmon/Makefile                          |   1 +
>  drivers/hwmon/occ/Kconfig                       |  29 ++
>  drivers/hwmon/occ/Makefile                      |   2 +
>  drivers/hwmon/occ/occ.c                         | 533 ++++++++++++++++++++++++
>  drivers/hwmon/occ/occ.h                         |  83 ++++
>  drivers/hwmon/occ/occ_p8.c                      | 254 +++++++++++
>  drivers/hwmon/occ/occ_p8.h                      |  31 ++
>  drivers/hwmon/occ/occ_p9.c                      | 314 ++++++++++++++
>  drivers/hwmon/occ/occ_p9.h                      |  31 ++
>  drivers/hwmon/occ/occ_scom_i2c.c                |  73 ++++
>  drivers/hwmon/occ/occ_scom_i2c.h                |  26 ++
>  drivers/hwmon/occ/occ_sysfs.c                   | 274 ++++++++++++
>  drivers/hwmon/occ/occ_sysfs.h                   |  44 ++
>  drivers/hwmon/occ/p8_occ_i2c.c                  | 123 ++++++
>  drivers/hwmon/occ/scom.h                        |  47 +++
>  18 files changed, 1994 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/occ.txt
>  create mode 100644 Documentation/hwmon/occ
>  create mode 100644 drivers/hwmon/occ/Kconfig
>  create mode 100644 drivers/hwmon/occ/Makefile
>  create mode 100644 drivers/hwmon/occ/occ.c
>  create mode 100644 drivers/hwmon/occ/occ.h
>  create mode 100644 drivers/hwmon/occ/occ_p8.c
>  create mode 100644 drivers/hwmon/occ/occ_p8.h
>  create mode 100644 drivers/hwmon/occ/occ_p9.c
>  create mode 100644 drivers/hwmon/occ/occ_p9.h
>  create mode 100644 drivers/hwmon/occ/occ_scom_i2c.c
>  create mode 100644 drivers/hwmon/occ/occ_scom_i2c.h
>  create mode 100644 drivers/hwmon/occ/occ_sysfs.c
>  create mode 100644 drivers/hwmon/occ/occ_sysfs.h
>  create mode 100644 drivers/hwmon/occ/p8_occ_i2c.c
>  create mode 100644 drivers/hwmon/occ/scom.h
>

^ permalink raw reply

* Re: [PATCH linux v1 2/2] drivers: hwmon: hwmon driver for ASPEED AST2400/2500 PWM and Fan tach controller
From: Guenter Roeck @ 2017-01-15 19:11 UTC (permalink / raw)
  To: Jaghathiswari Rankappagounder Natarajan, openbmc, joel, jdelvare,
	linux-hwmon, linux-kernel, corbet, linux-doc, robh+dt,
	mark.rutland, devicetree
In-Reply-To: <20170109215935.30067-3-jaghu@google.com>

On 01/09/2017 01:59 PM, Jaghathiswari Rankappagounder Natarajan wrote:
> The ASPEED AST2400/2500 PWM controller supports 8 PWM output ports.
> The ASPEED AST2400/2500 Fan tach controller supports 16 tachometer inputs.
> PWM clock types M, N and 0 are three types just to have three independent PWM
> sources.
> The device driver matches on the device tree node. The configuration values
> are read from the device tree and written to the respective registers.
> The driver provides a sysfs entries through which the user can
> configure the duty-cycle value (ranging from 0 to 100 percent) and read the
> fan tach rpm value.
>
> Signed-off-by: Jaghathiswari Rankappagounder Natarajan <jaghu@google.com>
> ---
>  Documentation/hwmon/aspeed-pwm-tacho |  22 +
>  drivers/hwmon/Kconfig                |   9 +
>  drivers/hwmon/Makefile               |   1 +
>  drivers/hwmon/aspeed-pwm-tacho.c     | 884 +++++++++++++++++++++++++++++++++++
>  4 files changed, 916 insertions(+)
>  create mode 100644 Documentation/hwmon/aspeed-pwm-tacho
>  create mode 100644 drivers/hwmon/aspeed-pwm-tacho.c
>
> diff --git a/Documentation/hwmon/aspeed-pwm-tacho b/Documentation/hwmon/aspeed-pwm-tacho
> new file mode 100644
> index 000000000000..0e9ec6d5f900
> --- /dev/null
> +++ b/Documentation/hwmon/aspeed-pwm-tacho
> @@ -0,0 +1,22 @@
> +Kernel driver aspeed-pwm-tacho
> +==============================
> +
> +Supported chips:
> +	ASPEED AST2400/2500
> +
> +Authors:
> +	<jaghu@google.com>
> +
> +Description:
> +------------
> +This driver implements support for ASPEED AST2400/2500 PWM and Fan Tacho
> +controller. The PWM controller supports upto 8 PWM outputs. The Fan tacho
> +controller supports upto 16 tachometer inputs.
> +
> +The driver provides the following sensor accesses in sysfs:
> +
> +fanX_input	ro	provide current fan rotation value in RPM as reported
> +			by the fan to the device.
> +
> +pwmX		rw	get or set PWM fan control value. This is an integer
> +			value between 0(off) and 255(full speed).
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 45cef3d2c75c..757b5b0705bf 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -341,6 +341,15 @@ config SENSORS_ASB100
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called asb100.
>
> +config SENSORS_ASPEED
> +	tristate "ASPEED AST2400/AST2500 PWM and Fan tach driver"
> +	help
> +	  This driver provides support for ASPEED AST2400/AST2500 PWM
> +	  and Fan Tacho controllers.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called aspeed_pwm_tacho.
> +
>  config SENSORS_ATXP1
>  	tristate "Attansic ATXP1 VID controller"
>  	depends on I2C
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index aecf4ba17460..83025cc9bb45 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -46,6 +46,7 @@ obj-$(CONFIG_SENSORS_ADT7475)	+= adt7475.o
>  obj-$(CONFIG_SENSORS_APPLESMC)	+= applesmc.o
>  obj-$(CONFIG_SENSORS_ARM_SCPI)	+= scpi-hwmon.o
>  obj-$(CONFIG_SENSORS_ASC7621)	+= asc7621.o
> +obj-$(CONFIG_SENSORS_ASPEED)	+= aspeed-pwm-tacho.o
>  obj-$(CONFIG_SENSORS_ATXP1)	+= atxp1.o
>  obj-$(CONFIG_SENSORS_CORETEMP)	+= coretemp.o
>  obj-$(CONFIG_SENSORS_DA9052_ADC)+= da9052-hwmon.o
> diff --git a/drivers/hwmon/aspeed-pwm-tacho.c b/drivers/hwmon/aspeed-pwm-tacho.c
> new file mode 100644
> index 000000000000..93eb3be2e506
> --- /dev/null
> +++ b/drivers/hwmon/aspeed-pwm-tacho.c
> @@ -0,0 +1,884 @@
> +/*
> + * Copyright (c) 2016 Google, Inc
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 or later as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/delay.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/sysfs.h>
> +
> +/* ASPEED PWM & FAN Tach Register Definition */
> +#define ASPEED_PTCR_CTRL		0x00
> +#define ASPEED_PTCR_CLK_CTRL		0x04
> +#define ASPEED_PTCR_DUTY0_CTRL		0x08
> +#define ASPEED_PTCR_DUTY1_CTRL		0x0c
> +#define ASPEED_PTCR_TYPEM_CTRL		0x10
> +#define ASPEED_PTCR_TYPEM_CTRL1		0x14
> +#define ASPEED_PTCR_TYPEN_CTRL		0x18
> +#define ASPEED_PTCR_TYPEN_CTRL1		0x1c
> +#define ASPEED_PTCR_TACH_SOURCE		0x20
> +#define ASPEED_PTCR_TRIGGER		0x28
> +#define ASPEED_PTCR_RESULT		0x2c
> +#define ASPEED_PTCR_INTR_CTRL		0x30
> +#define ASPEED_PTCR_INTR_STS		0x34
> +#define ASPEED_PTCR_TYPEM_LIMIT		0x38
> +#define ASPEED_PTCR_TYPEN_LIMIT		0x3C
> +#define ASPEED_PTCR_CTRL_EXT		0x40
> +#define ASPEED_PTCR_CLK_CTRL_EXT	0x44
> +#define ASPEED_PTCR_DUTY2_CTRL		0x48
> +#define ASPEED_PTCR_DUTY3_CTRL		0x4c
> +#define ASPEED_PTCR_TYPEO_CTRL		0x50
> +#define ASPEED_PTCR_TYPEO_CTRL1		0x54
> +#define ASPEED_PTCR_TACH_SOURCE_EXT	0x60
> +#define ASPEED_PTCR_TYPEO_LIMIT		0x78
> +
> +/* ASPEED_PTCR_CTRL : 0x00 - General Control Register */
> +#define ASPEED_PTCR_CTRL_SET_PWMD_TYPE_PART1	15
> +#define ASPEED_PTCR_CTRL_SET_PWMD_TYPE_PART2	6
> +#define ASPEED_PTCR_CTRL_SET_PWMD_TYPE_MASK	(BIT(7) | BIT(15))
> +
> +#define ASPEED_PTCR_CTRL_SET_PWMC_TYPE_PART1	14
> +#define ASPEED_PTCR_CTRL_SET_PWMC_TYPE_PART2	5
> +#define ASPEED_PTCR_CTRL_SET_PWMC_TYPE_MASK	(BIT(6) | BIT(14))
> +
> +#define ASPEED_PTCR_CTRL_SET_PWMB_TYPE_PART1	13
> +#define ASPEED_PTCR_CTRL_SET_PWMB_TYPE_PART2	4
> +#define ASPEED_PTCR_CTRL_SET_PWMB_TYPE_MASK	(BIT(5) | BIT(13))
> +
> +#define ASPEED_PTCR_CTRL_SET_PWMA_TYPE_PART1	12
> +#define ASPEED_PTCR_CTRL_SET_PWMA_TYPE_PART2	3
> +#define ASPEED_PTCR_CTRL_SET_PWMA_TYPE_MASK	(BIT(4) | BIT(12))
> +
> +#define	ASPEED_PTCR_CTRL_FAN_NUM_EN(x)	(0x1 << (16 + (x)))
> +
> +#define	ASPEED_PTCR_CTRL_PWMD_EN	(0x1 << 11)
> +#define	ASPEED_PTCR_CTRL_PWMC_EN	(0x1 << 10)
> +#define	ASPEED_PTCR_CTRL_PWMB_EN	(0x1 << 9)
> +#define	ASPEED_PTCR_CTRL_PWMA_EN	(0x1 << 8)
> +
Why not BIT() ?

> +#define	ASPEED_PTCR_CTRL_CLK_SRC	0x2
> +#define	ASPEED_PTCR_CTRL_CLK_EN		0x1

BIT(0) and BIT(1) ?

> +
> +/* ASPEED_PTCR_CLK_CTRL : 0x04 - Clock Control Register */
> +/* TYPE N */
> +#define ASPEED_PTCR_CLK_CTRL_TYPEN_UNIT		24
> +#define ASPEED_PTCR_CLK_CTRL_TYPEN_H		20
> +#define ASPEED_PTCR_CLK_CTRL_TYPEN_L		16
> +/* TYPE M */
> +#define ASPEED_PTCR_CLK_CTRL_TYPEM_UNIT		8
> +#define ASPEED_PTCR_CLK_CTRL_TYPEM_H		4
> +#define ASPEED_PTCR_CLK_CTRL_TYPEM_L		0
> +
> +/*
> + * ASPEED_PTCR_DUTY_CTRL/1/2/3 : 0x08/0x0C/0x48/0x4C - PWM-FAN duty control
> + * 0/1/2/3 register
> + */
> +#define DUTY_CTRL_PWM2_FALL_POINT	24
> +#define DUTY_CTRL_PWM2_RISE_POINT	16
> +#define DUTY_CTRL_PWM1_FALL_POINT	8
> +#define DUTY_CTRL_PWM1_RISE_POINT	0
> +
> +/* ASPEED_PTCR_TYPEM_CTRL : 0x10/0x18/0x50 - Type M/N/O Ctrl 0 Register */
> +#define TYPE_CTRL_FAN_PERIOD		16
> +#define TYPE_CTRL_FAN_MODE		4
> +#define TYPE_CTRL_FAN_DIVISION		1
> +#define TYPE_CTRL_FAN_TYPE_EN		1
> +
> +/* ASPEED_PTCR_TACH_SOURCE : 0x20/0x60 - Tach Source Register */
> +/* bit [0,1] at 0x20, bit [2] at 0x60 */
> +#define TACH_PWM_SOURCE_BIT01(x)	((x) * 2)
> +#define TACH_PWM_SOURCE_BIT2(x)		((x) * 2)
> +#define TACH_PWM_SOURCE_MASK_BIT01(x)	(0x3 << ((x) * 2))
> +#define TACH_PWM_SOURCE_MASK_BIT2(x)	(0x1 << ((x) * 2))
> +
> +/* ASPEED_PTCR_TRIGGER : 0x28 - Trigger Register */
> +#define TRIGGER_READ_FAN_NUM(x)		(0x1 << (x))

Why not BIT() ?

> +
> +/* ASPEED_PTCR_RESULT : 0x2c - Result Register */
> +#define RESULT_STATUS			31
> +#define RESULT_VALUE_MASK		0xfffff
> +
> +/* ASPEED_PTCR_CTRL_EXT : 0x40 - General Control Extension #1 Register */
> +#define ASPEED_PTCR_CTRL_SET_PWMH_TYPE_PART1	15
> +#define ASPEED_PTCR_CTRL_SET_PWMH_TYPE_PART2	6
> +#define ASPEED_PTCR_CTRL_SET_PWMH_TYPE_MASK	(BIT(7) | BIT(15))
> +
> +#define ASPEED_PTCR_CTRL_SET_PWMG_TYPE_PART1	14
> +#define ASPEED_PTCR_CTRL_SET_PWMG_TYPE_PART2	5
> +#define ASPEED_PTCR_CTRL_SET_PWMG_TYPE_MASK	(BIT(6) | BIT(14))
> +
> +#define ASPEED_PTCR_CTRL_SET_PWMF_TYPE_PART1	13
> +#define ASPEED_PTCR_CTRL_SET_PWMF_TYPE_PART2	4
> +#define ASPEED_PTCR_CTRL_SET_PWMF_TYPE_MASK	(BIT(5) | BIT(13))
> +
> +#define ASPEED_PTCR_CTRL_SET_PWME_TYPE_PART1	12
> +#define ASPEED_PTCR_CTRL_SET_PWME_TYPE_PART2	3
> +#define ASPEED_PTCR_CTRL_SET_PWME_TYPE_MASK	(BIT(4) | BIT(12))
> +
> +#define	ASPEED_PTCR_CTRL_PWMH_EN	(0x1 << 11)
> +#define	ASPEED_PTCR_CTRL_PWMG_EN	(0x1 << 10)
> +#define	ASPEED_PTCR_CTRL_PWMF_EN	(0x1 << 9)
> +#define	ASPEED_PTCR_CTRL_PWME_EN	(0x1 << 8)

Why not BIT() ?

> +
> +/* ASPEED_PTCR_CLK_EXT_CTRL : 0x44 - Clock Control Extension #1 Register */
> +/* TYPE O */
> +#define ASPEED_PTCR_CLK_CTRL_TYPEO_UNIT		8
> +#define ASPEED_PTCR_CLK_CTRL_TYPEO_H		4
> +#define ASPEED_PTCR_CLK_CTRL_TYPEO_L		0
> +
> +#define MCLK 1
> +#define PWM_MAX 255
> +#define MAX_HIGH_LOW_BIT 15
> +
> +struct aspeed_pwm_tacho_data {
> +	void __iomem *base;
> +	unsigned long clk_freq;
> +	const struct attribute_group *groups[24];
> +	u8 type_pwm_clock_unit[3];
> +	u8 type_pwm_clock_division_h[3];
> +	u8 type_pwm_clock_division_l[3];
> +	u8 type_fan_tach_clock_division[3];
> +	u16 type_fan_tach_unit[3];
> +	u8 pwm_port_type[8];
> +	u8 pwm_port_fan_ctrl[8];
> +	u8 fan_tach_ch_source[16];
> +};
> +
> +enum type { TYPEM, TYPEN, TYPEO };
> +
> +struct type_params {
> +	u32 l_value;
> +	u32 h_value;
> +	u32 unit_value;
> +	u32 clk_ctrl_reg;
> +	u32 ctrl_reg;
> +	u32 ctrl_reg1;
> +};
> +
> +static const struct type_params type_params[] = {
> +	[TYPEM] = {
> +		.l_value = ASPEED_PTCR_CLK_CTRL_TYPEM_L,
> +		.h_value = ASPEED_PTCR_CLK_CTRL_TYPEM_H,
> +		.unit_value = ASPEED_PTCR_CLK_CTRL_TYPEM_UNIT,
> +		.clk_ctrl_reg = ASPEED_PTCR_CLK_CTRL,
> +		.ctrl_reg = ASPEED_PTCR_TYPEM_CTRL,
> +		.ctrl_reg1 = ASPEED_PTCR_TYPEM_CTRL1,
> +	},
> +	[TYPEN] = {
> +		.l_value = ASPEED_PTCR_CLK_CTRL_TYPEN_L,
> +		.h_value = ASPEED_PTCR_CLK_CTRL_TYPEN_H,
> +		.unit_value = ASPEED_PTCR_CLK_CTRL_TYPEN_UNIT,
> +		.clk_ctrl_reg = ASPEED_PTCR_CLK_CTRL,
> +		.ctrl_reg = ASPEED_PTCR_TYPEN_CTRL,
> +		.ctrl_reg1 = ASPEED_PTCR_TYPEN_CTRL1,
> +	},
> +	[TYPEO] = {
> +		.l_value = ASPEED_PTCR_CLK_CTRL_TYPEO_L,
> +		.h_value = ASPEED_PTCR_CLK_CTRL_TYPEO_H,
> +		.unit_value = ASPEED_PTCR_CLK_CTRL_TYPEO_UNIT,
> +		.clk_ctrl_reg = ASPEED_PTCR_CLK_CTRL_EXT,
> +		.ctrl_reg = ASPEED_PTCR_TYPEO_CTRL,
> +		.ctrl_reg1 = ASPEED_PTCR_TYPEO_CTRL1,
> +	}
> +};
> +
> +enum pwm_port { PWMA, PWMB, PWMC, PWMD, PWME, PWMF, PWMG, PWMH };
> +
> +struct pwm_port_params {
> +	u32 pwm_en;
> +	u32 ctrl_reg;
> +	u32 type_part1;
> +	u32 type_part2;
> +	u32 type_mask;
> +	u32 duty_ctrl_rise_point;
> +	u32 duty_ctrl_fall_point;
> +	u32 duty_ctrl_reg;
> +	u8 duty_ctrl_calc_type;
> +};
> +
> +static const struct pwm_port_params pwm_port_params[] = {
> +	[PWMA] = {
> +		.pwm_en = ASPEED_PTCR_CTRL_PWMA_EN,
> +		.ctrl_reg = ASPEED_PTCR_CTRL,
> +		.type_part1 = ASPEED_PTCR_CTRL_SET_PWMA_TYPE_PART1,
> +		.type_part2 = ASPEED_PTCR_CTRL_SET_PWMA_TYPE_PART2,
> +		.type_mask = ASPEED_PTCR_CTRL_SET_PWMA_TYPE_MASK,
> +		.duty_ctrl_rise_point = DUTY_CTRL_PWM1_RISE_POINT,
> +		.duty_ctrl_fall_point = DUTY_CTRL_PWM1_FALL_POINT,
> +		.duty_ctrl_reg = ASPEED_PTCR_DUTY0_CTRL,
> +		.duty_ctrl_calc_type = 0,
> +	},
> +	[PWMB] = {
> +		.pwm_en = ASPEED_PTCR_CTRL_PWMB_EN,
> +		.ctrl_reg = ASPEED_PTCR_CTRL,
> +		.type_part1 = ASPEED_PTCR_CTRL_SET_PWMB_TYPE_PART1,
> +		.type_part2 = ASPEED_PTCR_CTRL_SET_PWMB_TYPE_PART2,
> +		.type_mask = ASPEED_PTCR_CTRL_SET_PWMB_TYPE_MASK,
> +		.duty_ctrl_rise_point = DUTY_CTRL_PWM2_RISE_POINT,
> +		.duty_ctrl_fall_point = DUTY_CTRL_PWM2_FALL_POINT,
> +		.duty_ctrl_reg = ASPEED_PTCR_DUTY0_CTRL,
> +		.duty_ctrl_calc_type = 1,
> +	},
> +	[PWMC] = {
> +		.pwm_en = ASPEED_PTCR_CTRL_PWMC_EN,
> +		.ctrl_reg = ASPEED_PTCR_CTRL,
> +		.type_part1 = ASPEED_PTCR_CTRL_SET_PWMC_TYPE_PART1,
> +		.type_part2 = ASPEED_PTCR_CTRL_SET_PWMC_TYPE_PART2,
> +		.type_mask = ASPEED_PTCR_CTRL_SET_PWMC_TYPE_MASK,
> +		.duty_ctrl_rise_point = DUTY_CTRL_PWM1_RISE_POINT,
> +		.duty_ctrl_fall_point = DUTY_CTRL_PWM1_FALL_POINT,
> +		.duty_ctrl_reg = ASPEED_PTCR_DUTY1_CTRL,
> +		.duty_ctrl_calc_type = 0,
> +	},
> +	[PWMD] = {
> +		.pwm_en = ASPEED_PTCR_CTRL_PWMD_EN,
> +		.ctrl_reg = ASPEED_PTCR_CTRL,
> +		.type_part1 = ASPEED_PTCR_CTRL_SET_PWMD_TYPE_PART1,
> +		.type_part2 = ASPEED_PTCR_CTRL_SET_PWMD_TYPE_PART2,
> +		.type_mask = ASPEED_PTCR_CTRL_SET_PWMD_TYPE_MASK,
> +		.duty_ctrl_rise_point = DUTY_CTRL_PWM2_RISE_POINT,
> +		.duty_ctrl_fall_point = DUTY_CTRL_PWM2_FALL_POINT,
> +		.duty_ctrl_reg = ASPEED_PTCR_DUTY1_CTRL,
> +		.duty_ctrl_calc_type = 1,
> +	},
> +	[PWME] = {
> +		.pwm_en = ASPEED_PTCR_CTRL_PWME_EN,
> +		.ctrl_reg = ASPEED_PTCR_CTRL_EXT,
> +		.type_part1 = ASPEED_PTCR_CTRL_SET_PWME_TYPE_PART1,
> +		.type_part2 = ASPEED_PTCR_CTRL_SET_PWME_TYPE_PART2,
> +		.type_mask = ASPEED_PTCR_CTRL_SET_PWME_TYPE_MASK,
> +		.duty_ctrl_rise_point = DUTY_CTRL_PWM1_RISE_POINT,
> +		.duty_ctrl_fall_point = DUTY_CTRL_PWM1_FALL_POINT,
> +		.duty_ctrl_reg = ASPEED_PTCR_DUTY2_CTRL,
> +		.duty_ctrl_calc_type = 0,
> +	},
> +	[PWMF] = {
> +		.pwm_en = ASPEED_PTCR_CTRL_PWMF_EN,
> +		.ctrl_reg = ASPEED_PTCR_CTRL_EXT,
> +		.type_part1 = ASPEED_PTCR_CTRL_SET_PWMF_TYPE_PART1,
> +		.type_part2 = ASPEED_PTCR_CTRL_SET_PWMF_TYPE_PART2,
> +		.type_mask = ASPEED_PTCR_CTRL_SET_PWMF_TYPE_MASK,
> +		.duty_ctrl_rise_point = DUTY_CTRL_PWM2_RISE_POINT,
> +		.duty_ctrl_fall_point = DUTY_CTRL_PWM2_FALL_POINT,
> +		.duty_ctrl_reg = ASPEED_PTCR_DUTY2_CTRL,
> +		.duty_ctrl_calc_type = 1,
> +	},
> +	[PWMG] = {
> +		.pwm_en = ASPEED_PTCR_CTRL_PWMG_EN,
> +		.ctrl_reg = ASPEED_PTCR_CTRL_EXT,
> +		.type_part1 = ASPEED_PTCR_CTRL_SET_PWMG_TYPE_PART1,
> +		.type_part2 = ASPEED_PTCR_CTRL_SET_PWMG_TYPE_PART2,
> +		.type_mask = ASPEED_PTCR_CTRL_SET_PWMG_TYPE_MASK,
> +		.duty_ctrl_rise_point = DUTY_CTRL_PWM1_RISE_POINT,
> +		.duty_ctrl_fall_point = DUTY_CTRL_PWM1_FALL_POINT,
> +		.duty_ctrl_reg = ASPEED_PTCR_DUTY3_CTRL,
> +		.duty_ctrl_calc_type = 0,
> +	},
> +	[PWMH] = {
> +		.pwm_en = ASPEED_PTCR_CTRL_PWMH_EN,
> +		.ctrl_reg = ASPEED_PTCR_CTRL_EXT,
> +		.type_part1 = ASPEED_PTCR_CTRL_SET_PWMH_TYPE_PART1,
> +		.type_part2 = ASPEED_PTCR_CTRL_SET_PWMH_TYPE_PART2,
> +		.type_mask = ASPEED_PTCR_CTRL_SET_PWMH_TYPE_MASK,
> +		.duty_ctrl_rise_point = DUTY_CTRL_PWM2_RISE_POINT,
> +		.duty_ctrl_fall_point = DUTY_CTRL_PWM2_FALL_POINT,
> +		.duty_ctrl_reg = ASPEED_PTCR_DUTY3_CTRL,
> +		.duty_ctrl_calc_type = 1,
> +	}
> +};
> +
> +static void aspeed_set_clock_enable(void __iomem *base, bool val)
> +{
> +	u32 reg_value = ioread32(base + ASPEED_PTCR_CTRL);
> +
> +	if (val)
> +		reg_value |= ASPEED_PTCR_CTRL_CLK_EN;
> +	else
> +		reg_value &= ~ASPEED_PTCR_CTRL_CLK_EN;
> +
> +	iowrite32(reg_value, base + ASPEED_PTCR_CTRL);

Those functions make me wonder if it would make sense to use regmap.

> +}
> +
> +static void aspeed_set_clock_source(void __iomem *base, int val)
> +{
> +	u32 reg_value = ioread32(base + ASPEED_PTCR_CTRL);
> +
> +	if (val == MCLK)
> +		reg_value |= ASPEED_PTCR_CTRL_CLK_SRC;
> +	else
> +		reg_value &= ~ASPEED_PTCR_CTRL_CLK_SRC;
> +
> +	iowrite32(reg_value, base + ASPEED_PTCR_CTRL);
> +}
> +
> +static void aspeed_set_pwm_clock_values(void __iomem *base, u8 type,
> +					u8 div_high, u8 div_low, u8 unit)
> +{
> +	u32 reg_offset = type_params[type].clk_ctrl_reg;
> +	u32 reg_value = ioread32(base + reg_offset);
> +
> +	reg_value &= ~((0xF << type_params[type].h_value) |
> +	(0xF << type_params[type].l_value) |
> +	(0xFF << type_params[type].unit_value));
> +	reg_value |= ((div_high << type_params[type].h_value) |
> +	(div_low << type_params[type].l_value) |
> +	(unit << type_params[type].unit_value));
> +
> +	iowrite32(reg_value, base + reg_offset);
> +}
> +
> +static void aspeed_set_pwm_port_enable(void __iomem *base, u8 pwm_port,
> +				       bool enable)
> +{
> +	u32 reg_offset = pwm_port_params[pwm_port].ctrl_reg;
> +	u32 reg_value = ioread32(base + reg_offset);
> +
> +	if (enable)
> +		reg_value |= pwm_port_params[pwm_port].pwm_en;
> +	else
> +		reg_value &= ~pwm_port_params[pwm_port].pwm_en;
> +	iowrite32(reg_value, base + reg_offset);
> +}
> +
> +static void aspeed_set_pwm_port_type(void __iomem *base, u8 pwm_port, u8 type)
> +{
> +	u32 reg_offset = pwm_port_params[pwm_port].ctrl_reg;
> +	u32 reg_value = ioread32(base + reg_offset);
> +
> +	reg_value &= ~pwm_port_params[pwm_port].type_mask;
> +	reg_value |= (type & 0x1) <<
> +		pwm_port_params[pwm_port].type_part1;
> +	reg_value |= (type & 0x2) <<
> +		pwm_port_params[pwm_port].type_part2;
> +
> +	iowrite32(reg_value, base + reg_offset);
> +}
> +
> +static void aspeed_set_pwm_port_duty_rising_falling(void __iomem *base,
> +						    u8 pwm_port, u8 rising,
> +						    u8 falling)
> +{
> +	u32 reg_offset = pwm_port_params[pwm_port].duty_ctrl_reg;
> +	u32 reg_value = ioread32(base + reg_offset);
> +
> +	reg_value &= ~(0xFF << pwm_port_params[pwm_port].duty_ctrl_rise_point);
> +	reg_value &= ~(0xFF << pwm_port_params[pwm_port].duty_ctrl_fall_point);
> +
> +	if (pwm_port_params[pwm_port].duty_ctrl_calc_type == 0) {
> +		reg_value |= rising;
> +	} else if (pwm_port_params[pwm_port].duty_ctrl_calc_type == 1) {
> +		reg_value |= (rising <<
> +			pwm_port_params[pwm_port].duty_ctrl_rise_point);
> +	}
> +	reg_value |= (falling <<
> +			pwm_port_params[pwm_port].duty_ctrl_fall_point);
> +
> +	iowrite32(reg_value, base + reg_offset);
> +}
> +
> +static void aspeed_set_tacho_type_enable(void __iomem *base, u8 type,
> +					 bool enable)
> +{
> +	u32 reg_offset = type_params[type].ctrl_reg;
> +	u32 reg_value = ioread32(base + reg_offset);
> +
> +	if (enable)
> +		reg_value |= TYPE_CTRL_FAN_TYPE_EN;
> +	else
> +		reg_value &= ~TYPE_CTRL_FAN_TYPE_EN;
> +
> +	iowrite32(reg_value, base + reg_offset);
> +}
> +
> +static void aspeed_set_tacho_type_values(void __iomem *base, u8 type, u8 mode,
> +					 u16 unit, u8 division)
> +{
> +	u32 reg_offset = type_params[type].ctrl_reg;
> +	u32 reg_offset1 = type_params[type].ctrl_reg1;
> +	u32 reg_value = ioread32(base + reg_offset);
> +
> +	reg_value &= ~((0x3 << TYPE_CTRL_FAN_MODE) |
> +			(0xFFFF << TYPE_CTRL_FAN_PERIOD) |
> +			(0x7 << TYPE_CTRL_FAN_DIVISION));
> +	reg_value |= ((mode << TYPE_CTRL_FAN_MODE) |
> +			(unit << TYPE_CTRL_FAN_PERIOD) |
> +			(division << TYPE_CTRL_FAN_DIVISION));
> +
> +	iowrite32(reg_value, base + reg_offset);
> +
> +	iowrite32(unit << 16, base + reg_offset1);
> +}
> +
> +static void aspeed_set_fan_tach_ch_enable(void __iomem *base, u8 fan_tach_ch,
> +					  bool enable)
> +{
> +	u32 reg_value = ioread32(base + ASPEED_PTCR_CTRL);
> +
> +	if (enable)
> +		reg_value |= ASPEED_PTCR_CTRL_FAN_NUM_EN(fan_tach_ch);
> +	else
> +		reg_value &= ~ASPEED_PTCR_CTRL_FAN_NUM_EN(fan_tach_ch);
> +
> +	iowrite32(reg_value, base + ASPEED_PTCR_CTRL);
> +}
> +
> +static void aspeed_set_fan_tach_ch_source(void __iomem *base, u8 fan_tach_ch,
> +					  u8 fan_tach_ch_source)
> +{
> +	u32 reg_value1 = ioread32(base + ASPEED_PTCR_TACH_SOURCE);
> +	u32 reg_value2 = ioread32(base + ASPEED_PTCR_TACH_SOURCE_EXT);
> +
> +	reg_value1 &= ~(TACH_PWM_SOURCE_MASK_BIT01(fan_tach_ch));
> +	reg_value1 |= ((fan_tach_ch_source & 0x3) <<
> +			(TACH_PWM_SOURCE_BIT01(fan_tach_ch)));
> +
> +	reg_value2 &= ~(TACH_PWM_SOURCE_MASK_BIT2(fan_tach_ch));
> +	reg_value2 |= (((fan_tach_ch_source & 0x4) >> 2) <<
> +			(TACH_PWM_SOURCE_BIT2(fan_tach_ch)));
> +
> +	iowrite32(reg_value1, base + ASPEED_PTCR_TACH_SOURCE);
> +	iowrite32(reg_value2, base + ASPEED_PTCR_TACH_SOURCE_EXT);
> +}
> +
> +static void aspeed_set_pwm_port_fan_ctrl(struct aspeed_pwm_tacho_data *priv,
> +					 u8 index, u8 fan_ctrl)
> +{
> +	u16 period;
> +	u16 dc_time_on;
> +
> +	period = priv->type_pwm_clock_unit[priv->pwm_port_type[index]];
> +	period += 1;
> +	dc_time_on = (fan_ctrl * period) / PWM_MAX;
> +
> +	if (dc_time_on == 0) {
> +		aspeed_set_pwm_port_enable(priv->base, index, false);
> +	} else {
> +		if (dc_time_on == period)
> +			dc_time_on = 0;
> +
> +		aspeed_set_pwm_port_duty_rising_falling(priv->base, index, 0,
> +							dc_time_on);
> +		aspeed_set_pwm_port_enable(priv->base, index, true);
> +	}
> +}
> +
> +static u32 aspeed_get_fan_tach_ch_measure_period(struct aspeed_pwm_tacho_data
> +						 *priv, u8 type)
> +{
> +	u32 clk;
> +	u16 tacho_unit;
> +	u8 clk_unit, div_h, div_l, tacho_div;
> +
> +	clk = priv->clk_freq;
> +
> +	clk_unit = priv->type_pwm_clock_unit[type];
> +	div_h = priv->type_pwm_clock_division_h[type];
> +	div_h = 0x1 << div_h;
> +	div_l = priv->type_pwm_clock_division_l[type];
> +	if (div_l == 0)
> +		div_l = 1;
> +	else
> +		div_l = div_l * 2;
> +
> +	tacho_unit = priv->type_fan_tach_unit[type];
> +	tacho_div = priv->type_fan_tach_clock_division[type];
> +
> +	tacho_div = 0x4 << (tacho_div * 2);
> +	return clk / (clk_unit * div_h * div_l * tacho_div * tacho_unit);
> +}
> +
> +static u32 aspeed_get_fan_tach_ch_rpm(struct aspeed_pwm_tacho_data *priv,
> +				      u8 fan_tach_ch)
> +{
> +	u32 raw_data, rpm, tach_div, clk_source, timeout = 0, sec;
> +	u8 fan_tach_ch_source, type;
> +	void __iomem *base = priv->base;
> +
> +	iowrite32(0, priv->base + ASPEED_PTCR_TRIGGER);
> +	iowrite32(0x1 << fan_tach_ch, priv->base + ASPEED_PTCR_TRIGGER);
> +
> +	fan_tach_ch_source = priv->fan_tach_ch_source[fan_tach_ch];
> +	type = priv->pwm_port_type[fan_tach_ch_source];
> +
> +	sec = (1000 / aspeed_get_fan_tach_ch_measure_period(priv, type));
> +
> +	msleep(sec);
> +
> +	while (!(ioread32(priv->base + ASPEED_PTCR_RESULT) &
> +				(0x1 << RESULT_STATUS))) {
> +		timeout++;
> +		if (timeout > 1)
> +			return 0;
> +		msleep(sec);
> +	};
> +
> +	raw_data = (ioread32(base + ASPEED_PTCR_RESULT)) &
> +				RESULT_VALUE_MASK;
> +	tach_div = priv->type_fan_tach_clock_division[type];
> +
> +	tach_div = 0x4 << (tach_div * 2);
> +	clk_source = priv->clk_freq;
> +	rpm = (clk_source * 60) / (2 * raw_data * tach_div);

Can raw_data be 0 ?

> +	return rpm;
> +}
> +
> +static ssize_t set_pwm(struct device *dev, struct device_attribute *attr,
> +		       const char *buf, size_t count)
> +{
> +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
> +	int index = sensor_attr->index;
> +	int ret;
> +
No empty line here, please.

> +	struct aspeed_pwm_tacho_data *priv = dev_get_drvdata(dev);
> +	long fan_ctrl;
> +
> +	ret = kstrtol(buf, 10, &fan_ctrl);
> +	if (ret != 0)
> +		return ret;
> +
> +	if (fan_ctrl < 0 || fan_ctrl > PWM_MAX)
> +		return -EINVAL;
> +
> +	if (priv->pwm_port_fan_ctrl[index] == fan_ctrl)
> +		return count;
> +
> +	priv->pwm_port_fan_ctrl[index] = fan_ctrl;
> +	aspeed_set_pwm_port_fan_ctrl(priv, index, fan_ctrl);
> +
> +	return count;
> +}
> +
> +static ssize_t show_pwm(struct device *dev, struct device_attribute *attr,
> +			char *buf)
> +{
> +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
> +	int index = sensor_attr->index;
> +
In general, please keep variable declarations together with no empty line in between.

> +	struct aspeed_pwm_tacho_data *priv = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%u\n", priv->pwm_port_fan_ctrl[index]);
> +}
> +
> +static ssize_t show_rpm(struct device *dev, struct device_attribute *attr,
> +			char *buf)
> +{
> +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
> +	int index = sensor_attr->index;
> +	u32 rpm;
> +
> +	struct aspeed_pwm_tacho_data *priv = dev_get_drvdata(dev);
> +
> +	rpm = aspeed_get_fan_tach_ch_rpm(priv, index);
> +
> +	return sprintf(buf, "%u\n", rpm);
> +}
> +
> +#define pwm_index(index)					\
> +static SENSOR_DEVICE_ATTR(pwm##index, 0644,	\
> +			show_pwm, set_pwm, index - 1);		\
> +								\
> +static struct attribute *pwm##index##_dev_attrs[] = {		\
> +	&sensor_dev_attr_pwm##index.dev_attr.attr,		\
> +	NULL,							\
> +};								\
> +static const struct attribute_group pwm##index##_dev_groups = {	\
> +	.attrs = pwm##index##_dev_attrs,			\
> +}
> +
> +#define fan_index(index)					\
> +static SENSOR_DEVICE_ATTR(fan##index##_input, 0444,		\
> +		show_rpm, NULL, index - 1);			\
> +								\
> +static struct attribute *fan##index##_dev_attrs[] = {		\
> +	&sensor_dev_attr_fan##index##_input.dev_attr.attr,	\
> +	NULL,							\
> +};								\
> +static const struct attribute_group fan##index##_dev_groups = {	\
> +	.attrs = fan##index##_dev_attrs,			\
> +}
> +
> +pwm_index(1);
> +pwm_index(2);
> +pwm_index(3);
> +pwm_index(4);
> +pwm_index(5);
> +pwm_index(6);
> +pwm_index(7);
> +pwm_index(8);
> +
> +fan_index(1);
> +fan_index(2);
> +fan_index(3);
> +fan_index(4);
> +fan_index(5);
> +fan_index(6);
> +fan_index(7);
> +fan_index(8);
> +fan_index(9);
> +fan_index(10);
> +fan_index(11);
> +fan_index(12);
> +fan_index(13);
> +fan_index(14);
> +fan_index(15);
> +fan_index(16);
> +
Please no such macros, and not that many groups. One or at most two groups
(one for pwm, one for fans), then use the is_visible callback to determine
if an attribute is visible.


> +/*
> + * If the clock type is type M then :
> + * The PWM frequency = 24MHz / (type M clock division L bit *
> + * type M clock division H bit * (type M PWM period bit + 1))
> + * Calculate type M clock division L bit and H bits given the other values
> + */
> +static int aspeed_create_type(struct device_node *child,
> +			      struct aspeed_pwm_tacho_data *priv,
> +			      u8 index)
> +{
> +	u8 period, div_l, div_h;
> +	bool enable;
> +	u8 mode, div;
> +	u16 unit;
> +
> +	of_property_read_u8(child, "pwm_period", &period);
> +	of_property_read_u8(child, "pwm_clock_division_h", &div_h);
> +	of_property_read_u8(child, "pwm_clock_division_l", &div_l);

What happens if those properties are undefined ?

> +	priv->type_pwm_clock_division_h[index] = div_h;
> +	priv->type_pwm_clock_division_l[index] = div_l;
> +	priv->type_pwm_clock_unit[index] = period;
> +	aspeed_set_pwm_clock_values(priv->base, index, div_h, div_l, period);
> +
> +	enable = of_property_read_bool(child, "fan_tach_enable");
> +	aspeed_set_tacho_type_enable(priv->base, index, enable);
> +
> +	of_property_read_u8(child, "fan_tach_clock_division", &div);
> +	priv->type_fan_tach_clock_division[index] = div;
> +
> +	of_property_read_u8(child, "fan_tach_mode_selection", &mode);
> +
> +	of_property_read_u16(child, "fan_tach_period", &unit);
> +	priv->type_fan_tach_unit[index] = unit;
> +	aspeed_set_tacho_type_values(priv->base, index, mode, unit, div);
> +
> +	return 0;
> +}
> +
> +static int aspeed_create_pwm_port(struct device_node *child,
> +				  struct aspeed_pwm_tacho_data *priv, u8 index,
> +				  u8 group_index)
> +{
> +	u8 val;
> +	bool enable;
> +
> +	switch (index) {
> +	case 1:
> +		priv->groups[group_index] = &pwm1_dev_groups;
> +		break;
> +	case 2:
> +		priv->groups[group_index] = &pwm2_dev_groups;
> +		break;
> +	case 3:
> +		priv->groups[group_index] = &pwm3_dev_groups;
> +		break;
> +	case 4:
> +		priv->groups[group_index] = &pwm4_dev_groups;
> +		break;
> +	case 5:
> +		priv->groups[group_index] = &pwm5_dev_groups;
> +		break;
> +	case 6:
> +		priv->groups[group_index] = &pwm6_dev_groups;
> +		break;
> +	case 7:
> +		priv->groups[group_index] = &pwm7_dev_groups;
> +		break;
> +	case 8:
> +		priv->groups[group_index] = &pwm8_dev_groups;
> +		break;
> +	}
> +
> +	enable = of_property_read_bool(child, "enable");
> +	aspeed_set_pwm_port_enable(priv->base, index - 1, enable);
> +
This looks wrong. If the child is not enabled in the devicetree,
per standard devicetree convention, we should not get here.

Overall I am a bit guessing here, but it seems that you want consecutive
attributes with no gap. This isn't the way to go, though. The attribute
names associate with a given port should not change if another port is
enabled or disabled. pwmX should always point to a well defined pwm
pin on the chip. So, again, please use one group, and enable the individual
attributes with is_visible.

> +	of_property_read_u8(child, "type", &val);

What happens if this property is undefined ?

> +	priv->pwm_port_type[index - 1] = val;
> +	aspeed_set_pwm_port_type(priv->base, index - 1, val);
> +
> +	of_property_read_u8(child, "fan_ctrl", &val);

and if undefined ?

> +	priv->pwm_port_fan_ctrl[index - 1] = val;
> +	aspeed_set_pwm_port_fan_ctrl(priv, index - 1, val);
> +
> +	return 0;
> +}
> +
> +static int aspeed_create_fan_tach_channel(struct device *dev,
> +					  struct device_node *child,
> +					  struct aspeed_pwm_tacho_data *priv,
> +					  u8 index, u8 group_index)
> +{
> +	u8 val;
> +	bool enable;
> +	struct gpio_desc *fan_ctrl = devm_gpiod_get(dev, "fan-ctrl", GPIOD_IN);
> +
> +	if (IS_ERR(fan_ctrl))
> +		return PTR_ERR(fan_ctrl);
> +
> +	switch (index) {
> +	case 1:
> +		priv->groups[group_index] = &fan1_dev_groups;
> +		break;
> +	case 2:
> +		priv->groups[group_index] = &fan2_dev_groups;
> +		break;
> +	case 3:
> +		priv->groups[group_index] = &fan3_dev_groups;
> +		break;
> +	case 4:
> +		priv->groups[group_index] = &fan4_dev_groups;
> +		break;
> +	case 5:
> +		priv->groups[group_index] = &fan5_dev_groups;
> +		break;
> +	case 6:
> +		priv->groups[group_index] = &fan6_dev_groups;
> +		break;
> +	case 7:
> +		priv->groups[group_index] = &fan7_dev_groups;
> +		break;
> +	case 8:
> +		priv->groups[group_index] = &fan8_dev_groups;
> +		break;
> +	case 9:
> +		priv->groups[group_index] = &fan9_dev_groups;
> +		break;
> +	case 10:
> +		priv->groups[group_index] = &fan10_dev_groups;
> +		break;
> +	case 11:
> +		priv->groups[group_index] = &fan11_dev_groups;
> +		break;
> +	case 12:
> +		priv->groups[group_index] = &fan12_dev_groups;
> +		break;
> +	case 13:
> +		priv->groups[group_index] = &fan13_dev_groups;
> +		break;
> +	case 14:
> +		priv->groups[group_index] = &fan14_dev_groups;
> +		break;
> +	case 15:
> +		priv->groups[group_index] = &fan15_dev_groups;
> +		break;
> +	case 16:
> +		priv->groups[group_index] = &fan16_dev_groups;
> +		break;
> +	}
> +
> +	enable = of_property_read_bool(child, "enable");
> +	aspeed_set_fan_tach_ch_enable(priv->base, index - 1, enable);
> +
> +	of_property_read_u8(child, "pwm_source", &val);

if undefined ?

> +	priv->fan_tach_ch_source[index - 1] = val;
> +	aspeed_set_fan_tach_ch_source(priv->base, index - 1, val);
> +
> +	return 0;
> +}
> +
> +static int aspeed_pwm_tacho_probe(struct platform_device *pdev)
> +{
	struct device *dev = &pdev->dev;

might make the rest of the code easier to read.

> +	struct device_node *np, *type_np, *pwm_np, *fan_tach_np, *child;
> +	u8 pwm_index = 1, fan_index = 1, type_index = 0, group_index = 0;
> +	struct aspeed_pwm_tacho_data *priv;
> +	struct resource *res;
> +	struct device *hwmon;
> +	void __iomem *base;
> +	struct clk *clk;
> +
> +	np = pdev->dev.of_node;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return -ENOENT;
> +
> +	base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
> +	if (!base)
> +		return -ENOMEM;
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +	priv->base = base;
> +
> +	iowrite32(0, base + ASPEED_PTCR_TACH_SOURCE);
> +	iowrite32(0, base + ASPEED_PTCR_TACH_SOURCE_EXT);
> +
> +	clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(clk))
> +		return -ENODEV;
> +
> +	priv->clk_freq = clk_get_rate(clk);
> +	aspeed_set_clock_enable(base, true);
> +	aspeed_set_clock_source(base, 0);
> +
> +	type_np = of_get_child_by_name(np, "type_values");
> +	for_each_child_of_node(type_np, child) {
> +		aspeed_create_type(child, priv, type_index++);
> +		of_node_put(child);

Wrong.

> +	}
> +	of_node_put(type_np);
> +	pwm_np = of_get_child_by_name(np, "pwm_port");
> +	for_each_child_of_node(pwm_np, child) {
> +		aspeed_create_pwm_port(child, priv, pwm_index++,
> +				       group_index++);
> +		of_node_put(child);

This is wrong (was there a 0day about it ? I dont recall).

> +	}
> +	of_node_put(pwm_np);
> +	fan_tach_np = of_get_child_by_name(np, "fan_tach_channel");
> +	for_each_child_of_node(fan_tach_np, child) {
> +		aspeed_create_fan_tach_channel(&pdev->dev, child, priv,
> +					       fan_index++, group_index++);
> +		of_node_put(child);

Wrong.

> +	}
> +	of_node_put(fan_tach_np);
> +	of_node_put(np);

I may be wrong, but I don't see why this would be needed.

> +
> +	hwmon = devm_hwmon_device_register_with_groups(&pdev->dev,
> +						       "aspeed_pwm_tacho",
> +						       priv, priv->groups);
> +	if (IS_ERR(hwmon))
> +		return PTR_ERR(hwmon);

return PTR_ERR_OR_ZERO(hwmon);

> +
> +	return 0;
> +}
> +
> +static const struct of_device_id of_pwm_tacho_match_table[] = {
> +	{ .compatible = "aspeed,aspeed2400-pwm-tacho", },
> +	{ .compatible = "aspeed,aspeed2500-pwm-tacho", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, of_pwm_tacho_match_table);
> +
> +static struct platform_driver aspeed_pwm_tacho_driver = {
> +	.probe		= aspeed_pwm_tacho_probe,
> +	.driver		= {
> +		.name	= "aspeed_pwm_tacho",
> +		.owner	= THIS_MODULE,

not needed.

> +		.of_match_table = of_pwm_tacho_match_table,
> +	},
> +};
> +
> +module_platform_driver(aspeed_pwm_tacho_driver);
> +
> +MODULE_AUTHOR("Jaghathiswari Rankappagounder Natarajan <jaghu@google.com>");
> +MODULE_DESCRIPTION("ASPEED PWM and Fan Tacho device driver");
> +MODULE_LICENSE("GPL");
> --
> 2.11.0.390.gc69c2f50cf-goog
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>


^ permalink raw reply

* Re: [PATCH v3 2/2] mmc: pwrseq: add support for Marvell SD8787 chip
From: Matt Ranostay @ 2017-01-15 21:41 UTC (permalink / raw)
  To: Shawn Lin
  Cc: linux-wireless, Linux Kernel, linux-mmc, devicetree,
	Tony Lindgren, Ulf Hansson
In-Reply-To: <cd669d08-0db4-57e4-478b-2b200ccef6f1@rock-chips.com>

On Thu, Jan 12, 2017 at 11:16 PM, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> On 2017/1/13 13:29, Matt Ranostay wrote:
>>
>> Allow power sequencing for the Marvell SD8787 Wifi/BT chip.
>> This can be abstracted to other chipsets if needed in the future.
>>
>> Cc: Tony Lindgren <tony@atomide.com>
>> Cc: Ulf Hansson <ulf.hansson@linaro.org>
>> Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
>> ---
>>  drivers/mmc/core/Kconfig         |  10 ++++
>>  drivers/mmc/core/Makefile        |   1 +
>>  drivers/mmc/core/pwrseq_sd8787.c | 117
>> +++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 128 insertions(+)
>>  create mode 100644 drivers/mmc/core/pwrseq_sd8787.c
>>
>> diff --git a/drivers/mmc/core/Kconfig b/drivers/mmc/core/Kconfig
>> index cdfa8520a4b1..fc1ecdaaa9ca 100644
>> --- a/drivers/mmc/core/Kconfig
>> +++ b/drivers/mmc/core/Kconfig
>> @@ -12,6 +12,16 @@ config PWRSEQ_EMMC
>>           This driver can also be built as a module. If so, the module
>>           will be called pwrseq_emmc.
>>
>> +config PWRSEQ_SD8787
>> +       tristate "HW reset support for SD8787 BT + Wifi module"
>> +       depends on OF && (MWIFIEX || BT_MRVL_SDIO)
>> +       help
>> +         This selects hardware reset support for the SD8787 BT + Wifi
>> +         module. By default this option is set to n.
>> +
>> +         This driver can also be built as a module. If so, the module
>> +         will be called pwrseq_sd8787.
>> +
>
>
> I don't like this way, as we have a chance to list lots
> configure options here. wifi A,B,C,D...Z, all of them need a
> new section here if needed?
>
> Instead, could you just extent pwrseq_simple.c and add you
> .compatible = "mmc-pwrseq-sd8787", "mmc-pwrseq-simple"?

You mean all the chipset pwrseqs linked into the pwrseq-simple module?

Ulf your thoughts on this?

>
>
>
>>  config PWRSEQ_SIMPLE
>>         tristate "Simple HW reset support for MMC"
>>         default y
>> diff --git a/drivers/mmc/core/Makefile b/drivers/mmc/core/Makefile
>> index b2a257dc644f..0f81464fa824 100644
>> --- a/drivers/mmc/core/Makefile
>> +++ b/drivers/mmc/core/Makefile
>> @@ -10,6 +10,7 @@ mmc_core-y                    := core.o bus.o host.o \
>>                                    quirks.o slot-gpio.o
>>  mmc_core-$(CONFIG_OF)          += pwrseq.o
>>  obj-$(CONFIG_PWRSEQ_SIMPLE)    += pwrseq_simple.o
>> +obj-$(CONFIG_PWRSEQ_SD8787)    += pwrseq_sd8787.o
>>  obj-$(CONFIG_PWRSEQ_EMMC)      += pwrseq_emmc.o
>>  mmc_core-$(CONFIG_DEBUG_FS)    += debugfs.o
>>  obj-$(CONFIG_MMC_BLOCK)                += mmc_block.o
>> diff --git a/drivers/mmc/core/pwrseq_sd8787.c
>> b/drivers/mmc/core/pwrseq_sd8787.c
>> new file mode 100644
>> index 000000000000..f4080fe6439e
>> --- /dev/null
>> +++ b/drivers/mmc/core/pwrseq_sd8787.c
>> @@ -0,0 +1,117 @@
>> +/*
>> + * pwrseq_sd8787.c - power sequence support for Marvell SD8787 BT + Wifi
>> chip
>> + *
>> + * Copyright (C) 2016 Matt Ranostay <matt@ranostay.consulting>
>> + *
>> + * Based on the original work pwrseq_simple.c
>> + *  Copyright (C) 2014 Linaro Ltd
>> + *  Author: Ulf Hansson <ulf.hansson@linaro.org>
>> + *
>> + * 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.
>> + *
>> + */
>> +
>> +#include <linux/delay.h>
>> +#include <linux/init.h>
>> +#include <linux/kernel.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/module.h>
>> +#include <linux/slab.h>
>> +#include <linux/device.h>
>> +#include <linux/err.h>
>> +#include <linux/gpio/consumer.h>
>> +
>> +#include <linux/mmc/host.h>
>> +
>> +#include "pwrseq.h"
>> +
>> +struct mmc_pwrseq_sd8787 {
>> +       struct mmc_pwrseq pwrseq;
>> +       struct gpio_desc *reset_gpio;
>> +       struct gpio_desc *pwrdn_gpio;
>> +};
>> +
>> +#define to_pwrseq_sd8787(p) container_of(p, struct mmc_pwrseq_sd8787,
>> pwrseq)
>> +
>> +static void mmc_pwrseq_sd8787_pre_power_on(struct mmc_host *host)
>> +{
>> +       struct mmc_pwrseq_sd8787 *pwrseq = to_pwrseq_sd8787(host->pwrseq);
>> +
>> +       gpiod_set_value_cansleep(pwrseq->reset_gpio, 1);
>> +
>> +       msleep(300);
>> +       gpiod_set_value_cansleep(pwrseq->pwrdn_gpio, 1);
>> +}
>> +
>> +static void mmc_pwrseq_sd8787_power_off(struct mmc_host *host)
>> +{
>> +       struct mmc_pwrseq_sd8787 *pwrseq = to_pwrseq_sd8787(host->pwrseq);
>> +
>> +       gpiod_set_value_cansleep(pwrseq->pwrdn_gpio, 0);
>> +       gpiod_set_value_cansleep(pwrseq->reset_gpio, 0);
>> +}
>> +
>> +static const struct mmc_pwrseq_ops mmc_pwrseq_sd8787_ops = {
>> +       .pre_power_on = mmc_pwrseq_sd8787_pre_power_on,
>> +       .power_off = mmc_pwrseq_sd8787_power_off,
>> +};
>> +
>> +static const struct of_device_id mmc_pwrseq_sd8787_of_match[] = {
>> +       { .compatible = "mmc-pwrseq-sd8787",},
>> +       {/* sentinel */},
>> +};
>> +MODULE_DEVICE_TABLE(of, mmc_pwrseq_sd8787_of_match);
>> +
>> +static int mmc_pwrseq_sd8787_probe(struct platform_device *pdev)
>> +{
>> +       struct mmc_pwrseq_sd8787 *pwrseq;
>> +       struct device *dev = &pdev->dev;
>> +
>> +       pwrseq = devm_kzalloc(dev, sizeof(*pwrseq), GFP_KERNEL);
>> +       if (!pwrseq)
>> +               return -ENOMEM;
>> +
>> +       pwrseq->pwrdn_gpio = devm_gpiod_get(dev, "pwrdn", GPIOD_OUT_LOW);
>> +       if (IS_ERR(pwrseq->pwrdn_gpio))
>> +               return PTR_ERR(pwrseq->pwrdn_gpio);
>> +
>> +       pwrseq->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
>> +       if (IS_ERR(pwrseq->reset_gpio))
>> +               return PTR_ERR(pwrseq->reset_gpio);
>> +
>> +       pwrseq->pwrseq.dev = dev;
>> +       pwrseq->pwrseq.ops = &mmc_pwrseq_sd8787_ops;
>> +       pwrseq->pwrseq.owner = THIS_MODULE;
>> +       platform_set_drvdata(pdev, pwrseq);
>> +
>> +       return mmc_pwrseq_register(&pwrseq->pwrseq);
>> +}
>> +
>> +static int mmc_pwrseq_sd8787_remove(struct platform_device *pdev)
>> +{
>> +       struct mmc_pwrseq_sd8787 *pwrseq = platform_get_drvdata(pdev);
>> +
>> +       mmc_pwrseq_unregister(&pwrseq->pwrseq);
>> +
>> +       return 0;
>> +}
>> +
>> +static struct platform_driver mmc_pwrseq_sd8787_driver = {
>> +       .probe = mmc_pwrseq_sd8787_probe,
>> +       .remove = mmc_pwrseq_sd8787_remove,
>> +       .driver = {
>> +               .name = "pwrseq_sd8787",
>> +               .of_match_table = mmc_pwrseq_sd8787_of_match,
>> +       },
>> +};
>> +
>> +module_platform_driver(mmc_pwrseq_sd8787_driver);
>> +MODULE_LICENSE("GPL v2");
>>
>
>
> --
> Best Regards
> Shawn Lin
>

^ permalink raw reply

* [PATCH 1/2] ARM: dts: imx6ul-isiot: Add eMMC node
From: Jagan Teki @ 2017-01-15 21:50 UTC (permalink / raw)
  To: Shawn Guo
  Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, Matteo Lisi,
	Michael Trimarchi, Jagan Teki

From: Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org>

Add usdhc2 node, which is eMMC for Engicam Is.IoT MX6UL modules.

dmesg:
-----
mmc1: SDHCI controller on 2194000.usdhc [2194000.usdhc] using ADMA
mmc1: new DDR MMC card at address 0001
mmcblk1: mmc1:0001 M62704 3.53 GiB

Cc: Matteo Lisi <matteo.lisi-4s7YQHO/iPVBDgjK7y7TUQ@public.gmane.org>
Cc: Michael Trimarchi <michael-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org>
Cc: Signed-off-by: Shawn Guo <shawnguo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Signed-off-by: Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org>
---
 arch/arm/boot/dts/imx6ul-isiot.dts | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/arch/arm/boot/dts/imx6ul-isiot.dts b/arch/arm/boot/dts/imx6ul-isiot.dts
index 077bc26..acb97bd 100644
--- a/arch/arm/boot/dts/imx6ul-isiot.dts
+++ b/arch/arm/boot/dts/imx6ul-isiot.dts
@@ -76,6 +76,15 @@
 	status = "okay";
 };
 
+&usdhc2 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_usdhc2>;
+	cd-gpios = <&gpio4 5 GPIO_ACTIVE_LOW>;
+	bus-width = <8>;
+	no-1-8-v;
+	status = "okay";
+};
+
 &iomuxc {
 	pinctrl_uart1: uart1grp {
 		fsl,pins = <
@@ -116,4 +125,20 @@
 			MX6UL_PAD_SD1_DATA3__USDHC1_DATA3 0x170f9
 		>;
 	};
+
+	pinctrl_usdhc2: usdhc2grp {
+		fsl,pins = <
+			MX6UL_PAD_NAND_RE_B__USDHC2_CLK      0x17070
+			MX6UL_PAD_NAND_WE_B__USDHC2_CMD      0x10070
+			MX6UL_PAD_NAND_DATA00__USDHC2_DATA0  0x17070
+			MX6UL_PAD_NAND_DATA01__USDHC2_DATA1  0x17070
+			MX6UL_PAD_NAND_DATA02__USDHC2_DATA2  0x17070
+			MX6UL_PAD_NAND_DATA03__USDHC2_DATA3  0x17070
+			MX6UL_PAD_NAND_DATA04__USDHC2_DATA4  0x17070
+			MX6UL_PAD_NAND_DATA05__USDHC2_DATA5  0x17070
+			MX6UL_PAD_NAND_DATA06__USDHC2_DATA6  0x17070
+			MX6UL_PAD_NAND_DATA07__USDHC2_DATA7  0x17070
+			MX6UL_PAD_NAND_ALE__USDHC2_RESET_B   0x17070
+		>;
+	};
 };
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox