From: Linus Walleij <linus.walleij@linaro.org>
To: Vincent Knecht <vincent.knecht@mailoo.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>,
Michael Srba <Michael.Srba@seznam.cz>,
Linux Input <linux-input@vger.kernel.org>,
"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
<devicetree@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v5 2/2] Input: add MStar MSG2638 touchscreen driver
Date: Tue, 2 Mar 2021 10:00:05 +0100 [thread overview]
Message-ID: <CACRpkdaLJzG3_M7fNC-MHOdQ6HMsqHqVccK7XkvL=PvJnycCHQ@mail.gmail.com> (raw)
In-Reply-To: <20210216163647.34264-2-vincent.knecht@mailoo.org>
Hi Vincent,
thanks for your patch!
On Tue, Feb 16, 2021 at 5:38 PM Vincent Knecht
<vincent.knecht@mailoo.org> wrote:
> Add support for the msg2638 touchscreen IC from MStar.
> This driver reuses zinitix.c structure, while the checksum and irq handler
> functions are based on out-of-tree driver for Alcatel Idol 3 (4.7").
>
> Signed-off-by: Vincent Knecht <vincent.knecht@mailoo.org>
> ---
> Changed in v5:
> - use gpiod_set_value_cansleep() (Stephan G)
> - use msleep/usleep_range() rathen than mdelay() (Stephan G)
(...)
> +#include <linux/delay.h>
> +#include <linux/gpio.h>
Please only include
#include <linux/gpio/consumer.h>
> +#define CHIP_ON_DELAY 15 // ms
> +#define FIRMWARE_ON_DELAY 50 // ms
> +#define RESET_DELAY_MIN 10000 // µs
> +#define RESET_DELAY_MAX 11000 // µs
Rename the defines with _MS and _US suffixes so you don't
need the comments, CHIP_ON_DELAY_MS etc.
> +static int msg2638_init_regulators(struct msg2638_ts_data *msg2638)
> +{
> + struct i2c_client *client = msg2638->client;
> + int error;
I usually prefer a short name like "err" (cognitive load) but your choice.
> +static u8 msg2638_checksum(u8 *data, u32 length)
> +{
> + s32 sum = 0;
> + u32 i;
> +
> + for (i = 0; i < length; i++)
> + sum += data[i];
> +
> + return (u8)((-sum) & 0xFF);
> +}
Interesting checksum algoritm.
> +static int msg2638_start(struct msg2638_ts_data *msg2638)
> +{
> + int error;
> +
> + error = regulator_bulk_enable(ARRAY_SIZE(msg2638->supplies),
> + msg2638->supplies);
> + if (error) {
> + dev_err(&msg2638->client->dev, "Failed to enable regulators: %d\n", error);
> + return error;
> + }
> +
> + msleep(CHIP_ON_DELAY);
> +
> + msg2638_power_on(msg2638);
Maybe move enable_irq() here from power on to mirror
the stop() function below?
> +#ifdef CONFIG_OF
> +static const struct of_device_id msg2638_of_match[] = {
> + { .compatible = "mstar,msg2638" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, msg2638_of_match);
> +#endif
I think these #ifdefs are not needed anymore. We just have struct of_device_id
available at all times.
Yours,
Linus Walleij
next prev parent reply other threads:[~2021-03-02 13:58 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-16 16:36 [PATCH v5 1/2] dt-bindings: input/touchscreen: add bindings for msg2638 Vincent Knecht
2021-02-16 16:36 ` [PATCH v5 2/2] Input: add MStar MSG2638 touchscreen driver Vincent Knecht
2021-03-02 9:00 ` Linus Walleij [this message]
2021-03-02 8:50 ` [PATCH v5 1/2] dt-bindings: input/touchscreen: add bindings for msg2638 Linus Walleij
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='CACRpkdaLJzG3_M7fNC-MHOdQ6HMsqHqVccK7XkvL=PvJnycCHQ@mail.gmail.com' \
--to=linus.walleij@linaro.org \
--cc=Michael.Srba@seznam.cz \
--cc=devicetree@vger.kernel.org \
--cc=dmitry.torokhov@gmail.com \
--cc=linux-input@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=phone-devel@vger.kernel.org \
--cc=robh+dt@kernel.org \
--cc=rydberg@bitmath.org \
--cc=vincent.knecht@mailoo.org \
--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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.