Re: [PATCH v5 2/3] Input: Add Novatek NT36xxx touchscreen driver

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Sun, Oct 18, 2020 at 12:40 AM <kholk11@gmail.com> wrote:
>
> From: AngeloGioacchino Del Regno <kholk11@gmail.com>
>
> This is a driver for the Novatek in-cell touch controller and
> supports various chips from the NT36xxx family, currently
> including NT36525, NT36672A, NT36676F, NT36772 and NT36870.
>
> Functionality like wake gestures and firmware flashing is not
> included: I am not aware of any of these DrIC+Touch combo
> chips not including a non-volatile memory and it should be
> highly unlikely to find one, since the touch firmware is
> embedded into the DriverIC one, which is obviously necessary
> to drive the display unit.
>
> However, the necessary address for the firmware update
> procedure was included into the address table in this driver
> so, in the event that someone finds the need to implement it
> for a reason or another, it will be pretty straightforward to.
>
> This driver is lightly based on the downstream implementation [1].
> [1] https://github.com/Rasenkai/caf-tsoft-Novatek-nt36xxx

...

> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>

> +#include <linux/of_gpio.h>

I haven't found evidence you need this header.
But mod_devicetable.h along with gpio/consumer.h seem missed.

> +#include <linux/input/mt.h>
> +#include <linux/input/touchscreen.h>
> +#include <linux/regulator/consumer.h>

...

> +enum nt36xxx_fw_state {
> +       NT36XXX_STATE_INIT = 0xa0,      /* IC reset */
> +       NT36XXX_STATE_REK,              /* ReK baseline */
> +       NT36XXX_STATE_REK_FINISH,       /* Baseline is ready */
> +       NT36XXX_STATE_NORMAL_RUN,       /* Normal run */
> +       NT36XXX_STATE_MAX = 0xaf
> +};

Perhaps kernel doc?

...

