On Thu, Nov 19, 2009 at 11:34:40AM +0000, Ben Dooks wrote:

Dmitry Torokhov wrote:

quoted

quoted

+
+static char *s3c2410ts_name = "s3c2410 TouchScreen";

Btw, "static char s3c2410ts_name[]" will save you a pointer. But, it
looks like it is used only once, in probe(), so just put it there.

quoted

quoted

+
+/* Per-touchscreen data. */
+
+/**
+ * struct s3c2410ts - driver touchscreen state.
+ * @client: The ADC client we registered with the core driver.
+ * @dev: The device we are bound to.
+ * @input: The input device we registered with the input subsystem.
+ * @clock: The clock for the adc.
+ * @io: Pointer to the IO base.
+ * @xp: The accumulated X position data.
+ * @yp: The accumulated Y position data.
+ * @irq_tc: The interrupt number for pen up/down interrupt
+ * @count: The number of samples collected.
+ * @shift: The log2 of the maximum count to read in one go.
+ */

These sructures are driver-internal and so don't need to be kernel-doc-ed.
Same goes for the driver-private functions.

I like having the documentation, and I would much prefer to leave it
in as useful.

Ah, I wasn't requiesting to remove the documentation, I was just saying
that since these data structures and fucntions are driver-provate they
don't need to use kernel-doc style.

quoted

quoted

+
+		input_report_key(ts.input, BTN_TOUCH, 1);
+		input_report_abs(ts.input, ABS_PRESSURE, 1);

No fake pressure events please, BTN_TOUCH should be enough.

I'd have to check, IIRC tslib needs these to function properly.

Just update your tslib, the issue was fixed there last November.

quoted

quoted

+{
+	struct s3c2410_ts_mach_info *info;
+	struct device *dev = &pdev->dev;
+	struct input_dev *input_dev;
+	struct resource *res;
+	int ret = -EINVAL;

Can we call it "error" (since that's what you use it for).

Is it really necessary to change this?

No, it is my personal preference/style. In case when it is used like:

	var = blah();
	if (var)
		goto err_unblah;

	return 0;

err_unblah:
	unblah();
	return var;
}

I prefer that var called 'error'. If the value is returned on both error
and success paths then I call it 'ret', 'retval', etc.

But no, if you prefer 'ret' that is fine too.

quoted

quoted

+	.suspend	= s3c2410ts_suspend,
+	.resume		= s3c2410ts_resume,

Switch to pm_ops.

ok, will do. may as well remove the #ifdef CONFIG_PM
for such small amount of code too.

As long as it does not break when CONFIG_PM is not set...

-- 
Dmitry