Re: [PATCH] Add generic GPIO-tilt driver

["Heiko Stübner" <heiko@sntech.de>]

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:
> > Hi Dmitry,
> > 
> > Am Mittwoch 16 November 2011, 08:18:22 schrieb Dmitry Torokhov:
> > > Hi Heiko,
> > > 
> > > On Fri, Oct 21, 2011 at 10:43:47AM +0200, Heiko Stübner wrote:
> > > > 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
> > 
> > [...]
> > 
> > > > +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
> > 
> > [...]
> > 
> > > > +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