Re: [PATCH] bma023: Support for Bosch BMA023, BMA150 and SMB380 accelerometers

On Monday 14 February 2011 05:57 PM, Alan Cox wrote:

On Mon, 14 Feb 2011 17:20:13 +0530
Shubhrajyoti[off-list ref]  wrote:

quoted

Hello,
Some comments inlined.

Please trim stuff when you add comments inline like that. Firstly it
saves bandwidth but more importantly it makes them easier to find !

Yes, will take care henceforth.

quoted

quoted

   obj-$(CONFIG_INPUT_SGI_BTNS)		+= sgi_btns.o
+obj-$(CONFIG_INPUT_BMA023)			+= bma023.o

May consider alphabetical order

oops - yes good point. I didn't check that when merging.

quoted

quoted

+ *	bma023_xyz_read_reg	-	read the axes values
+ *	@reg: register being updated
+ *	@val: value to write

Didn’t understand this

That would be because the comments need updating. Will fix that now.

quoted

quoted

+	struct bma023_sensor *sensor = dev_get_drvdata(dev);
+
+	pm_runtime_get_sync(dev);

Not a comment really , what does  pm_runtime_get_sync(dev); do ?

We need to be sure it's powered up when we read it via sysfs.

OK , so it powers up the device.thanks

quoted

quoted

+	pm_runtime_put(dev);

Also did you  pm_runtime_put(dev); here? Didnt understand this did you
mean sync?

No - as we don't care if it powers off immediately just that it gets
around to it.

quoted

quoted

+
+	return sprintf(buf, "(%d,%d,%d)\n",
+		sensor->data.x, sensor->data.y, sensor->data.z);
+}
+static DEVICE_ATTR(accel_data, S_IRUGO, bma023_show_xyz, NULL);

Since it is reported as an input device do you need the sysfs entry?

Yes - the device is usable both as an input device (think about tilt
based games) and for other purposes like orientation or drop detection.
The sysfs interface covers the latter.

quoted

quoted

+ *	Add an input device to the sensor. This will be used to report
+ *	events from the sensor itself.
+ */
+static int bma023_register_input_device(struct bma023_sensor *sensor)

this could be devinit as well as called only from probe.

Looks like it could will check

quoted

quoted

+	if (idev)
+		input_free_device(idev);

This check is there in the function

Yep

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html