> +static int nt36xxx_set_page(struct nt36xxx_i2c *ts, u32 pageaddr)
> +{
> +       u32 data = cpu_to_be32(pageaddr) >> 8;
> +       int ret;
> +
> +       ret = regmap_noinc_write(ts->fw_regmap, NT36XXX_CMD_SET_PAGE,
> +                                &data, sizeof(data));

> +       if (ret == 0)
> +               usleep_range(100, 200);
> +
> +       return ret;

Can you use traditional pattern, i.e.

  if (ret)
    return ret;

  ...
  return 0;

?

> +}
> +
> +static int nt36xxx_sw_reset_idle(struct nt36xxx_i2c *ts)
> +{
> +       int ret;
> +
> +       ret = regmap_write(ts->regmap, ts->hw_client->addr,
> +                          NT36XXX_CMD_SW_RESET);
> +

> +       usleep_range(15000, 16000);

Even in error case? Also, can you explain the value of delay?

> +       return ret;
> +}
> +
> +static int nt36xxx_bootloader_reset(struct nt36xxx_i2c *ts)
> +{
> +       int ret;
> +
> +       ret = regmap_write(ts->regmap, ts->hw_client->addr,
> +                          NT36XXX_CMD_BOOTLOADER_RESET);
> +

> +       msleep(35);

Same as above, esp. taking into account quite a long delay.

> +       return ret;
> +}
> +
> +static int nt36xxx_check_reset_state(struct nt36xxx_i2c *ts,
> +                                    enum nt36xxx_fw_state fw_state)
> +{
> +       u8 buf[2] = { 0 };
> +       int ret, retry = NT36XXX_MAX_FW_RST_RETRY;
> +
> +       do {
> +               ret = regmap_noinc_read(ts->fw_regmap,
> +                                       NT36XXX_EVT_RESET_COMPLETE,
> +                                       buf, sizeof(buf));
> +               if (likely(ret == 0) &&
> +                   (buf[0] >= fw_state) &&
> +                   (buf[0] <= NT36XXX_STATE_MAX)) {
> +                       ret = 0;
> +                       break;
> +               }
> +               usleep_range(10000, 11000);
> +       } while (--retry);
> +
> +       if (!retry) {
> +               dev_err(&ts->hw_client->dev, "Firmware reset failed.\n");
> +               ret = -EBUSY;
> +       }

This sounds like it should be a part of regmap API, such as
regmap_read_poll_timeout().

> +
> +       return ret;
> +}
> +
> +static int nt36xxx_read_pid(struct nt36xxx_i2c *ts)
> +{
> +       u8 buf[2] = { 0 };
> +       int ret = 0;

Redundant assignments.

> +       ret = nt36xxx_set_page(ts, ts->mmap->evtbuf_addr);
> +       if (unlikely(ret < 0))
> +               return ret;
> +
> +       ret = regmap_noinc_read(ts->fw_regmap, NT36XXX_EVT_PROJECTID,
> +                               buf, sizeof(buf));
> +       if (unlikely(ret < 0))
> +               return ret;
> +

> +       ts->fw_info.nvt_pid = (buf[1] << 8) + buf[0];

Define buf as __le16 and use le16_to_cpu().

> +
> +       return 0;
> +}
> +
> +static int __nt36xxx_get_fw_info(struct nt36xxx_i2c *ts)
> +{
> +       struct nt36xxx_fw_info *fwi = &ts->fw_info;
> +       u8 buf[11] = { 0 };
> +       int ret = 0;
> +
> +       ret = nt36xxx_set_page(ts, ts->mmap->evtbuf_addr);
> +       if (unlikely(ret < 0))
> +               return ret;
> +
> +       ret = regmap_noinc_read(ts->fw_regmap, NT36XXX_EVT_FWINFO,
> +                               buf, sizeof(buf));
> +       if (ret)
> +               return ret;
> +
> +       fwi->fw_ver = buf[0];
> +       fwi->x_num = buf[2];
> +       fwi->y_num = buf[3];
> +       fwi->abs_x_max = (u16)((buf[4] << 8) | buf[5]);
> +       fwi->abs_y_max = (u16)((buf[6] << 8) | buf[7]);

be16_to_cpu() or get_unaligned_be16(), though here it's aligned.

> +       fwi->max_buttons = buf[10];
> +
> +       /* Check fw info integrity and clear x_num, y_num if broken */
> +       if ((buf[0] + buf[1]) != 0xFF) {
> +               dev_err(&ts->hw_client->dev,
> +                       "FW info is broken! fw_ver=0x%02X, ~fw_ver=0x%02X\n",
> +                       buf[0], buf[1]);
> +               fwi->fw_ver = 0;
> +               fwi->x_num = 18;
> +               fwi->y_num = 32;
> +               fwi->abs_x_max = TOUCH_DEFAULT_MAX_WIDTH;
> +               fwi->abs_y_max = TOUCH_DEFAULT_MAX_HEIGHT;
> +               fwi->max_buttons = 0;
> +               return -EINVAL;
> +       }
> +
> +       /* Get Novatek ProjectID */
> +       return nt36xxx_read_pid(ts);
> +}

...

