Re: [PATCH v2 2/5] input: sun4i-ts: Add support for temperature sensor

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors




[Index of Archives]     [Linux Kernel]     [Linux Hardware Monitoring]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux