Date:   Wed, 16 May 2018 13:24:24 +0000
From:   "Jonas Mark (BT-FIR/ENG1)" <Mark.Jonas@...bosch.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
CC:     Dmitry Torokhov <dmitry.torokhov@...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 Andy,

> > Add Rohm BU21029 resistive touch panel controller support with I2C
> > interface.
> 
> > +#include <linux/of.h>
> 
> This becomes redundant (see below).

Removed.

> > +#define STOP_DELAY_US  50L
> > +#define START_DELAY_MS 2L
> > +#define BUF_LEN        8L
> 
> No need to use L for such small numbers. Integer promotion is a part
> of C standard.

OK.

> > +#define SCALE_12BIT    (1 << 12)
> > +#define MAX_12BIT      ((1 << 12) - 1)
> 
> BIT(12)
> GENMASK(11, 0)

We are not convinced that we should use BIT() and GENMASK() here.

The reason is that SCALE_12BIT is actually not used as a bit but as an
input value for DIV_ROUND_CLOSEST. We think that the BIT() macro will
hide the meaning of the value.

MAX_12BIT is also a value and not a bit mask. Thus, we also think that
using the GENMASK() macro will hide its purpose. Also, the
documentation of GENMASK() says that it is a mask and not a value.

> > +static int bu21029_touch_report(struct bu21029_ts_data *bu21029)
> > +{
> > +       struct i2c_client *i2c = bu21029->client;
> > +       u8 buf[BUF_LEN];
> > +       int error = bu21029_touch_report(bu21029);
> 
> > +
> 
> Redundant empty line.

Removed.

> > +       if (error) {
> 
> > +               dev_err(&i2c->dev, "failed to report (error: %d)\n", error);
> 
> Potential spamming case.
> 
> > +               return IRQ_NONE;
> > +       }

You are right, we will remove the error message.

> > +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.

> > +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()?!
> 
> > +
> > +       error = i2c_smbus_read_i2c_block_data(i2c,
> > +                                             BU21029_HWID_REG,
> > +                                             2,
> > +                                             (u8 *)&hwid);
> > +       if (error < 0) {
> > +               dev_err(&i2c->dev, "failed to read HW ID\n");
> > +               goto out;
> > +       }

After de-asserting the reset chip takes 1 millisecond until it is
operational. We added a 1 millisecond buffer to it. Thus,
START_DELAY_MS is 2.

The following I2C read will not succeed without waiting for the chip
being ready.

> > +       if (cpu_to_be16(hwid) != SUPPORTED_HWID) {
> 
> Hmm... Why cpu_to_be16() is required?
> 
> > +               dev_err(&i2c->dev, "unsupported HW ID 0x%x\n", hwid);
> > +               error = -ENODEV;
> > +               goto out;
> > +       }
> > +}

You are right, it works but what we meant to do here is to convert the
chip's value (big endian) into the CPU endianness. We will change it to
be16_to_cpu().

> > +static int bu21029_parse_dt(struct bu21029_ts_data *bu21029)
> 
> You can get rid of DT requirement by...
> 
> > +{
> > +       struct device *dev = &bu21029->client->dev;
> > +       struct device_node *np = dev->of_node;
> > +       u32 val32;
> > +       int error;
> 
> > +       if (!np) {
> > +               dev_err(dev, "no device tree data\n");
> > +               return -EINVAL;
> > +       }
> 
> (this becomes redundant)
> 
> > +
> > +       bu21029->reset_gpios = devm_gpiod_get(dev, "reset",
> GPIOD_OUT_HIGH);
> > +       if (IS_ERR(bu21029->reset_gpios)) {
> > +               error = PTR_ERR(bu21029->reset_gpios);
> > +               if (error != -EPROBE_DEFER)
> > +                       dev_err(dev, "invalid 'reset-gpios':%d\n", error);
> > +               return error;
> > +       }
> > +
> 
> > +       if (of_property_read_u32(np, "rohm,x-plate-ohms", &val32)) {
> 
> ...simple calling device_property_read_u32() instead.
> 
> > +               dev_err(dev, "invalid 'x-plate-ohms' supplied\n");
> > +               return -EINVAL;
> > +       }
> > +       bu21029->x_plate_ohms = val32;
> > +
> > +       touchscreen_parse_properties(bu21029->in_dev, false, &bu21029->prop);
> > +
> > +       return 0;
> > +}

Thank you, changed.

> > +#ifdef CONFIG_PM_SLEEP
> 
> Instead...
> 
> > +static int bu21029_suspend(struct device *dev)
> 
> ...use __maby_unused annotation.
> 
> > +static int bu21029_resume(struct device *dev)
> 
> Ditto.

OK, added.

Regards,
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