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

[Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

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.