| Message ID | 1388156399-29677-3-git-send-email-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <hdegoede@redhat.com> 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 <linux@roeck-us.net> 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 <hdegoede@redhat.com> > * > + * The hwmon parts are based on work by Corentin LABBE which is: > + * Copyright (C) 2013 Corentin LABBE <clabbe.montjoie@gmail.com> > + * > * 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 <linux/err.h> > +#include <linux/hwmon.h> > #include <linux/init.h> > #include <linux/input.h> > #include <linux/interrupt.h> > @@ -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. > + > + 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). > + > + 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); > + 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 > > > _______________________________________________ > lm-sensors mailing list > lm-sensors@lm-sensors.org > http://lists.lm-sensors.org/mailman/listinfo/lm-sensors >
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(...); > 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? Then you could do away with remove() method altogether. > } > > +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. > + 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.
On Fri, Dec 27, 2013 at 03:59:56PM +0100, Hans de Goede wrote: > > +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 I believe the guidance is to avoid describing kernel behavior in DT property so I think it should say: - ts-attached: boolean indicating that an actual touchscreen is attached to the controller. Thanks.
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 <hdegoede@redhat.com> > > 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 <linux@roeck-us.net> > > 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 <hdegoede@redhat.com> >> * >> + * The hwmon parts are based on work by Corentin LABBE which is: >> + * Copyright (C) 2013 Corentin LABBE <clabbe.montjoie@gmail.com> >> + * >> * 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 <linux/err.h> >> +#include <linux/hwmon.h> >> #include <linux/init.h> >> #include <linux/input.h> >> #include <linux/interrupt.h> >> @@ -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
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
Hi, On 12/28/2013 02:13 AM, Dmitry Torokhov wrote: > On Fri, Dec 27, 2013 at 03:59:56PM +0100, Hans de Goede wrote: >> >> +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 > > I believe the guidance is to avoid describing kernel behavior in DT > property so I think it should say: > > - ts-attached: boolean indicating that an actual touchscreen is > attached to the controller. Will also fix for v3. Regards, Hans
Hi, On Fri, Dec 27, 2013 at 05:13:04PM -0800, Dmitry Torokhov wrote: > On Fri, Dec 27, 2013 at 03:59:56PM +0100, Hans de Goede wrote: > > > > +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 > > I believe the guidance is to avoid describing kernel behavior in DT > property so I think it should say: > > - ts-attached: boolean indicating that an actual touchscreen is > attached to the controller. Custom properties are also supposed to be prefixed by the vendor prefix. Maxime
Hi, On 12/31/2013 02:00 PM, Maxime Ripard wrote: > Hi, > > On Fri, Dec 27, 2013 at 05:13:04PM -0800, Dmitry Torokhov wrote: >> On Fri, Dec 27, 2013 at 03:59:56PM +0100, Hans de Goede wrote: >>> >>> +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 >> >> I believe the guidance is to avoid describing kernel behavior in DT >> property so I think it should say: >> >> - ts-attached: boolean indicating that an actual touchscreen is >> attached to the controller. > > Custom properties are also supposed to be prefixed by the vendor prefix. Will fix for v3. Regards, Hans
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 <hdegoede@redhat.com> * + * The hwmon parts are based on work by Corentin LABBE which is: + * Copyright (C) 2013 Corentin LABBE <clabbe.montjoie@gmail.com> + * * 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 <linux/err.h> +#include <linux/hwmon.h> #include <linux/init.h> #include <linux/input.h> #include <linux/interrupt.h> @@ -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); + + 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; + 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); + 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);
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 <hdegoede@redhat.com> --- .../bindings/input/touchscreen/sun4i.txt | 6 + drivers/input/touchscreen/sun4i-ts.c | 129 ++++++++++++++++----- 2 files changed, 109 insertions(+), 26 deletions(-)