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

[Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.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