From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hans de Goede Subject: Re: [PATCH v2 2/5] input: sun4i-ts: Add support for temperature sensor Date: Tue, 31 Dec 2013 12:10:31 +0100 Message-ID: <52C2A627.4070208@redhat.com> References: <1388156399-29677-1-git-send-email-hdegoede@redhat.com> <1388156399-29677-3-git-send-email-hdegoede@redhat.com> <20131228011007.GA14188@core.coreip.homeip.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: <20131228011007.GA14188-WlK9ik9hQGAhIp7JRqBPierSzoNAToWh@public.gmane.org> List-Post: , List-Help: , List-Archive: Sender: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org List-Subscribe: , List-Unsubscribe: , To: Dmitry Torokhov Cc: linux-input-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, LM Sensors , Maxime Ripard , Corentin LABBE , linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org List-Id: linux-input@vger.kernel.org Hi, On 12/28/2013 02:10 AM, Dmitry Torokhov wrote: > Hi Hans, > > On Fri, Dec 27, 2013 at 03:59:56PM +0100, Hans de Goede wrote: >> + >> + 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: > > Can we please not introduce gotos where they are not needed? If you > concerned about extra indent just split touchscreen handling into a > separate function and then do > > if (ts->input) > sun4i_ts_handle_touchscreen_data(...); > Fixed. > >> 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); > > Hmm, it would be nice we touchscreen methods only affected touchscreen > functionality. Can you read current state and adjust it as needed > instead of clobbering it? I could do a read-modify-write of TP_INT_FIFOC here, but I already considered that and I decided not to do it because ... > Then you could do away with remove() method altogether. No it will still be needed to clear the TEMP_IRQ_EN bit in TP_INT_FIFOC. And it will still need to either explicitly deregister the input_dev, or take the input_dev mutex to avoid the modification of TP_INT_FIFOC in _remove racing with the read-modify-write in _close. So I decided to keep this kiss, and simply force _close to run before _remove clears the TEMP_IRQ_EN bit by doing an explicit unregister before this. > >> } >> >> +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; >> + 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); >> + } >> + >> + writel(TEMP_IRQ_EN(1), ts->base + TP_INT_FIFOC); >> + >> + if (ts_attached) { >> + ret = input_register_device(ts->input); >> + if (ret) { >> + writel(0, ts->base + TP_INT_FIFOC); >> + synchronize_irq(ts->irq); > > Given that we now can't avoid freeing irq before freeing ts memory or > input device I think you can do away with synchronize_irq() here and > elsewhere as freeing irq is guaranteed to wait for current interrupt to > complete. Ah yes, removed all calls to synchronize_irq. I'll run some tests to ensure I did not accidentally break anything and then I'll send a v3. > >> + return ret; >> + } >> + } >> >> platform_set_drvdata(pdev, ts); >> return 0; >> } >> >> +static int sun4i_ts_remove(struct platform_device *pdev) >> +{ >> + struct sun4i_ts_data *ts = platform_get_drvdata(pdev); >> + >> + /* Explicit unregister to avoid open/close changing the imask later */ >> + if (ts->input) >> + input_unregister_device(ts->input); >> + >> + /* Deactivate all IRQs */ >> + writel(0, ts->base + TP_INT_FIFOC); >> + synchronize_irq(ts->irq); >> + >> + return 0; >> +} >> + >> static const struct of_device_id sun4i_ts_of_match[] = { >> { .compatible = "allwinner,sun4i-ts", }, >> { /* sentinel */ } >> @@ -253,6 +329,7 @@ static struct platform_driver sun4i_ts_driver = { >> .of_match_table = of_match_ptr(sun4i_ts_of_match), >> }, >> .probe = sun4i_ts_probe, >> + .remove = sun4i_ts_remove, >> }; >> >> module_platform_driver(sun4i_ts_driver); >> -- >> 1.8.4.2 >> > > Thanks. > Regards, Hans