Re: [PATCH] Add generic GPIO-tilt driver

On Wed, Nov 16, 2011 at 12:02:07PM +0100, Heiko Stübner wrote:

Hi Dmitry,

Am Mittwoch 16 November 2011, 08:18:22 schrieb Dmitry Torokhov:

quoted

Hi Heiko,

On Fri, Oct 21, 2011 at 10:43:47AM +0200, Heiko Stübner wrote:

quoted

There exist tilt switches that simply report their tilt-state via some
gpios.

The number and orientation of their axes can vary depending on the switch
used and the build of the device. Also two or more one-axis switches
could be combined to provide multi-dimensional orientation.

One example of a device using such a switch is the family of Qisda
ebook readers, where the switch provides information about the
landscape / portrait orientation of the device. The example in
Documentation/input/gpio-tilt.txt documents exactly this one-axis device.

Looks very nice, just a few comments below.

thanks for your review, comments below

[...]

quoted

quoted

+static void gpio_tilt_polled_poll(struct input_polled_dev *dev)
+{
+	struct gpio_tilt_polled_dev *bdev = dev->private;

Call it tdev or tilt or tilt_dev or something? bdev is not very fitting
here.

ok, will change this

[...]

quoted

quoted

+static int __devinit gpio_tilt_polled_probe(struct platform_device
*pdev) +{
+	struct gpio_tilt_platform_data *pdata = pdev->dev.platform_data;
+	struct device *dev = &pdev->dev;
+	struct gpio_tilt_polled_dev *bdev;
+	struct input_polled_dev *poll_dev;
+	struct input_dev *input;
+	int error, i;
+
+	if (!pdata || !pdata->poll_interval)
+		return -EINVAL;
+
+	bdev = kzalloc(sizeof(struct gpio_tilt_polled_dev), GFP_KERNEL);
+	if (!bdev) {
+		dev_err(dev, "no memory for private data\n");
+		return -ENOMEM;
+	}
+
+	bdev->gpios = kmemdup(pdata->gpios,
+				 pdata->nr_gpios * sizeof(struct gpio),
+				 GFP_KERNEL);
+	if (bdev->gpios == NULL) {
+		dev_err(&pdev->dev, "Failed to allocate gpio data\n");
+		error = -ENOMEM;
+		goto err_free_bdev;
+	}
+	bdev->nr_gpios = pdata->nr_gpios;

Do you actually need to do this? Can't you just store the pointer to
the platform data and use it instead? Same goes for the rest of items
you are cloning.

As I understand it from previous patches, platform data should be markable as 
initdata which means the kernel discards it after boot. Therefore it cannot be 

Absolutely not. Consider what happens if your driver is a module and is
loaded after boot is complete. Or you unload and reload it;
platform_data has to stay there, it  later must not be marked initdata
and freed.

Thanks.

-- 
Dmitry
--
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