> +static int nvt_stop_crc_reboot(struct nt36xxx_i2c *ts)
> +{
> +       u8 buf[3] = { 0 };
> +       u8 val;
> +       int ret, retry = NT36XXX_MAX_RETRIES;
> +
> +       /* Read dummy buffer to check CRC fail reboot is happening or not */
> +
> +       /* Change I2C index to prevent getting 0xFF, but not 0xFC */
> +       ret = nt36xxx_set_page(ts, NT36XXX_PAGE_CHIP_INFO);
> +       if (ret) {
> +               dev_dbg(&ts->hw_client->dev,
> +                       "CRC reset failed: Cannot select page.\n");
> +               return ret;
> +       }
> +
> +       /* If ChipID command returns 0xFC or 0xFF, the MCU is in CRC reboot */
> +       ret = regmap_noinc_read(ts->fw_regmap, NT36XXX_EVT_CHIPID,
> +                               buf, sizeof(buf));
> +       if (ret)
> +               return ret;
> +
> +       if (((buf[0] == 0xFC) && (buf[1] == 0xFC) && (buf[2] == 0xFC)) ||
> +           ((buf[0] == 0xFF) && (buf[1] == 0xFF) && (buf[2] == 0xFF))) {

You may decrese indentation by switching above to be negative conditional.

> +               /* IC is in CRC fail reboot loop, needs to be stopped! */
> +               do {
> +                       /* Special reset-idle sequence for CRC failure */
> +                       ret = regmap_write(ts->regmap, ts->hw_client->addr,
> +                                          NT36XXX_CMD_SW_RESET);

> +                       ret += regmap_write(ts->regmap, ts->hw_client->addr,
> +                                          NT36XXX_CMD_SW_RESET);

It's not good practice to sum error codes.

> +                       if (ret)
> +                               dev_dbg(&ts->hw_client->dev,
> +                                       "SW Reset failed: may not recover\n");
> +                       usleep_range(1000, 1100);
> +
> +                       /* Clear CRC_ERR_FLAG */
> +                       ret = nt36xxx_set_page(ts, NT36XXX_PAGE_CRC);
> +                       if (ret)
> +                               continue;
> +
> +                       val = 0xA5;
> +                       ret = regmap_raw_write(ts->fw_regmap, NT36XXX_EVT_CRC,
> +                                              &val, sizeof(val));
> +                       if (ret)
> +                               continue;
> +
> +                       /* Check CRC_ERR_FLAG */
> +                       ret = nt36xxx_set_page(ts, NT36XXX_PAGE_CRC);
> +                       if (ret)
> +                               continue;
> +
> +                       ret = regmap_noinc_read(ts->fw_regmap, NT36XXX_EVT_CRC,
> +                                               &buf, sizeof(buf));
> +                       if (ret)
> +                               continue;
> +
> +                       if (buf[0] == 0xA5)
> +                               break;
> +               } while (--retry);
> +
> +               if (retry == 0) {
> +                       dev_err(&ts->hw_client->dev,
> +                               "CRC reset failed: buf=0x%02x%02x\n",
> +                               buf[0], buf[1]);

%2ph

> +               }
> +       }
> +
> +       return 0;
> +}
> +
> +static int nt36xxx_i2c_chip_version_init(struct nt36xxx_i2c *ts)
> +{
> +       u8 buf[7] = { 0 };
> +       int retry = NT36XXX_MAX_RETRIES;
> +       int sz = sizeof(trim_id_table) / sizeof(struct nt36xxx_trim_table);
> +       int i, list, mapid, ret;
> +
> +       ret = nt36xxx_bootloader_reset(ts);
> +       if (ret < 0) {
> +               dev_err(&ts->hw_client->dev, "Can't reset the nvt IC\n");
> +               return ret;
> +       }
> +
> +       do {
> +               ret = nt36xxx_sw_reset_idle(ts);
> +               if (ret < 0)
> +                       continue;
> +
> +               ret = regmap_write(ts->regmap, ts->hw_client->addr, NT36XXX_CMD_UNLOCK);
> +               if (ret)
> +                       continue;
> +               usleep_range(10000, 11000);
> +
> +               ret = nt36xxx_set_page(ts, NT36XXX_PAGE_CHIP_INFO);
> +               if (ret)
> +                       continue;
> +
> +               memset(buf, 0, ARRAY_SIZE(buf));
> +               ret = regmap_noinc_read(ts->fw_regmap, NT36XXX_EVT_CHIPID,
> +                                       buf, sizeof(buf));
> +               if (ret)
> +                       continue;
> +
> +               /* Compare read chip id with trim list */
> +               for (list = 0; list < sz; list++) {
> +                       /* Compare each not masked byte */
> +                       for (i = 0; i < NT36XXX_ID_LEN_MAX; i++) {
> +                               if (trim_id_table[list].mask[i] &&
> +                                   buf[i] != trim_id_table[list].id[i])
> +                                       break;
> +                       }
> +
> +                       if (i == NT36XXX_ID_LEN_MAX) {
> +                               mapid = trim_id_table[list].mapid;
> +                               ts->mmap = &nt36xxx_memory_maps[mapid];
> +                               return 0;
> +                       }
> +
> +                       ts->mmap = NULL;
> +                       ret = -ENOENT;
> +               }
> +
> +               /* Stop CRC check to prevent IC auto reboot */
> +               if (((buf[1] == 0xFC) && (buf[2] == 0xFC) &&
> +                    (buf[3] == 0xFC)) ||
> +                   ((buf[1] == 0xFF) && (buf[2] == 0xFF) &&
> +                    (buf[3] == 0xFF))) {

You already have the same code above. Perhaps helper is good to have for this.

> +                       ret = nvt_stop_crc_reboot(ts);
> +                       if (ret < 0)
> +                               continue;
> +               }
> +
> +               usleep_range(10000, 11000);
> +       } while (--retry);
> +
> +       return ret;
> +}
> +
> +static const struct regmap_config nt36xxx_i2c_regmap_hw_config = {
> +       .name = "nt36xxx_i2c_hw",
> +       .reg_bits = 8,
> +       .val_bits = 8,
> +       .cache_type = REGCACHE_NONE,
> +};
> +
> +static const struct regmap_config nt36xxx_i2c_regmap_fw_config = {
> +       .name = "nt36xxx_i2c_fw",
> +       .reg_bits = 8,
> +       .val_bits = 8,
> +       .cache_type = REGCACHE_NONE,
> +};
> +
> +static void nt36xxx_disable_regulators(void *data)
> +{
> +       struct nt36xxx_i2c *ts = data;
> +
> +       regulator_bulk_disable(NT36XXX_NUM_SUPPLIES, ts->supplies);
> +}
> +
> +static int nt36xxx_i2c_probe(struct i2c_client *hw_client,
> +                            const struct i2c_device_id *id)
> +{
> +       struct nt36xxx_i2c *ts;
> +       struct input_dev *input;
> +       int ret;
> +
> +       if (!i2c_check_functionality(hw_client->adapter, I2C_FUNC_I2C)) {
> +               dev_err(&hw_client->dev, "i2c_check_functionality error\n");
> +               return -EIO;
> +       }
> +
> +       if (!hw_client->irq) {
> +               dev_err(&hw_client->dev, "No irq specified\n");
> +               return -EINVAL;
> +       }
> +
> +       ts = devm_kzalloc(&hw_client->dev, sizeof(struct nt36xxx_i2c), GFP_KERNEL);
> +       if (!ts)
> +               return -ENOMEM;
> +
> +       ts->supplies = devm_kcalloc(&hw_client->dev,
> +                                   NT36XXX_NUM_SUPPLIES,
> +                                   sizeof(struct regulator_bulk_data),
> +                                   GFP_KERNEL);
> +       if (!ts->supplies)
> +               return -ENOMEM;
> +
> +       input = devm_input_allocate_device(&hw_client->dev);
> +       if (!input)
> +               return -ENOMEM;
> +
> +       ts->fw_client = i2c_new_dummy_device(hw_client->adapter,
> +                                            NT36XXX_FW_ADDR);
> +       if (IS_ERR(ts->fw_client)) {
> +               dev_err(&hw_client->dev, "Cannot add FW I2C device\n");
> +               return PTR_ERR(ts->fw_client);
> +       }
> +
> +       ts->hw_client = hw_client;
> +       ts->input = input;
> +       i2c_set_clientdata(ts->hw_client, ts);
> +       i2c_set_clientdata(ts->fw_client, ts);
> +
> +       ts->reset_gpio = devm_gpiod_get_optional(&hw_client->dev, "reset",
> +                                                GPIOD_OUT_HIGH);
> +       if (IS_ERR(ts->reset_gpio))
> +               return PTR_ERR(ts->reset_gpio);

> +       if (ts->reset_gpio)

Redundant. If you mark GPIO as optional all those checks should not be here.

> +               gpiod_set_consumer_name(ts->reset_gpio, "nt36xxx reset");
> +
> +       /* These supplies are optional */
> +       ts->supplies[0].supply = "vdd";
> +       ts->supplies[1].supply = "vio";
> +       ret = devm_regulator_bulk_get(&hw_client->dev,
> +                                     NT36XXX_NUM_SUPPLIES,
> +                                     ts->supplies);
> +       if (ret != 0) {

if (ret)

> +               if (ret != -EPROBE_DEFER)
> +                       dev_err(&hw_client->dev,
> +                               "Cannot get supplies: %d\n", ret);
> +               return ret;

I think there is no return dev_err_probe() here b/c Dmitry is against it?

> +       }
> +
> +       ts->regmap = devm_regmap_init_i2c(ts->hw_client,
> +                                         &nt36xxx_i2c_regmap_hw_config);
> +       if (IS_ERR(ts->regmap)) {
> +               dev_err(&hw_client->dev, "regmap (hw-addr) init failed\n");
> +               return PTR_ERR(ts->regmap);
> +       }
> +
> +       ts->fw_regmap = devm_regmap_init_i2c(ts->fw_client,
> +                                            &nt36xxx_i2c_regmap_fw_config);
> +       if (IS_ERR(ts->fw_regmap)) {
> +               dev_err(&hw_client->dev, "regmap (fw-addr) init failed\n");
> +               return PTR_ERR(ts->fw_regmap);
> +       }
> +
> +       ret = regulator_bulk_enable(NT36XXX_NUM_SUPPLIES, ts->supplies);
> +       if (ret)
> +               return ret;

> +       usleep_range(10000, 11000);

Why?

> +       ret = devm_add_action_or_reset(&hw_client->dev,
> +                                      nt36xxx_disable_regulators, ts);
> +       if (ret)
> +               return ret;
> +
> +       mutex_init(&ts->lock);
> +
> +       /* Set memory maps for the specific chip version */
> +       ret = nt36xxx_i2c_chip_version_init(ts);
> +       if (ret) {
> +               dev_err(&hw_client->dev, "Failed to check chip version\n");
> +               return ret;
> +       }
> +
> +       /* Reset the MCU */
> +       ret = nt36xxx_bootloader_reset(ts);

> +       if (unlikely(ret < 0))

All these unlikely() calls are unlikely needed. Please, drop them for
the sake of readability.

> +               return ret;
> +
> +       /* Check and eventually wait until the MCU goes in reset state */
> +       ret = nt36xxx_check_reset_state(ts, NT36XXX_STATE_INIT);
> +       if (unlikely(ret < 0))
> +               return ret;
> +
> +       /* Get informations from the TS firmware */
> +       ret = nt36xxx_get_fw_info(ts);
> +       if (unlikely(ret < 0))
> +               return ret;
> +
> +       input->phys = devm_kasprintf(&hw_client->dev, GFP_KERNEL,
> +                                    "%s/input0", dev_name(&hw_client->dev));
> +
> +       input->name = "Novatek NT36XXX Touchscreen";
> +       input->id.bustype = BUS_I2C;
> +       input->dev.parent = &hw_client->dev;
> +
> +       __set_bit(EV_KEY, input->evbit);
> +       __set_bit(EV_ABS, input->evbit);
> +       input_set_capability(input, EV_KEY, BTN_TOUCH);
> +
> +       ret = input_mt_init_slots(input, TOUCH_MAX_FINGER_NUM,
> +                                 INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
> +       if (ret) {
> +               dev_err(&hw_client->dev, "Cannot init MT slots (%d)\n", ret);
> +               return ret;
> +       }
> +
> +       input_set_abs_params(input, ABS_MT_PRESSURE, 0,
> +                            TOUCH_MAX_PRESSURE, 0, 0);
> +       input_set_abs_params(input, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
> +
> +       input_set_abs_params(input, ABS_MT_POSITION_X, 0,
> +                            ts->fw_info.abs_x_max - 1, 0, 0);
> +       input_set_abs_params(input, ABS_MT_POSITION_Y, 0,
> +                            ts->fw_info.abs_y_max - 1, 0, 0);
> +
> +       /* Override the firmware defaults, if needed */
> +       touchscreen_parse_properties(input, true, &ts->prop);
> +
> +       input_set_drvdata(input, ts);
> +
> +       ret = input_register_device(ts->input);
> +       if (ret) {
> +               dev_err(&hw_client->dev, "Failed to register input device: %d\n",
> +                       ret);
> +               return ret;
> +       }
> +
> +       ret = devm_request_threaded_irq(&hw_client->dev, hw_client->irq, NULL,
> +                                       nt36xxx_i2c_irq_handler, IRQF_ONESHOT,
> +                                       hw_client->name, ts);
> +       if (ret) {
> +               dev_err(&hw_client->dev, "request irq failed: %d\n", ret);

> +               return ret;
> +       }
> +
> +       return 0;

return ret; ?

> +}
> +
> +static int __maybe_unused nt36xxx_i2c_suspend(struct device *dev)
> +{
> +       struct nt36xxx_i2c *ts = i2c_get_clientdata(to_i2c_client(dev));
> +       int ret;
> +
> +       disable_irq(ts->hw_client->irq);
> +
> +       ret = regmap_write(ts->fw_regmap, NT36XXX_EVT_HOST_CMD,
> +                          NT36XXX_CMD_ENTER_SLEEP);
> +       if (unlikely(ret)) {
> +               dev_err(&ts->hw_client->dev, "Cannot enter suspend!!\n");
> +               return ret;
> +       }

> +       if (ts->reset_gpio)

Redundant.

> +               gpiod_set_value(ts->reset_gpio, 1);
> +
> +       return 0;
> +}
> +
> +static int __maybe_unused nt36xxx_i2c_resume(struct device *dev)
> +{
> +       struct nt36xxx_i2c *ts = i2c_get_clientdata(to_i2c_client(dev));
> +       int ret;
> +
> +       mutex_lock(&ts->lock);

> +       if (ts->reset_gpio)

Redundant.

> +               gpiod_set_value(ts->reset_gpio, 0);

> +       /* Reboot the MCU (also recalibrates the TS) */
> +       ret = nt36xxx_bootloader_reset(ts);
> +       if (ret < 0)
> +               goto end;
> +
> +       ret = nt36xxx_check_reset_state(ts, NT36XXX_STATE_REK);
> +       if (ret < 0)
> +               goto end;
> +
> +       enable_irq(ts->hw_client->irq);
> +end:
> +       mutex_unlock(&ts->lock);
> +       return ret;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(nt36xxx_i2c_pm,
> +                        nt36xxx_i2c_suspend, nt36xxx_i2c_resume);
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id nt36xxx_of_match[] = {
> +       { .compatible = "novatek,nt36525" },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(of, nt36xxx_of_match);
> +#endif
> +
> +static const struct i2c_device_id nt36xxx_i2c_ts_id[] = {
> +       { "NVT-ts", 0 },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(i2c, nt36xxx_i2c_ts_id);
> +
> +static struct i2c_driver nt36xxx_i2c_ts_driver = {
> +       .driver = {
> +               .name   = "nt36xxx_ts",
> +               .pm     = &nt36xxx_i2c_pm,

> +               .of_match_table = of_match_ptr(nt36xxx_of_match),

You may do a good job by dropping these of_match_ptr() along with ugly
ifdeffery above. It will allow to use it on ACPI-enabled platforms.

> +       },


--
With Best Regards,
Andy Shevchenko