From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hans de Goede Subject: Re: [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver Date: Tue, 27 Jan 2015 10:03:35 +0100 Message-ID: <54C75467.7010909@redhat.com> References: <1422291516-24895-1-git-send-email-plaes@plaes.org> <54C6955D.403@redhat.com> <20150126220622.GA16941@dtor-ws> Reply-To: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Return-path: In-Reply-To: <20150126220622.GA16941@dtor-ws> List-Post: , List-Help: , List-Archive: , List-Unsubscribe: , To: Dmitry Torokhov Cc: Priit Laes , linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org, Maxime Ripard , ABI/API , "moderated list:ARM/Allwinner A1X..." , open list , "open list:SUN4I LOW RES ADC..." List-Id: linux-api@vger.kernel.org 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 >>> +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