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.
Thanks.
--
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
