From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hans de Goede Subject: Re: [lm-sensors] [PATCH v2 2/5] input: sun4i-ts: Add support for temperature sensor Date: Tue, 31 Dec 2013 11:57:18 +0100 Message-ID: <52C2A30E.5010705@redhat.com> References: <1388156399-29677-1-git-send-email-hdegoede@redhat.com> <1388156399-29677-3-git-send-email-hdegoede@redhat.com> <20131227163419.GA20135@roeck-us.net> Reply-To: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Return-path: In-Reply-To: <20131227163419.GA20135-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> List-Post: , List-Help: , List-Archive: Sender: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org List-Subscribe: , List-Unsubscribe: , To: Guenter Roeck Cc: Dmitry Torokhov , LM Sensors , linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org, Corentin LABBE , linux-input-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Maxime Ripard , linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org List-Id: linux-input@vger.kernel.org Hi, On 12/27/2013 05:34 PM, Guenter Roeck wrote: > On Fri, Dec 27, 2013 at 03:59:56PM +0100, Hans de Goede wrote: >> The sun4i resisitive touchscreen controller also comes with a built-in >> temperature sensor. This commit adds support for it. >> >> This commit also introduces a new "ts-attached" device-tree property, >> when this is not set, the input part of the driver won't register. This way >> the internal temperature sensor can be used to measure the SoC temperature >> independent of there actually being a touchscreen attached to the controller. >> >> Signed-off-by: Hans de Goede > > Hi Hans, > > Almost good, except for a couple of nitpicks and one moe question. > Assuming you fix the nitpicks and the question is not a concern, > feel free to add > > Reviewed-by: Guenter Roeck > > Guenter > >> --- >> .../bindings/input/touchscreen/sun4i.txt | 6 + >> drivers/input/touchscreen/sun4i-ts.c | 129 ++++++++++++++++----- >> 2 files changed, 109 insertions(+), 26 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/input/touchscreen/sun4i.txt b/Documentation/devicetree/bindings/input/touchscreen/sun4i.txt >> index e45927e..1fffd11 100644 >> --- a/Documentation/devicetree/bindings/input/touchscreen/sun4i.txt >> +++ b/Documentation/devicetree/bindings/input/touchscreen/sun4i.txt >> @@ -6,10 +6,16 @@ Required properties: >> - reg: mmio address range of the chip >> - interrupts: interrupt to which the chip is connected >> >> +Optional properties: >> + - ts-attached: boolean set this to tell the driver that an actual touchscreen >> + is attached and that it should register an input device, >> + without this it only registers the builtin temperate sensor >> + >> Example: >> >> rtp: rtp@01c25000 { >> compatible = "allwinner,sun4i-ts"; >> reg = <0x01c25000 0x100>; >> interrupts = <29>; >> + ts-attached; >> }; >> diff --git a/drivers/input/touchscreen/sun4i-ts.c b/drivers/input/touchscreen/sun4i-ts.c >> index 10839d2..2d437f6 100644 >> --- a/drivers/input/touchscreen/sun4i-ts.c >> +++ b/drivers/input/touchscreen/sun4i-ts.c >> @@ -3,6 +3,9 @@ >> * >> * Copyright (C) 2013 - 2014 Hans de Goede >> * >> + * The hwmon parts are based on work by Corentin LABBE which is: >> + * Copyright (C) 2013 Corentin LABBE >> + * >> * This program is free software; you can redistribute it and/or modify >> * it under the terms of the GNU General Public License as published by >> * the Free Software Foundation; either version 2 of the License, or >> @@ -30,6 +33,7 @@ >> */ >> >> #include >> +#include >> #include >> #include >> #include >> @@ -106,6 +110,7 @@ struct sun4i_ts_data { >> void __iomem *base; >> unsigned int irq; >> bool ignore_fifo_data; >> + int temp_data; >> }; >> >> static irqreturn_t sun4i_ts_irq(int irq, void *dev_id) >> @@ -115,6 +120,12 @@ static irqreturn_t sun4i_ts_irq(int irq, void *dev_id) >> >> reg_val = readl(ts->base + TP_INT_FIFOS); >> >> + if (reg_val & TEMP_DATA_PENDING) >> + ts->temp_data = readl(ts->base + TEMP_DATA); > > Is this read guaranteed to return 0 in the upper bit, or in other words is it > guaranteed to never return 0xffffffff ? Otherwise there might be a problem with > the implicit conversion to a negative integer. Yes what is being read is a 12 bit unsigned value, with all the upper bits set to 0. > >> + >> + if (!ts->input) >> + goto leave; >> + >> if (reg_val & FIFO_DATA_PENDING) { >> x = readl(ts->base + TP_DATA); >> y = readl(ts->base + TP_DATA); >> @@ -140,6 +151,7 @@ static irqreturn_t sun4i_ts_irq(int irq, void *dev_id) >> input_sync(ts->input); >> } >> >> +leave: >> writel(reg_val, ts->base + TP_INT_FIFOS); >> >> return IRQ_HANDLED; >> @@ -149,9 +161,9 @@ static int sun4i_ts_open(struct input_dev *dev) >> { >> struct sun4i_ts_data *ts = input_get_drvdata(dev); >> >> - /* Flush, set trig level to 1, enable data and up irqs */ >> - writel(DATA_IRQ_EN(1) | FIFO_TRIG(1) |FIFO_FLUSH(1) | TP_UP_IRQ_EN(1), >> - ts->base + TP_INT_FIFOC); >> + /* Flush, set trig level to 1, enable temp, data and up irqs */ >> + writel(TEMP_IRQ_EN(1) | DATA_IRQ_EN(1) | FIFO_TRIG(1) | FIFO_FLUSH(1) | >> + TP_UP_IRQ_EN(1), ts->base + TP_INT_FIFOC); >> >> return 0; >> } >> @@ -160,40 +172,76 @@ static void sun4i_ts_close(struct input_dev *dev) >> { >> struct sun4i_ts_data *ts = input_get_drvdata(dev); >> >> - /* Deactivate all IRQs */ >> - writel(0, ts->base + TP_INT_FIFOC); >> + /* Deactivate all input IRQs */ >> + writel(TEMP_IRQ_EN(1), ts->base + TP_INT_FIFOC); >> synchronize_irq(ts->irq); >> } >> >> +static ssize_t show_temp(struct device *dev, struct device_attribute *devattr, >> + char *buf) >> +{ >> + struct sun4i_ts_data *ts = dev_get_drvdata(dev); >> + >> + /* No temp_data until the first irq */ >> + if (ts->temp_data == -1) >> + return -EAGAIN; >> + >> + return sprintf(buf, "%d\n", (ts->temp_data - 1447) * 100); >> +} >> + >> +static ssize_t show_temp_label(struct device *dev, >> + struct device_attribute *devattr, char *buf) >> +{ >> + return sprintf(buf, "SoC temperature\n"); >> +} >> + >> +static DEVICE_ATTR(temp1_input, S_IRUGO, show_temp, NULL); >> +static DEVICE_ATTR(temp1_label, S_IRUGO, show_temp_label, NULL); >> + >> +static struct attribute *sun4i_ts_attrs[] = { >> + &dev_attr_temp1_input.attr, >> + &dev_attr_temp1_label.attr, >> + NULL >> +}; >> +ATTRIBUTE_GROUPS(sun4i_ts); >> + >> static int sun4i_ts_probe(struct platform_device *pdev) >> { >> struct sun4i_ts_data *ts; >> struct device *dev = &pdev->dev; >> + struct device_node *np =dev->of_node; > > Missing space after '=' (checkpatch error). > >> + struct device *hwmon; >> int ret; >> + bool ts_attached; >> + >> + ts_attached = of_property_read_bool(np, "ts-attached"); >> >> ts = devm_kzalloc(dev, sizeof(struct sun4i_ts_data), GFP_KERNEL); >> if (!ts) >> return -ENOMEM; >> >> ts->dev = dev; >> - >> - ts->input = devm_input_allocate_device(dev); >> - if (!ts->input) >> - return -ENOMEM; >> - >> - ts->input->name = pdev->name; >> - ts->input->phys = "sun4i_ts/input0"; >> - ts->input->open = sun4i_ts_open; >> - ts->input->close = sun4i_ts_close; >> - ts->input->id.bustype = BUS_HOST; >> - ts->input->id.vendor = 0x0001; >> - ts->input->id.product = 0x0001; >> - ts->input->id.version = 0x0100; >> - ts->input->evbit[0] = BIT(EV_SYN) | BIT(EV_KEY) | BIT(EV_ABS); >> - set_bit(BTN_TOUCH, ts->input->keybit); >> - input_set_abs_params(ts->input, ABS_X, 0, 4095, 0, 0); >> - input_set_abs_params(ts->input, ABS_Y, 0, 4095, 0, 0); >> - input_set_drvdata(ts->input, ts); >> + ts->temp_data = -1; >> + >> + if (ts_attached) { >> + ts->input = devm_input_allocate_device(dev); >> + if (!ts->input) >> + return -ENOMEM; >> + >> + ts->input->name = pdev->name; >> + ts->input->phys = "sun4i_ts/input0"; >> + ts->input->open = sun4i_ts_open; >> + ts->input->close = sun4i_ts_close; >> + ts->input->id.bustype = BUS_HOST; >> + ts->input->id.vendor = 0x0001; >> + ts->input->id.product = 0x0001; >> + ts->input->id.version = 0x0100; >> + ts->input->evbit[0] = BIT(EV_SYN) | BIT(EV_KEY) | BIT(EV_ABS); >> + set_bit(BTN_TOUCH, ts->input->keybit); >> + input_set_abs_params(ts->input, ABS_X, 0, 4095, 0, 0); >> + input_set_abs_params(ts->input, ABS_Y, 0, 4095, 0, 0); >> + input_set_drvdata(ts->input, ts); >> + } >> >> ts->base = devm_ioremap_resource(dev, >> platform_get_resource(pdev, IORESOURCE_MEM, 0)); >> @@ -232,14 +280,42 @@ static int sun4i_ts_probe(struct platform_device *pdev) >> writel(STYLUS_UP_DEBOUN(5) | STYLUS_UP_DEBOUN_EN(1) | TP_MODE_EN(1), >> ts->base + TP_CTRL1); >> >> - ret = input_register_device(ts->input); >> - if (ret) >> - return ret; >> + hwmon = devm_hwmon_device_register_with_groups(ts->dev, "sun4i_ts", >> + ts, sun4i_ts_groups); >> + if (IS_ERR(hwmon)) { >> + return PTR_ERR(hwmon); >> + } > > { } are not needed here (checkpatch warning). Oops, should have run checkpatch on v2 too, my bad. Both fixed now. Regards, Hans