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
used directly in the drivers after probe and necessary informations must be
copied.
[...]
> > + /* report initial state of the buttons */
> > + bdev->last_state = -1;
> > + bdev->count = bdev->threshold;
> > + gpio_tilt_polled_poll(poll_dev);
>
> This should probably be in gpio_tilt_polled_open().
ok
Heiko
--
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
