* [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver
@ 2015-01-26 16:58 ` Priit Laes
0 siblings, 0 replies; 32+ messages in thread
From: Priit Laes @ 2015-01-26 16:58 UTC (permalink / raw)
To: linux-arm-kernel
---
.../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
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver
@ 2015-01-26 16:58 ` Priit Laes
0 siblings, 0 replies; 32+ messages in thread
From: Priit Laes @ 2015-01-26 16:58 UTC (permalink / raw)
Cc: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Priit Laes, Maxime Ripard,
Hans de Goede, Dmitry Torokhov, open list:ABI/API,
moderated list:ARM/Allwinner A1X...,
open list, open list:SUN4I LOW RES ADC...
---
.../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
^ permalink raw reply related [flat|nested] 32+ messages in thread[parent not found: <1422291516-24895-1-git-send-email-plaes-q/aMd4JkU83YtjvyW6yDsg@public.gmane.org>]
* Re: [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver
2015-01-26 16:58 ` Priit Laes
@ 2015-01-26 19:28 ` Hans de Goede
-1 siblings, 0 replies; 32+ messages in thread
From: Hans de Goede @ 2015-01-26 19:28 UTC (permalink / raw)
To: Priit Laes
Cc: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Maxime Ripard,
Dmitry Torokhov, ABI/API, moderated list:ARM/Allwinner A1X...,
open list, open list:SUN4I LOW RES ADC...
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.
^ permalink raw reply [flat|nested] 32+ messages in thread* [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver
@ 2015-01-26 19:28 ` Hans de Goede
0 siblings, 0 replies; 32+ messages in thread
From: Hans de Goede @ 2015-01-26 19:28 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 32+ messages in thread[parent not found: <54C6955D.403-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]
* Re: [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver
2015-01-26 19:28 ` Hans de Goede
@ 2015-01-26 22:06 ` Dmitry Torokhov
-1 siblings, 0 replies; 32+ messages in thread
From: Dmitry Torokhov @ 2015-01-26 22:06 UTC (permalink / raw)
To: Hans de Goede
Cc: Priit Laes, linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Maxime Ripard,
ABI/API, moderated list:ARM/Allwinner A1X...,
open list, open list:SUN4I LOW RES ADC...
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
^ permalink raw reply [flat|nested] 32+ messages in thread* [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver
@ 2015-01-26 22:06 ` Dmitry Torokhov
0 siblings, 0 replies; 32+ messages in thread
From: Dmitry Torokhov @ 2015-01-26 22:06 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver
2015-01-26 22:06 ` Dmitry Torokhov
@ 2015-01-27 9:03 ` Hans de Goede
-1 siblings, 0 replies; 32+ messages in thread
From: Hans de Goede @ 2015-01-27 9:03 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Priit Laes, linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Maxime Ripard,
ABI/API, moderated list:ARM/Allwinner A1X...,
open list, open list:SUN4I LOW RES ADC...
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
^ permalink raw reply [flat|nested] 32+ messages in thread* [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver
@ 2015-01-27 9:03 ` Hans de Goede
0 siblings, 0 replies; 32+ messages in thread
From: Hans de Goede @ 2015-01-27 9:03 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 32+ messages in thread[parent not found: <54C75467.7010909-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]
* Re: [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver
2015-01-27 9:03 ` Hans de Goede
@ 2015-01-27 19:31 ` Dmitry Torokhov
-1 siblings, 0 replies; 32+ messages in thread
From: Dmitry Torokhov @ 2015-01-27 19:31 UTC (permalink / raw)
To: Hans de Goede
Cc: Priit Laes, linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Maxime Ripard,
ABI/API, moderated list:ARM/Allwinner A1X...,
open list, open list:SUN4I LOW RES ADC...
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
^ permalink raw reply [flat|nested] 32+ messages in thread* [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver
@ 2015-01-27 19:31 ` Dmitry Torokhov
0 siblings, 0 replies; 32+ messages in thread
From: Dmitry Torokhov @ 2015-01-27 19:31 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver
[not found] ` <1422291516-24895-1-git-send-email-plaes-q/aMd4JkU83YtjvyW6yDsg@public.gmane.org>
2015-01-26 19:28 ` Hans de Goede
@ 2015-01-27 9:18 ` Maxime Ripard
0 siblings, 0 replies; 32+ messages in thread
From: Maxime Ripard @ 2015-01-27 9:18 UTC (permalink / raw)
To: Priit Laes
Cc: linux-sunxi, Hans de Goede, Dmitry Torokhov, open list:ABI/API,
moderated list:ARM/Allwinner A1X...,
open list, open list:SUN4I LOW RES ADC...
[-- 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 --]
^ permalink raw reply [flat|nested] 32+ messages in thread* [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver
@ 2015-01-27 9:18 ` Maxime Ripard
0 siblings, 0 replies; 32+ messages in thread
From: Maxime Ripard @ 2015-01-27 9:18 UTC (permalink / raw)
To: linux-arm-kernel
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>
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver
@ 2015-01-27 9:18 ` Maxime Ripard
0 siblings, 0 replies; 32+ messages in thread
From: Maxime Ripard @ 2015-01-27 9:18 UTC (permalink / raw)
To: Priit Laes
Cc: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Hans de Goede,
Dmitry Torokhov, open list:ABI/API,
moderated list:ARM/Allwinner A1X...,
open list, open list:SUN4I LOW RES ADC...
[-- 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 --]
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [linux-sunxi] Re: [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver
2015-01-27 9:18 ` Maxime Ripard
(?)
@ 2015-01-27 9:49 ` Priit Laes
-1 siblings, 0 replies; 32+ messages in thread
From: Priit Laes @ 2015-01-27 9:49 UTC (permalink / raw)
To: Maxime Ripard
Cc: linux-sunxi, Hans de Goede, Dmitry Torokhov, linux-arm-kernel,
linux-kernel, linux-input
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 :)
^ permalink raw reply [flat|nested] 32+ messages in thread
* [linux-sunxi] Re: [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver
@ 2015-01-27 9:49 ` Priit Laes
0 siblings, 0 replies; 32+ messages in thread
From: Priit Laes @ 2015-01-27 9:49 UTC (permalink / raw)
To: linux-arm-kernel
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 :)
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: Re: [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver
@ 2015-01-27 9:49 ` Priit Laes
0 siblings, 0 replies; 32+ messages in thread
From: Priit Laes @ 2015-01-27 9:49 UTC (permalink / raw)
To: Maxime Ripard
Cc: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Hans de Goede,
Dmitry Torokhov,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA
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.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [linux-sunxi] Re: [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver
@ 2015-01-27 10:52 ` Hans de Goede
0 siblings, 0 replies; 32+ messages in thread
From: Hans de Goede @ 2015-01-27 10:52 UTC (permalink / raw)
To: Priit Laes, Maxime Ripard
Cc: linux-sunxi, Dmitry Torokhov, linux-arm-kernel, linux-kernel,
linux-input
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* [linux-sunxi] Re: [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver
@ 2015-01-27 10:52 ` Hans de Goede
0 siblings, 0 replies; 32+ messages in thread
From: Hans de Goede @ 2015-01-27 10:52 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: Re: [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver
@ 2015-01-27 10:52 ` Hans de Goede
0 siblings, 0 replies; 32+ messages in thread
From: Hans de Goede @ 2015-01-27 10:52 UTC (permalink / raw)
To: Priit Laes, Maxime Ripard
Cc: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Dmitry Torokhov,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [linux-sunxi] Re: [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver
@ 2015-01-27 19:44 ` Maxime Ripard
0 siblings, 0 replies; 32+ messages in thread
From: Maxime Ripard @ 2015-01-27 19:44 UTC (permalink / raw)
To: Hans de Goede
Cc: Priit Laes, linux-sunxi, Dmitry Torokhov, linux-arm-kernel,
linux-kernel, linux-input
[-- 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 --]
^ permalink raw reply [flat|nested] 32+ messages in thread
* [linux-sunxi] Re: [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver
@ 2015-01-27 19:44 ` Maxime Ripard
0 siblings, 0 replies; 32+ messages in thread
From: Maxime Ripard @ 2015-01-27 19:44 UTC (permalink / raw)
To: linux-arm-kernel
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>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: Re: [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver
@ 2015-01-27 19:44 ` Maxime Ripard
0 siblings, 0 replies; 32+ messages in thread
From: Maxime Ripard @ 2015-01-27 19:44 UTC (permalink / raw)
To: Hans de Goede
Cc: Priit Laes, linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Dmitry Torokhov,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA
[-- 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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [linux-sunxi] Re: [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver
2015-01-27 19:44 ` Maxime Ripard
(?)
@ 2015-01-28 1:15 ` Dmitry Torokhov
-1 siblings, 0 replies; 32+ messages in thread
From: Dmitry Torokhov @ 2015-01-28 1:15 UTC (permalink / raw)
To: Maxime Ripard
Cc: Hans de Goede, Priit Laes, linux-sunxi, linux-arm-kernel,
linux-kernel, linux-input
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* [linux-sunxi] Re: [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver
@ 2015-01-28 1:15 ` Dmitry Torokhov
0 siblings, 0 replies; 32+ messages in thread
From: Dmitry Torokhov @ 2015-01-28 1:15 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: Re: [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver
@ 2015-01-28 1:15 ` Dmitry Torokhov
0 siblings, 0 replies; 32+ messages in thread
From: Dmitry Torokhov @ 2015-01-28 1:15 UTC (permalink / raw)
To: Maxime Ripard
Cc: Hans de Goede, Priit Laes, linux-sunxi-/JYPxA39Uh5TLH3MbocFFw,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [linux-sunxi] Re: [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver
@ 2015-01-27 19:34 ` Dmitry Torokhov
0 siblings, 0 replies; 32+ messages in thread
From: Dmitry Torokhov @ 2015-01-27 19:34 UTC (permalink / raw)
To: Priit Laes
Cc: Maxime Ripard, linux-sunxi, Hans de Goede, linux-arm-kernel,
linux-kernel, linux-input
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* [linux-sunxi] Re: [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver
@ 2015-01-27 19:34 ` Dmitry Torokhov
0 siblings, 0 replies; 32+ messages in thread
From: Dmitry Torokhov @ 2015-01-27 19:34 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: Re: [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver
@ 2015-01-27 19:34 ` Dmitry Torokhov
0 siblings, 0 replies; 32+ messages in thread
From: Dmitry Torokhov @ 2015-01-27 19:34 UTC (permalink / raw)
To: Priit Laes
Cc: Maxime Ripard, linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Hans de Goede,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [linux-sunxi] Re: [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver
@ 2015-01-27 19:40 ` Maxime Ripard
0 siblings, 0 replies; 32+ messages in thread
From: Maxime Ripard @ 2015-01-27 19:40 UTC (permalink / raw)
To: Priit Laes
Cc: linux-sunxi, Hans de Goede, Dmitry Torokhov, linux-arm-kernel,
linux-kernel, linux-input
[-- 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 --]
^ permalink raw reply [flat|nested] 32+ messages in thread
* [linux-sunxi] Re: [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver
@ 2015-01-27 19:40 ` Maxime Ripard
0 siblings, 0 replies; 32+ messages in thread
From: Maxime Ripard @ 2015-01-27 19:40 UTC (permalink / raw)
To: linux-arm-kernel
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>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: Re: [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver
@ 2015-01-27 19:40 ` Maxime Ripard
0 siblings, 0 replies; 32+ messages in thread
From: Maxime Ripard @ 2015-01-27 19:40 UTC (permalink / raw)
To: Priit Laes
Cc: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Hans de Goede,
Dmitry Torokhov,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA
[-- 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
^ permalink raw reply [flat|nested] 32+ messages in thread