Re: [PATCH] Add generic GPIO-tilt driver

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

On Wednesday, November 16, 2011 11:56:43 AM Heiko Stübner wrote:
> Am Mittwoch 16 November 2011, 20:18:00 schrieben Sie:
> > On Wed, Nov 16, 2011 at 07:28:35PM +0100, Heiko Stübner wrote:
> > > 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.
> > 
> > Huh? Forcing non-modular build is "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.
> > 
> > Quoting Russell King "That's totally buggered, and that's putting it
> > kindly.":
> > 
> > http://www.spinics.net/lists/linux-input/msg16335.html
> :
> :-), still strange to see two differing opinions

No, one is just wrong. I guess we need to revert patch you mentioned.
Unless we prohibit modules and hotplug platform data should stay.

> 
> But I will get rid of the cloned platform data.
> 

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