Re: [PATCH v3] Input: add bu21029 touch driver

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

On Wed, May 16, 2018 at 01:24:24PM +0000, Jonas Mark (BT-FIR/ENG1) wrote:
> 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.

I agree.

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

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.

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

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

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.

Thanks.

-- 
Dmitry