[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <7f0a4cb041354801b732c744ad695495@de.bosch.com>
Date: Thu, 17 May 2018 20:10:27 +0000
From: "Jonas Mark (BT-FIR/ENG1)" <Mark.Jonas@...bosch.com>
To: Dmitry Torokhov <dmitry.torokhov@...il.com>
CC: Andy Shevchenko <andy.shevchenko@...il.com>,
Rob Herring <robh+dt@...nel.org>,
Mark Rutland <mark.rutland@....com>,
linux-input <linux-input@...r.kernel.org>,
devicetree <devicetree@...r.kernel.org>,
"Linux Kernel Mailing List" <linux-kernel@...r.kernel.org>,
Heiko Schocher <hs@...x.de>,
"ZHU Yi (BT-FIR/ENG1-Zhu)" <Yi.Zhu5@...bosch.com>,
"Jonas Mark (BT-FIR/ENG1)" <Mark.Jonas@...bosch.com>
Subject: AW: [PATCH v3] Input: add bu21029 touch driver
Hello Dmitry,
> > > > +static void bu21029_stop_chip(struct input_dev *dev)
> > > > +{
> > > > + struct bu21029_ts_data *bu21029 = input_get_drvdata(dev);
> > > > +
> > > > + disable_irq(bu21029->client->irq);
> > > > + del_timer_sync(&bu21029->timer);
> > > > +
> > > > + /* put chip into reset */
> > > > + gpiod_set_value_cansleep(bu21029->reset_gpios, 1);
> > >
> > > > + udelay(STOP_DELAY_US);
> > >
> > > udelay() ?!
> > >
> > > > +}
> >
> > According to the datasheet disabling the chip will take 30 microseconds.
> > In the defines we added a buffer of 20 microseconds and thus
> > STOP_DELAY_US is 50. The function guarantees that the chip is stopped
> > before it returns.
> >
> > We think that it is ok to use udelay() here because in normal operation
> > the chip is not stopped. It is only stopped when loading or unloading
> > the driver, or when the system suspends.
> >
> > We would like to keep it like it is.
>
> The issue is not with having delay here, but the kind of delay you are
> using: udelay makes CPU spin for given amount of time; you really want
> msleep() or usleep_range() here.
Understood and changed.
> > > > +static int bu21029_start_chip(struct input_dev *dev)
> > > > +{
> > >
> > > > + u16 hwid;
> > > > +
> > > > + /* take chip out of reset */
> > > > + gpiod_set_value_cansleep(bu21029->reset_gpios, 0);
> > >
> > > > + mdelay(START_DELAY_MS);
> > >
> > > mdelay()?!
>
> Same here - replace with msleep().
Replaced.
> > > Instead...
> > >
> > > > +static int bu21029_suspend(struct device *dev)
> > >
> > > ...use __maby_unused annotation.
> > >
> > > > +static int bu21029_resume(struct device *dev)
> > >
> > > Ditto.
> >
> > OK, added.
>
> You also need to drop #ifdef CONFIG_SLEEP. That's the point: we want to
> reduce amount of conditionally compiled code and use __maybe_unused to
> shut off compiler warning about some functions not used in certain
> configurations. We rely on linker to eliminate dead functions from
> executable.
Done.
Greetings,
Mark
Mark Jonas
Building Technologies, Panel Software Fire (BT-FIR/ENG1)
Bosch Sicherheitssysteme GmbH | Postfach 11 11 | 85626 Grasbrunn | GERMANY | www.boschsecurity.com
Sitz: Stuttgart, Registergericht: Amtsgericht Stuttgart HRB 23118
Aufsichtsratsvorsitzender: Stefan Hartung; Geschäftsführung: Gert van Iperen, Andreas Bartz, Thomas Quante, Bernhard Schuster
Powered by blists - more mailing lists