[PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver

[PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver

[Priit Laes]

---
 .../ABI/testing/sysfs-driver-input-sun4i-lradc     |  4 ++
 drivers/input/keyboard/sun4i-lradc-keys.c          | 49 +++++++++++++++++-----
 2 files changed, 43 insertions(+), 10 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc

diff --git a/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
new file mode 100644
index 0000000..e4e6448
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
@@ -0,0 +1,4 @@
+What:		/sys/class/input/input(x)/device/voltage
+Date:		February 2015
+Contact:	Priit Laes <plaes@plaes.org>
+Description:	ADC output voltage in microvolts or 0 if device is not opened.
diff --git a/drivers/input/keyboard/sun4i-lradc-keys.c b/drivers/input/keyboard/sun4i-lradc-keys.c
index cc8f7dd..c0ab8ec 100644
--- a/drivers/input/keyboard/sun4i-lradc-keys.c
+++ b/drivers/input/keyboard/sun4i-lradc-keys.c
@@ -79,10 +79,27 @@ struct sun4i_lradc_data {
 	u32 vref;
 };
 
+static u32 sun4i_lradc_read_voltage(struct sun4i_lradc_data *lradc)
+{
+	u32 val = readl(lradc->base + LRADC_DATA0) & 0x3f;
+	return val * lradc->vref / 63;
+};
+
+static ssize_t
+sun4i_lradc_dev_voltage_show(struct device *dev,
+			struct device_attribute *attr, char *buf)
+{
+	struct sun4i_lradc_data *lradc = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%u\n", sun4i_lradc_read_voltage(lradc));
+}
+
+static const DEVICE_ATTR(voltage, S_IRUGO, sun4i_lradc_dev_voltage_show, NULL);
+
 static irqreturn_t sun4i_lradc_irq(int irq, void *dev_id)
 {
 	struct sun4i_lradc_data *lradc = dev_id;
-	u32 i, ints, val, voltage, diff, keycode = 0, closest = 0xffffffff;
+	u32 i, ints, voltage, diff, keycode = 0, closest = 0xffffffff;
 
 	ints  = readl(lradc->base + LRADC_INTS);
 
@@ -97,8 +114,7 @@ static irqreturn_t sun4i_lradc_irq(int irq, void *dev_id)
 	}
 
 	if ((ints & CHAN0_KEYDOWN_IRQ) && lradc->chan0_keycode == 0) {
-		val = readl(lradc->base + LRADC_DATA0) & 0x3f;
-		voltage = val * lradc->vref / 63;
+		voltage = sun4i_lradc_read_voltage(lradc);
 
 		for (i = 0; i < lradc->chan0_map_count; i++) {
 			diff = abs(lradc->chan0_map[i].voltage - voltage);
@@ -156,7 +172,7 @@ static void sun4i_lradc_close(struct input_dev *dev)
 }
 
 static int sun4i_lradc_load_dt_keymap(struct device *dev,
-				      struct sun4i_lradc_data *lradc)
+				    struct sun4i_lradc_data *lradc)
 {
 	struct device_node *np, *pp;
 	int i;
@@ -168,8 +184,8 @@ static int sun4i_lradc_load_dt_keymap(struct device *dev,
 
 	lradc->chan0_map_count = of_get_child_count(np);
 	if (lradc->chan0_map_count == 0) {
-		dev_err(dev, "keymap is missing in device tree\n");
-		return -EINVAL;
+		dev_info(dev, "keymap is missing in device tree\n");
+		return 0;
 	}
 
 	lradc->chan0_map = devm_kmalloc_array(dev, lradc->chan0_map_count,
@@ -185,19 +201,19 @@ static int sun4i_lradc_load_dt_keymap(struct device *dev,
 
 		error = of_property_read_u32(pp, "channel", &channel);
 		if (error || channel != 0) {
-			dev_err(dev, "%s: Inval channel prop\n", pp->name);
+			dev_err(dev, "%s: Invalid 'channel' property\n", pp->name);
 			return -EINVAL;
 		}
 
 		error = of_property_read_u32(pp, "voltage", &map->voltage);
 		if (error) {
-			dev_err(dev, "%s: Inval voltage prop\n", pp->name);
+			dev_err(dev, "%s: Invalid 'voltage' property\n", pp->name);
 			return -EINVAL;
 		}
 
 		error = of_property_read_u32(pp, "linux,code", &map->keycode);
 		if (error) {
-			dev_err(dev, "%s: Inval linux,code prop\n", pp->name);
+			dev_err(dev, "%s: Invalid 'linux,code' property\n", pp->name);
 			return -EINVAL;
 		}
 
@@ -257,14 +273,26 @@ static int sun4i_lradc_probe(struct platform_device *pdev)
 	if (error)
 		return error;
 
-	error = input_register_device(lradc->input);
+	error = device_create_file(dev, &dev_attr_voltage);
 	if (error)
 		return error;
 
+	error = input_register_device(lradc->input);
+	if (error) {
+		device_remove_file(&pdev->dev, &dev_attr_voltage);
+		return error;
+	}
+
 	platform_set_drvdata(pdev, lradc);
 	return 0;
 }
 
+static int sun4i_lradc_remove(struct platform_device *pdev)
+{
+	device_remove_file(&pdev->dev, &dev_attr_voltage);
+	return 0;
+}
+
 static const struct of_device_id sun4i_lradc_of_match[] = {
 	{ .compatible = "allwinner,sun4i-a10-lradc-keys", },
 	{ /* sentinel */ }
@@ -277,6 +305,7 @@ static struct platform_driver sun4i_lradc_driver = {
 		.of_match_table = of_match_ptr(sun4i_lradc_of_match),
 	},
 	.probe	= sun4i_lradc_probe,
+	.remove = sun4i_lradc_remove,
 };
 
 module_platform_driver(sun4i_lradc_driver);
-- 
2.2.2

[PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver

[Priit Laes]

---
 .../ABI/testing/sysfs-driver-input-sun4i-lradc     |  4 ++
 drivers/input/keyboard/sun4i-lradc-keys.c          | 49 +++++++++++++++++-----
 2 files changed, 43 insertions(+), 10 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc

diff --git a/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
new file mode 100644
index 0000000..e4e6448
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
@@ -0,0 +1,4 @@
+What:		/sys/class/input/input(x)/device/voltage
+Date:		February 2015
+Contact:	Priit Laes <plaes-q/aMd4JkU83YtjvyW6yDsg@public.gmane.org>
+Description:	ADC output voltage in microvolts or 0 if device is not opened.
diff --git a/drivers/input/keyboard/sun4i-lradc-keys.c b/drivers/input/keyboard/sun4i-lradc-keys.c
index cc8f7dd..c0ab8ec 100644
--- a/drivers/input/keyboard/sun4i-lradc-keys.c
+++ b/drivers/input/keyboard/sun4i-lradc-keys.c
@@ -79,10 +79,27 @@ struct sun4i_lradc_data {
 	u32 vref;
 };
 
+static u32 sun4i_lradc_read_voltage(struct sun4i_lradc_data *lradc)
+{
+	u32 val = readl(lradc->base + LRADC_DATA0) & 0x3f;
+	return val * lradc->vref / 63;
+};
+
+static ssize_t
+sun4i_lradc_dev_voltage_show(struct device *dev,
+			struct device_attribute *attr, char *buf)
+{
+	struct sun4i_lradc_data *lradc = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%u\n", sun4i_lradc_read_voltage(lradc));
+}
+
+static const DEVICE_ATTR(voltage, S_IRUGO, sun4i_lradc_dev_voltage_show, NULL);
+
 static irqreturn_t sun4i_lradc_irq(int irq, void *dev_id)
 {
 	struct sun4i_lradc_data *lradc = dev_id;
-	u32 i, ints, val, voltage, diff, keycode = 0, closest = 0xffffffff;
+	u32 i, ints, voltage, diff, keycode = 0, closest = 0xffffffff;
 
 	ints  = readl(lradc->base + LRADC_INTS);
 
@@ -97,8 +114,7 @@ static irqreturn_t sun4i_lradc_irq(int irq, void *dev_id)
 	}
 
 	if ((ints & CHAN0_KEYDOWN_IRQ) && lradc->chan0_keycode == 0) {
-		val = readl(lradc->base + LRADC_DATA0) & 0x3f;
-		voltage = val * lradc->vref / 63;
+		voltage = sun4i_lradc_read_voltage(lradc);
 
 		for (i = 0; i < lradc->chan0_map_count; i++) {
 			diff = abs(lradc->chan0_map[i].voltage - voltage);
@@ -156,7 +172,7 @@ static void sun4i_lradc_close(struct input_dev *dev)
 }
 
 static int sun4i_lradc_load_dt_keymap(struct device *dev,
-				      struct sun4i_lradc_data *lradc)
+				    struct sun4i_lradc_data *lradc)
 {
 	struct device_node *np, *pp;
 	int i;
@@ -168,8 +184,8 @@ static int sun4i_lradc_load_dt_keymap(struct device *dev,
 
 	lradc->chan0_map_count = of_get_child_count(np);
 	if (lradc->chan0_map_count == 0) {
-		dev_err(dev, "keymap is missing in device tree\n");
-		return -EINVAL;
+		dev_info(dev, "keymap is missing in device tree\n");
+		return 0;
 	}
 
 	lradc->chan0_map = devm_kmalloc_array(dev, lradc->chan0_map_count,
@@ -185,19 +201,19 @@ static int sun4i_lradc_load_dt_keymap(struct device *dev,
 
 		error = of_property_read_u32(pp, "channel", &channel);
 		if (error || channel != 0) {
-			dev_err(dev, "%s: Inval channel prop\n", pp->name);
+			dev_err(dev, "%s: Invalid 'channel' property\n", pp->name);
 			return -EINVAL;
 		}
 
 		error = of_property_read_u32(pp, "voltage", &map->voltage);
 		if (error) {
-			dev_err(dev, "%s: Inval voltage prop\n", pp->name);
+			dev_err(dev, "%s: Invalid 'voltage' property\n", pp->name);
 			return -EINVAL;
 		}
 
 		error = of_property_read_u32(pp, "linux,code", &map->keycode);
 		if (error) {
-			dev_err(dev, "%s: Inval linux,code prop\n", pp->name);
+			dev_err(dev, "%s: Invalid 'linux,code' property\n", pp->name);
 			return -EINVAL;
 		}
 
@@ -257,14 +273,26 @@ static int sun4i_lradc_probe(struct platform_device *pdev)
 	if (error)
 		return error;
 
-	error = input_register_device(lradc->input);
+	error = device_create_file(dev, &dev_attr_voltage);
 	if (error)
 		return error;
 
+	error = input_register_device(lradc->input);
+	if (error) {
+		device_remove_file(&pdev->dev, &dev_attr_voltage);
+		return error;
+	}
+
 	platform_set_drvdata(pdev, lradc);
 	return 0;
 }
 
+static int sun4i_lradc_remove(struct platform_device *pdev)
+{
+	device_remove_file(&pdev->dev, &dev_attr_voltage);
+	return 0;
+}
+
 static const struct of_device_id sun4i_lradc_of_match[] = {
 	{ .compatible = "allwinner,sun4i-a10-lradc-keys", },
 	{ /* sentinel */ }
@@ -277,6 +305,7 @@ static struct platform_driver sun4i_lradc_driver = {
 		.of_match_table = of_match_ptr(sun4i_lradc_of_match),
 	},
 	.probe	= sun4i_lradc_probe,
+	.remove = sun4i_lradc_remove,
 };
 
 module_platform_driver(sun4i_lradc_driver);
-- 
2.2.2

[PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver

[Priit Laes]

---
 .../ABI/testing/sysfs-driver-input-sun4i-lradc     |  4 ++
 drivers/input/keyboard/sun4i-lradc-keys.c          | 49 +++++++++++++++++-----
 2 files changed, 43 insertions(+), 10 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc

diff --git a/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
new file mode 100644
index 0000000..e4e6448
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
@@ -0,0 +1,4 @@
+What:		/sys/class/input/input(x)/device/voltage
+Date:		February 2015
+Contact:	Priit Laes <plaes@plaes.org>
+Description:	ADC output voltage in microvolts or 0 if device is not opened.
diff --git a/drivers/input/keyboard/sun4i-lradc-keys.c b/drivers/input/keyboard/sun4i-lradc-keys.c
index cc8f7dd..c0ab8ec 100644
--- a/drivers/input/keyboard/sun4i-lradc-keys.c
+++ b/drivers/input/keyboard/sun4i-lradc-keys.c
@@ -79,10 +79,27 @@ struct sun4i_lradc_data {
 	u32 vref;
 };
 
+static u32 sun4i_lradc_read_voltage(struct sun4i_lradc_data *lradc)
+{
+	u32 val = readl(lradc->base + LRADC_DATA0) & 0x3f;
+	return val * lradc->vref / 63;
+};
+
+static ssize_t
+sun4i_lradc_dev_voltage_show(struct device *dev,
+			struct device_attribute *attr, char *buf)
+{
+	struct sun4i_lradc_data *lradc = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%u\n", sun4i_lradc_read_voltage(lradc));
+}
+
+static const DEVICE_ATTR(voltage, S_IRUGO, sun4i_lradc_dev_voltage_show, NULL);
+
 static irqreturn_t sun4i_lradc_irq(int irq, void *dev_id)
 {
 	struct sun4i_lradc_data *lradc = dev_id;
-	u32 i, ints, val, voltage, diff, keycode = 0, closest = 0xffffffff;
+	u32 i, ints, voltage, diff, keycode = 0, closest = 0xffffffff;
 
 	ints  = readl(lradc->base + LRADC_INTS);
 
@@ -97,8 +114,7 @@ static irqreturn_t sun4i_lradc_irq(int irq, void *dev_id)
 	}
 
 	if ((ints & CHAN0_KEYDOWN_IRQ) && lradc->chan0_keycode == 0) {
-		val = readl(lradc->base + LRADC_DATA0) & 0x3f;
-		voltage = val * lradc->vref / 63;
+		voltage = sun4i_lradc_read_voltage(lradc);
 
 		for (i = 0; i < lradc->chan0_map_count; i++) {
 			diff = abs(lradc->chan0_map[i].voltage - voltage);
@@ -156,7 +172,7 @@ static void sun4i_lradc_close(struct input_dev *dev)
 }
 
 static int sun4i_lradc_load_dt_keymap(struct device *dev,
-				      struct sun4i_lradc_data *lradc)
+				    struct sun4i_lradc_data *lradc)
 {
 	struct device_node *np, *pp;
 	int i;
@@ -168,8 +184,8 @@ static int sun4i_lradc_load_dt_keymap(struct device *dev,
 
 	lradc->chan0_map_count = of_get_child_count(np);
 	if (lradc->chan0_map_count == 0) {
-		dev_err(dev, "keymap is missing in device tree\n");
-		return -EINVAL;
+		dev_info(dev, "keymap is missing in device tree\n");
+		return 0;
 	}
 
 	lradc->chan0_map = devm_kmalloc_array(dev, lradc->chan0_map_count,
@@ -185,19 +201,19 @@ static int sun4i_lradc_load_dt_keymap(struct device *dev,
 
 		error = of_property_read_u32(pp, "channel", &channel);
 		if (error || channel != 0) {
-			dev_err(dev, "%s: Inval channel prop\n", pp->name);
+			dev_err(dev, "%s: Invalid 'channel' property\n", pp->name);
 			return -EINVAL;
 		}
 
 		error = of_property_read_u32(pp, "voltage", &map->voltage);
 		if (error) {
-			dev_err(dev, "%s: Inval voltage prop\n", pp->name);
+			dev_err(dev, "%s: Invalid 'voltage' property\n", pp->name);
 			return -EINVAL;
 		}
 
 		error = of_property_read_u32(pp, "linux,code", &map->keycode);
 		if (error) {
-			dev_err(dev, "%s: Inval linux,code prop\n", pp->name);
+			dev_err(dev, "%s: Invalid 'linux,code' property\n", pp->name);
 			return -EINVAL;
 		}
 
@@ -257,14 +273,26 @@ static int sun4i_lradc_probe(struct platform_device *pdev)
 	if (error)
 		return error;
 
-	error = input_register_device(lradc->input);
+	error = device_create_file(dev, &dev_attr_voltage);
 	if (error)
 		return error;
 
+	error = input_register_device(lradc->input);
+	if (error) {
+		device_remove_file(&pdev->dev, &dev_attr_voltage);
+		return error;
+	}
+
 	platform_set_drvdata(pdev, lradc);
 	return 0;
 }
 
+static int sun4i_lradc_remove(struct platform_device *pdev)
+{
+	device_remove_file(&pdev->dev, &dev_attr_voltage);
+	return 0;
+}
+
 static const struct of_device_id sun4i_lradc_of_match[] = {
 	{ .compatible = "allwinner,sun4i-a10-lradc-keys", },
 	{ /* sentinel */ }
@@ -277,6 +305,7 @@ static struct platform_driver sun4i_lradc_driver = {
 		.of_match_table = of_match_ptr(sun4i_lradc_of_match),
 	},
 	.probe	= sun4i_lradc_probe,
+	.remove = sun4i_lradc_remove,
 };
 
 module_platform_driver(sun4i_lradc_driver);
-- 
2.2.2

Re: [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver

[Hans de Goede]

Hi,

On 26-01-15 17:58, Priit Laes wrote:

No commit message? Please write an informative commit msg, like why we want this patch,
I guess it is to help figuring out the voltage levels for various buttons when creating
a dts, but I would prefer to not guess, which is where a good commit message would
come in handy ...

> ---
>   .../ABI/testing/sysfs-driver-input-sun4i-lradc     |  4 ++
>   drivers/input/keyboard/sun4i-lradc-keys.c          | 49 +++++++++++++++++-----
>   2 files changed, 43 insertions(+), 10 deletions(-)
>   create mode 100644 Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
>
> diff --git a/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> new file mode 100644
> index 0000000..e4e6448
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> @@ -0,0 +1,4 @@
> +What:		/sys/class/input/input(x)/device/voltage
> +Date:		February 2015
> +Contact:	Priit Laes <plaes-q/aMd4JkU83YtjvyW6yDsg@public.gmane.org>
> +Description:	ADC output voltage in microvolts or 0 if device is not opened.
> diff --git a/drivers/input/keyboard/sun4i-lradc-keys.c b/drivers/input/keyboard/sun4i-lradc-keys.c
> index cc8f7dd..c0ab8ec 100644
> --- a/drivers/input/keyboard/sun4i-lradc-keys.c
> +++ b/drivers/input/keyboard/sun4i-lradc-keys.c
> @@ -79,10 +79,27 @@ struct sun4i_lradc_data {
>   	u32 vref;
>   };
>
> +static u32 sun4i_lradc_read_voltage(struct sun4i_lradc_data *lradc)
> +{
> +	u32 val = readl(lradc->base + LRADC_DATA0) & 0x3f;
> +	return val * lradc->vref / 63;
> +};
> +
> +static ssize_t
> +sun4i_lradc_dev_voltage_show(struct device *dev,
> +			struct device_attribute *attr, char *buf)
> +{
> +	struct sun4i_lradc_data *lradc = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%u\n", sun4i_lradc_read_voltage(lradc));
> +}
> +
> +static const DEVICE_ATTR(voltage, S_IRUGO, sun4i_lradc_dev_voltage_show, NULL);
> +
>   static irqreturn_t sun4i_lradc_irq(int irq, void *dev_id)
>   {
>   	struct sun4i_lradc_data *lradc = dev_id;
> -	u32 i, ints, val, voltage, diff, keycode = 0, closest = 0xffffffff;
> +	u32 i, ints, voltage, diff, keycode = 0, closest = 0xffffffff;
>
>   	ints  = readl(lradc->base + LRADC_INTS);
>
> @@ -97,8 +114,7 @@ static irqreturn_t sun4i_lradc_irq(int irq, void *dev_id)
>   	}
>
>   	if ((ints & CHAN0_KEYDOWN_IRQ) && lradc->chan0_keycode == 0) {
> -		val = readl(lradc->base + LRADC_DATA0) & 0x3f;
> -		voltage = val * lradc->vref / 63;
> +		voltage = sun4i_lradc_read_voltage(lradc);
>
>   		for (i = 0; i < lradc->chan0_map_count; i++) {
>   			diff = abs(lradc->chan0_map[i].voltage - voltage);
> @@ -156,7 +172,7 @@ static void sun4i_lradc_close(struct input_dev *dev)
>   }
>
>   static int sun4i_lradc_load_dt_keymap(struct device *dev,
> -				      struct sun4i_lradc_data *lradc)
> +				    struct sun4i_lradc_data *lradc)
>   {
>   	struct device_node *np, *pp;
>   	int i;

Why this identation change ?

> @@ -168,8 +184,8 @@ static int sun4i_lradc_load_dt_keymap(struct device *dev,
>
>   	lradc->chan0_map_count = of_get_child_count(np);
>   	if (lradc->chan0_map_count == 0) {
> -		dev_err(dev, "keymap is missing in device tree\n");
> -		return -EINVAL;
> +		dev_info(dev, "keymap is missing in device tree\n");
> +		return 0;
>   	}
>
>   	lradc->chan0_map = devm_kmalloc_array(dev, lradc->chan0_map_count,

I assume this is so that people can still use the sysfs node, to create a dts, right
not sure I like this, might be better to document to simple create a dts with
a single button mapping for 200 mV (most board use 200 mV steps between the buttons).

> @@ -185,19 +201,19 @@ static int sun4i_lradc_load_dt_keymap(struct device *dev,
>
>   		error = of_property_read_u32(pp, "channel", &channel);
>   		if (error || channel != 0) {
> -			dev_err(dev, "%s: Inval channel prop\n", pp->name);
> +			dev_err(dev, "%s: Invalid 'channel' property\n", pp->name);
>   			return -EINVAL;
>   		}
>
>   		error = of_property_read_u32(pp, "voltage", &map->voltage);
>   		if (error) {
> -			dev_err(dev, "%s: Inval voltage prop\n", pp->name);
> +			dev_err(dev, "%s: Invalid 'voltage' property\n", pp->name);
>   			return -EINVAL;
>   		}
>
>   		error = of_property_read_u32(pp, "linux,code", &map->keycode);
>   		if (error) {
> -			dev_err(dev, "%s: Inval linux,code prop\n", pp->name);
> +			dev_err(dev, "%s: Invalid 'linux,code' property\n", pp->name);
>   			return -EINVAL;
>   		}
>

This hunk / 3 changes belong in a separate patch. Also please run checkpatch, I think
you're running over 80 chars here.


> @@ -257,14 +273,26 @@ static int sun4i_lradc_probe(struct platform_device *pdev)
>   	if (error)
>   		return error;
>
> -	error = input_register_device(lradc->input);
> +	error = device_create_file(dev, &dev_attr_voltage);
>   	if (error)
>   		return error;
>
> +	error = input_register_device(lradc->input);
> +	if (error) {
> +		device_remove_file(&pdev->dev, &dev_attr_voltage);
> +		return error;
> +	}
> +
>   	platform_set_drvdata(pdev, lradc);
>   	return 0;
>   }
>
> +static int sun4i_lradc_remove(struct platform_device *pdev)
> +{
> +	device_remove_file(&pdev->dev, &dev_attr_voltage);
> +	return 0;
> +}
> +

This looks wrong, I think (*) that we've a bug here because we're not
unregistering the input device, so maybe do 2 patches, 1 fixing the
not unregistering bug, and then just add the device_remove_file()
in the sysfs patch.

>   static const struct of_device_id sun4i_lradc_of_match[] = {
>   	{ .compatible = "allwinner,sun4i-a10-lradc-keys", },
>   	{ /* sentinel */ }


> @@ -277,6 +305,7 @@ static struct platform_driver sun4i_lradc_driver = {
>   		.of_match_table = of_match_ptr(sun4i_lradc_of_match),
>   	},
>   	.probe	= sun4i_lradc_probe,
> +	.remove = sun4i_lradc_remove,
>   };
>
>   module_platform_driver(sun4i_lradc_driver);
>

Regards,

Hans


*) But I'm not sure, better double check.

[PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver

[Hans de Goede]

Hi,

On 26-01-15 17:58, Priit Laes wrote:

No commit message? Please write an informative commit msg, like why we want this patch,
I guess it is to help figuring out the voltage levels for various buttons when creating
a dts, but I would prefer to not guess, which is where a good commit message would
come in handy ...

> ---
>   .../ABI/testing/sysfs-driver-input-sun4i-lradc     |  4 ++
>   drivers/input/keyboard/sun4i-lradc-keys.c          | 49 +++++++++++++++++-----
>   2 files changed, 43 insertions(+), 10 deletions(-)
>   create mode 100644 Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
>
> diff --git a/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> new file mode 100644
> index 0000000..e4e6448
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> @@ -0,0 +1,4 @@
> +What:		/sys/class/input/input(x)/device/voltage
> +Date:		February 2015
> +Contact:	Priit Laes <plaes@plaes.org>
> +Description:	ADC output voltage in microvolts or 0 if device is not opened.
> diff --git a/drivers/input/keyboard/sun4i-lradc-keys.c b/drivers/input/keyboard/sun4i-lradc-keys.c
> index cc8f7dd..c0ab8ec 100644
> --- a/drivers/input/keyboard/sun4i-lradc-keys.c
> +++ b/drivers/input/keyboard/sun4i-lradc-keys.c
> @@ -79,10 +79,27 @@ struct sun4i_lradc_data {
>   	u32 vref;
>   };
>
> +static u32 sun4i_lradc_read_voltage(struct sun4i_lradc_data *lradc)
> +{
> +	u32 val = readl(lradc->base + LRADC_DATA0) & 0x3f;
> +	return val * lradc->vref / 63;
> +};
> +
> +static ssize_t
> +sun4i_lradc_dev_voltage_show(struct device *dev,
> +			struct device_attribute *attr, char *buf)
> +{
> +	struct sun4i_lradc_data *lradc = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%u\n", sun4i_lradc_read_voltage(lradc));
> +}
> +
> +static const DEVICE_ATTR(voltage, S_IRUGO, sun4i_lradc_dev_voltage_show, NULL);
> +
>   static irqreturn_t sun4i_lradc_irq(int irq, void *dev_id)
>   {
>   	struct sun4i_lradc_data *lradc = dev_id;
> -	u32 i, ints, val, voltage, diff, keycode = 0, closest = 0xffffffff;
> +	u32 i, ints, voltage, diff, keycode = 0, closest = 0xffffffff;
>
>   	ints  = readl(lradc->base + LRADC_INTS);
>
> @@ -97,8 +114,7 @@ static irqreturn_t sun4i_lradc_irq(int irq, void *dev_id)
>   	}
>
>   	if ((ints & CHAN0_KEYDOWN_IRQ) && lradc->chan0_keycode == 0) {
> -		val = readl(lradc->base + LRADC_DATA0) & 0x3f;
> -		voltage = val * lradc->vref / 63;
> +		voltage = sun4i_lradc_read_voltage(lradc);
>
>   		for (i = 0; i < lradc->chan0_map_count; i++) {
>   			diff = abs(lradc->chan0_map[i].voltage - voltage);
> @@ -156,7 +172,7 @@ static void sun4i_lradc_close(struct input_dev *dev)
>   }
>
>   static int sun4i_lradc_load_dt_keymap(struct device *dev,
> -				      struct sun4i_lradc_data *lradc)
> +				    struct sun4i_lradc_data *lradc)
>   {
>   	struct device_node *np, *pp;
>   	int i;

Why this identation change ?

> @@ -168,8 +184,8 @@ static int sun4i_lradc_load_dt_keymap(struct device *dev,
>
>   	lradc->chan0_map_count = of_get_child_count(np);
>   	if (lradc->chan0_map_count == 0) {
> -		dev_err(dev, "keymap is missing in device tree\n");
> -		return -EINVAL;
> +		dev_info(dev, "keymap is missing in device tree\n");
> +		return 0;
>   	}
>
>   	lradc->chan0_map = devm_kmalloc_array(dev, lradc->chan0_map_count,

I assume this is so that people can still use the sysfs node, to create a dts, right
not sure I like this, might be better to document to simple create a dts with
a single button mapping for 200 mV (most board use 200 mV steps between the buttons).

> @@ -185,19 +201,19 @@ static int sun4i_lradc_load_dt_keymap(struct device *dev,
>
>   		error = of_property_read_u32(pp, "channel", &channel);
>   		if (error || channel != 0) {
> -			dev_err(dev, "%s: Inval channel prop\n", pp->name);
> +			dev_err(dev, "%s: Invalid 'channel' property\n", pp->name);
>   			return -EINVAL;
>   		}
>
>   		error = of_property_read_u32(pp, "voltage", &map->voltage);
>   		if (error) {
> -			dev_err(dev, "%s: Inval voltage prop\n", pp->name);
> +			dev_err(dev, "%s: Invalid 'voltage' property\n", pp->name);
>   			return -EINVAL;
>   		}
>
>   		error = of_property_read_u32(pp, "linux,code", &map->keycode);
>   		if (error) {
> -			dev_err(dev, "%s: Inval linux,code prop\n", pp->name);
> +			dev_err(dev, "%s: Invalid 'linux,code' property\n", pp->name);
>   			return -EINVAL;
>   		}
>

This hunk / 3 changes belong in a separate patch. Also please run checkpatch, I think
you're running over 80 chars here.


> @@ -257,14 +273,26 @@ static int sun4i_lradc_probe(struct platform_device *pdev)
>   	if (error)
>   		return error;
>
> -	error = input_register_device(lradc->input);
> +	error = device_create_file(dev, &dev_attr_voltage);
>   	if (error)
>   		return error;
>
> +	error = input_register_device(lradc->input);
> +	if (error) {
> +		device_remove_file(&pdev->dev, &dev_attr_voltage);
> +		return error;
> +	}
> +
>   	platform_set_drvdata(pdev, lradc);
>   	return 0;
>   }
>
> +static int sun4i_lradc_remove(struct platform_device *pdev)
> +{
> +	device_remove_file(&pdev->dev, &dev_attr_voltage);
> +	return 0;
> +}
> +

This looks wrong, I think (*) that we've a bug here because we're not
unregistering the input device, so maybe do 2 patches, 1 fixing the
not unregistering bug, and then just add the device_remove_file()
in the sysfs patch.

>   static const struct of_device_id sun4i_lradc_of_match[] = {
>   	{ .compatible = "allwinner,sun4i-a10-lradc-keys", },
>   	{ /* sentinel */ }


> @@ -277,6 +305,7 @@ static struct platform_driver sun4i_lradc_driver = {
>   		.of_match_table = of_match_ptr(sun4i_lradc_of_match),
>   	},
>   	.probe	= sun4i_lradc_probe,
> +	.remove = sun4i_lradc_remove,
>   };
>
>   module_platform_driver(sun4i_lradc_driver);
>

Regards,

Hans


*) But I'm not sure, better double check.

Re: [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver

[Dmitry Torokhov]

On Mon, Jan 26, 2015 at 08:28:29PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 26-01-15 17:58, Priit Laes wrote:
> 
> No commit message? Please write an informative commit msg, like why we want this patch,
> I guess it is to help figuring out the voltage levels for various buttons when creating
> a dts, but I would prefer to not guess, which is where a good commit message would
> come in handy ...
> 
> >---
> >  .../ABI/testing/sysfs-driver-input-sun4i-lradc     |  4 ++
> >  drivers/input/keyboard/sun4i-lradc-keys.c          | 49 +++++++++++++++++-----
> >  2 files changed, 43 insertions(+), 10 deletions(-)
> >  create mode 100644 Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> >
> >diff --git a/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> >new file mode 100644
> >index 0000000..e4e6448
> >--- /dev/null
> >+++ b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> >@@ -0,0 +1,4 @@
> >+What:		/sys/class/input/input(x)/device/voltage
> >+Date:		February 2015
> >+Contact:	Priit Laes <plaes-q/aMd4JkU83YtjvyW6yDsg@public.gmane.org>
> >+Description:	ADC output voltage in microvolts or 0 if device is not opened.
> >diff --git a/drivers/input/keyboard/sun4i-lradc-keys.c b/drivers/input/keyboard/sun4i-lradc-keys.c
> >index cc8f7dd..c0ab8ec 100644
> >--- a/drivers/input/keyboard/sun4i-lradc-keys.c
> >+++ b/drivers/input/keyboard/sun4i-lradc-keys.c
> >@@ -79,10 +79,27 @@ struct sun4i_lradc_data {
> >  	u32 vref;
> >  };
> >
> >+static u32 sun4i_lradc_read_voltage(struct sun4i_lradc_data *lradc)
> >+{
> >+	u32 val = readl(lradc->base + LRADC_DATA0) & 0x3f;
> >+	return val * lradc->vref / 63;
> >+};
> >+
> >+static ssize_t
> >+sun4i_lradc_dev_voltage_show(struct device *dev,
> >+			struct device_attribute *attr, char *buf)
> >+{
> >+	struct sun4i_lradc_data *lradc = dev_get_drvdata(dev);
> >+
> >+	return sprintf(buf, "%u\n", sun4i_lradc_read_voltage(lradc));
> >+}
> >+
> >+static const DEVICE_ATTR(voltage, S_IRUGO, sun4i_lradc_dev_voltage_show, NULL);
> >+
> >  static irqreturn_t sun4i_lradc_irq(int irq, void *dev_id)
> >  {
> >  	struct sun4i_lradc_data *lradc = dev_id;
> >-	u32 i, ints, val, voltage, diff, keycode = 0, closest = 0xffffffff;
> >+	u32 i, ints, voltage, diff, keycode = 0, closest = 0xffffffff;
> >
> >  	ints  = readl(lradc->base + LRADC_INTS);
> >
> >@@ -97,8 +114,7 @@ static irqreturn_t sun4i_lradc_irq(int irq, void *dev_id)
> >  	}
> >
> >  	if ((ints & CHAN0_KEYDOWN_IRQ) && lradc->chan0_keycode == 0) {
> >-		val = readl(lradc->base + LRADC_DATA0) & 0x3f;
> >-		voltage = val * lradc->vref / 63;
> >+		voltage = sun4i_lradc_read_voltage(lradc);
> >
> >  		for (i = 0; i < lradc->chan0_map_count; i++) {
> >  			diff = abs(lradc->chan0_map[i].voltage - voltage);
> >@@ -156,7 +172,7 @@ static void sun4i_lradc_close(struct input_dev *dev)
> >  }
> >
> >  static int sun4i_lradc_load_dt_keymap(struct device *dev,
> >-				      struct sun4i_lradc_data *lradc)
> >+				    struct sun4i_lradc_data *lradc)
> >  {
> >  	struct device_node *np, *pp;
> >  	int i;
> 
> Why this identation change ?
> 
> >@@ -168,8 +184,8 @@ static int sun4i_lradc_load_dt_keymap(struct device *dev,
> >
> >  	lradc->chan0_map_count = of_get_child_count(np);
> >  	if (lradc->chan0_map_count == 0) {
> >-		dev_err(dev, "keymap is missing in device tree\n");
> >-		return -EINVAL;
> >+		dev_info(dev, "keymap is missing in device tree\n");
> >+		return 0;
> >  	}
> >
> >  	lradc->chan0_map = devm_kmalloc_array(dev, lradc->chan0_map_count,
> 
> I assume this is so that people can still use the sysfs node, to create a dts, right
> not sure I like this, might be better to document to simple create a dts with
> a single button mapping for 200 mV (most board use 200 mV steps between the buttons).
> 
> >@@ -185,19 +201,19 @@ static int sun4i_lradc_load_dt_keymap(struct device *dev,
> >
> >  		error = of_property_read_u32(pp, "channel", &channel);
> >  		if (error || channel != 0) {
> >-			dev_err(dev, "%s: Inval channel prop\n", pp->name);
> >+			dev_err(dev, "%s: Invalid 'channel' property\n", pp->name);
> >  			return -EINVAL;
> >  		}
> >
> >  		error = of_property_read_u32(pp, "voltage", &map->voltage);
> >  		if (error) {
> >-			dev_err(dev, "%s: Inval voltage prop\n", pp->name);
> >+			dev_err(dev, "%s: Invalid 'voltage' property\n", pp->name);
> >  			return -EINVAL;
> >  		}
> >
> >  		error = of_property_read_u32(pp, "linux,code", &map->keycode);
> >  		if (error) {
> >-			dev_err(dev, "%s: Inval linux,code prop\n", pp->name);
> >+			dev_err(dev, "%s: Invalid 'linux,code' property\n", pp->name);
> >  			return -EINVAL;
> >  		}
> >
> 
> This hunk / 3 changes belong in a separate patch. Also please run checkpatch, I think
> you're running over 80 chars here.
> 
> 
> >@@ -257,14 +273,26 @@ static int sun4i_lradc_probe(struct platform_device *pdev)
> >  	if (error)
> >  		return error;
> >
> >-	error = input_register_device(lradc->input);
> >+	error = device_create_file(dev, &dev_attr_voltage);
> >  	if (error)
> >  		return error;
> >
> >+	error = input_register_device(lradc->input);
> >+	if (error) {
> >+		device_remove_file(&pdev->dev, &dev_attr_voltage);
> >+		return error;
> >+	}
> >+
> >  	platform_set_drvdata(pdev, lradc);
> >  	return 0;
> >  }
> >
> >+static int sun4i_lradc_remove(struct platform_device *pdev)
> >+{
> >+	device_remove_file(&pdev->dev, &dev_attr_voltage);
> >+	return 0;
> >+}
> >+
> 
> This looks wrong, I think (*) that we've a bug here because we're not
> unregistering the input device, so maybe do 2 patches, 1 fixing the
> not unregistering bug, and then just add the device_remove_file()
> in the sysfs patch.

The unregister was not necessary since the input device is managed.

-- 
Dmitry

[PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver

[Dmitry Torokhov]

On Mon, Jan 26, 2015 at 08:28:29PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 26-01-15 17:58, Priit Laes wrote:
> 
> No commit message? Please write an informative commit msg, like why we want this patch,
> I guess it is to help figuring out the voltage levels for various buttons when creating
> a dts, but I would prefer to not guess, which is where a good commit message would
> come in handy ...
> 
> >---
> >  .../ABI/testing/sysfs-driver-input-sun4i-lradc     |  4 ++
> >  drivers/input/keyboard/sun4i-lradc-keys.c          | 49 +++++++++++++++++-----
> >  2 files changed, 43 insertions(+), 10 deletions(-)
> >  create mode 100644 Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> >
> >diff --git a/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> >new file mode 100644
> >index 0000000..e4e6448
> >--- /dev/null
> >+++ b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> >@@ -0,0 +1,4 @@
> >+What:		/sys/class/input/input(x)/device/voltage
> >+Date:		February 2015
> >+Contact:	Priit Laes <plaes@plaes.org>
> >+Description:	ADC output voltage in microvolts or 0 if device is not opened.
> >diff --git a/drivers/input/keyboard/sun4i-lradc-keys.c b/drivers/input/keyboard/sun4i-lradc-keys.c
> >index cc8f7dd..c0ab8ec 100644
> >--- a/drivers/input/keyboard/sun4i-lradc-keys.c
> >+++ b/drivers/input/keyboard/sun4i-lradc-keys.c
> >@@ -79,10 +79,27 @@ struct sun4i_lradc_data {
> >  	u32 vref;
> >  };
> >
> >+static u32 sun4i_lradc_read_voltage(struct sun4i_lradc_data *lradc)
> >+{
> >+	u32 val = readl(lradc->base + LRADC_DATA0) & 0x3f;
> >+	return val * lradc->vref / 63;
> >+};
> >+
> >+static ssize_t
> >+sun4i_lradc_dev_voltage_show(struct device *dev,
> >+			struct device_attribute *attr, char *buf)
> >+{
> >+	struct sun4i_lradc_data *lradc = dev_get_drvdata(dev);
> >+
> >+	return sprintf(buf, "%u\n", sun4i_lradc_read_voltage(lradc));
> >+}
> >+
> >+static const DEVICE_ATTR(voltage, S_IRUGO, sun4i_lradc_dev_voltage_show, NULL);
> >+
> >  static irqreturn_t sun4i_lradc_irq(int irq, void *dev_id)
> >  {
> >  	struct sun4i_lradc_data *lradc = dev_id;
> >-	u32 i, ints, val, voltage, diff, keycode = 0, closest = 0xffffffff;
> >+	u32 i, ints, voltage, diff, keycode = 0, closest = 0xffffffff;
> >
> >  	ints  = readl(lradc->base + LRADC_INTS);
> >
> >@@ -97,8 +114,7 @@ static irqreturn_t sun4i_lradc_irq(int irq, void *dev_id)
> >  	}
> >
> >  	if ((ints & CHAN0_KEYDOWN_IRQ) && lradc->chan0_keycode == 0) {
> >-		val = readl(lradc->base + LRADC_DATA0) & 0x3f;
> >-		voltage = val * lradc->vref / 63;
> >+		voltage = sun4i_lradc_read_voltage(lradc);
> >
> >  		for (i = 0; i < lradc->chan0_map_count; i++) {
> >  			diff = abs(lradc->chan0_map[i].voltage - voltage);
> >@@ -156,7 +172,7 @@ static void sun4i_lradc_close(struct input_dev *dev)
> >  }
> >
> >  static int sun4i_lradc_load_dt_keymap(struct device *dev,
> >-				      struct sun4i_lradc_data *lradc)
> >+				    struct sun4i_lradc_data *lradc)
> >  {
> >  	struct device_node *np, *pp;
> >  	int i;
> 
> Why this identation change ?
> 
> >@@ -168,8 +184,8 @@ static int sun4i_lradc_load_dt_keymap(struct device *dev,
> >
> >  	lradc->chan0_map_count = of_get_child_count(np);
> >  	if (lradc->chan0_map_count == 0) {
> >-		dev_err(dev, "keymap is missing in device tree\n");
> >-		return -EINVAL;
> >+		dev_info(dev, "keymap is missing in device tree\n");
> >+		return 0;
> >  	}
> >
> >  	lradc->chan0_map = devm_kmalloc_array(dev, lradc->chan0_map_count,
> 
> I assume this is so that people can still use the sysfs node, to create a dts, right
> not sure I like this, might be better to document to simple create a dts with
> a single button mapping for 200 mV (most board use 200 mV steps between the buttons).
> 
> >@@ -185,19 +201,19 @@ static int sun4i_lradc_load_dt_keymap(struct device *dev,
> >
> >  		error = of_property_read_u32(pp, "channel", &channel);
> >  		if (error || channel != 0) {
> >-			dev_err(dev, "%s: Inval channel prop\n", pp->name);
> >+			dev_err(dev, "%s: Invalid 'channel' property\n", pp->name);
> >  			return -EINVAL;
> >  		}
> >
> >  		error = of_property_read_u32(pp, "voltage", &map->voltage);
> >  		if (error) {
> >-			dev_err(dev, "%s: Inval voltage prop\n", pp->name);
> >+			dev_err(dev, "%s: Invalid 'voltage' property\n", pp->name);
> >  			return -EINVAL;
> >  		}
> >
> >  		error = of_property_read_u32(pp, "linux,code", &map->keycode);
> >  		if (error) {
> >-			dev_err(dev, "%s: Inval linux,code prop\n", pp->name);
> >+			dev_err(dev, "%s: Invalid 'linux,code' property\n", pp->name);
> >  			return -EINVAL;
> >  		}
> >
> 
> This hunk / 3 changes belong in a separate patch. Also please run checkpatch, I think
> you're running over 80 chars here.
> 
> 
> >@@ -257,14 +273,26 @@ static int sun4i_lradc_probe(struct platform_device *pdev)
> >  	if (error)
> >  		return error;
> >
> >-	error = input_register_device(lradc->input);
> >+	error = device_create_file(dev, &dev_attr_voltage);
> >  	if (error)
> >  		return error;
> >
> >+	error = input_register_device(lradc->input);
> >+	if (error) {
> >+		device_remove_file(&pdev->dev, &dev_attr_voltage);
> >+		return error;
> >+	}
> >+
> >  	platform_set_drvdata(pdev, lradc);
> >  	return 0;
> >  }
> >
> >+static int sun4i_lradc_remove(struct platform_device *pdev)
> >+{
> >+	device_remove_file(&pdev->dev, &dev_attr_voltage);
> >+	return 0;
> >+}
> >+
> 
> This looks wrong, I think (*) that we've a bug here because we're not
> unregistering the input device, so maybe do 2 patches, 1 fixing the
> not unregistering bug, and then just add the device_remove_file()
> in the sysfs patch.

The unregister was not necessary since the input device is managed.

-- 
Dmitry

Re: [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver

[Hans de Goede]

Hi,

On 26-01-15 23:06, Dmitry Torokhov wrote:
> On Mon, Jan 26, 2015 at 08:28:29PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 26-01-15 17:58, Priit Laes wrote:
>>
>> No commit message? Please write an informative commit msg, like why we want this patch,
>> I guess it is to help figuring out the voltage levels for various buttons when creating
>> a dts, but I would prefer to not guess, which is where a good commit message would
>> come in handy ...
>>
>>> ---
>>>   .../ABI/testing/sysfs-driver-input-sun4i-lradc     |  4 ++
>>>   drivers/input/keyboard/sun4i-lradc-keys.c          | 49 +++++++++++++++++-----
>>>   2 files changed, 43 insertions(+), 10 deletions(-)
>>>   create mode 100644 Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
>>> new file mode 100644
>>> index 0000000..e4e6448
>>> --- /dev/null
>>> +++ b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
>>> @@ -0,0 +1,4 @@
>>> +What:		/sys/class/input/input(x)/device/voltage
>>> +Date:		February 2015
>>> +Contact:	Priit Laes <plaes-q/aMd4JkU83YtjvyW6yDsg@public.gmane.org>
>>> +Description:	ADC output voltage in microvolts or 0 if device is not opened.
>>> diff --git a/drivers/input/keyboard/sun4i-lradc-keys.c b/drivers/input/keyboard/sun4i-lradc-keys.c
>>> index cc8f7dd..c0ab8ec 100644
>>> --- a/drivers/input/keyboard/sun4i-lradc-keys.c
>>> +++ b/drivers/input/keyboard/sun4i-lradc-keys.c
>>> @@ -79,10 +79,27 @@ struct sun4i_lradc_data {
>>>   	u32 vref;
>>>   };
>>>
>>> +static u32 sun4i_lradc_read_voltage(struct sun4i_lradc_data *lradc)
>>> +{
>>> +	u32 val = readl(lradc->base + LRADC_DATA0) & 0x3f;
>>> +	return val * lradc->vref / 63;
>>> +};
>>> +
>>> +static ssize_t
>>> +sun4i_lradc_dev_voltage_show(struct device *dev,
>>> +			struct device_attribute *attr, char *buf)
>>> +{
>>> +	struct sun4i_lradc_data *lradc = dev_get_drvdata(dev);
>>> +
>>> +	return sprintf(buf, "%u\n", sun4i_lradc_read_voltage(lradc));
>>> +}
>>> +
>>> +static const DEVICE_ATTR(voltage, S_IRUGO, sun4i_lradc_dev_voltage_show, NULL);
>>> +
>>>   static irqreturn_t sun4i_lradc_irq(int irq, void *dev_id)
>>>   {
>>>   	struct sun4i_lradc_data *lradc = dev_id;
>>> -	u32 i, ints, val, voltage, diff, keycode = 0, closest = 0xffffffff;
>>> +	u32 i, ints, voltage, diff, keycode = 0, closest = 0xffffffff;
>>>
>>>   	ints  = readl(lradc->base + LRADC_INTS);
>>>
>>> @@ -97,8 +114,7 @@ static irqreturn_t sun4i_lradc_irq(int irq, void *dev_id)
>>>   	}
>>>
>>>   	if ((ints & CHAN0_KEYDOWN_IRQ) && lradc->chan0_keycode == 0) {
>>> -		val = readl(lradc->base + LRADC_DATA0) & 0x3f;
>>> -		voltage = val * lradc->vref / 63;
>>> +		voltage = sun4i_lradc_read_voltage(lradc);
>>>
>>>   		for (i = 0; i < lradc->chan0_map_count; i++) {
>>>   			diff = abs(lradc->chan0_map[i].voltage - voltage);
>>> @@ -156,7 +172,7 @@ static void sun4i_lradc_close(struct input_dev *dev)
>>>   }
>>>
>>>   static int sun4i_lradc_load_dt_keymap(struct device *dev,
>>> -				      struct sun4i_lradc_data *lradc)
>>> +				    struct sun4i_lradc_data *lradc)
>>>   {
>>>   	struct device_node *np, *pp;
>>>   	int i;
>>
>> Why this identation change ?
>>
>>> @@ -168,8 +184,8 @@ static int sun4i_lradc_load_dt_keymap(struct device *dev,
>>>
>>>   	lradc->chan0_map_count = of_get_child_count(np);
>>>   	if (lradc->chan0_map_count == 0) {
>>> -		dev_err(dev, "keymap is missing in device tree\n");
>>> -		return -EINVAL;
>>> +		dev_info(dev, "keymap is missing in device tree\n");
>>> +		return 0;
>>>   	}
>>>
>>>   	lradc->chan0_map = devm_kmalloc_array(dev, lradc->chan0_map_count,
>>
>> I assume this is so that people can still use the sysfs node, to create a dts, right
>> not sure I like this, might be better to document to simple create a dts with
>> a single button mapping for 200 mV (most board use 200 mV steps between the buttons).
>>
>>> @@ -185,19 +201,19 @@ static int sun4i_lradc_load_dt_keymap(struct device *dev,
>>>
>>>   		error = of_property_read_u32(pp, "channel", &channel);
>>>   		if (error || channel != 0) {
>>> -			dev_err(dev, "%s: Inval channel prop\n", pp->name);
>>> +			dev_err(dev, "%s: Invalid 'channel' property\n", pp->name);
>>>   			return -EINVAL;
>>>   		}
>>>
>>>   		error = of_property_read_u32(pp, "voltage", &map->voltage);
>>>   		if (error) {
>>> -			dev_err(dev, "%s: Inval voltage prop\n", pp->name);
>>> +			dev_err(dev, "%s: Invalid 'voltage' property\n", pp->name);
>>>   			return -EINVAL;
>>>   		}
>>>
>>>   		error = of_property_read_u32(pp, "linux,code", &map->keycode);
>>>   		if (error) {
>>> -			dev_err(dev, "%s: Inval linux,code prop\n", pp->name);
>>> +			dev_err(dev, "%s: Invalid 'linux,code' property\n", pp->name);
>>>   			return -EINVAL;
>>>   		}
>>>
>>
>> This hunk / 3 changes belong in a separate patch. Also please run checkpatch, I think
>> you're running over 80 chars here.
>>
>>
>>> @@ -257,14 +273,26 @@ static int sun4i_lradc_probe(struct platform_device *pdev)
>>>   	if (error)
>>>   		return error;
>>>
>>> -	error = input_register_device(lradc->input);
>>> +	error = device_create_file(dev, &dev_attr_voltage);
>>>   	if (error)
>>>   		return error;
>>>
>>> +	error = input_register_device(lradc->input);
>>> +	if (error) {
>>> +		device_remove_file(&pdev->dev, &dev_attr_voltage);
>>> +		return error;
>>> +	}
>>> +
>>>   	platform_set_drvdata(pdev, lradc);
>>>   	return 0;
>>>   }
>>>
>>> +static int sun4i_lradc_remove(struct platform_device *pdev)
>>> +{
>>> +	device_remove_file(&pdev->dev, &dev_attr_voltage);
>>> +	return 0;
>>> +}
>>> +
>>
>> This looks wrong, I think (*) that we've a bug here because we're not
>> unregistering the input device, so maybe do 2 patches, 1 fixing the
>> not unregistering bug, and then just add the device_remove_file()
>> in the sysfs patch.
>
> The unregister was not necessary since the input device is managed.

Ah right, looking at the code again I see we use devm_input_allocate_device()
is there no devm_create_file for creating sysfs entries ?

Regards,

Hans

[PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver

[Hans de Goede]

Hi,

On 26-01-15 23:06, Dmitry Torokhov wrote:
> On Mon, Jan 26, 2015 at 08:28:29PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 26-01-15 17:58, Priit Laes wrote:
>>
>> No commit message? Please write an informative commit msg, like why we want this patch,
>> I guess it is to help figuring out the voltage levels for various buttons when creating
>> a dts, but I would prefer to not guess, which is where a good commit message would
>> come in handy ...
>>
>>> ---
>>>   .../ABI/testing/sysfs-driver-input-sun4i-lradc     |  4 ++
>>>   drivers/input/keyboard/sun4i-lradc-keys.c          | 49 +++++++++++++++++-----
>>>   2 files changed, 43 insertions(+), 10 deletions(-)
>>>   create mode 100644 Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
>>> new file mode 100644
>>> index 0000000..e4e6448
>>> --- /dev/null
>>> +++ b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
>>> @@ -0,0 +1,4 @@
>>> +What:		/sys/class/input/input(x)/device/voltage
>>> +Date:		February 2015
>>> +Contact:	Priit Laes <plaes@plaes.org>
>>> +Description:	ADC output voltage in microvolts or 0 if device is not opened.
>>> diff --git a/drivers/input/keyboard/sun4i-lradc-keys.c b/drivers/input/keyboard/sun4i-lradc-keys.c
>>> index cc8f7dd..c0ab8ec 100644
>>> --- a/drivers/input/keyboard/sun4i-lradc-keys.c
>>> +++ b/drivers/input/keyboard/sun4i-lradc-keys.c
>>> @@ -79,10 +79,27 @@ struct sun4i_lradc_data {
>>>   	u32 vref;
>>>   };
>>>
>>> +static u32 sun4i_lradc_read_voltage(struct sun4i_lradc_data *lradc)
>>> +{
>>> +	u32 val = readl(lradc->base + LRADC_DATA0) & 0x3f;
>>> +	return val * lradc->vref / 63;
>>> +};
>>> +
>>> +static ssize_t
>>> +sun4i_lradc_dev_voltage_show(struct device *dev,
>>> +			struct device_attribute *attr, char *buf)
>>> +{
>>> +	struct sun4i_lradc_data *lradc = dev_get_drvdata(dev);
>>> +
>>> +	return sprintf(buf, "%u\n", sun4i_lradc_read_voltage(lradc));
>>> +}
>>> +
>>> +static const DEVICE_ATTR(voltage, S_IRUGO, sun4i_lradc_dev_voltage_show, NULL);
>>> +
>>>   static irqreturn_t sun4i_lradc_irq(int irq, void *dev_id)
>>>   {
>>>   	struct sun4i_lradc_data *lradc = dev_id;
>>> -	u32 i, ints, val, voltage, diff, keycode = 0, closest = 0xffffffff;
>>> +	u32 i, ints, voltage, diff, keycode = 0, closest = 0xffffffff;
>>>
>>>   	ints  = readl(lradc->base + LRADC_INTS);
>>>
>>> @@ -97,8 +114,7 @@ static irqreturn_t sun4i_lradc_irq(int irq, void *dev_id)
>>>   	}
>>>
>>>   	if ((ints & CHAN0_KEYDOWN_IRQ) && lradc->chan0_keycode == 0) {
>>> -		val = readl(lradc->base + LRADC_DATA0) & 0x3f;
>>> -		voltage = val * lradc->vref / 63;
>>> +		voltage = sun4i_lradc_read_voltage(lradc);
>>>
>>>   		for (i = 0; i < lradc->chan0_map_count; i++) {
>>>   			diff = abs(lradc->chan0_map[i].voltage - voltage);
>>> @@ -156,7 +172,7 @@ static void sun4i_lradc_close(struct input_dev *dev)
>>>   }
>>>
>>>   static int sun4i_lradc_load_dt_keymap(struct device *dev,
>>> -				      struct sun4i_lradc_data *lradc)
>>> +				    struct sun4i_lradc_data *lradc)
>>>   {
>>>   	struct device_node *np, *pp;
>>>   	int i;
>>
>> Why this identation change ?
>>
>>> @@ -168,8 +184,8 @@ static int sun4i_lradc_load_dt_keymap(struct device *dev,
>>>
>>>   	lradc->chan0_map_count = of_get_child_count(np);
>>>   	if (lradc->chan0_map_count == 0) {
>>> -		dev_err(dev, "keymap is missing in device tree\n");
>>> -		return -EINVAL;
>>> +		dev_info(dev, "keymap is missing in device tree\n");
>>> +		return 0;
>>>   	}
>>>
>>>   	lradc->chan0_map = devm_kmalloc_array(dev, lradc->chan0_map_count,
>>
>> I assume this is so that people can still use the sysfs node, to create a dts, right
>> not sure I like this, might be better to document to simple create a dts with
>> a single button mapping for 200 mV (most board use 200 mV steps between the buttons).
>>
>>> @@ -185,19 +201,19 @@ static int sun4i_lradc_load_dt_keymap(struct device *dev,
>>>
>>>   		error = of_property_read_u32(pp, "channel", &channel);
>>>   		if (error || channel != 0) {
>>> -			dev_err(dev, "%s: Inval channel prop\n", pp->name);
>>> +			dev_err(dev, "%s: Invalid 'channel' property\n", pp->name);
>>>   			return -EINVAL;
>>>   		}
>>>
>>>   		error = of_property_read_u32(pp, "voltage", &map->voltage);
>>>   		if (error) {
>>> -			dev_err(dev, "%s: Inval voltage prop\n", pp->name);
>>> +			dev_err(dev, "%s: Invalid 'voltage' property\n", pp->name);
>>>   			return -EINVAL;
>>>   		}
>>>
>>>   		error = of_property_read_u32(pp, "linux,code", &map->keycode);
>>>   		if (error) {
>>> -			dev_err(dev, "%s: Inval linux,code prop\n", pp->name);
>>> +			dev_err(dev, "%s: Invalid 'linux,code' property\n", pp->name);
>>>   			return -EINVAL;
>>>   		}
>>>
>>
>> This hunk / 3 changes belong in a separate patch. Also please run checkpatch, I think
>> you're running over 80 chars here.
>>
>>
>>> @@ -257,14 +273,26 @@ static int sun4i_lradc_probe(struct platform_device *pdev)
>>>   	if (error)
>>>   		return error;
>>>
>>> -	error = input_register_device(lradc->input);
>>> +	error = device_create_file(dev, &dev_attr_voltage);
>>>   	if (error)
>>>   		return error;
>>>
>>> +	error = input_register_device(lradc->input);
>>> +	if (error) {
>>> +		device_remove_file(&pdev->dev, &dev_attr_voltage);
>>> +		return error;
>>> +	}
>>> +
>>>   	platform_set_drvdata(pdev, lradc);
>>>   	return 0;
>>>   }
>>>
>>> +static int sun4i_lradc_remove(struct platform_device *pdev)
>>> +{
>>> +	device_remove_file(&pdev->dev, &dev_attr_voltage);
>>> +	return 0;
>>> +}
>>> +
>>
>> This looks wrong, I think (*) that we've a bug here because we're not
>> unregistering the input device, so maybe do 2 patches, 1 fixing the
>> not unregistering bug, and then just add the device_remove_file()
>> in the sysfs patch.
>
> The unregister was not necessary since the input device is managed.

Ah right, looking at the code again I see we use devm_input_allocate_device()
is there no devm_create_file for creating sysfs entries ?

Regards,

Hans

Re: [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4697 bytes --]

Hi,

On Mon, Jan 26, 2015 at 06:58:32PM +0200, Priit Laes wrote:
> ---

Like Hans was pointing out, commit log and signed-off-by please

>  .../ABI/testing/sysfs-driver-input-sun4i-lradc     |  4 ++
>  drivers/input/keyboard/sun4i-lradc-keys.c          | 49 +++++++++++++++++-----
>  2 files changed, 43 insertions(+), 10 deletions(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> 
> diff --git a/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> new file mode 100644
> index 0000000..e4e6448
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> @@ -0,0 +1,4 @@
> +What:		/sys/class/input/input(x)/device/voltage
> +Date:		February 2015
> +Contact:	Priit Laes <plaes@plaes.org>
> +Description:	ADC output voltage in microvolts or 0 if device is not opened.

Why is it returning 0 when "device is not opened" ? What does that
even mean? You can't read that file without opening it.

> diff --git a/drivers/input/keyboard/sun4i-lradc-keys.c b/drivers/input/keyboard/sun4i-lradc-keys.c
> index cc8f7dd..c0ab8ec 100644
> --- a/drivers/input/keyboard/sun4i-lradc-keys.c
> +++ b/drivers/input/keyboard/sun4i-lradc-keys.c
> @@ -79,10 +79,27 @@ struct sun4i_lradc_data {
>  	u32 vref;
>  };
>  
> +static u32 sun4i_lradc_read_voltage(struct sun4i_lradc_data *lradc)
> +{
> +	u32 val = readl(lradc->base + LRADC_DATA0) & 0x3f;
> +	return val * lradc->vref / 63;
> +};
> +
> +static ssize_t
> +sun4i_lradc_dev_voltage_show(struct device *dev,
> +			struct device_attribute *attr, char *buf)
> +{
> +	struct sun4i_lradc_data *lradc = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%u\n", sun4i_lradc_read_voltage(lradc));
> +}
> +
> +static const DEVICE_ATTR(voltage, S_IRUGO, sun4i_lradc_dev_voltage_show, NULL);
> +
>  static irqreturn_t sun4i_lradc_irq(int irq, void *dev_id)
>  {
>  	struct sun4i_lradc_data *lradc = dev_id;
> -	u32 i, ints, val, voltage, diff, keycode = 0, closest = 0xffffffff;
> +	u32 i, ints, voltage, diff, keycode = 0, closest = 0xffffffff;
>  
>  	ints  = readl(lradc->base + LRADC_INTS);
>  
> @@ -97,8 +114,7 @@ static irqreturn_t sun4i_lradc_irq(int irq, void *dev_id)
>  	}
>  
>  	if ((ints & CHAN0_KEYDOWN_IRQ) && lradc->chan0_keycode == 0) {
> -		val = readl(lradc->base + LRADC_DATA0) & 0x3f;
> -		voltage = val * lradc->vref / 63;
> +		voltage = sun4i_lradc_read_voltage(lradc);
>  
>  		for (i = 0; i < lradc->chan0_map_count; i++) {
>  			diff = abs(lradc->chan0_map[i].voltage - voltage);
> @@ -156,7 +172,7 @@ static void sun4i_lradc_close(struct input_dev *dev)
>  }
>  
>  static int sun4i_lradc_load_dt_keymap(struct device *dev,
> -				      struct sun4i_lradc_data *lradc)
> +				    struct sun4i_lradc_data *lradc)
>  {
>  	struct device_node *np, *pp;
>  	int i;
> @@ -168,8 +184,8 @@ static int sun4i_lradc_load_dt_keymap(struct device *dev,
>  
>  	lradc->chan0_map_count = of_get_child_count(np);
>  	if (lradc->chan0_map_count == 0) {
> -		dev_err(dev, "keymap is missing in device tree\n");
> -		return -EINVAL;
> +		dev_info(dev, "keymap is missing in device tree\n");
> +		return 0;
>  	}
>  
>  	lradc->chan0_map = devm_kmalloc_array(dev, lradc->chan0_map_count,
> @@ -185,19 +201,19 @@ static int sun4i_lradc_load_dt_keymap(struct device *dev,
>  
>  		error = of_property_read_u32(pp, "channel", &channel);
>  		if (error || channel != 0) {
> -			dev_err(dev, "%s: Inval channel prop\n", pp->name);
> +			dev_err(dev, "%s: Invalid 'channel' property\n", pp->name);
>  			return -EINVAL;
>  		}
>  
>  		error = of_property_read_u32(pp, "voltage", &map->voltage);
>  		if (error) {
> -			dev_err(dev, "%s: Inval voltage prop\n", pp->name);
> +			dev_err(dev, "%s: Invalid 'voltage' property\n", pp->name);
>  			return -EINVAL;
>  		}
>  
>  		error = of_property_read_u32(pp, "linux,code", &map->keycode);
>  		if (error) {
> -			dev_err(dev, "%s: Inval linux,code prop\n", pp->name);
> +			dev_err(dev, "%s: Invalid 'linux,code' property\n", pp->name);
>  			return -EINVAL;
>  		}
>  
> @@ -257,14 +273,26 @@ static int sun4i_lradc_probe(struct platform_device *pdev)
>  	if (error)
>  		return error;
>  
> -	error = input_register_device(lradc->input);
> +	error = device_create_file(dev, &dev_attr_voltage);

As I told you already, if you're going to expose this an ADC in the
end, the proper solution is to use the IIO framework, not adding a
custom sysfs file.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4727 bytes --]

Hi,

On Mon, Jan 26, 2015 at 06:58:32PM +0200, Priit Laes wrote:
> ---

Like Hans was pointing out, commit log and signed-off-by please

>  .../ABI/testing/sysfs-driver-input-sun4i-lradc     |  4 ++
>  drivers/input/keyboard/sun4i-lradc-keys.c          | 49 +++++++++++++++++-----
>  2 files changed, 43 insertions(+), 10 deletions(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> 
> diff --git a/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> new file mode 100644
> index 0000000..e4e6448
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> @@ -0,0 +1,4 @@
> +What:		/sys/class/input/input(x)/device/voltage
> +Date:		February 2015
> +Contact:	Priit Laes <plaes-q/aMd4JkU83YtjvyW6yDsg@public.gmane.org>
> +Description:	ADC output voltage in microvolts or 0 if device is not opened.

Why is it returning 0 when "device is not opened" ? What does that
even mean? You can't read that file without opening it.

> diff --git a/drivers/input/keyboard/sun4i-lradc-keys.c b/drivers/input/keyboard/sun4i-lradc-keys.c
> index cc8f7dd..c0ab8ec 100644
> --- a/drivers/input/keyboard/sun4i-lradc-keys.c
> +++ b/drivers/input/keyboard/sun4i-lradc-keys.c
> @@ -79,10 +79,27 @@ struct sun4i_lradc_data {
>  	u32 vref;
>  };
>  
> +static u32 sun4i_lradc_read_voltage(struct sun4i_lradc_data *lradc)
> +{
> +	u32 val = readl(lradc->base + LRADC_DATA0) & 0x3f;
> +	return val * lradc->vref / 63;
> +};
> +
> +static ssize_t
> +sun4i_lradc_dev_voltage_show(struct device *dev,
> +			struct device_attribute *attr, char *buf)
> +{
> +	struct sun4i_lradc_data *lradc = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%u\n", sun4i_lradc_read_voltage(lradc));
> +}
> +
> +static const DEVICE_ATTR(voltage, S_IRUGO, sun4i_lradc_dev_voltage_show, NULL);
> +
>  static irqreturn_t sun4i_lradc_irq(int irq, void *dev_id)
>  {
>  	struct sun4i_lradc_data *lradc = dev_id;
> -	u32 i, ints, val, voltage, diff, keycode = 0, closest = 0xffffffff;
> +	u32 i, ints, voltage, diff, keycode = 0, closest = 0xffffffff;
>  
>  	ints  = readl(lradc->base + LRADC_INTS);
>  
> @@ -97,8 +114,7 @@ static irqreturn_t sun4i_lradc_irq(int irq, void *dev_id)
>  	}
>  
>  	if ((ints & CHAN0_KEYDOWN_IRQ) && lradc->chan0_keycode == 0) {
> -		val = readl(lradc->base + LRADC_DATA0) & 0x3f;
> -		voltage = val * lradc->vref / 63;
> +		voltage = sun4i_lradc_read_voltage(lradc);
>  
>  		for (i = 0; i < lradc->chan0_map_count; i++) {
>  			diff = abs(lradc->chan0_map[i].voltage - voltage);
> @@ -156,7 +172,7 @@ static void sun4i_lradc_close(struct input_dev *dev)
>  }
>  
>  static int sun4i_lradc_load_dt_keymap(struct device *dev,
> -				      struct sun4i_lradc_data *lradc)
> +				    struct sun4i_lradc_data *lradc)
>  {
>  	struct device_node *np, *pp;
>  	int i;
> @@ -168,8 +184,8 @@ static int sun4i_lradc_load_dt_keymap(struct device *dev,
>  
>  	lradc->chan0_map_count = of_get_child_count(np);
>  	if (lradc->chan0_map_count == 0) {
> -		dev_err(dev, "keymap is missing in device tree\n");
> -		return -EINVAL;
> +		dev_info(dev, "keymap is missing in device tree\n");
> +		return 0;
>  	}
>  
>  	lradc->chan0_map = devm_kmalloc_array(dev, lradc->chan0_map_count,
> @@ -185,19 +201,19 @@ static int sun4i_lradc_load_dt_keymap(struct device *dev,
>  
>  		error = of_property_read_u32(pp, "channel", &channel);
>  		if (error || channel != 0) {
> -			dev_err(dev, "%s: Inval channel prop\n", pp->name);
> +			dev_err(dev, "%s: Invalid 'channel' property\n", pp->name);
>  			return -EINVAL;
>  		}
>  
>  		error = of_property_read_u32(pp, "voltage", &map->voltage);
>  		if (error) {
> -			dev_err(dev, "%s: Inval voltage prop\n", pp->name);
> +			dev_err(dev, "%s: Invalid 'voltage' property\n", pp->name);
>  			return -EINVAL;
>  		}
>  
>  		error = of_property_read_u32(pp, "linux,code", &map->keycode);
>  		if (error) {
> -			dev_err(dev, "%s: Inval linux,code prop\n", pp->name);
> +			dev_err(dev, "%s: Invalid 'linux,code' property\n", pp->name);
>  			return -EINVAL;
>  		}
>  
> @@ -257,14 +273,26 @@ static int sun4i_lradc_probe(struct platform_device *pdev)
>  	if (error)
>  		return error;
>  
> -	error = input_register_device(lradc->input);
> +	error = device_create_file(dev, &dev_attr_voltage);

As I told you already, if you're going to expose this an ADC in the
end, the proper solution is to use the IIO framework, not adding a
custom sysfs file.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver

[Maxime Ripard]

Hi,

On Mon, Jan 26, 2015 at 06:58:32PM +0200, Priit Laes wrote:
> ---

Like Hans was pointing out, commit log and signed-off-by please

>  .../ABI/testing/sysfs-driver-input-sun4i-lradc     |  4 ++
>  drivers/input/keyboard/sun4i-lradc-keys.c          | 49 +++++++++++++++++-----
>  2 files changed, 43 insertions(+), 10 deletions(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> 
> diff --git a/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> new file mode 100644
> index 0000000..e4e6448
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> @@ -0,0 +1,4 @@
> +What:		/sys/class/input/input(x)/device/voltage
> +Date:		February 2015
> +Contact:	Priit Laes <plaes@plaes.org>
> +Description:	ADC output voltage in microvolts or 0 if device is not opened.

Why is it returning 0 when "device is not opened" ? What does that
even mean? You can't read that file without opening it.

> diff --git a/drivers/input/keyboard/sun4i-lradc-keys.c b/drivers/input/keyboard/sun4i-lradc-keys.c
> index cc8f7dd..c0ab8ec 100644
> --- a/drivers/input/keyboard/sun4i-lradc-keys.c
> +++ b/drivers/input/keyboard/sun4i-lradc-keys.c
> @@ -79,10 +79,27 @@ struct sun4i_lradc_data {
>  	u32 vref;
>  };
>  
> +static u32 sun4i_lradc_read_voltage(struct sun4i_lradc_data *lradc)
> +{
> +	u32 val = readl(lradc->base + LRADC_DATA0) & 0x3f;
> +	return val * lradc->vref / 63;
> +};
> +
> +static ssize_t
> +sun4i_lradc_dev_voltage_show(struct device *dev,
> +			struct device_attribute *attr, char *buf)
> +{
> +	struct sun4i_lradc_data *lradc = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%u\n", sun4i_lradc_read_voltage(lradc));
> +}
> +
> +static const DEVICE_ATTR(voltage, S_IRUGO, sun4i_lradc_dev_voltage_show, NULL);
> +
>  static irqreturn_t sun4i_lradc_irq(int irq, void *dev_id)
>  {
>  	struct sun4i_lradc_data *lradc = dev_id;
> -	u32 i, ints, val, voltage, diff, keycode = 0, closest = 0xffffffff;
> +	u32 i, ints, voltage, diff, keycode = 0, closest = 0xffffffff;
>  
>  	ints  = readl(lradc->base + LRADC_INTS);
>  
> @@ -97,8 +114,7 @@ static irqreturn_t sun4i_lradc_irq(int irq, void *dev_id)
>  	}
>  
>  	if ((ints & CHAN0_KEYDOWN_IRQ) && lradc->chan0_keycode == 0) {
> -		val = readl(lradc->base + LRADC_DATA0) & 0x3f;
> -		voltage = val * lradc->vref / 63;
> +		voltage = sun4i_lradc_read_voltage(lradc);
>  
>  		for (i = 0; i < lradc->chan0_map_count; i++) {
>  			diff = abs(lradc->chan0_map[i].voltage - voltage);
> @@ -156,7 +172,7 @@ static void sun4i_lradc_close(struct input_dev *dev)
>  }
>  
>  static int sun4i_lradc_load_dt_keymap(struct device *dev,
> -				      struct sun4i_lradc_data *lradc)
> +				    struct sun4i_lradc_data *lradc)
>  {
>  	struct device_node *np, *pp;
>  	int i;
> @@ -168,8 +184,8 @@ static int sun4i_lradc_load_dt_keymap(struct device *dev,
>  
>  	lradc->chan0_map_count = of_get_child_count(np);
>  	if (lradc->chan0_map_count == 0) {
> -		dev_err(dev, "keymap is missing in device tree\n");
> -		return -EINVAL;
> +		dev_info(dev, "keymap is missing in device tree\n");
> +		return 0;
>  	}
>  
>  	lradc->chan0_map = devm_kmalloc_array(dev, lradc->chan0_map_count,
> @@ -185,19 +201,19 @@ static int sun4i_lradc_load_dt_keymap(struct device *dev,
>  
>  		error = of_property_read_u32(pp, "channel", &channel);
>  		if (error || channel != 0) {
> -			dev_err(dev, "%s: Inval channel prop\n", pp->name);
> +			dev_err(dev, "%s: Invalid 'channel' property\n", pp->name);
>  			return -EINVAL;
>  		}
>  
>  		error = of_property_read_u32(pp, "voltage", &map->voltage);
>  		if (error) {
> -			dev_err(dev, "%s: Inval voltage prop\n", pp->name);
> +			dev_err(dev, "%s: Invalid 'voltage' property\n", pp->name);
>  			return -EINVAL;
>  		}
>  
>  		error = of_property_read_u32(pp, "linux,code", &map->keycode);
>  		if (error) {
> -			dev_err(dev, "%s: Inval linux,code prop\n", pp->name);
> +			dev_err(dev, "%s: Invalid 'linux,code' property\n", pp->name);
>  			return -EINVAL;
>  		}
>  
> @@ -257,14 +273,26 @@ static int sun4i_lradc_probe(struct platform_device *pdev)
>  	if (error)
>  		return error;
>  
> -	error = input_register_device(lradc->input);
> +	error = device_create_file(dev, &dev_attr_voltage);

As I told you already, if you're going to expose this an ADC in the
end, the proper solution is to use the IIO framework, not adding a
custom sysfs file.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150127/b90b0164/attachment.sig>

Re: [linux-sunxi] Re: [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver

[Priit Laes]

On Tue, 2015-01-27 at 10:18 +0100, Maxime Ripard wrote:
> Hi,
> 
> On Mon, Jan 26, 2015 at 06:58:32PM +0200, Priit Laes wrote:
> > ---
> 
> Like Hans was pointing out, commit log and signed-off-by please
> 
> >  .../ABI/testing/sysfs-driver-input-sun4i-lradc     |  4 ++
> >  drivers/input/keyboard/sun4i-lradc-keys.c          | 49 
> > +++++++++++++++++-----
> >  2 files changed, 43 insertions(+), 10 deletions(-)
> >  create mode 100644 Documentation/ABI/testing/sysfs-driver-input-
> > sun4i-lradc
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-driver-input-sun4i-
> > lradc b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> > new file mode 100644
> > index 0000000..e4e6448
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> > @@ -0,0 +1,4 @@
> > +What:          /sys/class/input/input(x)/device/voltage
> > +Date:          February 2015
> > +Contact:       Priit Laes <plaes@plaes.org>
> > +Description:   ADC output voltage in microvolts or 0 if device is 
> > not opened.
> 
> Why is it returning 0 when "device is not opened" ? What does that 
> even mean? You can't read that file without opening it.

It means that something has to open the /dev/input/inputX device which 
sets up the ADC before the voltage can be read from the sysfs file.

[...]


> 
> As I told you already, if you're going to expose this an ADC in the 
> end, the proper solution is to use the IIO framework, not adding a 
> custom sysfs file.

My intention was to expose just a simple debug output, so one can 
press the buttons and read the voltages for devicetree keymap.

If anyone can suggest a simpler approach than current sysfs based one, 
I would do it. But full blown iio driver is currently out of scope. 
Also, Carlo's (ccaione) initially submitted (?) driver for lradc 
utilized iio subsystem.

Päikest,
Priit :)

Re: Re: [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver

[Priit Laes]

On Tue, 2015-01-27 at 10:18 +0100, Maxime Ripard wrote:
> Hi,
> 
> On Mon, Jan 26, 2015 at 06:58:32PM +0200, Priit Laes wrote:
> > ---
> 
> Like Hans was pointing out, commit log and signed-off-by please
> 
> >  .../ABI/testing/sysfs-driver-input-sun4i-lradc     |  4 ++
> >  drivers/input/keyboard/sun4i-lradc-keys.c          | 49 
> > +++++++++++++++++-----
> >  2 files changed, 43 insertions(+), 10 deletions(-)
> >  create mode 100644 Documentation/ABI/testing/sysfs-driver-input-
> > sun4i-lradc
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-driver-input-sun4i-
> > lradc b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> > new file mode 100644
> > index 0000000..e4e6448
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> > @@ -0,0 +1,4 @@
> > +What:          /sys/class/input/input(x)/device/voltage
> > +Date:          February 2015
> > +Contact:       Priit Laes <plaes-q/aMd4JkU83YtjvyW6yDsg@public.gmane.org>
> > +Description:   ADC output voltage in microvolts or 0 if device is 
> > not opened.
> 
> Why is it returning 0 when "device is not opened" ? What does that 
> even mean? You can't read that file without opening it.

It means that something has to open the /dev/input/inputX device which 
sets up the ADC before the voltage can be read from the sysfs file.

[...]


> 
> As I told you already, if you're going to expose this an ADC in the 
> end, the proper solution is to use the IIO framework, not adding a 
> custom sysfs file.

My intention was to expose just a simple debug output, so one can 
press the buttons and read the voltages for devicetree keymap.

If anyone can suggest a simpler approach than current sysfs based one, 
I would do it. But full blown iio driver is currently out of scope. 
Also, Carlo's (ccaione) initially submitted (?) driver for lradc 
utilized iio subsystem.

Päikest,
Priit :)

-- 
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

[linux-sunxi] Re: [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver

[Priit Laes]

On Tue, 2015-01-27 at 10:18 +0100, Maxime Ripard wrote:
> Hi,
> 
> On Mon, Jan 26, 2015 at 06:58:32PM +0200, Priit Laes wrote:
> > ---
> 
> Like Hans was pointing out, commit log and signed-off-by please
> 
> >  .../ABI/testing/sysfs-driver-input-sun4i-lradc     |  4 ++
> >  drivers/input/keyboard/sun4i-lradc-keys.c          | 49 
> > +++++++++++++++++-----
> >  2 files changed, 43 insertions(+), 10 deletions(-)
> >  create mode 100644 Documentation/ABI/testing/sysfs-driver-input-
> > sun4i-lradc
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-driver-input-sun4i-
> > lradc b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> > new file mode 100644
> > index 0000000..e4e6448
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> > @@ -0,0 +1,4 @@
> > +What:          /sys/class/input/input(x)/device/voltage
> > +Date:          February 2015
> > +Contact:       Priit Laes <plaes@plaes.org>
> > +Description:   ADC output voltage in microvolts or 0 if device is 
> > not opened.
> 
> Why is it returning 0 when "device is not opened" ? What does that 
> even mean? You can't read that file without opening it.

It means that something has to open the /dev/input/inputX device which 
sets up the ADC before the voltage can be read from the sysfs file.

[...]


> 
> As I told you already, if you're going to expose this an ADC in the 
> end, the proper solution is to use the IIO framework, not adding a 
> custom sysfs file.

My intention was to expose just a simple debug output, so one can 
press the buttons and read the voltages for devicetree keymap.

If anyone can suggest a simpler approach than current sysfs based one, 
I would do it. But full blown iio driver is currently out of scope. 
Also, Carlo's (ccaione) initially submitted (?) driver for lradc 
utilized iio subsystem.

P?ikest,
Priit :)

Re: [linux-sunxi] Re: [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver

[Hans de Goede]

Hi,

On 27-01-15 10:49, Priit Laes wrote:
>
> On Tue, 2015-01-27 at 10:18 +0100, Maxime Ripard wrote:
>> Hi,
>>
>> On Mon, Jan 26, 2015 at 06:58:32PM +0200, Priit Laes wrote:
>>> ---
>>
>> Like Hans was pointing out, commit log and signed-off-by please
>>
>>>   .../ABI/testing/sysfs-driver-input-sun4i-lradc     |  4 ++
>>>   drivers/input/keyboard/sun4i-lradc-keys.c          | 49
>>> +++++++++++++++++-----
>>>   2 files changed, 43 insertions(+), 10 deletions(-)
>>>   create mode 100644 Documentation/ABI/testing/sysfs-driver-input-
>>> sun4i-lradc
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-driver-input-sun4i-
>>> lradc b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
>>> new file mode 100644
>>> index 0000000..e4e6448
>>> --- /dev/null
>>> +++ b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
>>> @@ -0,0 +1,4 @@
>>> +What:          /sys/class/input/input(x)/device/voltage
>>> +Date:          February 2015
>>> +Contact:       Priit Laes <plaes@plaes.org>
>>> +Description:   ADC output voltage in microvolts or 0 if device is
>>> not opened.
>>
>> Why is it returning 0 when "device is not opened" ? What does that
>> even mean? You can't read that file without opening it.
>
> It means that something has to open the /dev/input/inputX device which
> sets up the ADC before the voltage can be read from the sysfs file.
>
> [...]
>
>
>>
>> As I told you already, if you're going to expose this an ADC in the
>> end, the proper solution is to use the IIO framework, not adding a
>> custom sysfs file.
>
> My intention was to expose just a simple debug output, so one can
> press the buttons and read the voltages for devicetree keymap.
>
> If anyone can suggest a simpler approach than current sysfs based one,
> I would do it.

The android driver always uses 0.2V / 200mV steps, so what I do is
simply create a mapping with 200mV mapped to KEY_VOLUMEUP, 400mV mapped
to KEY_VOLUMEDOWN, etc. following the hardcoded android driver mapping:

https://github.com/linux-sunxi/linux-sunxi/blob/sunxi-3.4/drivers/input/keyboard/sun4i-keyboard.c#L136

Usually this will be correct in one go, after testing one can shuffle
key codes as needed (usually not needed) and/or remove unused entries.

With that said I do think that a sysfs file to see the actual voltages,
or a kernel parameter to printk them on keypress interrupt would be useful.

I guess the printk option would be better as it would show the actual
keypress value read, not some semi-random sample.

Regards,

Hans

Re: Re: [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver

[Hans de Goede]

Hi,

On 27-01-15 10:49, Priit Laes wrote:
>
> On Tue, 2015-01-27 at 10:18 +0100, Maxime Ripard wrote:
>> Hi,
>>
>> On Mon, Jan 26, 2015 at 06:58:32PM +0200, Priit Laes wrote:
>>> ---
>>
>> Like Hans was pointing out, commit log and signed-off-by please
>>
>>>   .../ABI/testing/sysfs-driver-input-sun4i-lradc     |  4 ++
>>>   drivers/input/keyboard/sun4i-lradc-keys.c          | 49
>>> +++++++++++++++++-----
>>>   2 files changed, 43 insertions(+), 10 deletions(-)
>>>   create mode 100644 Documentation/ABI/testing/sysfs-driver-input-
>>> sun4i-lradc
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-driver-input-sun4i-
>>> lradc b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
>>> new file mode 100644
>>> index 0000000..e4e6448
>>> --- /dev/null
>>> +++ b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
>>> @@ -0,0 +1,4 @@
>>> +What:          /sys/class/input/input(x)/device/voltage
>>> +Date:          February 2015
>>> +Contact:       Priit Laes <plaes-q/aMd4JkU83YtjvyW6yDsg@public.gmane.org>
>>> +Description:   ADC output voltage in microvolts or 0 if device is
>>> not opened.
>>
>> Why is it returning 0 when "device is not opened" ? What does that
>> even mean? You can't read that file without opening it.
>
> It means that something has to open the /dev/input/inputX device which
> sets up the ADC before the voltage can be read from the sysfs file.
>
> [...]
>
>
>>
>> As I told you already, if you're going to expose this an ADC in the
>> end, the proper solution is to use the IIO framework, not adding a
>> custom sysfs file.
>
> My intention was to expose just a simple debug output, so one can
> press the buttons and read the voltages for devicetree keymap.
>
> If anyone can suggest a simpler approach than current sysfs based one,
> I would do it.

The android driver always uses 0.2V / 200mV steps, so what I do is
simply create a mapping with 200mV mapped to KEY_VOLUMEUP, 400mV mapped
to KEY_VOLUMEDOWN, etc. following the hardcoded android driver mapping:

https://github.com/linux-sunxi/linux-sunxi/blob/sunxi-3.4/drivers/input/keyboard/sun4i-keyboard.c#L136

Usually this will be correct in one go, after testing one can shuffle
key codes as needed (usually not needed) and/or remove unused entries.

With that said I do think that a sysfs file to see the actual voltages,
or a kernel parameter to printk them on keypress interrupt would be useful.

I guess the printk option would be better as it would show the actual
keypress value read, not some semi-random sample.

Regards,

Hans

[linux-sunxi] Re: [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver

[Hans de Goede]

Hi,

On 27-01-15 10:49, Priit Laes wrote:
>
> On Tue, 2015-01-27 at 10:18 +0100, Maxime Ripard wrote:
>> Hi,
>>
>> On Mon, Jan 26, 2015 at 06:58:32PM +0200, Priit Laes wrote:
>>> ---
>>
>> Like Hans was pointing out, commit log and signed-off-by please
>>
>>>   .../ABI/testing/sysfs-driver-input-sun4i-lradc     |  4 ++
>>>   drivers/input/keyboard/sun4i-lradc-keys.c          | 49
>>> +++++++++++++++++-----
>>>   2 files changed, 43 insertions(+), 10 deletions(-)
>>>   create mode 100644 Documentation/ABI/testing/sysfs-driver-input-
>>> sun4i-lradc
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-driver-input-sun4i-
>>> lradc b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
>>> new file mode 100644
>>> index 0000000..e4e6448
>>> --- /dev/null
>>> +++ b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
>>> @@ -0,0 +1,4 @@
>>> +What:          /sys/class/input/input(x)/device/voltage
>>> +Date:          February 2015
>>> +Contact:       Priit Laes <plaes@plaes.org>
>>> +Description:   ADC output voltage in microvolts or 0 if device is
>>> not opened.
>>
>> Why is it returning 0 when "device is not opened" ? What does that
>> even mean? You can't read that file without opening it.
>
> It means that something has to open the /dev/input/inputX device which
> sets up the ADC before the voltage can be read from the sysfs file.
>
> [...]
>
>
>>
>> As I told you already, if you're going to expose this an ADC in the
>> end, the proper solution is to use the IIO framework, not adding a
>> custom sysfs file.
>
> My intention was to expose just a simple debug output, so one can
> press the buttons and read the voltages for devicetree keymap.
>
> If anyone can suggest a simpler approach than current sysfs based one,
> I would do it.

The android driver always uses 0.2V / 200mV steps, so what I do is
simply create a mapping with 200mV mapped to KEY_VOLUMEUP, 400mV mapped
to KEY_VOLUMEDOWN, etc. following the hardcoded android driver mapping:

https://github.com/linux-sunxi/linux-sunxi/blob/sunxi-3.4/drivers/input/keyboard/sun4i-keyboard.c#L136

Usually this will be correct in one go, after testing one can shuffle
key codes as needed (usually not needed) and/or remove unused entries.

With that said I do think that a sysfs file to see the actual voltages,
or a kernel parameter to printk them on keypress interrupt would be useful.

I guess the printk option would be better as it would show the actual
keypress value read, not some semi-random sample.

Regards,

Hans

Re: [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver

[Dmitry Torokhov]

On Tue, Jan 27, 2015 at 10:03:35AM +0100, Hans de Goede wrote:
> Hi,
> 
> On 26-01-15 23:06, Dmitry Torokhov wrote:
> >On Mon, Jan 26, 2015 at 08:28:29PM +0100, Hans de Goede wrote:
> >>Hi,
> >>
> >>On 26-01-15 17:58, Priit Laes wrote:
> >>
> >>No commit message? Please write an informative commit msg, like why we want this patch,
> >>I guess it is to help figuring out the voltage levels for various buttons when creating
> >>a dts, but I would prefer to not guess, which is where a good commit message would
> >>come in handy ...
> >>
> >>>---
> >>>  .../ABI/testing/sysfs-driver-input-sun4i-lradc     |  4 ++
> >>>  drivers/input/keyboard/sun4i-lradc-keys.c          | 49 +++++++++++++++++-----
> >>>  2 files changed, 43 insertions(+), 10 deletions(-)
> >>>  create mode 100644 Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> >>>
> >>>diff --git a/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> >>>new file mode 100644
> >>>index 0000000..e4e6448
> >>>--- /dev/null
> >>>+++ b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> >>>@@ -0,0 +1,4 @@
> >>>+What:		/sys/class/input/input(x)/device/voltage
> >>>+Date:		February 2015
> >>>+Contact:	Priit Laes <plaes-q/aMd4JkU83YtjvyW6yDsg@public.gmane.org>
> >>>+Description:	ADC output voltage in microvolts or 0 if device is not opened.
> >>>diff --git a/drivers/input/keyboard/sun4i-lradc-keys.c b/drivers/input/keyboard/sun4i-lradc-keys.c
> >>>index cc8f7dd..c0ab8ec 100644
> >>>--- a/drivers/input/keyboard/sun4i-lradc-keys.c
> >>>+++ b/drivers/input/keyboard/sun4i-lradc-keys.c
> >>>@@ -79,10 +79,27 @@ struct sun4i_lradc_data {
> >>>  	u32 vref;
> >>>  };
> >>>
> >>>+static u32 sun4i_lradc_read_voltage(struct sun4i_lradc_data *lradc)
> >>>+{
> >>>+	u32 val = readl(lradc->base + LRADC_DATA0) & 0x3f;
> >>>+	return val * lradc->vref / 63;
> >>>+};
> >>>+
> >>>+static ssize_t
> >>>+sun4i_lradc_dev_voltage_show(struct device *dev,
> >>>+			struct device_attribute *attr, char *buf)
> >>>+{
> >>>+	struct sun4i_lradc_data *lradc = dev_get_drvdata(dev);
> >>>+
> >>>+	return sprintf(buf, "%u\n", sun4i_lradc_read_voltage(lradc));
> >>>+}
> >>>+
> >>>+static const DEVICE_ATTR(voltage, S_IRUGO, sun4i_lradc_dev_voltage_show, NULL);
> >>>+
> >>>  static irqreturn_t sun4i_lradc_irq(int irq, void *dev_id)
> >>>  {
> >>>  	struct sun4i_lradc_data *lradc = dev_id;
> >>>-	u32 i, ints, val, voltage, diff, keycode = 0, closest = 0xffffffff;
> >>>+	u32 i, ints, voltage, diff, keycode = 0, closest = 0xffffffff;
> >>>
> >>>  	ints  = readl(lradc->base + LRADC_INTS);
> >>>
> >>>@@ -97,8 +114,7 @@ static irqreturn_t sun4i_lradc_irq(int irq, void *dev_id)
> >>>  	}
> >>>
> >>>  	if ((ints & CHAN0_KEYDOWN_IRQ) && lradc->chan0_keycode == 0) {
> >>>-		val = readl(lradc->base + LRADC_DATA0) & 0x3f;
> >>>-		voltage = val * lradc->vref / 63;
> >>>+		voltage = sun4i_lradc_read_voltage(lradc);
> >>>
> >>>  		for (i = 0; i < lradc->chan0_map_count; i++) {
> >>>  			diff = abs(lradc->chan0_map[i].voltage - voltage);
> >>>@@ -156,7 +172,7 @@ static void sun4i_lradc_close(struct input_dev *dev)
> >>>  }
> >>>
> >>>  static int sun4i_lradc_load_dt_keymap(struct device *dev,
> >>>-				      struct sun4i_lradc_data *lradc)
> >>>+				    struct sun4i_lradc_data *lradc)
> >>>  {
> >>>  	struct device_node *np, *pp;
> >>>  	int i;
> >>
> >>Why this identation change ?
> >>
> >>>@@ -168,8 +184,8 @@ static int sun4i_lradc_load_dt_keymap(struct device *dev,
> >>>
> >>>  	lradc->chan0_map_count = of_get_child_count(np);
> >>>  	if (lradc->chan0_map_count == 0) {
> >>>-		dev_err(dev, "keymap is missing in device tree\n");
> >>>-		return -EINVAL;
> >>>+		dev_info(dev, "keymap is missing in device tree\n");
> >>>+		return 0;
> >>>  	}
> >>>
> >>>  	lradc->chan0_map = devm_kmalloc_array(dev, lradc->chan0_map_count,
> >>
> >>I assume this is so that people can still use the sysfs node, to create a dts, right
> >>not sure I like this, might be better to document to simple create a dts with
> >>a single button mapping for 200 mV (most board use 200 mV steps between the buttons).
> >>
> >>>@@ -185,19 +201,19 @@ static int sun4i_lradc_load_dt_keymap(struct device *dev,
> >>>
> >>>  		error = of_property_read_u32(pp, "channel", &channel);
> >>>  		if (error || channel != 0) {
> >>>-			dev_err(dev, "%s: Inval channel prop\n", pp->name);
> >>>+			dev_err(dev, "%s: Invalid 'channel' property\n", pp->name);
> >>>  			return -EINVAL;
> >>>  		}
> >>>
> >>>  		error = of_property_read_u32(pp, "voltage", &map->voltage);
> >>>  		if (error) {
> >>>-			dev_err(dev, "%s: Inval voltage prop\n", pp->name);
> >>>+			dev_err(dev, "%s: Invalid 'voltage' property\n", pp->name);
> >>>  			return -EINVAL;
> >>>  		}
> >>>
> >>>  		error = of_property_read_u32(pp, "linux,code", &map->keycode);
> >>>  		if (error) {
> >>>-			dev_err(dev, "%s: Inval linux,code prop\n", pp->name);
> >>>+			dev_err(dev, "%s: Invalid 'linux,code' property\n", pp->name);
> >>>  			return -EINVAL;
> >>>  		}
> >>>
> >>
> >>This hunk / 3 changes belong in a separate patch. Also please run checkpatch, I think
> >>you're running over 80 chars here.
> >>
> >>
> >>>@@ -257,14 +273,26 @@ static int sun4i_lradc_probe(struct platform_device *pdev)
> >>>  	if (error)
> >>>  		return error;
> >>>
> >>>-	error = input_register_device(lradc->input);
> >>>+	error = device_create_file(dev, &dev_attr_voltage);
> >>>  	if (error)
> >>>  		return error;
> >>>
> >>>+	error = input_register_device(lradc->input);
> >>>+	if (error) {
> >>>+		device_remove_file(&pdev->dev, &dev_attr_voltage);
> >>>+		return error;
> >>>+	}
> >>>+
> >>>  	platform_set_drvdata(pdev, lradc);
> >>>  	return 0;
> >>>  }
> >>>
> >>>+static int sun4i_lradc_remove(struct platform_device *pdev)
> >>>+{
> >>>+	device_remove_file(&pdev->dev, &dev_attr_voltage);
> >>>+	return 0;
> >>>+}
> >>>+
> >>
> >>This looks wrong, I think (*) that we've a bug here because we're not
> >>unregistering the input device, so maybe do 2 patches, 1 fixing the
> >>not unregistering bug, and then just add the device_remove_file()
> >>in the sysfs patch.
> >
> >The unregister was not necessary since the input device is managed.
> 
> Ah right, looking at the code again I see we use devm_input_allocate_device()
> is there no devm_create_file for creating sysfs entries ?

Greg was pushing the viewpoint that no drivers should create device attributes
manually (since it is somewhat racy - attributes are created after devices show
up) but I do not think he's gonna win that ever. So if someone were to add
devm_create_attribute_group() API I think that would be great. In absence of
this there is always devm_add_action().

Thanks.

-- 
Dmitry

[PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver

[Dmitry Torokhov]

On Tue, Jan 27, 2015 at 10:03:35AM +0100, Hans de Goede wrote:
> Hi,
> 
> On 26-01-15 23:06, Dmitry Torokhov wrote:
> >On Mon, Jan 26, 2015 at 08:28:29PM +0100, Hans de Goede wrote:
> >>Hi,
> >>
> >>On 26-01-15 17:58, Priit Laes wrote:
> >>
> >>No commit message? Please write an informative commit msg, like why we want this patch,
> >>I guess it is to help figuring out the voltage levels for various buttons when creating
> >>a dts, but I would prefer to not guess, which is where a good commit message would
> >>come in handy ...
> >>
> >>>---
> >>>  .../ABI/testing/sysfs-driver-input-sun4i-lradc     |  4 ++
> >>>  drivers/input/keyboard/sun4i-lradc-keys.c          | 49 +++++++++++++++++-----
> >>>  2 files changed, 43 insertions(+), 10 deletions(-)
> >>>  create mode 100644 Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> >>>
> >>>diff --git a/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> >>>new file mode 100644
> >>>index 0000000..e4e6448
> >>>--- /dev/null
> >>>+++ b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> >>>@@ -0,0 +1,4 @@
> >>>+What:		/sys/class/input/input(x)/device/voltage
> >>>+Date:		February 2015
> >>>+Contact:	Priit Laes <plaes@plaes.org>
> >>>+Description:	ADC output voltage in microvolts or 0 if device is not opened.
> >>>diff --git a/drivers/input/keyboard/sun4i-lradc-keys.c b/drivers/input/keyboard/sun4i-lradc-keys.c
> >>>index cc8f7dd..c0ab8ec 100644
> >>>--- a/drivers/input/keyboard/sun4i-lradc-keys.c
> >>>+++ b/drivers/input/keyboard/sun4i-lradc-keys.c
> >>>@@ -79,10 +79,27 @@ struct sun4i_lradc_data {
> >>>  	u32 vref;
> >>>  };
> >>>
> >>>+static u32 sun4i_lradc_read_voltage(struct sun4i_lradc_data *lradc)
> >>>+{
> >>>+	u32 val = readl(lradc->base + LRADC_DATA0) & 0x3f;
> >>>+	return val * lradc->vref / 63;
> >>>+};
> >>>+
> >>>+static ssize_t
> >>>+sun4i_lradc_dev_voltage_show(struct device *dev,
> >>>+			struct device_attribute *attr, char *buf)
> >>>+{
> >>>+	struct sun4i_lradc_data *lradc = dev_get_drvdata(dev);
> >>>+
> >>>+	return sprintf(buf, "%u\n", sun4i_lradc_read_voltage(lradc));
> >>>+}
> >>>+
> >>>+static const DEVICE_ATTR(voltage, S_IRUGO, sun4i_lradc_dev_voltage_show, NULL);
> >>>+
> >>>  static irqreturn_t sun4i_lradc_irq(int irq, void *dev_id)
> >>>  {
> >>>  	struct sun4i_lradc_data *lradc = dev_id;
> >>>-	u32 i, ints, val, voltage, diff, keycode = 0, closest = 0xffffffff;
> >>>+	u32 i, ints, voltage, diff, keycode = 0, closest = 0xffffffff;
> >>>
> >>>  	ints  = readl(lradc->base + LRADC_INTS);
> >>>
> >>>@@ -97,8 +114,7 @@ static irqreturn_t sun4i_lradc_irq(int irq, void *dev_id)
> >>>  	}
> >>>
> >>>  	if ((ints & CHAN0_KEYDOWN_IRQ) && lradc->chan0_keycode == 0) {
> >>>-		val = readl(lradc->base + LRADC_DATA0) & 0x3f;
> >>>-		voltage = val * lradc->vref / 63;
> >>>+		voltage = sun4i_lradc_read_voltage(lradc);
> >>>
> >>>  		for (i = 0; i < lradc->chan0_map_count; i++) {
> >>>  			diff = abs(lradc->chan0_map[i].voltage - voltage);
> >>>@@ -156,7 +172,7 @@ static void sun4i_lradc_close(struct input_dev *dev)
> >>>  }
> >>>
> >>>  static int sun4i_lradc_load_dt_keymap(struct device *dev,
> >>>-				      struct sun4i_lradc_data *lradc)
> >>>+				    struct sun4i_lradc_data *lradc)
> >>>  {
> >>>  	struct device_node *np, *pp;
> >>>  	int i;
> >>
> >>Why this identation change ?
> >>
> >>>@@ -168,8 +184,8 @@ static int sun4i_lradc_load_dt_keymap(struct device *dev,
> >>>
> >>>  	lradc->chan0_map_count = of_get_child_count(np);
> >>>  	if (lradc->chan0_map_count == 0) {
> >>>-		dev_err(dev, "keymap is missing in device tree\n");
> >>>-		return -EINVAL;
> >>>+		dev_info(dev, "keymap is missing in device tree\n");
> >>>+		return 0;
> >>>  	}
> >>>
> >>>  	lradc->chan0_map = devm_kmalloc_array(dev, lradc->chan0_map_count,
> >>
> >>I assume this is so that people can still use the sysfs node, to create a dts, right
> >>not sure I like this, might be better to document to simple create a dts with
> >>a single button mapping for 200 mV (most board use 200 mV steps between the buttons).
> >>
> >>>@@ -185,19 +201,19 @@ static int sun4i_lradc_load_dt_keymap(struct device *dev,
> >>>
> >>>  		error = of_property_read_u32(pp, "channel", &channel);
> >>>  		if (error || channel != 0) {
> >>>-			dev_err(dev, "%s: Inval channel prop\n", pp->name);
> >>>+			dev_err(dev, "%s: Invalid 'channel' property\n", pp->name);
> >>>  			return -EINVAL;
> >>>  		}
> >>>
> >>>  		error = of_property_read_u32(pp, "voltage", &map->voltage);
> >>>  		if (error) {
> >>>-			dev_err(dev, "%s: Inval voltage prop\n", pp->name);
> >>>+			dev_err(dev, "%s: Invalid 'voltage' property\n", pp->name);
> >>>  			return -EINVAL;
> >>>  		}
> >>>
> >>>  		error = of_property_read_u32(pp, "linux,code", &map->keycode);
> >>>  		if (error) {
> >>>-			dev_err(dev, "%s: Inval linux,code prop\n", pp->name);
> >>>+			dev_err(dev, "%s: Invalid 'linux,code' property\n", pp->name);
> >>>  			return -EINVAL;
> >>>  		}
> >>>
> >>
> >>This hunk / 3 changes belong in a separate patch. Also please run checkpatch, I think
> >>you're running over 80 chars here.
> >>
> >>
> >>>@@ -257,14 +273,26 @@ static int sun4i_lradc_probe(struct platform_device *pdev)
> >>>  	if (error)
> >>>  		return error;
> >>>
> >>>-	error = input_register_device(lradc->input);
> >>>+	error = device_create_file(dev, &dev_attr_voltage);
> >>>  	if (error)
> >>>  		return error;
> >>>
> >>>+	error = input_register_device(lradc->input);
> >>>+	if (error) {
> >>>+		device_remove_file(&pdev->dev, &dev_attr_voltage);
> >>>+		return error;
> >>>+	}
> >>>+
> >>>  	platform_set_drvdata(pdev, lradc);
> >>>  	return 0;
> >>>  }
> >>>
> >>>+static int sun4i_lradc_remove(struct platform_device *pdev)
> >>>+{
> >>>+	device_remove_file(&pdev->dev, &dev_attr_voltage);
> >>>+	return 0;
> >>>+}
> >>>+
> >>
> >>This looks wrong, I think (*) that we've a bug here because we're not
> >>unregistering the input device, so maybe do 2 patches, 1 fixing the
> >>not unregistering bug, and then just add the device_remove_file()
> >>in the sysfs patch.
> >
> >The unregister was not necessary since the input device is managed.
> 
> Ah right, looking at the code again I see we use devm_input_allocate_device()
> is there no devm_create_file for creating sysfs entries ?

Greg was pushing the viewpoint that no drivers should create device attributes
manually (since it is somewhat racy - attributes are created after devices show
up) but I do not think he's gonna win that ever. So if someone were to add
devm_create_attribute_group() API I think that would be great. In absence of
this there is always devm_add_action().

Thanks.

-- 
Dmitry

Re: [linux-sunxi] Re: [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver

[Dmitry Torokhov]

On Tue, Jan 27, 2015 at 11:49:49AM +0200, Priit Laes wrote:
> 
> On Tue, 2015-01-27 at 10:18 +0100, Maxime Ripard wrote:
> > Hi,
> > 
> > On Mon, Jan 26, 2015 at 06:58:32PM +0200, Priit Laes wrote:
> > > ---
> > 
> > Like Hans was pointing out, commit log and signed-off-by please
> > 
> > >  .../ABI/testing/sysfs-driver-input-sun4i-lradc     |  4 ++
> > >  drivers/input/keyboard/sun4i-lradc-keys.c          | 49 
> > > +++++++++++++++++-----
> > >  2 files changed, 43 insertions(+), 10 deletions(-)
> > >  create mode 100644 Documentation/ABI/testing/sysfs-driver-input-
> > > sun4i-lradc
> > > 
> > > diff --git a/Documentation/ABI/testing/sysfs-driver-input-sun4i-
> > > lradc b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> > > new file mode 100644
> > > index 0000000..e4e6448
> > > --- /dev/null
> > > +++ b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> > > @@ -0,0 +1,4 @@
> > > +What:          /sys/class/input/input(x)/device/voltage
> > > +Date:          February 2015
> > > +Contact:       Priit Laes <plaes@plaes.org>
> > > +Description:   ADC output voltage in microvolts or 0 if device is 
> > > not opened.
> > 
> > Why is it returning 0 when "device is not opened" ? What does that 
> > even mean? You can't read that file without opening it.
> 
> It means that something has to open the /dev/input/inputX device which 
> sets up the ADC before the voltage can be read from the sysfs file.

I'd consider this a bug. Why someone has to use an unrelated interface for
another interface to work correctly?

> 
> [...]
> 
> 
> > 
> > As I told you already, if you're going to expose this an ADC in the 
> > end, the proper solution is to use the IIO framework, not adding a 
> > custom sysfs file.
> 
> My intention was to expose just a simple debug output, so one can 
> press the buttons and read the voltages for devicetree keymap.
> 
> If anyone can suggest a simpler approach than current sysfs based one, 
> I would do it. But full blown iio driver is currently out of scope. 
> Also, Carlo's (ccaione) initially submitted (?) driver for lradc 
> utilized iio subsystem.

For stuff like this debugfs (AKA dumping ground) is better suited as it does
not establish ABI.  i

Thanks.

-- 
Dmitry

Re: Re: [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver

[Dmitry Torokhov]

On Tue, Jan 27, 2015 at 11:49:49AM +0200, Priit Laes wrote:
> 
> On Tue, 2015-01-27 at 10:18 +0100, Maxime Ripard wrote:
> > Hi,
> > 
> > On Mon, Jan 26, 2015 at 06:58:32PM +0200, Priit Laes wrote:
> > > ---
> > 
> > Like Hans was pointing out, commit log and signed-off-by please
> > 
> > >  .../ABI/testing/sysfs-driver-input-sun4i-lradc     |  4 ++
> > >  drivers/input/keyboard/sun4i-lradc-keys.c          | 49 
> > > +++++++++++++++++-----
> > >  2 files changed, 43 insertions(+), 10 deletions(-)
> > >  create mode 100644 Documentation/ABI/testing/sysfs-driver-input-
> > > sun4i-lradc
> > > 
> > > diff --git a/Documentation/ABI/testing/sysfs-driver-input-sun4i-
> > > lradc b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> > > new file mode 100644
> > > index 0000000..e4e6448
> > > --- /dev/null
> > > +++ b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> > > @@ -0,0 +1,4 @@
> > > +What:          /sys/class/input/input(x)/device/voltage
> > > +Date:          February 2015
> > > +Contact:       Priit Laes <plaes-q/aMd4JkU83YtjvyW6yDsg@public.gmane.org>
> > > +Description:   ADC output voltage in microvolts or 0 if device is 
> > > not opened.
> > 
> > Why is it returning 0 when "device is not opened" ? What does that 
> > even mean? You can't read that file without opening it.
> 
> It means that something has to open the /dev/input/inputX device which 
> sets up the ADC before the voltage can be read from the sysfs file.

I'd consider this a bug. Why someone has to use an unrelated interface for
another interface to work correctly?

> 
> [...]
> 
> 
> > 
> > As I told you already, if you're going to expose this an ADC in the 
> > end, the proper solution is to use the IIO framework, not adding a 
> > custom sysfs file.
> 
> My intention was to expose just a simple debug output, so one can 
> press the buttons and read the voltages for devicetree keymap.
> 
> If anyone can suggest a simpler approach than current sysfs based one, 
> I would do it. But full blown iio driver is currently out of scope. 
> Also, Carlo's (ccaione) initially submitted (?) driver for lradc 
> utilized iio subsystem.

For stuff like this debugfs (AKA dumping ground) is better suited as it does
not establish ABI.  i

Thanks.

-- 
Dmitry

[linux-sunxi] Re: [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver

[Dmitry Torokhov]

On Tue, Jan 27, 2015 at 11:49:49AM +0200, Priit Laes wrote:
> 
> On Tue, 2015-01-27 at 10:18 +0100, Maxime Ripard wrote:
> > Hi,
> > 
> > On Mon, Jan 26, 2015 at 06:58:32PM +0200, Priit Laes wrote:
> > > ---
> > 
> > Like Hans was pointing out, commit log and signed-off-by please
> > 
> > >  .../ABI/testing/sysfs-driver-input-sun4i-lradc     |  4 ++
> > >  drivers/input/keyboard/sun4i-lradc-keys.c          | 49 
> > > +++++++++++++++++-----
> > >  2 files changed, 43 insertions(+), 10 deletions(-)
> > >  create mode 100644 Documentation/ABI/testing/sysfs-driver-input-
> > > sun4i-lradc
> > > 
> > > diff --git a/Documentation/ABI/testing/sysfs-driver-input-sun4i-
> > > lradc b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> > > new file mode 100644
> > > index 0000000..e4e6448
> > > --- /dev/null
> > > +++ b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> > > @@ -0,0 +1,4 @@
> > > +What:          /sys/class/input/input(x)/device/voltage
> > > +Date:          February 2015
> > > +Contact:       Priit Laes <plaes@plaes.org>
> > > +Description:   ADC output voltage in microvolts or 0 if device is 
> > > not opened.
> > 
> > Why is it returning 0 when "device is not opened" ? What does that 
> > even mean? You can't read that file without opening it.
> 
> It means that something has to open the /dev/input/inputX device which 
> sets up the ADC before the voltage can be read from the sysfs file.

I'd consider this a bug. Why someone has to use an unrelated interface for
another interface to work correctly?

> 
> [...]
> 
> 
> > 
> > As I told you already, if you're going to expose this an ADC in the 
> > end, the proper solution is to use the IIO framework, not adding a 
> > custom sysfs file.
> 
> My intention was to expose just a simple debug output, so one can 
> press the buttons and read the voltages for devicetree keymap.
> 
> If anyone can suggest a simpler approach than current sysfs based one, 
> I would do it. But full blown iio driver is currently out of scope. 
> Also, Carlo's (ccaione) initially submitted (?) driver for lradc 
> utilized iio subsystem.

For stuff like this debugfs (AKA dumping ground) is better suited as it does
not establish ABI.  i

Thanks.

-- 
Dmitry

Re: [linux-sunxi] Re: [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2353 bytes --]

On Tue, Jan 27, 2015 at 11:49:49AM +0200, Priit Laes wrote:
> 
> On Tue, 2015-01-27 at 10:18 +0100, Maxime Ripard wrote:
> > Hi,
> > 
> > On Mon, Jan 26, 2015 at 06:58:32PM +0200, Priit Laes wrote:
> > > ---
> > 
> > Like Hans was pointing out, commit log and signed-off-by please
> > 
> > >  .../ABI/testing/sysfs-driver-input-sun4i-lradc     |  4 ++
> > >  drivers/input/keyboard/sun4i-lradc-keys.c          | 49 
> > > +++++++++++++++++-----
> > >  2 files changed, 43 insertions(+), 10 deletions(-)
> > >  create mode 100644 Documentation/ABI/testing/sysfs-driver-input-
> > > sun4i-lradc
> > > 
> > > diff --git a/Documentation/ABI/testing/sysfs-driver-input-sun4i-
> > > lradc b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> > > new file mode 100644
> > > index 0000000..e4e6448
> > > --- /dev/null
> > > +++ b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> > > @@ -0,0 +1,4 @@
> > > +What:          /sys/class/input/input(x)/device/voltage
> > > +Date:          February 2015
> > > +Contact:       Priit Laes <plaes@plaes.org>
> > > +Description:   ADC output voltage in microvolts or 0 if device is 
> > > not opened.
> > 
> > Why is it returning 0 when "device is not opened" ? What does that 
> > even mean? You can't read that file without opening it.
> 
> It means that something has to open the /dev/input/inputX device which 
> sets up the ADC before the voltage can be read from the sysfs file.

It's a bug.

> > As I told you already, if you're going to expose this an ADC in the 
> > end, the proper solution is to use the IIO framework, not adding a 
> > custom sysfs file.
> 
> My intention was to expose just a simple debug output, so one can 
> press the buttons and read the voltages for devicetree keymap.
> 
> If anyone can suggest a simpler approach than current sysfs based one, 
> I would do it. But full blown iio driver is currently out of scope.

For this kind of ADCs, it's really not that hard, and provide more or
less the same interface.

> Also, Carlo's (ccaione) initially submitted (?) driver for lradc 
> utilized iio subsystem.

And it was dropped because
no-one-would-use-this-to-retrieve-the-voltage-ever :)

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: Re: [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2319 bytes --]

On Tue, Jan 27, 2015 at 11:49:49AM +0200, Priit Laes wrote:
> 
> On Tue, 2015-01-27 at 10:18 +0100, Maxime Ripard wrote:
> > Hi,
> > 
> > On Mon, Jan 26, 2015 at 06:58:32PM +0200, Priit Laes wrote:
> > > ---
> > 
> > Like Hans was pointing out, commit log and signed-off-by please
> > 
> > >  .../ABI/testing/sysfs-driver-input-sun4i-lradc     |  4 ++
> > >  drivers/input/keyboard/sun4i-lradc-keys.c          | 49 
> > > +++++++++++++++++-----
> > >  2 files changed, 43 insertions(+), 10 deletions(-)
> > >  create mode 100644 Documentation/ABI/testing/sysfs-driver-input-
> > > sun4i-lradc
> > > 
> > > diff --git a/Documentation/ABI/testing/sysfs-driver-input-sun4i-
> > > lradc b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> > > new file mode 100644
> > > index 0000000..e4e6448
> > > --- /dev/null
> > > +++ b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> > > @@ -0,0 +1,4 @@
> > > +What:          /sys/class/input/input(x)/device/voltage
> > > +Date:          February 2015
> > > +Contact:       Priit Laes <plaes-q/aMd4JkU83YtjvyW6yDsg@public.gmane.org>
> > > +Description:   ADC output voltage in microvolts or 0 if device is 
> > > not opened.
> > 
> > Why is it returning 0 when "device is not opened" ? What does that 
> > even mean? You can't read that file without opening it.
> 
> It means that something has to open the /dev/input/inputX device which 
> sets up the ADC before the voltage can be read from the sysfs file.

It's a bug.

> > As I told you already, if you're going to expose this an ADC in the 
> > end, the proper solution is to use the IIO framework, not adding a 
> > custom sysfs file.
> 
> My intention was to expose just a simple debug output, so one can 
> press the buttons and read the voltages for devicetree keymap.
> 
> If anyone can suggest a simpler approach than current sysfs based one, 
> I would do it. But full blown iio driver is currently out of scope.

For this kind of ADCs, it's really not that hard, and provide more or
less the same interface.

> Also, Carlo's (ccaione) initially submitted (?) driver for lradc 
> utilized iio subsystem.

And it was dropped because
no-one-would-use-this-to-retrieve-the-voltage-ever :)

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[linux-sunxi] Re: [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver

[Maxime Ripard]

On Tue, Jan 27, 2015 at 11:49:49AM +0200, Priit Laes wrote:
> 
> On Tue, 2015-01-27 at 10:18 +0100, Maxime Ripard wrote:
> > Hi,
> > 
> > On Mon, Jan 26, 2015 at 06:58:32PM +0200, Priit Laes wrote:
> > > ---
> > 
> > Like Hans was pointing out, commit log and signed-off-by please
> > 
> > >  .../ABI/testing/sysfs-driver-input-sun4i-lradc     |  4 ++
> > >  drivers/input/keyboard/sun4i-lradc-keys.c          | 49 
> > > +++++++++++++++++-----
> > >  2 files changed, 43 insertions(+), 10 deletions(-)
> > >  create mode 100644 Documentation/ABI/testing/sysfs-driver-input-
> > > sun4i-lradc
> > > 
> > > diff --git a/Documentation/ABI/testing/sysfs-driver-input-sun4i-
> > > lradc b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> > > new file mode 100644
> > > index 0000000..e4e6448
> > > --- /dev/null
> > > +++ b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> > > @@ -0,0 +1,4 @@
> > > +What:          /sys/class/input/input(x)/device/voltage
> > > +Date:          February 2015
> > > +Contact:       Priit Laes <plaes@plaes.org>
> > > +Description:   ADC output voltage in microvolts or 0 if device is 
> > > not opened.
> > 
> > Why is it returning 0 when "device is not opened" ? What does that 
> > even mean? You can't read that file without opening it.
> 
> It means that something has to open the /dev/input/inputX device which 
> sets up the ADC before the voltage can be read from the sysfs file.

It's a bug.

> > As I told you already, if you're going to expose this an ADC in the 
> > end, the proper solution is to use the IIO framework, not adding a 
> > custom sysfs file.
> 
> My intention was to expose just a simple debug output, so one can 
> press the buttons and read the voltages for devicetree keymap.
> 
> If anyone can suggest a simpler approach than current sysfs based one, 
> I would do it. But full blown iio driver is currently out of scope.

For this kind of ADCs, it's really not that hard, and provide more or
less the same interface.

> Also, Carlo's (ccaione) initially submitted (?) driver for lradc 
> utilized iio subsystem.

And it was dropped because
no-one-would-use-this-to-retrieve-the-voltage-ever :)

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150127/c3dc213a/attachment-0001.sig>

Re: [linux-sunxi] Re: [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2995 bytes --]

On Tue, Jan 27, 2015 at 11:52:34AM +0100, Hans de Goede wrote:
> Hi,
> 
> On 27-01-15 10:49, Priit Laes wrote:
> >
> >On Tue, 2015-01-27 at 10:18 +0100, Maxime Ripard wrote:
> >>Hi,
> >>
> >>On Mon, Jan 26, 2015 at 06:58:32PM +0200, Priit Laes wrote:
> >>>---
> >>
> >>Like Hans was pointing out, commit log and signed-off-by please
> >>
> >>>  .../ABI/testing/sysfs-driver-input-sun4i-lradc     |  4 ++
> >>>  drivers/input/keyboard/sun4i-lradc-keys.c          | 49
> >>>+++++++++++++++++-----
> >>>  2 files changed, 43 insertions(+), 10 deletions(-)
> >>>  create mode 100644 Documentation/ABI/testing/sysfs-driver-input-
> >>>sun4i-lradc
> >>>
> >>>diff --git a/Documentation/ABI/testing/sysfs-driver-input-sun4i-
> >>>lradc b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> >>>new file mode 100644
> >>>index 0000000..e4e6448
> >>>--- /dev/null
> >>>+++ b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> >>>@@ -0,0 +1,4 @@
> >>>+What:          /sys/class/input/input(x)/device/voltage
> >>>+Date:          February 2015
> >>>+Contact:       Priit Laes <plaes@plaes.org>
> >>>+Description:   ADC output voltage in microvolts or 0 if device is
> >>>not opened.
> >>
> >>Why is it returning 0 when "device is not opened" ? What does that
> >>even mean? You can't read that file without opening it.
> >
> >It means that something has to open the /dev/input/inputX device which
> >sets up the ADC before the voltage can be read from the sysfs file.
> >
> >[...]
> >
> >
> >>
> >>As I told you already, if you're going to expose this an ADC in the
> >>end, the proper solution is to use the IIO framework, not adding a
> >>custom sysfs file.
> >
> >My intention was to expose just a simple debug output, so one can
> >press the buttons and read the voltages for devicetree keymap.
> >
> >If anyone can suggest a simpler approach than current sysfs based one,
> >I would do it.
> 
> The android driver always uses 0.2V / 200mV steps, so what I do is
> simply create a mapping with 200mV mapped to KEY_VOLUMEUP, 400mV mapped
> to KEY_VOLUMEDOWN, etc. following the hardcoded android driver mapping:
> 
> https://github.com/linux-sunxi/linux-sunxi/blob/sunxi-3.4/drivers/input/keyboard/sun4i-keyboard.c#L136
> 
> Usually this will be correct in one go, after testing one can shuffle
> key codes as needed (usually not needed) and/or remove unused entries.
> 
> With that said I do think that a sysfs file to see the actual voltages,
> or a kernel parameter to printk them on keypress interrupt would be useful.
> 
> I guess the printk option would be better as it would show the actual
> keypress value read, not some semi-random sample.

That wouldn't require that much code actually. Either using dev_dbg,
or debugfs like Dmitry was suggesting would be two nice solutions I
guess.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: Re: [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2946 bytes --]

On Tue, Jan 27, 2015 at 11:52:34AM +0100, Hans de Goede wrote:
> Hi,
> 
> On 27-01-15 10:49, Priit Laes wrote:
> >
> >On Tue, 2015-01-27 at 10:18 +0100, Maxime Ripard wrote:
> >>Hi,
> >>
> >>On Mon, Jan 26, 2015 at 06:58:32PM +0200, Priit Laes wrote:
> >>>---
> >>
> >>Like Hans was pointing out, commit log and signed-off-by please
> >>
> >>>  .../ABI/testing/sysfs-driver-input-sun4i-lradc     |  4 ++
> >>>  drivers/input/keyboard/sun4i-lradc-keys.c          | 49
> >>>+++++++++++++++++-----
> >>>  2 files changed, 43 insertions(+), 10 deletions(-)
> >>>  create mode 100644 Documentation/ABI/testing/sysfs-driver-input-
> >>>sun4i-lradc
> >>>
> >>>diff --git a/Documentation/ABI/testing/sysfs-driver-input-sun4i-
> >>>lradc b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> >>>new file mode 100644
> >>>index 0000000..e4e6448
> >>>--- /dev/null
> >>>+++ b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> >>>@@ -0,0 +1,4 @@
> >>>+What:          /sys/class/input/input(x)/device/voltage
> >>>+Date:          February 2015
> >>>+Contact:       Priit Laes <plaes-q/aMd4JkU83YtjvyW6yDsg@public.gmane.org>
> >>>+Description:   ADC output voltage in microvolts or 0 if device is
> >>>not opened.
> >>
> >>Why is it returning 0 when "device is not opened" ? What does that
> >>even mean? You can't read that file without opening it.
> >
> >It means that something has to open the /dev/input/inputX device which
> >sets up the ADC before the voltage can be read from the sysfs file.
> >
> >[...]
> >
> >
> >>
> >>As I told you already, if you're going to expose this an ADC in the
> >>end, the proper solution is to use the IIO framework, not adding a
> >>custom sysfs file.
> >
> >My intention was to expose just a simple debug output, so one can
> >press the buttons and read the voltages for devicetree keymap.
> >
> >If anyone can suggest a simpler approach than current sysfs based one,
> >I would do it.
> 
> The android driver always uses 0.2V / 200mV steps, so what I do is
> simply create a mapping with 200mV mapped to KEY_VOLUMEUP, 400mV mapped
> to KEY_VOLUMEDOWN, etc. following the hardcoded android driver mapping:
> 
> https://github.com/linux-sunxi/linux-sunxi/blob/sunxi-3.4/drivers/input/keyboard/sun4i-keyboard.c#L136
> 
> Usually this will be correct in one go, after testing one can shuffle
> key codes as needed (usually not needed) and/or remove unused entries.
> 
> With that said I do think that a sysfs file to see the actual voltages,
> or a kernel parameter to printk them on keypress interrupt would be useful.
> 
> I guess the printk option would be better as it would show the actual
> keypress value read, not some semi-random sample.

That wouldn't require that much code actually. Either using dev_dbg,
or debugfs like Dmitry was suggesting would be two nice solutions I
guess.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[linux-sunxi] Re: [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver

[Maxime Ripard]

On Tue, Jan 27, 2015 at 11:52:34AM +0100, Hans de Goede wrote:
> Hi,
> 
> On 27-01-15 10:49, Priit Laes wrote:
> >
> >On Tue, 2015-01-27 at 10:18 +0100, Maxime Ripard wrote:
> >>Hi,
> >>
> >>On Mon, Jan 26, 2015 at 06:58:32PM +0200, Priit Laes wrote:
> >>>---
> >>
> >>Like Hans was pointing out, commit log and signed-off-by please
> >>
> >>>  .../ABI/testing/sysfs-driver-input-sun4i-lradc     |  4 ++
> >>>  drivers/input/keyboard/sun4i-lradc-keys.c          | 49
> >>>+++++++++++++++++-----
> >>>  2 files changed, 43 insertions(+), 10 deletions(-)
> >>>  create mode 100644 Documentation/ABI/testing/sysfs-driver-input-
> >>>sun4i-lradc
> >>>
> >>>diff --git a/Documentation/ABI/testing/sysfs-driver-input-sun4i-
> >>>lradc b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> >>>new file mode 100644
> >>>index 0000000..e4e6448
> >>>--- /dev/null
> >>>+++ b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> >>>@@ -0,0 +1,4 @@
> >>>+What:          /sys/class/input/input(x)/device/voltage
> >>>+Date:          February 2015
> >>>+Contact:       Priit Laes <plaes@plaes.org>
> >>>+Description:   ADC output voltage in microvolts or 0 if device is
> >>>not opened.
> >>
> >>Why is it returning 0 when "device is not opened" ? What does that
> >>even mean? You can't read that file without opening it.
> >
> >It means that something has to open the /dev/input/inputX device which
> >sets up the ADC before the voltage can be read from the sysfs file.
> >
> >[...]
> >
> >
> >>
> >>As I told you already, if you're going to expose this an ADC in the
> >>end, the proper solution is to use the IIO framework, not adding a
> >>custom sysfs file.
> >
> >My intention was to expose just a simple debug output, so one can
> >press the buttons and read the voltages for devicetree keymap.
> >
> >If anyone can suggest a simpler approach than current sysfs based one,
> >I would do it.
> 
> The android driver always uses 0.2V / 200mV steps, so what I do is
> simply create a mapping with 200mV mapped to KEY_VOLUMEUP, 400mV mapped
> to KEY_VOLUMEDOWN, etc. following the hardcoded android driver mapping:
> 
> https://github.com/linux-sunxi/linux-sunxi/blob/sunxi-3.4/drivers/input/keyboard/sun4i-keyboard.c#L136
> 
> Usually this will be correct in one go, after testing one can shuffle
> key codes as needed (usually not needed) and/or remove unused entries.
> 
> With that said I do think that a sysfs file to see the actual voltages,
> or a kernel parameter to printk them on keypress interrupt would be useful.
> 
> I guess the printk option would be better as it would show the actual
> keypress value read, not some semi-random sample.

That wouldn't require that much code actually. Either using dev_dbg,
or debugfs like Dmitry was suggesting would be two nice solutions I
guess.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150127/c9b38f4d/attachment.sig>

Re: [linux-sunxi] Re: [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver

[Dmitry Torokhov]

On Tue, Jan 27, 2015 at 08:44:47PM +0100, Maxime Ripard wrote:
> On Tue, Jan 27, 2015 at 11:52:34AM +0100, Hans de Goede wrote:
> > Hi,
> > 
> > On 27-01-15 10:49, Priit Laes wrote:
> > >
> > >On Tue, 2015-01-27 at 10:18 +0100, Maxime Ripard wrote:
> > >>Hi,
> > >>
> > >>On Mon, Jan 26, 2015 at 06:58:32PM +0200, Priit Laes wrote:
> > >>>---
> > >>
> > >>Like Hans was pointing out, commit log and signed-off-by please
> > >>
> > >>>  .../ABI/testing/sysfs-driver-input-sun4i-lradc     |  4 ++
> > >>>  drivers/input/keyboard/sun4i-lradc-keys.c          | 49
> > >>>+++++++++++++++++-----
> > >>>  2 files changed, 43 insertions(+), 10 deletions(-)
> > >>>  create mode 100644 Documentation/ABI/testing/sysfs-driver-input-
> > >>>sun4i-lradc
> > >>>
> > >>>diff --git a/Documentation/ABI/testing/sysfs-driver-input-sun4i-
> > >>>lradc b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> > >>>new file mode 100644
> > >>>index 0000000..e4e6448
> > >>>--- /dev/null
> > >>>+++ b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> > >>>@@ -0,0 +1,4 @@
> > >>>+What:          /sys/class/input/input(x)/device/voltage
> > >>>+Date:          February 2015
> > >>>+Contact:       Priit Laes <plaes@plaes.org>
> > >>>+Description:   ADC output voltage in microvolts or 0 if device is
> > >>>not opened.
> > >>
> > >>Why is it returning 0 when "device is not opened" ? What does that
> > >>even mean? You can't read that file without opening it.
> > >
> > >It means that something has to open the /dev/input/inputX device which
> > >sets up the ADC before the voltage can be read from the sysfs file.
> > >
> > >[...]
> > >
> > >
> > >>
> > >>As I told you already, if you're going to expose this an ADC in the
> > >>end, the proper solution is to use the IIO framework, not adding a
> > >>custom sysfs file.
> > >
> > >My intention was to expose just a simple debug output, so one can
> > >press the buttons and read the voltages for devicetree keymap.
> > >
> > >If anyone can suggest a simpler approach than current sysfs based one,
> > >I would do it.
> > 
> > The android driver always uses 0.2V / 200mV steps, so what I do is
> > simply create a mapping with 200mV mapped to KEY_VOLUMEUP, 400mV mapped
> > to KEY_VOLUMEDOWN, etc. following the hardcoded android driver mapping:
> > 
> > https://github.com/linux-sunxi/linux-sunxi/blob/sunxi-3.4/drivers/input/keyboard/sun4i-keyboard.c#L136
> > 
> > Usually this will be correct in one go, after testing one can shuffle
> > key codes as needed (usually not needed) and/or remove unused entries.
> > 
> > With that said I do think that a sysfs file to see the actual voltages,
> > or a kernel parameter to printk them on keypress interrupt would be useful.
> > 
> > I guess the printk option would be better as it would show the actual
> > keypress value read, not some semi-random sample.
> 
> That wouldn't require that much code actually. Either using dev_dbg,
> or debugfs like Dmitry was suggesting would be two nice solutions I
> guess.

Given the stated purpose I'd say dev_dbg() and call it a day.

Thanks.

-- 
Dmitry

Re: Re: [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver

[Dmitry Torokhov]

On Tue, Jan 27, 2015 at 08:44:47PM +0100, Maxime Ripard wrote:
> On Tue, Jan 27, 2015 at 11:52:34AM +0100, Hans de Goede wrote:
> > Hi,
> > 
> > On 27-01-15 10:49, Priit Laes wrote:
> > >
> > >On Tue, 2015-01-27 at 10:18 +0100, Maxime Ripard wrote:
> > >>Hi,
> > >>
> > >>On Mon, Jan 26, 2015 at 06:58:32PM +0200, Priit Laes wrote:
> > >>>---
> > >>
> > >>Like Hans was pointing out, commit log and signed-off-by please
> > >>
> > >>>  .../ABI/testing/sysfs-driver-input-sun4i-lradc     |  4 ++
> > >>>  drivers/input/keyboard/sun4i-lradc-keys.c          | 49
> > >>>+++++++++++++++++-----
> > >>>  2 files changed, 43 insertions(+), 10 deletions(-)
> > >>>  create mode 100644 Documentation/ABI/testing/sysfs-driver-input-
> > >>>sun4i-lradc
> > >>>
> > >>>diff --git a/Documentation/ABI/testing/sysfs-driver-input-sun4i-
> > >>>lradc b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> > >>>new file mode 100644
> > >>>index 0000000..e4e6448
> > >>>--- /dev/null
> > >>>+++ b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> > >>>@@ -0,0 +1,4 @@
> > >>>+What:          /sys/class/input/input(x)/device/voltage
> > >>>+Date:          February 2015
> > >>>+Contact:       Priit Laes <plaes-q/aMd4JkU83YtjvyW6yDsg@public.gmane.org>
> > >>>+Description:   ADC output voltage in microvolts or 0 if device is
> > >>>not opened.
> > >>
> > >>Why is it returning 0 when "device is not opened" ? What does that
> > >>even mean? You can't read that file without opening it.
> > >
> > >It means that something has to open the /dev/input/inputX device which
> > >sets up the ADC before the voltage can be read from the sysfs file.
> > >
> > >[...]
> > >
> > >
> > >>
> > >>As I told you already, if you're going to expose this an ADC in the
> > >>end, the proper solution is to use the IIO framework, not adding a
> > >>custom sysfs file.
> > >
> > >My intention was to expose just a simple debug output, so one can
> > >press the buttons and read the voltages for devicetree keymap.
> > >
> > >If anyone can suggest a simpler approach than current sysfs based one,
> > >I would do it.
> > 
> > The android driver always uses 0.2V / 200mV steps, so what I do is
> > simply create a mapping with 200mV mapped to KEY_VOLUMEUP, 400mV mapped
> > to KEY_VOLUMEDOWN, etc. following the hardcoded android driver mapping:
> > 
> > https://github.com/linux-sunxi/linux-sunxi/blob/sunxi-3.4/drivers/input/keyboard/sun4i-keyboard.c#L136
> > 
> > Usually this will be correct in one go, after testing one can shuffle
> > key codes as needed (usually not needed) and/or remove unused entries.
> > 
> > With that said I do think that a sysfs file to see the actual voltages,
> > or a kernel parameter to printk them on keypress interrupt would be useful.
> > 
> > I guess the printk option would be better as it would show the actual
> > keypress value read, not some semi-random sample.
> 
> That wouldn't require that much code actually. Either using dev_dbg,
> or debugfs like Dmitry was suggesting would be two nice solutions I
> guess.

Given the stated purpose I'd say dev_dbg() and call it a day.

Thanks.

-- 
Dmitry

[linux-sunxi] Re: [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver

[Dmitry Torokhov]

On Tue, Jan 27, 2015 at 08:44:47PM +0100, Maxime Ripard wrote:
> On Tue, Jan 27, 2015 at 11:52:34AM +0100, Hans de Goede wrote:
> > Hi,
> > 
> > On 27-01-15 10:49, Priit Laes wrote:
> > >
> > >On Tue, 2015-01-27 at 10:18 +0100, Maxime Ripard wrote:
> > >>Hi,
> > >>
> > >>On Mon, Jan 26, 2015 at 06:58:32PM +0200, Priit Laes wrote:
> > >>>---
> > >>
> > >>Like Hans was pointing out, commit log and signed-off-by please
> > >>
> > >>>  .../ABI/testing/sysfs-driver-input-sun4i-lradc     |  4 ++
> > >>>  drivers/input/keyboard/sun4i-lradc-keys.c          | 49
> > >>>+++++++++++++++++-----
> > >>>  2 files changed, 43 insertions(+), 10 deletions(-)
> > >>>  create mode 100644 Documentation/ABI/testing/sysfs-driver-input-
> > >>>sun4i-lradc
> > >>>
> > >>>diff --git a/Documentation/ABI/testing/sysfs-driver-input-sun4i-
> > >>>lradc b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> > >>>new file mode 100644
> > >>>index 0000000..e4e6448
> > >>>--- /dev/null
> > >>>+++ b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> > >>>@@ -0,0 +1,4 @@
> > >>>+What:          /sys/class/input/input(x)/device/voltage
> > >>>+Date:          February 2015
> > >>>+Contact:       Priit Laes <plaes@plaes.org>
> > >>>+Description:   ADC output voltage in microvolts or 0 if device is
> > >>>not opened.
> > >>
> > >>Why is it returning 0 when "device is not opened" ? What does that
> > >>even mean? You can't read that file without opening it.
> > >
> > >It means that something has to open the /dev/input/inputX device which
> > >sets up the ADC before the voltage can be read from the sysfs file.
> > >
> > >[...]
> > >
> > >
> > >>
> > >>As I told you already, if you're going to expose this an ADC in the
> > >>end, the proper solution is to use the IIO framework, not adding a
> > >>custom sysfs file.
> > >
> > >My intention was to expose just a simple debug output, so one can
> > >press the buttons and read the voltages for devicetree keymap.
> > >
> > >If anyone can suggest a simpler approach than current sysfs based one,
> > >I would do it.
> > 
> > The android driver always uses 0.2V / 200mV steps, so what I do is
> > simply create a mapping with 200mV mapped to KEY_VOLUMEUP, 400mV mapped
> > to KEY_VOLUMEDOWN, etc. following the hardcoded android driver mapping:
> > 
> > https://github.com/linux-sunxi/linux-sunxi/blob/sunxi-3.4/drivers/input/keyboard/sun4i-keyboard.c#L136
> > 
> > Usually this will be correct in one go, after testing one can shuffle
> > key codes as needed (usually not needed) and/or remove unused entries.
> > 
> > With that said I do think that a sysfs file to see the actual voltages,
> > or a kernel parameter to printk them on keypress interrupt would be useful.
> > 
> > I guess the printk option would be better as it would show the actual
> > keypress value read, not some semi-random sample.
> 
> That wouldn't require that much code actually. Either using dev_dbg,
> or debugfs like Dmitry was suggesting would be two nice solutions I
> guess.

Given the stated purpose I'd say dev_dbg() and call it a day.

Thanks.

-- 
Dmitry