| Message ID | 1422291516-24895-1-git-send-email-plaes@plaes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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.
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
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
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 :)
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
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.
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.
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
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
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.
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);