Re: [PATCH] Add generic GPIO-tilt driver

Am Mittwoch 16 November 2011, 17:48:07 schrieb Dmitry Torokhov:

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

quoted

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.

I don't claim to grasp every bit the initdata routines do :-) . But I was 
under the impression that allowing people to make it initdata if necessary and 
not building stuff as modules was the correct way.

Last time I submitted a regulator patch Mark Brown wrote:

Monday 29 August 2011, 11:18:07 Mark Brown wrote
[...]

This also fixes the use of platform data after probe - platform data
should be able to be marked as initdata which may mean that the kernel
discards it after boot.

The bq24022 driver in question can be build as a module.

And there also seem to exist patches whith the sole purpose of allowing 
platform_data to be initdata if neccessary [1] (the max8903-charger can also 
be built as module, but its platformdata should not be initdata then).

I'm ok either way, so in the end it's your call on how it should be handled.

Heiko


[1] http://comments.gmane.org/gmane.linux.kernel/1161697
--
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