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

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

[Priit Laes]

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

diff --git a/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
new file mode 100644
index 0000000..e4e6448
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
@@ -0,0 +1,4 @@
+What:		/sys/class/input/input(x)/device/voltage
+Date:		February 2015
+Contact:	Priit Laes <plaes-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

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

[Hans de Goede]

Hi,

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

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

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

Why this identation change ?

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

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

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

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


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

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

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


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

Regards,

Hans


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

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

[Dmitry Torokhov]

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

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

-- 
Dmitry

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

[Hans de Goede]

Hi,

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

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

Regards,

Hans

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

[Dmitry Torokhov]

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

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

Thanks.

-- 
Dmitry

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

[Maxime Ripard]

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

Hi,

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

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

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

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

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

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

Maxime

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

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