From: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
To: Markuss Broks <markuss.broks@gmail.com>, linux-kernel@vger.kernel.org
Cc: phone-devel@vger.kernel.org,
~postmarketos/upstreaming@lists.sr.ht,
Dmitry Torokhov <dmitry.torokhov@gmail.com>,
Rob Herring <robh+dt@kernel.org>,
Henrik Rydberg <rydberg@bitmath.org>,
Stephen Rothwell <sfr@canb.auug.org.au>,
linux-input@vger.kernel.org, devicetree@vger.kernel.org
Subject: Re: [PATCH v3 2/2] Input: add Imagis touchscreen driver
Date: Tue, 15 Feb 2022 17:05:41 +0100 [thread overview]
Message-ID: <3fbb2e11-ca60-0b25-8c59-9d73eb4addd1@canonical.com> (raw)
In-Reply-To: <20220215151527.84634-3-markuss.broks@gmail.com>
On 15/02/2022 16:15, Markuss Broks wrote:
> Add support for the IST3038C touchscreen IC from Imagis, based on
> downstream driver. The driver supports multi-touch (10 touch points)
> The IST3038C IC supports touch keys, but the support isn't added
> because the touch screen used for testing doesn't utilize touch keys.
> Looking at the downstream driver, it is possible to add support
> for other Imagis ICs of IST30**C series.
>
> Signed-off-by: Markuss Broks <markuss.broks@gmail.com>
> ---
> MAINTAINERS | 6 +
> drivers/input/touchscreen/Kconfig | 10 +
> drivers/input/touchscreen/Makefile | 1 +
> drivers/input/touchscreen/imagis.c | 330 +++++++++++++++++++++++++++++
> 4 files changed, 347 insertions(+)
> create mode 100644 drivers/input/touchscreen/imagis.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a899828a8d4e..f7f717ae926a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9405,6 +9405,12 @@ F: Documentation/devicetree/bindings/iio/afe/current-sense-shunt.yaml
> F: Documentation/devicetree/bindings/iio/afe/voltage-divider.yaml
> F: drivers/iio/afe/iio-rescale.c
>
> +IMAGIS TOUCHSCREEN DRIVER
Please read the line 143-144 of this file.
> +M: Markuss Broks <markuss.broks@gmail.com>
> +S: Maintained
> +F: Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml
> +F: drivers/input/touchscreen/imagis.c
> +
> IKANOS/ADI EAGLE ADSL USB DRIVER
> M: Matthieu Castet <castet.matthieu@free.fr>
> M: Stanislaw Gruszka <stf_xl@wp.pl>
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index 2f6adfb7b938..6810b4b094e8 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -1367,4 +1367,14 @@ config TOUCHSCREEN_ZINITIX
> To compile this driver as a module, choose M here: the
> module will be called zinitix.
>
> +config TOUCHSCREEN_IMAGIS
Here and in Makefile - do not add entries at the end, but
alphabetically. This reduces conflicts.
> + tristate "Imagis touchscreen support"
> + depends on I2C
> + help
> + Say Y here if you have an Imagis IST30xxC touchscreen.
> + If unsure, say N.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called imagis.
> +
> endif
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index 39a8127cf6a5..989bb1d563d3 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -115,3 +115,4 @@ obj-$(CONFIG_TOUCHSCREEN_ROHM_BU21023) += rohm_bu21023.o
> obj-$(CONFIG_TOUCHSCREEN_RASPBERRYPI_FW) += raspberrypi-ts.o
> obj-$(CONFIG_TOUCHSCREEN_IQS5XX) += iqs5xx.o
> obj-$(CONFIG_TOUCHSCREEN_ZINITIX) += zinitix.o
> +obj-$(CONFIG_TOUCHSCREEN_IMAGIS) += imagis.o
(...)
> +
> +static int imagis_init_regulators(struct imagis_ts *ts)
> +{
> + struct i2c_client *client = ts->client;
> + int error;
> +
> + ts->supplies[0].supply = "vdd";
> + ts->supplies[1].supply = "vddio";
> + error = devm_regulator_bulk_get(&client->dev,
> + ARRAY_SIZE(ts->supplies),
> + ts->supplies);
> + if (error < 0) {
> + dev_err(&client->dev, "Failed to get regulators: %d\n", error);
This should be also dev_err_probe() in case of deferred probing. On the
other hand, you already handle printing in probe(), so why doing it twice?
> + return error;
> + }
> +
> + return 0;
> +}
> +
> +static int imagis_probe(struct i2c_client *i2c)
> +{
> + struct device *dev;
> + struct imagis_ts *ts;
> + int chip_id, ret;
> +
> + dev = &i2c->dev;
> +
> + ts = devm_kzalloc(dev, sizeof(*ts), GFP_KERNEL);
> + if (!ts)
> + return -ENOMEM;
> +
> + ts->client = i2c;
> +
> + ret = imagis_init_regulators(ts);
> + if (ret)
> + return dev_err_probe(dev, ret, "regulator init error: %d\n", ret);
> +
> + ret = regulator_bulk_enable(ARRAY_SIZE(ts->supplies),
> + ts->supplies);
> + if (ret)
> + return dev_err_probe(dev, ret, "failed to enable regulators: %d\n", ret);
> +
> + msleep(IST3038C_CHIP_ON_DELAY);
> +
> + ret = imagis_i2c_read_reg(ts, IST3038C_REG_CHIPID | IST3038C_DIRECT_ACCESS, &chip_id);
> + if (ret)
You miss here and other error paths the regulator cleanup (disabling).
> + return dev_err_probe(dev, ret, "chip ID read failure: %d\n", ret);
> +
> + if (chip_id == IST3038C_WHOAMI)
> + dev_dbg(dev, "Detected IST3038C chip\n");
> + else
> + return dev_err_probe(dev, -EINVAL, "unknown chip ID: 0x%x\n", chip_id);
> +
> + ret = devm_request_threaded_irq(dev, i2c->irq,
> + NULL, imagis_interrupt,
> + IRQF_ONESHOT | IRQF_TRIGGER_FALLING | IRQF_NO_AUTOEN,
The interrupt edge should come from DT, not be hard-coded. I think you
can safely skip IRQF_TRIGGER_FALLING.
> + "imagis-touchscreen", ts);
> + if (ret)
> + return dev_err_probe(dev, ret, "IRQ allocation failure: %d\n", ret);
> +
> + ret = imagis_init_input_dev(ts);
> + if (ret)
> + return dev_err_probe(dev, ret, "input subsystem init error: %d\n", ret);
> +
> + return 0;
> +}
> +
Best regards,
Krzysztof
next prev parent reply other threads:[~2022-02-15 16:05 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-02-10 16:37 [PATCH v2 0/2] Add support for Imagis touchscreens Markuss Broks
2022-02-10 16:37 ` [PATCH v2 1/2] dt-bindings: input/touchscreen: bindings for Imagis Markuss Broks
2022-02-10 22:25 ` Rob Herring
2022-02-15 15:15 ` [PATCH v3 0/2] Add support for Imagis touchscreens Markuss Broks
2022-02-15 15:15 ` [PATCH v3 1/2] dt-bindings: input/touchscreen: bindings for Imagis Markuss Broks
2022-02-15 15:54 ` Krzysztof Kozlowski
2022-02-15 15:15 ` [PATCH v3 2/2] Input: add Imagis touchscreen driver Markuss Broks
2022-02-15 16:05 ` Krzysztof Kozlowski [this message]
2022-02-10 16:37 ` [PATCH v2 " Markuss Broks
2022-02-11 0:01 ` Jeff LaBundy
2022-02-11 0:50 ` Jeff LaBundy
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3fbb2e11-ca60-0b25-8c59-9d73eb4addd1@canonical.com \
--to=krzysztof.kozlowski@canonical.com \
--cc=devicetree@vger.kernel.org \
--cc=dmitry.torokhov@gmail.com \
--cc=linux-input@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=markuss.broks@gmail.com \
--cc=phone-devel@vger.kernel.org \
--cc=robh+dt@kernel.org \
--cc=rydberg@bitmath.org \
--cc=sfr@canb.auug.org.au \
--cc=~postmarketos/upstreaming@lists.sr.ht \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).