* Re: [PATCH v2 2/2] Input: add Himax HX852x(ES) touchscreen driver
From: Stephan Gerhold @ 2023-10-22 18:49 UTC (permalink / raw)
To: Jeff LaBundy
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Henrik Rydberg, linux-input, devicetree, linux-kernel,
Christophe JAILLET, Jonathan Albrieux
In-Reply-To: <ZTVoiklUJaDn5576@nixie71>
Hi Jeff,
Thanks a lot for reviewing this again!
On Sun, Oct 22, 2023 at 01:23:06PM -0500, Jeff LaBundy wrote:
> Hi Stephan,
>
> This looks great, just a few remaining comments.
>
> On Sat, Sep 30, 2023 at 05:32:01PM +0200, Stephan Gerhold wrote:
> > From: Jonathan Albrieux <jonathan.albrieux@gmail.com>
> >
> > Add a simple driver for the Himax HX852x(ES) touch panel controller,
> > with support for multi-touch and capacitive touch keys.
> >
> > The driver is somewhat based on sample code from Himax. However, that
> > code was so extremely confusing that we spent a significant amount of
> > time just trying to understand the packet format and register commands.
> > In this driver they are described with clean structs and defines rather
> > than lots of magic numbers and offset calculations.
> >
> > Signed-off-by: Jonathan Albrieux <jonathan.albrieux@gmail.com>
> > Co-developed-by: Stephan Gerhold <stephan@gerhold.net>
> > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> > ---
> > MAINTAINERS | 7 +
> > drivers/input/touchscreen/Kconfig | 10 +
> > drivers/input/touchscreen/Makefile | 1 +
> > drivers/input/touchscreen/himax_hx852x.c | 499 +++++++++++++++++++++++++++++++
> > 4 files changed, 517 insertions(+)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 90f13281d297..c551c60b0598 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -9331,6 +9331,13 @@ S: Maintained
> > F: Documentation/devicetree/bindings/input/touchscreen/himax,hx83112b.yaml
> > F: drivers/input/touchscreen/himax_hx83112b.c
> >
> > +HIMAX HX852X TOUCHSCREEN DRIVER
> > +M: Stephan Gerhold <stephan@gerhold.net>
> > +L: linux-input@vger.kernel.org
> > +S: Maintained
> > +F: Documentation/devicetree/bindings/input/touchscreen/himax,hx852es.yaml
> > +F: drivers/input/touchscreen/himax_hx852x.c
> > +
> > HIPPI
> > M: Jes Sorensen <jes@trained-monkey.org>
> > L: linux-hippi@sunsite.dk
> > diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> > index e3e2324547b9..8e5667ae5dab 100644
> > --- a/drivers/input/touchscreen/Kconfig
> > +++ b/drivers/input/touchscreen/Kconfig
> > @@ -427,6 +427,16 @@ config TOUCHSCREEN_HIDEEP
> > To compile this driver as a module, choose M here : the
> > module will be called hideep_ts.
> >
> > +config TOUCHSCREEN_HIMAX_HX852X
> > + tristate "Himax HX852x(ES) touchscreen"
> > + depends on I2C
> > + help
> > + Say Y here if you have a Himax HX852x(ES) touchscreen.
> > + If unsure, say N.
> > +
> > + To compile this driver as a module, choose M here: the module
> > + will be called himax_hx852x.
> > +
> > config TOUCHSCREEN_HYCON_HY46XX
> > tristate "Hycon hy46xx touchscreen support"
> > depends on I2C
> > diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> > index 62bd24f3ac8e..f42a87faa86c 100644
> > --- a/drivers/input/touchscreen/Makefile
> > +++ b/drivers/input/touchscreen/Makefile
> > @@ -48,6 +48,7 @@ obj-$(CONFIG_TOUCHSCREEN_EXC3000) += exc3000.o
> > obj-$(CONFIG_TOUCHSCREEN_FUJITSU) += fujitsu_ts.o
> > obj-$(CONFIG_TOUCHSCREEN_GOODIX) += goodix_ts.o
> > obj-$(CONFIG_TOUCHSCREEN_HIDEEP) += hideep.o
> > +obj-$(CONFIG_TOUCHSCREEN_HIMAX_HX852X) += himax_hx852x.o
> > obj-$(CONFIG_TOUCHSCREEN_HYNITRON_CSTXXX) += hynitron_cstxxx.o
> > obj-$(CONFIG_TOUCHSCREEN_ILI210X) += ili210x.o
> > obj-$(CONFIG_TOUCHSCREEN_ILITEK) += ilitek_ts_i2c.o
> > diff --git a/drivers/input/touchscreen/himax_hx852x.c b/drivers/input/touchscreen/himax_hx852x.c
> > new file mode 100644
> > index 000000000000..98a55be7891d
> > --- /dev/null
> > +++ b/drivers/input/touchscreen/himax_hx852x.c
> > @@ -0,0 +1,499 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Himax HX852x(ES) Touchscreen Driver
> > + * Copyright (c) 2020-2023 Stephan Gerhold <stephan@gerhold.net>
> > + * Copyright (c) 2020 Jonathan Albrieux <jonathan.albrieux@gmail.com>
> > + *
> > + * Based on the Himax Android Driver Sample Code Ver 0.3 for HMX852xES chipset:
> > + * Copyright (c) 2014 Himax Corporation.
> > + */
> > +
> > +#include <asm/unaligned.h>
> > +#include <linux/delay.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/i2c.h>
> > +#include <linux/input.h>
> > +#include <linux/input/mt.h>
> > +#include <linux/input/touchscreen.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/kernel.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/regulator/consumer.h>
> > +
> > +#define HX852X_COORD_SIZE(fingers) ((fingers) * sizeof(struct hx852x_coord))
> > +#define HX852X_WIDTH_SIZE(fingers) ALIGN(fingers, 4)
> > +#define HX852X_BUF_SIZE(fingers) (HX852X_COORD_SIZE(fingers) + \
> > + HX852X_WIDTH_SIZE(fingers) + \
> > + sizeof(struct hx852x_touch_info))
> > +
> > +#define HX852X_MAX_FINGERS 12
> > +#define HX852X_MAX_KEY_COUNT 4
> > +#define HX852X_MAX_BUF_SIZE HX852X_BUF_SIZE(HX852X_MAX_FINGERS)
> > +
> > +#define HX852X_TS_SLEEP_IN 0x80
> > +#define HX852X_TS_SLEEP_OUT 0x81
> > +#define HX852X_TS_SENSE_OFF 0x82
> > +#define HX852X_TS_SENSE_ON 0x83
> > +#define HX852X_READ_ONE_EVENT 0x85
> > +#define HX852X_READ_ALL_EVENTS 0x86
> > +#define HX852X_READ_LATEST_EVENT 0x87
> > +#define HX852X_CLEAR_EVENT_STACK 0x88
> > +
> > +#define HX852X_REG_SRAM_SWITCH 0x8c
> > +#define HX852X_REG_SRAM_ADDR 0x8b
> > +#define HX852X_REG_FLASH_RPLACE 0x5a
> > +
> > +#define HX852X_SRAM_SWITCH_TEST_MODE 0x14
> > +#define HX852X_SRAM_ADDR_CONFIG 0x7000
> > +
> > +struct hx852x {
> > + struct i2c_client *client;
> > + struct input_dev *input_dev;
> > + struct touchscreen_properties props;
> > + struct gpio_desc *reset_gpiod;
> > + struct regulator_bulk_data supplies[2];
> > + unsigned int max_fingers;
> > + unsigned int keycount;
> > + unsigned int keycodes[HX852X_MAX_KEY_COUNT];
> > +};
> > +
> > +struct hx852x_config {
> > + u8 rx_num;
> > + u8 tx_num;
> > + u8 max_pt;
> > + u8 padding1[3];
> > + __be16 x_res;
> > + __be16 y_res;
> > + u8 padding2[2];
> > +} __packed __aligned(4);
> > +
> > +struct hx852x_coord {
> > + __be16 x;
> > + __be16 y;
> > +} __packed __aligned(4);
> > +
> > +struct hx852x_touch_info {
> > + u8 finger_num;
> > + __le16 finger_pressed;
> > + u8 padding;
> > +} __packed __aligned(4);
> > +
> > +static int hx852x_i2c_read(struct hx852x *hx, u8 cmd, void *data, u16 len)
> > +{
> > + struct i2c_client *client = hx->client;
> > + int ret;
> > +
> > + struct i2c_msg msg[] = {
> > + {
> > + .addr = client->addr,
> > + .flags = 0,
> > + .len = 1,
> > + .buf = &cmd,
> > + },
> > + {
> > + .addr = client->addr,
> > + .flags = I2C_M_RD,
> > + .len = len,
> > + .buf = data,
> > + }
>
> Nit: standard practice is to close all array elements with a trailing comma,
> even the final element, in order to avoid an extra line in the diff (i.e.
> } --> },) if another element is added later. Obviously we wouldn't add more
> elements to this array in the future, but it's best to remain consistent.
>
Sure, I'll fix this in v3!
> > + };
> > +
> > + ret = i2c_transfer(client->adapter, msg, ARRAY_SIZE(msg));
> > + if (ret != ARRAY_SIZE(msg)) {
> > + dev_err(&client->dev, "failed to read %#x: %d\n", cmd, ret);
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int hx852x_power_on(struct hx852x *hx)
> > +{
> > + struct device *dev = &hx->client->dev;
> > + int error;
> > +
> > + error = regulator_bulk_enable(ARRAY_SIZE(hx->supplies), hx->supplies);
> > + if (error) {
> > + dev_err(dev, "failed to enable regulators: %d\n", error);
> > + return error;
> > + }
> > +
> > + gpiod_set_value_cansleep(hx->reset_gpiod, 1);
> > + msleep(20);
> > + gpiod_set_value_cansleep(hx->reset_gpiod, 0);
> > + msleep(20);
> > +
> > + return 0;
> > +}
> > +
> > +static int hx852x_start(struct hx852x *hx)
> > +{
> > + struct device *dev = &hx->client->dev;
> > + int error;
> > +
> > + error = i2c_smbus_write_byte(hx->client, HX852X_TS_SLEEP_OUT);
> > + if (error) {
> > + dev_err(dev, "failed to send TS_SLEEP_OUT: %d\n", error);
> > + return error;
> > + }
> > + msleep(30);
> > +
> > + error = i2c_smbus_write_byte(hx->client, HX852X_TS_SENSE_ON);
> > + if (error) {
> > + dev_err(dev, "failed to send TS_SENSE_ON: %d\n", error);
> > + return error;
> > + }
> > + msleep(20);
> > +
> > + return 0;
> > +}
> > +
> > +static int hx852x_stop(struct hx852x *hx)
> > +{
> > + struct device *dev = &hx->client->dev;
> > + int error;
> > +
> > + error = i2c_smbus_write_byte(hx->client, HX852X_TS_SENSE_OFF);
> > + if (error) {
> > + dev_err(dev, "failed to send TS_SENSE_OFF: %d\n", error);
> > + return error;
> > + }
> > + msleep(20);
> > +
> > + error = i2c_smbus_write_byte(hx->client, HX852X_TS_SLEEP_IN);
> > + if (error) {
> > + dev_err(dev, "failed to send TS_SLEEP_IN: %d\n", error);
> > + return error;
> > + }
> > + msleep(30);
> > +
> > + return 0;
> > +}
> > +
> > +static int hx852x_power_off(struct hx852x *hx)
> > +{
> > + struct device *dev = &hx->client->dev;
> > + int error;
> > +
> > + error = regulator_bulk_disable(ARRAY_SIZE(hx->supplies), hx->supplies);
> > + if (error)
> > + dev_err(dev, "failed to disable regulators: %d\n", error);
> > + return error;
> > +}
> > +
> > +static int hx852x_read_config(struct hx852x *hx)
> > +{
> > + struct device *dev = &hx->client->dev;
> > + struct hx852x_config conf;
> > + int x_res, y_res;
> > + int error;
> > +
> > + error = hx852x_power_on(hx);
> > + if (error)
> > + return error;
> > +
> > + /* Sensing must be turned on briefly to load the config */
> > + error = hx852x_start(hx);
> > + if (error)
> > + goto err_power_off;
> > +
> > + error = hx852x_stop(hx);
> > + if (error)
> > + goto err_power_off;
> > +
> > + error = i2c_smbus_write_byte_data(hx->client, HX852X_REG_SRAM_SWITCH,
> > + HX852X_SRAM_SWITCH_TEST_MODE);
> > + if (error)
> > + goto err_power_off;
> > +
> > + error = i2c_smbus_write_word_data(hx->client, HX852X_REG_SRAM_ADDR,
> > + HX852X_SRAM_ADDR_CONFIG);
> > + if (error)
> > + goto err_test_mode;
> > +
> > + error = hx852x_i2c_read(hx, HX852X_REG_FLASH_RPLACE, &conf, sizeof(conf));
> > + if (error)
> > + goto err_test_mode;
> > +
> > + x_res = be16_to_cpu(conf.x_res);
> > + y_res = be16_to_cpu(conf.y_res);
> > + hx->max_fingers = (conf.max_pt & 0xf0) >> 4;
> > + dev_dbg(dev, "x res: %u, y res: %u, max fingers: %u\n",
> > + x_res, y_res, hx->max_fingers);
> > +
> > + if (hx->max_fingers > HX852X_MAX_FINGERS) {
> > + dev_err(dev, "max supported fingers: %u, found: %u\n",
> > + HX852X_MAX_FINGERS, hx->max_fingers);
> > + error = -EINVAL;
> > + goto err_test_mode;
> > + }
> > +
> > + if (x_res && y_res) {
> > + input_set_abs_params(hx->input_dev, ABS_MT_POSITION_X, 0, x_res - 1, 0, 0);
> > + input_set_abs_params(hx->input_dev, ABS_MT_POSITION_Y, 0, y_res - 1, 0, 0);
> > + }
> > +
> > + error = i2c_smbus_write_byte_data(hx->client, HX852X_REG_SRAM_SWITCH, 0);
> > + if (error)
> > + goto err_power_off;
> > +
> > + return hx852x_power_off(hx);
> > +
> > +err_test_mode:
> > + i2c_smbus_write_byte_data(hx->client, HX852X_REG_SRAM_SWITCH, 0);
> > +err_power_off:
> > + hx852x_power_off(hx);
> > + return error;
>
> Your new version is an improvement, but maybe we can remove duplicate
> code by introducing some helper variables:
>
> int error, error2 = 0, error3;
>
> /* ... */
>
> err_test_mode:
> error2 = i2c_smbus_write_byte_data(...);
>
> err_power_off:
> error3 = hx852x_power_off(...);
>
> if (error)
> return error;
>
> return error2 ? : error3;
>
> In this case we achieve our goal of attempting to return the device to a
> safe state in both passing and failing cases. In the event of multiple
> errors, we return the first to occur.
>
Right, this would work as well. Personally I think my solution is
slightly easier to read though. In your version my eyes somewhat
"stumble" over the multiple "error" variables and then about the purpose
of the "? : " construction. This is probably highly subjective. :-)
Do you mind if keep this as-is? I don't have a strong opinion about
this, but a slight preference for my current solution.
> > +}
> > +
> > +static int hx852x_handle_events(struct hx852x *hx)
> > +{
> > + /*
> > + * The event packets have variable size, depending on the amount of
> > + * supported fingers (hx->max_fingers). They are laid out as follows:
> > + * - struct hx852x_coord[hx->max_fingers]: Coordinates for each finger
> > + * - u8[ALIGN(hx->max_fingers, 4)]: Touch width for each finger
> > + * with padding for 32-bit alignment
> > + * - struct hx852x_touch_info
> > + *
> > + * Load everything into a 32-bit aligned buffer so the coordinates
> > + * can be assigned directly, without using get_unaligned_*().
> > + */
> > + u8 buf[HX852X_MAX_BUF_SIZE] __aligned(4);
> > + struct hx852x_coord *coord = (struct hx852x_coord *)buf;
> > + u8 *width = &buf[HX852X_COORD_SIZE(hx->max_fingers)];
> > + struct hx852x_touch_info *info = (struct hx852x_touch_info *)
> > + &width[HX852X_WIDTH_SIZE(hx->max_fingers)];
> > + unsigned long finger_pressed, key_pressed;
> > + unsigned int i, x, y, w;
> > + int error;
> > +
> > + error = hx852x_i2c_read(hx, HX852X_READ_ALL_EVENTS, buf,
> > + HX852X_BUF_SIZE(hx->max_fingers));
> > + if (error)
> > + return error;
> > +
> > + finger_pressed = get_unaligned_le16(&info->finger_pressed);
> > + key_pressed = finger_pressed >> HX852X_MAX_FINGERS;
> > +
> > + /* All bits are set when no touch is detected */
> > + if (info->finger_num == 0xff || !(info->finger_num & 0x0f))
> > + finger_pressed = 0;
> > + if (key_pressed == 0xf)
> > + key_pressed = 0;
> > +
> > + for_each_set_bit(i, &finger_pressed, hx->max_fingers) {
> > + x = be16_to_cpu(coord[i].x);
> > + y = be16_to_cpu(coord[i].y);
> > + w = width[i];
> > +
> > + input_mt_slot(hx->input_dev, i);
> > + input_mt_report_slot_state(hx->input_dev, MT_TOOL_FINGER, 1);
> > + touchscreen_report_pos(hx->input_dev, &hx->props, x, y, true);
> > + input_report_abs(hx->input_dev, ABS_MT_TOUCH_MAJOR, w);
> > + }
> > + input_mt_sync_frame(hx->input_dev);
> > +
> > + for (i = 0; i < hx->keycount; i++)
> > + input_report_key(hx->input_dev, hx->keycodes[i], key_pressed & BIT(i));
> > +
> > + input_sync(hx->input_dev);
> > + return 0;
> > +}
> > +
> > +static irqreturn_t hx852x_interrupt(int irq, void *ptr)
> > +{
> > + struct hx852x *hx = ptr;
> > + int error;
> > +
> > + error = hx852x_handle_events(hx);
> > + if (error) {
> > + dev_err_ratelimited(&hx->client->dev, "failed to handle events: %d\n", error);
> > + return IRQ_NONE;
> > + }
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static int hx852x_input_open(struct input_dev *dev)
> > +{
> > + struct hx852x *hx = input_get_drvdata(dev);
> > + int error;
> > +
> > + error = hx852x_power_on(hx);
> > + if (error)
> > + return error;
> > +
> > + error = hx852x_start(hx);
> > + if (error) {
> > + hx852x_power_off(hx);
> > + return error;
> > + }
> > +
> > + enable_irq(hx->client->irq);
> > + return 0;
> > +}
> > +
> > +static void hx852x_input_close(struct input_dev *dev)
> > +{
> > + struct hx852x *hx = input_get_drvdata(dev);
> > +
> > + hx852x_stop(hx);
> > + disable_irq(hx->client->irq);
> > + hx852x_power_off(hx);
> > +}
> > +
> > +static int hx852x_parse_properties(struct hx852x *hx)
> > +{
> > + struct device *dev = &hx->client->dev;
> > + int error;
> > +
> > + hx->keycount = device_property_count_u32(dev, "linux,keycodes");
> > + if (hx->keycount <= 0) {
> > + hx->keycount = 0;
> > + return 0;
> > + }
>
> With negative return values of device_property_count_u32() squashed in this
> way, there is no way to warn the developer that the array is invalid. The
> error message associated with device_property_read_u32_array() below is
> essentially dead code, because the latter is a wrapper around the former,
> and any syntax error that causes one to fail will cause the other to fail.
>
> Since device_property_count_u32() returns -EINVAL in case of a missing array,
> we can do the following. Note there is no need to explicitly set hx->keycount
> to zero in case no keys are specified, since the hx structure is correctly
> allocated using devm_kzalloc().
>
> int count;
>
> count = device_property_count_u32(...);
> if (count == -EINVAL) {
> return 0;
> } else if (count < 0) {
> dev_err(...);
> return count;
> } else if (count > HX852X_MAX_KEY_COUNT) {
> dev_err(...);
> return -EINVAL;
> }
>
> hx->keycount = count;
>
> error = device_property_read_u32_array(...);
> if (error)
> dev_err(...);
>
> return error;
>
Ah right, thanks for spotting this! Will use this in v3.
> > +
> > + if (hx->keycount > HX852X_MAX_KEY_COUNT) {
> > + dev_err(dev, "max supported keys: %u, found: %u\n",
> > + HX852X_MAX_KEY_COUNT, hx->keycount);
> > + return -EINVAL;
> > + }
> > +
> > + error = device_property_read_u32_array(dev, "linux,keycodes",
> > + hx->keycodes, hx->keycount);
> > + if (error)
> > + dev_err(dev, "failed to read linux,keycodes: %d\n", error);
> > +
> > + return error;
> > +}
> > +
> > +static int hx852x_probe(struct i2c_client *client)
> > +{
> > + struct device *dev = &client->dev;
> > + struct hx852x *hx;
> > + int error, i;
> > +
> > + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C |
> > + I2C_FUNC_SMBUS_WRITE_BYTE |
> > + I2C_FUNC_SMBUS_WRITE_BYTE_DATA |
> > + I2C_FUNC_SMBUS_WRITE_WORD_DATA)) {
> > + dev_err(dev, "not all required i2c functionality supported\n");
> > + return -ENXIO;
> > + }
> > +
> > + hx = devm_kzalloc(dev, sizeof(*hx), GFP_KERNEL);
> > + if (!hx)
> > + return -ENOMEM;
> > +
> > + hx->client = client;
> > + hx->input_dev = devm_input_allocate_device(dev);
> > + if (!hx->input_dev)
> > + return -ENOMEM;
> > +
> > + hx->input_dev->name = "Himax HX852x";
> > + hx->input_dev->id.bustype = BUS_I2C;
> > + hx->input_dev->open = hx852x_input_open;
> > + hx->input_dev->close = hx852x_input_close;
> > +
> > + i2c_set_clientdata(client, hx);
> > + input_set_drvdata(hx->input_dev, hx);
> > +
> > + hx->supplies[0].supply = "vcca";
> > + hx->supplies[1].supply = "vccd";
> > + error = devm_regulator_bulk_get(dev, ARRAY_SIZE(hx->supplies), hx->supplies);
> > + if (error)
> > + return dev_err_probe(dev, error, "failed to get regulators\n");
> > +
> > + hx->reset_gpiod = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> > + if (IS_ERR(hx->reset_gpiod))
> > + return dev_err_probe(dev, PTR_ERR(hx->reset_gpiod),
> > + "failed to get reset gpio\n");
> > +
> > + error = devm_request_threaded_irq(dev, client->irq, NULL, hx852x_interrupt,
> > + IRQF_ONESHOT | IRQF_NO_AUTOEN, NULL, hx);
> > + if (error)
> > + return dev_err_probe(dev, error, "failed to request irq %d", client->irq);
> > +
> > + error = hx852x_read_config(hx);
> > + if (error)
> > + return error;
> > +
> > + input_set_capability(hx->input_dev, EV_ABS, ABS_MT_POSITION_X);
> > + input_set_capability(hx->input_dev, EV_ABS, ABS_MT_POSITION_Y);
> > + input_set_abs_params(hx->input_dev, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
> > +
> > + touchscreen_parse_properties(hx->input_dev, true, &hx->props);
> > + error = hx852x_parse_properties(hx);
> > + if (error)
> > + return error;
> > +
> > + hx->input_dev->keycode = hx->keycodes;
> > + hx->input_dev->keycodemax = hx->keycount;
> > + hx->input_dev->keycodesize = sizeof(hx->keycodes[0]);
> > + for (i = 0; i < hx->keycount; i++)
> > + input_set_capability(hx->input_dev, EV_KEY, hx->keycodes[i]);
> > +
> > + error = input_mt_init_slots(hx->input_dev, hx->max_fingers,
> > + INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
> > + if (error)
> > + return dev_err_probe(dev, error, "failed to init MT slots\n");
> > +
> > + error = input_register_device(hx->input_dev);
> > + if (error)
> > + return dev_err_probe(dev, error, "failed to register input device\n");
> > +
> > + return 0;
> > +}
> > +
> > +static int hx852x_suspend(struct device *dev)
> > +{
> > + struct hx852x *hx = dev_get_drvdata(dev);
>
> int error = 0;
>
> > +
> > + mutex_lock(&hx->input_dev->mutex);
> > + if (input_device_enabled(hx->input_dev))
> > + hx852x_stop(hx);
>
> error = hx852x_stop(...);
>
> > + mutex_unlock(&hx->input_dev->mutex);
> > +
> > + return 0;
>
> return error;
>
Oops. I meant to change this in v2 but forgot it apparently.
Will fix fix this in v3 as well!
I'll wait for your reply regarding the error handling in
hx852x_read_config(), in case you have a strong opinion about that. :)
Thanks!
Stephan
^ permalink raw reply
* Re: [PATCH 2/2] Input: add Himax HX852x(ES) touchscreen driver
From: Jeff LaBundy @ 2023-10-22 18:28 UTC (permalink / raw)
To: Stephan Gerhold
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Henrik Rydberg, linux-input, devicetree, linux-kernel,
Jonathan Albrieux
In-Reply-To: <ZRg-uZskk07jxup0@gerhold.net>
Hi Stephan,
On Sat, Sep 30, 2023 at 05:28:57PM +0200, Stephan Gerhold wrote:
[...]
> In v2 I have added linux/of.h and linux/mod_devicetable.h, since I'm
> actually using definitions from these two only. Seems like including
> of_device.h is discouraged nowadays, see commit dbce1a7d5dce ("Input:
> Explicitly include correct DT includes").
Apologies for the delayed response and some confusion from my side. What
you have added in v2 is correct, and what I should have suggested in the
first place.
[...]
> > Nit: it would still be nice to preserve as many return values as possible, perhaps
> > as follows:
> >
> > +exit_test_mode:
> > error = i2c_smbus_write_byte_data(...) ? : error;
> >
> > > +power_off:
> > > + hx852x_power_off(hx);
> > > + return error;
> >
> > Similarly, with hx852x_power_off() being promoted to int as suggested above,
> > this could be:
> >
> > return hx852x_power_off(...) ? : error;
> >
> > There are other idiomatic ways to do the same thing based on your preference.
> > Another (perhaps more clear) option would be to move some of these test mode
> > functions into a helper, which would also avoid some goto statements.
> >
>
> I played with this for a bit. A problem of the "? : error" approach is
> that it hides the original error in case the new calls error again.
That's correct; good catch.
>
> Let's assume
>
> error = hx852x_start(hx);
> if (error)
> goto power_off;
>
> fails with error = -ENXIO. We jump to power_off:
>
> power_off:
> return hx852x_power_off(hx) ? : error;
>
> Let's say for whatever reason hx852x_power_off() fails too but returns
> -EINVAL. Then the final return value will be -EINVAL, while with the
> current approach in this patch it would return the original cause
> (-ENXIO). I think that's more clear.
>
> I also played with moving code to a separate function to avoid the
> gotos, but I feel like that makes the fairly focused logic of this
> function (reading the configuration by temporarily entering the test
> mode) just more confusing.
>
> To still fix the error handling I ended up with duplicating the
> "success" code path and the "error" code path (it's just two function
> calls), i.e.:
>
> error = i2c_smbus_write_byte_data(hx->client, HX852X_REG_SRAM_SWITCH, 0);
> if (error)
> goto err_power_off;
>
> return hx852x_power_off(hx);
>
> err_test_mode:
> i2c_smbus_write_byte_data(hx->client, HX852X_REG_SRAM_SWITCH, 0);
> err_power_off:
> hx852x_power_off(hx);
> return error;
>
> I hope that's fine too. A bit ugly maybe but in this case I would prefer
> having the main code path (reading the configuration) clearly readable.
>
> Let me know if you have a better suggestion for these (I'll send v2 in a
> bit so that you can see the full diff there).
Maybe we can massage this just a bit more; I have followed up with another
suggestion in v2.
>
> Thanks!
> Stephan
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH v2 2/2] Input: add Himax HX852x(ES) touchscreen driver
From: Jeff LaBundy @ 2023-10-22 18:23 UTC (permalink / raw)
To: Stephan Gerhold
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Henrik Rydberg, linux-input, devicetree, linux-kernel,
Christophe JAILLET, Jonathan Albrieux
In-Reply-To: <20230930-hx852x-v2-2-c5821947b225@gerhold.net>
Hi Stephan,
This looks great, just a few remaining comments.
On Sat, Sep 30, 2023 at 05:32:01PM +0200, Stephan Gerhold wrote:
> From: Jonathan Albrieux <jonathan.albrieux@gmail.com>
>
> Add a simple driver for the Himax HX852x(ES) touch panel controller,
> with support for multi-touch and capacitive touch keys.
>
> The driver is somewhat based on sample code from Himax. However, that
> code was so extremely confusing that we spent a significant amount of
> time just trying to understand the packet format and register commands.
> In this driver they are described with clean structs and defines rather
> than lots of magic numbers and offset calculations.
>
> Signed-off-by: Jonathan Albrieux <jonathan.albrieux@gmail.com>
> Co-developed-by: Stephan Gerhold <stephan@gerhold.net>
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> ---
> MAINTAINERS | 7 +
> drivers/input/touchscreen/Kconfig | 10 +
> drivers/input/touchscreen/Makefile | 1 +
> drivers/input/touchscreen/himax_hx852x.c | 499 +++++++++++++++++++++++++++++++
> 4 files changed, 517 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 90f13281d297..c551c60b0598 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9331,6 +9331,13 @@ S: Maintained
> F: Documentation/devicetree/bindings/input/touchscreen/himax,hx83112b.yaml
> F: drivers/input/touchscreen/himax_hx83112b.c
>
> +HIMAX HX852X TOUCHSCREEN DRIVER
> +M: Stephan Gerhold <stephan@gerhold.net>
> +L: linux-input@vger.kernel.org
> +S: Maintained
> +F: Documentation/devicetree/bindings/input/touchscreen/himax,hx852es.yaml
> +F: drivers/input/touchscreen/himax_hx852x.c
> +
> HIPPI
> M: Jes Sorensen <jes@trained-monkey.org>
> L: linux-hippi@sunsite.dk
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index e3e2324547b9..8e5667ae5dab 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -427,6 +427,16 @@ config TOUCHSCREEN_HIDEEP
> To compile this driver as a module, choose M here : the
> module will be called hideep_ts.
>
> +config TOUCHSCREEN_HIMAX_HX852X
> + tristate "Himax HX852x(ES) touchscreen"
> + depends on I2C
> + help
> + Say Y here if you have a Himax HX852x(ES) touchscreen.
> + If unsure, say N.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called himax_hx852x.
> +
> config TOUCHSCREEN_HYCON_HY46XX
> tristate "Hycon hy46xx touchscreen support"
> depends on I2C
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index 62bd24f3ac8e..f42a87faa86c 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -48,6 +48,7 @@ obj-$(CONFIG_TOUCHSCREEN_EXC3000) += exc3000.o
> obj-$(CONFIG_TOUCHSCREEN_FUJITSU) += fujitsu_ts.o
> obj-$(CONFIG_TOUCHSCREEN_GOODIX) += goodix_ts.o
> obj-$(CONFIG_TOUCHSCREEN_HIDEEP) += hideep.o
> +obj-$(CONFIG_TOUCHSCREEN_HIMAX_HX852X) += himax_hx852x.o
> obj-$(CONFIG_TOUCHSCREEN_HYNITRON_CSTXXX) += hynitron_cstxxx.o
> obj-$(CONFIG_TOUCHSCREEN_ILI210X) += ili210x.o
> obj-$(CONFIG_TOUCHSCREEN_ILITEK) += ilitek_ts_i2c.o
> diff --git a/drivers/input/touchscreen/himax_hx852x.c b/drivers/input/touchscreen/himax_hx852x.c
> new file mode 100644
> index 000000000000..98a55be7891d
> --- /dev/null
> +++ b/drivers/input/touchscreen/himax_hx852x.c
> @@ -0,0 +1,499 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Himax HX852x(ES) Touchscreen Driver
> + * Copyright (c) 2020-2023 Stephan Gerhold <stephan@gerhold.net>
> + * Copyright (c) 2020 Jonathan Albrieux <jonathan.albrieux@gmail.com>
> + *
> + * Based on the Himax Android Driver Sample Code Ver 0.3 for HMX852xES chipset:
> + * Copyright (c) 2014 Himax Corporation.
> + */
> +
> +#include <asm/unaligned.h>
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/input.h>
> +#include <linux/input/mt.h>
> +#include <linux/input/touchscreen.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/regulator/consumer.h>
> +
> +#define HX852X_COORD_SIZE(fingers) ((fingers) * sizeof(struct hx852x_coord))
> +#define HX852X_WIDTH_SIZE(fingers) ALIGN(fingers, 4)
> +#define HX852X_BUF_SIZE(fingers) (HX852X_COORD_SIZE(fingers) + \
> + HX852X_WIDTH_SIZE(fingers) + \
> + sizeof(struct hx852x_touch_info))
> +
> +#define HX852X_MAX_FINGERS 12
> +#define HX852X_MAX_KEY_COUNT 4
> +#define HX852X_MAX_BUF_SIZE HX852X_BUF_SIZE(HX852X_MAX_FINGERS)
> +
> +#define HX852X_TS_SLEEP_IN 0x80
> +#define HX852X_TS_SLEEP_OUT 0x81
> +#define HX852X_TS_SENSE_OFF 0x82
> +#define HX852X_TS_SENSE_ON 0x83
> +#define HX852X_READ_ONE_EVENT 0x85
> +#define HX852X_READ_ALL_EVENTS 0x86
> +#define HX852X_READ_LATEST_EVENT 0x87
> +#define HX852X_CLEAR_EVENT_STACK 0x88
> +
> +#define HX852X_REG_SRAM_SWITCH 0x8c
> +#define HX852X_REG_SRAM_ADDR 0x8b
> +#define HX852X_REG_FLASH_RPLACE 0x5a
> +
> +#define HX852X_SRAM_SWITCH_TEST_MODE 0x14
> +#define HX852X_SRAM_ADDR_CONFIG 0x7000
> +
> +struct hx852x {
> + struct i2c_client *client;
> + struct input_dev *input_dev;
> + struct touchscreen_properties props;
> + struct gpio_desc *reset_gpiod;
> + struct regulator_bulk_data supplies[2];
> + unsigned int max_fingers;
> + unsigned int keycount;
> + unsigned int keycodes[HX852X_MAX_KEY_COUNT];
> +};
> +
> +struct hx852x_config {
> + u8 rx_num;
> + u8 tx_num;
> + u8 max_pt;
> + u8 padding1[3];
> + __be16 x_res;
> + __be16 y_res;
> + u8 padding2[2];
> +} __packed __aligned(4);
> +
> +struct hx852x_coord {
> + __be16 x;
> + __be16 y;
> +} __packed __aligned(4);
> +
> +struct hx852x_touch_info {
> + u8 finger_num;
> + __le16 finger_pressed;
> + u8 padding;
> +} __packed __aligned(4);
> +
> +static int hx852x_i2c_read(struct hx852x *hx, u8 cmd, void *data, u16 len)
> +{
> + struct i2c_client *client = hx->client;
> + int ret;
> +
> + struct i2c_msg msg[] = {
> + {
> + .addr = client->addr,
> + .flags = 0,
> + .len = 1,
> + .buf = &cmd,
> + },
> + {
> + .addr = client->addr,
> + .flags = I2C_M_RD,
> + .len = len,
> + .buf = data,
> + }
Nit: standard practice is to close all array elements with a trailing comma,
even the final element, in order to avoid an extra line in the diff (i.e.
} --> },) if another element is added later. Obviously we wouldn't add more
elements to this array in the future, but it's best to remain consistent.
> + };
> +
> + ret = i2c_transfer(client->adapter, msg, ARRAY_SIZE(msg));
> + if (ret != ARRAY_SIZE(msg)) {
> + dev_err(&client->dev, "failed to read %#x: %d\n", cmd, ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int hx852x_power_on(struct hx852x *hx)
> +{
> + struct device *dev = &hx->client->dev;
> + int error;
> +
> + error = regulator_bulk_enable(ARRAY_SIZE(hx->supplies), hx->supplies);
> + if (error) {
> + dev_err(dev, "failed to enable regulators: %d\n", error);
> + return error;
> + }
> +
> + gpiod_set_value_cansleep(hx->reset_gpiod, 1);
> + msleep(20);
> + gpiod_set_value_cansleep(hx->reset_gpiod, 0);
> + msleep(20);
> +
> + return 0;
> +}
> +
> +static int hx852x_start(struct hx852x *hx)
> +{
> + struct device *dev = &hx->client->dev;
> + int error;
> +
> + error = i2c_smbus_write_byte(hx->client, HX852X_TS_SLEEP_OUT);
> + if (error) {
> + dev_err(dev, "failed to send TS_SLEEP_OUT: %d\n", error);
> + return error;
> + }
> + msleep(30);
> +
> + error = i2c_smbus_write_byte(hx->client, HX852X_TS_SENSE_ON);
> + if (error) {
> + dev_err(dev, "failed to send TS_SENSE_ON: %d\n", error);
> + return error;
> + }
> + msleep(20);
> +
> + return 0;
> +}
> +
> +static int hx852x_stop(struct hx852x *hx)
> +{
> + struct device *dev = &hx->client->dev;
> + int error;
> +
> + error = i2c_smbus_write_byte(hx->client, HX852X_TS_SENSE_OFF);
> + if (error) {
> + dev_err(dev, "failed to send TS_SENSE_OFF: %d\n", error);
> + return error;
> + }
> + msleep(20);
> +
> + error = i2c_smbus_write_byte(hx->client, HX852X_TS_SLEEP_IN);
> + if (error) {
> + dev_err(dev, "failed to send TS_SLEEP_IN: %d\n", error);
> + return error;
> + }
> + msleep(30);
> +
> + return 0;
> +}
> +
> +static int hx852x_power_off(struct hx852x *hx)
> +{
> + struct device *dev = &hx->client->dev;
> + int error;
> +
> + error = regulator_bulk_disable(ARRAY_SIZE(hx->supplies), hx->supplies);
> + if (error)
> + dev_err(dev, "failed to disable regulators: %d\n", error);
> + return error;
> +}
> +
> +static int hx852x_read_config(struct hx852x *hx)
> +{
> + struct device *dev = &hx->client->dev;
> + struct hx852x_config conf;
> + int x_res, y_res;
> + int error;
> +
> + error = hx852x_power_on(hx);
> + if (error)
> + return error;
> +
> + /* Sensing must be turned on briefly to load the config */
> + error = hx852x_start(hx);
> + if (error)
> + goto err_power_off;
> +
> + error = hx852x_stop(hx);
> + if (error)
> + goto err_power_off;
> +
> + error = i2c_smbus_write_byte_data(hx->client, HX852X_REG_SRAM_SWITCH,
> + HX852X_SRAM_SWITCH_TEST_MODE);
> + if (error)
> + goto err_power_off;
> +
> + error = i2c_smbus_write_word_data(hx->client, HX852X_REG_SRAM_ADDR,
> + HX852X_SRAM_ADDR_CONFIG);
> + if (error)
> + goto err_test_mode;
> +
> + error = hx852x_i2c_read(hx, HX852X_REG_FLASH_RPLACE, &conf, sizeof(conf));
> + if (error)
> + goto err_test_mode;
> +
> + x_res = be16_to_cpu(conf.x_res);
> + y_res = be16_to_cpu(conf.y_res);
> + hx->max_fingers = (conf.max_pt & 0xf0) >> 4;
> + dev_dbg(dev, "x res: %u, y res: %u, max fingers: %u\n",
> + x_res, y_res, hx->max_fingers);
> +
> + if (hx->max_fingers > HX852X_MAX_FINGERS) {
> + dev_err(dev, "max supported fingers: %u, found: %u\n",
> + HX852X_MAX_FINGERS, hx->max_fingers);
> + error = -EINVAL;
> + goto err_test_mode;
> + }
> +
> + if (x_res && y_res) {
> + input_set_abs_params(hx->input_dev, ABS_MT_POSITION_X, 0, x_res - 1, 0, 0);
> + input_set_abs_params(hx->input_dev, ABS_MT_POSITION_Y, 0, y_res - 1, 0, 0);
> + }
> +
> + error = i2c_smbus_write_byte_data(hx->client, HX852X_REG_SRAM_SWITCH, 0);
> + if (error)
> + goto err_power_off;
> +
> + return hx852x_power_off(hx);
> +
> +err_test_mode:
> + i2c_smbus_write_byte_data(hx->client, HX852X_REG_SRAM_SWITCH, 0);
> +err_power_off:
> + hx852x_power_off(hx);
> + return error;
Your new version is an improvement, but maybe we can remove duplicate
code by introducing some helper variables:
int error, error2 = 0, error3;
/* ... */
err_test_mode:
error2 = i2c_smbus_write_byte_data(...);
err_power_off:
error3 = hx852x_power_off(...);
if (error)
return error;
return error2 ? : error3;
In this case we achieve our goal of attempting to return the device to a
safe state in both passing and failing cases. In the event of multiple
errors, we return the first to occur.
> +}
> +
> +static int hx852x_handle_events(struct hx852x *hx)
> +{
> + /*
> + * The event packets have variable size, depending on the amount of
> + * supported fingers (hx->max_fingers). They are laid out as follows:
> + * - struct hx852x_coord[hx->max_fingers]: Coordinates for each finger
> + * - u8[ALIGN(hx->max_fingers, 4)]: Touch width for each finger
> + * with padding for 32-bit alignment
> + * - struct hx852x_touch_info
> + *
> + * Load everything into a 32-bit aligned buffer so the coordinates
> + * can be assigned directly, without using get_unaligned_*().
> + */
> + u8 buf[HX852X_MAX_BUF_SIZE] __aligned(4);
> + struct hx852x_coord *coord = (struct hx852x_coord *)buf;
> + u8 *width = &buf[HX852X_COORD_SIZE(hx->max_fingers)];
> + struct hx852x_touch_info *info = (struct hx852x_touch_info *)
> + &width[HX852X_WIDTH_SIZE(hx->max_fingers)];
> + unsigned long finger_pressed, key_pressed;
> + unsigned int i, x, y, w;
> + int error;
> +
> + error = hx852x_i2c_read(hx, HX852X_READ_ALL_EVENTS, buf,
> + HX852X_BUF_SIZE(hx->max_fingers));
> + if (error)
> + return error;
> +
> + finger_pressed = get_unaligned_le16(&info->finger_pressed);
> + key_pressed = finger_pressed >> HX852X_MAX_FINGERS;
> +
> + /* All bits are set when no touch is detected */
> + if (info->finger_num == 0xff || !(info->finger_num & 0x0f))
> + finger_pressed = 0;
> + if (key_pressed == 0xf)
> + key_pressed = 0;
> +
> + for_each_set_bit(i, &finger_pressed, hx->max_fingers) {
> + x = be16_to_cpu(coord[i].x);
> + y = be16_to_cpu(coord[i].y);
> + w = width[i];
> +
> + input_mt_slot(hx->input_dev, i);
> + input_mt_report_slot_state(hx->input_dev, MT_TOOL_FINGER, 1);
> + touchscreen_report_pos(hx->input_dev, &hx->props, x, y, true);
> + input_report_abs(hx->input_dev, ABS_MT_TOUCH_MAJOR, w);
> + }
> + input_mt_sync_frame(hx->input_dev);
> +
> + for (i = 0; i < hx->keycount; i++)
> + input_report_key(hx->input_dev, hx->keycodes[i], key_pressed & BIT(i));
> +
> + input_sync(hx->input_dev);
> + return 0;
> +}
> +
> +static irqreturn_t hx852x_interrupt(int irq, void *ptr)
> +{
> + struct hx852x *hx = ptr;
> + int error;
> +
> + error = hx852x_handle_events(hx);
> + if (error) {
> + dev_err_ratelimited(&hx->client->dev, "failed to handle events: %d\n", error);
> + return IRQ_NONE;
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int hx852x_input_open(struct input_dev *dev)
> +{
> + struct hx852x *hx = input_get_drvdata(dev);
> + int error;
> +
> + error = hx852x_power_on(hx);
> + if (error)
> + return error;
> +
> + error = hx852x_start(hx);
> + if (error) {
> + hx852x_power_off(hx);
> + return error;
> + }
> +
> + enable_irq(hx->client->irq);
> + return 0;
> +}
> +
> +static void hx852x_input_close(struct input_dev *dev)
> +{
> + struct hx852x *hx = input_get_drvdata(dev);
> +
> + hx852x_stop(hx);
> + disable_irq(hx->client->irq);
> + hx852x_power_off(hx);
> +}
> +
> +static int hx852x_parse_properties(struct hx852x *hx)
> +{
> + struct device *dev = &hx->client->dev;
> + int error;
> +
> + hx->keycount = device_property_count_u32(dev, "linux,keycodes");
> + if (hx->keycount <= 0) {
> + hx->keycount = 0;
> + return 0;
> + }
With negative return values of device_property_count_u32() squashed in this
way, there is no way to warn the developer that the array is invalid. The
error message associated with device_property_read_u32_array() below is
essentially dead code, because the latter is a wrapper around the former,
and any syntax error that causes one to fail will cause the other to fail.
Since device_property_count_u32() returns -EINVAL in case of a missing array,
we can do the following. Note there is no need to explicitly set hx->keycount
to zero in case no keys are specified, since the hx structure is correctly
allocated using devm_kzalloc().
int count;
count = device_property_count_u32(...);
if (count == -EINVAL) {
return 0;
} else if (count < 0) {
dev_err(...);
return count;
} else if (count > HX852X_MAX_KEY_COUNT) {
dev_err(...);
return -EINVAL;
}
hx->keycount = count;
error = device_property_read_u32_array(...);
if (error)
dev_err(...);
return error;
> +
> + if (hx->keycount > HX852X_MAX_KEY_COUNT) {
> + dev_err(dev, "max supported keys: %u, found: %u\n",
> + HX852X_MAX_KEY_COUNT, hx->keycount);
> + return -EINVAL;
> + }
> +
> + error = device_property_read_u32_array(dev, "linux,keycodes",
> + hx->keycodes, hx->keycount);
> + if (error)
> + dev_err(dev, "failed to read linux,keycodes: %d\n", error);
> +
> + return error;
> +}
> +
> +static int hx852x_probe(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + struct hx852x *hx;
> + int error, i;
> +
> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C |
> + I2C_FUNC_SMBUS_WRITE_BYTE |
> + I2C_FUNC_SMBUS_WRITE_BYTE_DATA |
> + I2C_FUNC_SMBUS_WRITE_WORD_DATA)) {
> + dev_err(dev, "not all required i2c functionality supported\n");
> + return -ENXIO;
> + }
> +
> + hx = devm_kzalloc(dev, sizeof(*hx), GFP_KERNEL);
> + if (!hx)
> + return -ENOMEM;
> +
> + hx->client = client;
> + hx->input_dev = devm_input_allocate_device(dev);
> + if (!hx->input_dev)
> + return -ENOMEM;
> +
> + hx->input_dev->name = "Himax HX852x";
> + hx->input_dev->id.bustype = BUS_I2C;
> + hx->input_dev->open = hx852x_input_open;
> + hx->input_dev->close = hx852x_input_close;
> +
> + i2c_set_clientdata(client, hx);
> + input_set_drvdata(hx->input_dev, hx);
> +
> + hx->supplies[0].supply = "vcca";
> + hx->supplies[1].supply = "vccd";
> + error = devm_regulator_bulk_get(dev, ARRAY_SIZE(hx->supplies), hx->supplies);
> + if (error)
> + return dev_err_probe(dev, error, "failed to get regulators\n");
> +
> + hx->reset_gpiod = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> + if (IS_ERR(hx->reset_gpiod))
> + return dev_err_probe(dev, PTR_ERR(hx->reset_gpiod),
> + "failed to get reset gpio\n");
> +
> + error = devm_request_threaded_irq(dev, client->irq, NULL, hx852x_interrupt,
> + IRQF_ONESHOT | IRQF_NO_AUTOEN, NULL, hx);
> + if (error)
> + return dev_err_probe(dev, error, "failed to request irq %d", client->irq);
> +
> + error = hx852x_read_config(hx);
> + if (error)
> + return error;
> +
> + input_set_capability(hx->input_dev, EV_ABS, ABS_MT_POSITION_X);
> + input_set_capability(hx->input_dev, EV_ABS, ABS_MT_POSITION_Y);
> + input_set_abs_params(hx->input_dev, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
> +
> + touchscreen_parse_properties(hx->input_dev, true, &hx->props);
> + error = hx852x_parse_properties(hx);
> + if (error)
> + return error;
> +
> + hx->input_dev->keycode = hx->keycodes;
> + hx->input_dev->keycodemax = hx->keycount;
> + hx->input_dev->keycodesize = sizeof(hx->keycodes[0]);
> + for (i = 0; i < hx->keycount; i++)
> + input_set_capability(hx->input_dev, EV_KEY, hx->keycodes[i]);
> +
> + error = input_mt_init_slots(hx->input_dev, hx->max_fingers,
> + INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
> + if (error)
> + return dev_err_probe(dev, error, "failed to init MT slots\n");
> +
> + error = input_register_device(hx->input_dev);
> + if (error)
> + return dev_err_probe(dev, error, "failed to register input device\n");
> +
> + return 0;
> +}
> +
> +static int hx852x_suspend(struct device *dev)
> +{
> + struct hx852x *hx = dev_get_drvdata(dev);
int error = 0;
> +
> + mutex_lock(&hx->input_dev->mutex);
> + if (input_device_enabled(hx->input_dev))
> + hx852x_stop(hx);
error = hx852x_stop(...);
> + mutex_unlock(&hx->input_dev->mutex);
> +
> + return 0;
return error;
> +}
> +
> +static int hx852x_resume(struct device *dev)
> +{
> + struct hx852x *hx = dev_get_drvdata(dev);
> + int error = 0;
> +
> + mutex_lock(&hx->input_dev->mutex);
> + if (input_device_enabled(hx->input_dev))
> + error = hx852x_start(hx);
> + mutex_unlock(&hx->input_dev->mutex);
> +
> + return error;
> +}
> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(hx852x_pm_ops, hx852x_suspend, hx852x_resume);
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id hx852x_of_match[] = {
> + { .compatible = "himax,hx852es" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, hx852x_of_match);
> +#endif
> +
> +static struct i2c_driver hx852x_driver = {
> + .probe = hx852x_probe,
> + .driver = {
> + .name = "himax_hx852x",
> + .pm = pm_sleep_ptr(&hx852x_pm_ops),
> + .of_match_table = of_match_ptr(hx852x_of_match),
> + },
> +};
> +module_i2c_driver(hx852x_driver);
> +
> +MODULE_DESCRIPTION("Himax HX852x(ES) Touchscreen Driver");
> +MODULE_AUTHOR("Jonathan Albrieux <jonathan.albrieux@gmail.com>");
> +MODULE_AUTHOR("Stephan Gerhold <stephan@gerhold.net>");
> +MODULE_LICENSE("GPL");
>
> --
> 2.42.0
>
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH 2/2] input: Imagis: add support for the IST3032C touchscreen
From: Jeff LaBundy @ 2023-10-22 17:17 UTC (permalink / raw)
To: Karel Balej
Cc: Karel Balej, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Markuss Broks, linux-input, devicetree,
linux-kernel, Duje Mihanović, ~postmarketos/upstreaming
In-Reply-To: <CVUQTCJBG265.1MTSYOLY0FR7Q@gimli.ms.mff.cuni.cz>
Hi Karel,
On Thu, Sep 28, 2023 at 07:56:45PM +0200, Karel Balej wrote:
> Hello, Jeff,
>
> thank you very much for your feedback.
>
> > > + if (chip_id == IST3032C_WHOAMI) {
> > > + /*
> > > + * if the regulator voltage is not set like this, the touchscreen
> > > + * generates random touches without user interaction
> > > + */
> > > + error = regulator_set_voltage(ts->supplies[0].consumer, 3100000, 3100000);
> > > + if (error)
> > > + dev_warn(dev, "failed to set regulator voltage\n");
> > > + }
> > > +
> >
> > Opinions may vary, but mine is that this kind of hard-coded board-level policy
> > does not belong in the driver. Surely the supply need not be equal to exactly
> > 3.1 V and this is merely an upper or lower limit? Assuming so, what if the board
> > designer opts to share this supply with another consumer that requires a specific
> > voltage not equal to 3.1 V, but still within the safe range of IST3032C?
> >
> > To me, this restriction belongs in dts, specifically within the regulator child
> > node referenced by the client which bears the new 'ist3032c' compatible string.
> > Maybe a better solution is to simply note this presumed silicon erratum in the
> > description of the vdd supply in the binding which, as Conor states, must not be
> > clubbed with driver patches.
>
> I agree that the voltage should not be hardcoded. I do not know what the
> safe range for the touchscreen is though, because the downstream driver
> does exactly this. I will try to test it with several values within the
> range allowed by the regulator and see if I can determine some limits on
> when the "ghost" touches do not appear.
>
> However I am not sure whether this setting should be moved to the
> regulator DT - it is my understanding that the DT for the regulator
> should list the min/max range *supported* by the regulator, not conform
> to requirements of its consumers, which should instead ask for the
> regulator to be set to a range they require themselves, via their driver
> - is it not so?
>
> The regulator driver is not mainlined yet (although I managed to get the
> downstream code working with mainline), however the downstream DT
> contains much wider range of supported voltage (compared to those 3.1 V
> used by the touchscreen) - an information which would get lost if I set
> the DT for the regulator by the requirements of the touchscreen, which I
> believe would have similiar implications as what you said regarding
> using this regulator with other consumers.
>
> What would seem a reasonable solution to me would be to move the voltage
> range values to the touchscreen DT (which incidentally is what the
> downstream driver does also, except it uses one value for both min and
> max), so that they would be set by the driver but not hardcoded in the
> code - what do you think about this?
I believe this has been clarified in the other thread with Markuss, but
just to close this out: in general, individual drivers should not be
setting the output value of a regulator. Instead, they should merely mark
themselves as a consumer of a regulator, and increment or decrement its
usage counter by enabling and disabling the regulator, respectively.
You are correct that the regulator node typically specifies the minimum
and maximum voltages supported by the regulator, but if your board has
a stricter range because said regulator is tied to something such as this
touchscreen controller, then you can override the maximum voltage with
something smaller (e.g. 3.1 V). The regulator framework will then set the
output voltage according to the ultimate range defined in the device tree.
Often times the baseline regulator nodes are defined in a PMIC-specific
.dtsi file, or part of a more generic board definition. Then, some nodes
are overridden at a higher level in the heirarchy tailored to the board,
or an overlay applied by the bootloader.
>
> Best regards,
> K. B.
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH v2 0/2] Input: ilitek_ts_i2c - Fix spurious input events
From: Francesco Dolcini @ 2023-10-22 15:13 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Emanuele Ghidoli, Emanuele Ghidoli, linux-kernel, linux-input,
Joe Hung
In-Reply-To: <20230920074650.922292-1-ghidoliemanuele@gmail.com>
Hello Dmitry,
On Wed, Sep 20, 2023 at 09:46:48AM +0200, Emanuele Ghidoli wrote:
> From: Emanuele Ghidoli <emanuele.ghidoli@toradex.com>
>
> A couple of fixes to prevent spurious events when the data buffer is not the expected one.
>
> Emanuele Ghidoli (2):
> Input: ilitek_ts_i2c - avoid wrong input subsystem sync
> Input: ilitek_ts_i2c - add report id message validation
>
> drivers/input/touchscreen/ilitek_ts_i2c.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
Just a gently ping on this series.
Thanks,
Francesco
^ permalink raw reply
* Re: [PATCH v3 1/3] pwm: make it possible to apply pwm changes in atomic context
From: Sean Young @ 2023-10-22 10:46 UTC (permalink / raw)
To: Hans de Goede
Cc: Uwe Kleine-König, linux-media, linux-pwm, Ivaylo Dimitrov,
Thierry Reding, Jonathan Corbet, Jani Nikula, Joonas Lahtinen,
Rodrigo Vivi, Tvrtko Ursulin, David Airlie, Daniel Vetter,
Javier Martinez Canillas, Jean Delvare, Guenter Roeck,
Support Opensource, Dmitry Torokhov, Pavel Machek, Lee Jones,
Mauro Carvalho Chehab, Ilpo Järvinen, Mark Gross,
Liam Girdwood, Mark Brown, Daniel Thompson, Jingoo Han,
Helge Deller, linux-doc, linux-kernel, intel-gfx, dri-devel,
linux-hwmon, linux-input, linux-leds, platform-driver-x86,
linux-arm-kernel, linux-fbdev
In-Reply-To: <01a505ac-320f-3819-a58d-2b82c1bf2a86@redhat.com>
Hi Hans,
On Sat, Oct 21, 2023 at 11:08:22AM +0200, Hans de Goede wrote:
> On 10/19/23 12:51, Uwe Kleine-König wrote:
> > On Wed, Oct 18, 2023 at 03:57:48PM +0200, Hans de Goede wrote:
> >> On 10/17/23 11:17, Sean Young wrote:
> >>> Some drivers require sleeping, for example if the pwm device is connected
> >>> over i2c. The pwm-ir-tx requires precise timing, and sleeping causes havoc
> >>> with the generated IR signal when sleeping occurs.
> >>>
> >>> This patch makes it possible to use pwm when the driver does not sleep,
> >>> by introducing the pwm_can_sleep() function.
> >>>
> >>> Signed-off-by: Sean Young <sean@mess.org>
> >>
> >> I have no objection to this patch by itself, but it seems a bit
> >> of unnecessary churn to change all current callers of pwm_apply_state()
> >> to a new API.
> >
> > The idea is to improve the semantic of the function name, see
> > https://lore.kernel.org/linux-pwm/20231013180449.mcdmklbsz2rlymzz@pengutronix.de
> > for more context.
>
> Hmm, so the argument here is that the GPIO API has this, but GPIOs
> generally speaking can be set atomically, so there not being able
> to set it atomically is special.
>
> OTOH we have many many many other kernel functions which may sleep
> and we don't all postfix them with _can_sleep.
>
> And for PWM controllers pwm_apply_state is IMHO sorta expected to
> sleep. Many of these are attached over I2C so things will sleep,
> others have a handshake to wait for the current dutycycle to
> end before you can apply a second change on top of an earlier
> change during the current dutycycle which often also involves
> sleeping.
>
> So the natural/expeected thing for pwm_apply_state() is to sleep
> and thus it does not need a postfix for this IMHO.
Most pwm drivers look like they can be made to work in atomic context,
I think. Like you say this is not the case for all of them. Whatever
we choose to be the default for pwm_apply_state(), we should have a
clear function name for the alternative. This is essentially why
pam_apply_cansleep() was picked.
The alternative to pwm_apply_cansleep() is to have a function name
which implies it can be used from atomic context. However,
pwm_apply_atomic() is not great because the "atomic" could be
confused with the PWM atomic API, not the kernel process/atomic
context.
So what should the non-sleeping function be called then?
- pwm_apply_cannotsleep()
- pwm_apply_nosleep()
- pwm_apply_nonsleeping()
- pwm_apply_atomic_context()
> > I think it's very subjective if you consider this
> > churn or not.
>
> I consider it churn because I don't think adding a postfix
> for what is the default/expected behavior is a good idea
> (with GPIOs not sleeping is the expected behavior).
>
> I agree that this is very subjective and very much goes
> into the territory of bikeshedding. So please consider
> the above my 2 cents on this and lets leave it at that.
You have a valid point. Let's focus on having descriptive function names.
> > While it's nice to have every caller converted in a single
> > step, I'd go for
> >
> > #define pwm_apply_state(pwm, state) pwm_apply_cansleep(pwm, state)
> >
> > , keep that macro for a while and convert all users step by step. This
> > way we don't needlessly break oot code and the changes to convert to the
> > new API can go via their usual trees without time pressure.
>
> I don't think there are enough users of pwm_apply_state() to warrant
> such an exercise.
>
> So if people want to move ahead with the _can_sleep postfix addition
> (still not a fan) here is my acked-by for the drivers/platform/x86
> changes, for merging this through the PWM tree in a single commit:
>
> Acked-by: Hans de Goede <hdegoede@redhat.com>
Thanks,
Sean
^ permalink raw reply
* Re: [PATCH v4 3/4] mfd: cs40l50: Add support for CS40L50 core driver
From: kernel test robot @ 2023-10-21 14:56 UTC (permalink / raw)
To: James Ogletree
Cc: oe-kbuild-all, James Ogletree, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, Lee Jones, Fred Treven, Ben Bright,
linux-input, devicetree, linux-kernel
In-Reply-To: <20231018175726.3879955-4-james.ogletree@opensource.cirrus.com>
Hi James,
kernel test robot noticed the following build errors:
[auto build test ERROR on dtor-input/next]
[also build test ERROR on dtor-input/for-linus lee-mfd/for-mfd-next robh/for-next linus/master lee-mfd/for-mfd-fixes v6.6-rc6 next-20231020]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/James-Ogletree/dt-bindings-input-cirrus-cs40l50-Add-initial-DT-binding/20231019-015950
base: https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next
patch link: https://lore.kernel.org/r/20231018175726.3879955-4-james.ogletree%40opensource.cirrus.com
patch subject: [PATCH v4 3/4] mfd: cs40l50: Add support for CS40L50 core driver
config: sh-allyesconfig (https://download.01.org/0day-ci/archive/20231021/202310212247.exrCjFhj-lkp@intel.com/config)
compiler: sh4-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231021/202310212247.exrCjFhj-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310212247.exrCjFhj-lkp@intel.com/
All errors (new ones prefixed by >>):
>> drivers/mfd/cs40l50-spi.c:9:10: fatal error: linux/input/cs40l50.h: No such file or directory
9 | #include <linux/input/cs40l50.h>
| ^~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
vim +9 drivers/mfd/cs40l50-spi.c
> 9 #include <linux/input/cs40l50.h>
10 #include <linux/mfd/spi.h>
11
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* [PATCH v9 4/4] Input: goodix-berlin - add SPI support for Goodix Berlin Touchscreen IC
From: Neil Armstrong @ 2023-10-21 11:09 UTC (permalink / raw)
To: Dmitry Torokhov, linux-input
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bastien Nocera,
Hans de Goede, Henrik Rydberg, Jeff LaBundy, linux-arm-msm,
devicetree, linux-kernel, Neil Armstrong
In-Reply-To: <20231021-topic-goodix-berlin-upstream-initial-v9-0-13fb4e887156@linaro.org>
Add initial support for the new Goodix "Berlin" touchscreen ICs
over the SPI interface.
The driver doesn't use the regmap_spi code since the SPI messages
needs to be prefixed, thus this custom regmap code.
This initial driver is derived from the Goodix goodix_ts_berlin
available at [1] and [2] and only supports the GT9916 IC
present on the Qualcomm SM8550 MTP & QRD touch panel.
The current implementation only supports BerlinD, aka GT9916.
[1] https://github.com/goodix/goodix_ts_berlin
[2] https://git.codelinaro.org/clo/la/platform/vendor/opensource/touch-drivers
Reviewed-by: Jeff LaBundy <jeff@labundy.com>
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
drivers/input/touchscreen/Kconfig | 14 +++
drivers/input/touchscreen/Makefile | 1 +
drivers/input/touchscreen/goodix_berlin_spi.c | 173 ++++++++++++++++++++++++++
3 files changed, 188 insertions(+)
diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index cc7b88118158..c821fe3ee794 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -433,6 +433,20 @@ config TOUCHSCREEN_GOODIX_BERLIN_I2C
To compile this driver as a module, choose M here: the
module will be called goodix_berlin_i2c.
+config TOUCHSCREEN_GOODIX_BERLIN_SPI
+ tristate "Goodix Berlin SPI touchscreen"
+ depends on SPI_MASTER
+ select REGMAP
+ select TOUCHSCREEN_GOODIX_BERLIN_CORE
+ help
+ Say Y here if you have a Goodix Berlin IC connected to
+ your system via SPI.
+
+ If unsure, say N.
+
+ To compile this driver as a module, choose M here: the
+ module will be called goodix_berlin_spi.
+
config TOUCHSCREEN_HIDEEP
tristate "HiDeep Touch IC"
depends on I2C
diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
index 7ef677cf7a10..a81cb5aa21a5 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -49,6 +49,7 @@ obj-$(CONFIG_TOUCHSCREEN_FUJITSU) += fujitsu_ts.o
obj-$(CONFIG_TOUCHSCREEN_GOODIX) += goodix_ts.o
obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_CORE) += goodix_berlin_core.o
obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_I2C) += goodix_berlin_i2c.o
+obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_SPI) += goodix_berlin_spi.o
obj-$(CONFIG_TOUCHSCREEN_HIDEEP) += hideep.o
obj-$(CONFIG_TOUCHSCREEN_HYNITRON_CSTXXX) += hynitron_cstxxx.o
obj-$(CONFIG_TOUCHSCREEN_ILI210X) += ili210x.o
diff --git a/drivers/input/touchscreen/goodix_berlin_spi.c b/drivers/input/touchscreen/goodix_berlin_spi.c
new file mode 100644
index 000000000000..1f57c3592918
--- /dev/null
+++ b/drivers/input/touchscreen/goodix_berlin_spi.c
@@ -0,0 +1,173 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Goodix Berlin Touchscreen Driver
+ *
+ * Copyright (C) 2020 - 2021 Goodix, Inc.
+ * Copyright (C) 2023 Linaro Ltd.
+ *
+ * Based on goodix_ts_berlin driver.
+ */
+#include <asm/unaligned.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/spi/spi.h>
+
+#include "goodix_berlin.h"
+
+#define GOODIX_BERLIN_SPI_TRANS_PREFIX_LEN 1
+#define GOODIX_BERLIN_REGISTER_WIDTH 4
+#define GOODIX_BERLIN_SPI_READ_DUMMY_LEN 3
+#define GOODIX_BERLIN_SPI_READ_PREFIX_LEN (GOODIX_BERLIN_SPI_TRANS_PREFIX_LEN + \
+ GOODIX_BERLIN_REGISTER_WIDTH + \
+ GOODIX_BERLIN_SPI_READ_DUMMY_LEN)
+#define GOODIX_BERLIN_SPI_WRITE_PREFIX_LEN (GOODIX_BERLIN_SPI_TRANS_PREFIX_LEN + \
+ GOODIX_BERLIN_REGISTER_WIDTH)
+
+#define GOODIX_BERLIN_SPI_WRITE_FLAG 0xF0
+#define GOODIX_BERLIN_SPI_READ_FLAG 0xF1
+
+static int goodix_berlin_spi_read(void *context, const void *reg_buf,
+ size_t reg_size, void *val_buf,
+ size_t val_size)
+{
+ struct spi_device *spi = context;
+ struct spi_transfer xfers;
+ struct spi_message spi_msg;
+ const u32 *reg = reg_buf; /* reg is stored as native u32 at start of buffer */
+ u8 *buf;
+ int error;
+
+ if (reg_size != GOODIX_BERLIN_REGISTER_WIDTH)
+ return -EINVAL;
+
+ buf = kzalloc(GOODIX_BERLIN_SPI_READ_PREFIX_LEN + val_size, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ spi_message_init(&spi_msg);
+ memset(&xfers, 0, sizeof(xfers));
+
+ /* buffer format: 0xF1 + addr(4bytes) + dummy(3bytes) + data */
+ buf[0] = GOODIX_BERLIN_SPI_READ_FLAG;
+ put_unaligned_be32(*reg, buf + GOODIX_BERLIN_SPI_TRANS_PREFIX_LEN);
+ memset(buf + GOODIX_BERLIN_SPI_TRANS_PREFIX_LEN + GOODIX_BERLIN_REGISTER_WIDTH,
+ 0xff, GOODIX_BERLIN_SPI_READ_DUMMY_LEN);
+
+ xfers.tx_buf = buf;
+ xfers.rx_buf = buf;
+ xfers.len = GOODIX_BERLIN_SPI_READ_PREFIX_LEN + val_size;
+ xfers.cs_change = 0;
+ spi_message_add_tail(&xfers, &spi_msg);
+
+ error = spi_sync(spi, &spi_msg);
+ if (error < 0)
+ dev_err(&spi->dev, "spi transfer error, %d", error);
+ else
+ memcpy(val_buf, buf + GOODIX_BERLIN_SPI_READ_PREFIX_LEN, val_size);
+
+ kfree(buf);
+ return error;
+}
+
+static int goodix_berlin_spi_write(void *context, const void *data,
+ size_t count)
+{
+ unsigned int len = count - GOODIX_BERLIN_REGISTER_WIDTH;
+ struct spi_device *spi = context;
+ struct spi_transfer xfers;
+ struct spi_message spi_msg;
+ const u32 *reg = data; /* reg is stored as native u32 at start of buffer */
+ u8 *buf;
+ int error;
+
+ buf = kzalloc(GOODIX_BERLIN_SPI_WRITE_PREFIX_LEN + len, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ spi_message_init(&spi_msg);
+ memset(&xfers, 0, sizeof(xfers));
+
+ buf[0] = GOODIX_BERLIN_SPI_WRITE_FLAG;
+ put_unaligned_be32(*reg, buf + GOODIX_BERLIN_SPI_TRANS_PREFIX_LEN);
+ memcpy(buf + GOODIX_BERLIN_SPI_WRITE_PREFIX_LEN,
+ data + GOODIX_BERLIN_REGISTER_WIDTH, len);
+
+ xfers.tx_buf = buf;
+ xfers.len = GOODIX_BERLIN_SPI_WRITE_PREFIX_LEN + len;
+ xfers.cs_change = 0;
+ spi_message_add_tail(&xfers, &spi_msg);
+
+ error = spi_sync(spi, &spi_msg);
+ if (error < 0)
+ dev_err(&spi->dev, "spi transfer error, %d", error);
+
+ kfree(buf);
+ return error;
+}
+
+static const struct regmap_config goodix_berlin_spi_regmap_conf = {
+ .reg_bits = 32,
+ .val_bits = 8,
+ .read = goodix_berlin_spi_read,
+ .write = goodix_berlin_spi_write,
+};
+
+/* vendor & product left unassigned here, should probably be updated from fw info */
+static const struct input_id goodix_berlin_spi_input_id = {
+ .bustype = BUS_SPI,
+};
+
+static int goodix_berlin_spi_probe(struct spi_device *spi)
+{
+ struct regmap_config regmap_config;
+ struct regmap *regmap;
+ size_t max_size;
+ int error = 0;
+
+ spi->mode = SPI_MODE_0;
+ spi->bits_per_word = 8;
+ error = spi_setup(spi);
+ if (error)
+ return error;
+
+ max_size = spi_max_transfer_size(spi);
+
+ regmap_config = goodix_berlin_spi_regmap_conf;
+ regmap_config.max_raw_read = max_size - GOODIX_BERLIN_SPI_READ_PREFIX_LEN;
+ regmap_config.max_raw_write = max_size - GOODIX_BERLIN_SPI_WRITE_PREFIX_LEN;
+
+ regmap = devm_regmap_init(&spi->dev, NULL, spi, ®map_config);
+ if (IS_ERR(regmap))
+ return PTR_ERR(regmap);
+
+ return goodix_berlin_probe(&spi->dev, spi->irq,
+ &goodix_berlin_spi_input_id, regmap);
+}
+
+static const struct spi_device_id goodix_berlin_spi_ids[] = {
+ { "gt9916" },
+ { },
+};
+MODULE_DEVICE_TABLE(spi, goodix_berlin_spi_ids);
+
+static const struct of_device_id goodix_berlin_spi_of_match[] = {
+ { .compatible = "goodix,gt9916", },
+ { }
+};
+MODULE_DEVICE_TABLE(of, goodix_berlin_spi_of_match);
+
+static struct spi_driver goodix_berlin_spi_driver = {
+ .driver = {
+ .name = "goodix-berlin-spi",
+ .of_match_table = goodix_berlin_spi_of_match,
+ .pm = pm_sleep_ptr(&goodix_berlin_pm_ops),
+ },
+ .probe = goodix_berlin_spi_probe,
+ .id_table = goodix_berlin_spi_ids,
+};
+module_spi_driver(goodix_berlin_spi_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Goodix Berlin SPI Touchscreen driver");
+MODULE_AUTHOR("Neil Armstrong <neil.armstrong@linaro.org>");
--
2.34.1
^ permalink raw reply related
* [PATCH v9 3/4] Input: goodix-berlin - add I2C support for Goodix Berlin Touchscreen IC
From: Neil Armstrong @ 2023-10-21 11:09 UTC (permalink / raw)
To: Dmitry Torokhov, linux-input
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bastien Nocera,
Hans de Goede, Henrik Rydberg, Jeff LaBundy, linux-arm-msm,
devicetree, linux-kernel, Neil Armstrong
In-Reply-To: <20231021-topic-goodix-berlin-upstream-initial-v9-0-13fb4e887156@linaro.org>
Add initial support for the new Goodix "Berlin" touchscreen ICs
over the I2C interface.
This initial driver is derived from the Goodix goodix_ts_berlin
available at [1] and [2] and only supports the GT9916 IC
present on the Qualcomm SM8550 MTP & QRD touch panel.
The current implementation only supports BerlinD, aka GT9916.
[1] https://github.com/goodix/goodix_ts_berlin
[2] https://git.codelinaro.org/clo/la/platform/vendor/opensource/touch-drivers
Reviewed-by: Jeff LaBundy <jeff@labundy.com>
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
drivers/input/touchscreen/Kconfig | 14 ++++++
drivers/input/touchscreen/Makefile | 1 +
drivers/input/touchscreen/goodix_berlin_i2c.c | 69 +++++++++++++++++++++++++++
3 files changed, 84 insertions(+)
diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index 950da599ae1a..cc7b88118158 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -419,6 +419,20 @@ config TOUCHSCREEN_GOODIX
config TOUCHSCREEN_GOODIX_BERLIN_CORE
tristate
+config TOUCHSCREEN_GOODIX_BERLIN_I2C
+ tristate "Goodix Berlin I2C touchscreen"
+ depends on I2C
+ select REGMAP_I2C
+ select TOUCHSCREEN_GOODIX_BERLIN_CORE
+ help
+ Say Y here if you have a Goodix Berlin IC connected to
+ your system via I2C.
+
+ If unsure, say N.
+
+ To compile this driver as a module, choose M here: the
+ module will be called goodix_berlin_i2c.
+
config TOUCHSCREEN_HIDEEP
tristate "HiDeep Touch IC"
depends on I2C
diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
index 2e2f3e70cd2c..7ef677cf7a10 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -48,6 +48,7 @@ obj-$(CONFIG_TOUCHSCREEN_EXC3000) += exc3000.o
obj-$(CONFIG_TOUCHSCREEN_FUJITSU) += fujitsu_ts.o
obj-$(CONFIG_TOUCHSCREEN_GOODIX) += goodix_ts.o
obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_CORE) += goodix_berlin_core.o
+obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_I2C) += goodix_berlin_i2c.o
obj-$(CONFIG_TOUCHSCREEN_HIDEEP) += hideep.o
obj-$(CONFIG_TOUCHSCREEN_HYNITRON_CSTXXX) += hynitron_cstxxx.o
obj-$(CONFIG_TOUCHSCREEN_ILI210X) += ili210x.o
diff --git a/drivers/input/touchscreen/goodix_berlin_i2c.c b/drivers/input/touchscreen/goodix_berlin_i2c.c
new file mode 100644
index 000000000000..6407b2258eb1
--- /dev/null
+++ b/drivers/input/touchscreen/goodix_berlin_i2c.c
@@ -0,0 +1,69 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Goodix Berlin Touchscreen Driver
+ *
+ * Copyright (C) 2020 - 2021 Goodix, Inc.
+ * Copyright (C) 2023 Linaro Ltd.
+ *
+ * Based on goodix_ts_berlin driver.
+ */
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+
+#include "goodix_berlin.h"
+
+#define I2C_MAX_TRANSFER_SIZE 256
+
+static const struct regmap_config goodix_berlin_i2c_regmap_conf = {
+ .reg_bits = 32,
+ .val_bits = 8,
+ .max_raw_read = I2C_MAX_TRANSFER_SIZE,
+ .max_raw_write = I2C_MAX_TRANSFER_SIZE,
+};
+
+/* vendor & product left unassigned here, should probably be updated from fw info */
+static const struct input_id goodix_berlin_i2c_input_id = {
+ .bustype = BUS_I2C,
+};
+
+static int goodix_berlin_i2c_probe(struct i2c_client *client)
+{
+ struct regmap *regmap;
+
+ regmap = devm_regmap_init_i2c(client, &goodix_berlin_i2c_regmap_conf);
+ if (IS_ERR(regmap))
+ return PTR_ERR(regmap);
+
+ return goodix_berlin_probe(&client->dev, client->irq,
+ &goodix_berlin_i2c_input_id, regmap);
+}
+
+static const struct i2c_device_id goodix_berlin_i2c_id[] = {
+ { "gt9916", 0 },
+ { }
+};
+
+MODULE_DEVICE_TABLE(i2c, goodix_berlin_i2c_id);
+
+static const struct of_device_id goodix_berlin_i2c_of_match[] = {
+ { .compatible = "goodix,gt9916", },
+ { }
+};
+MODULE_DEVICE_TABLE(of, goodix_berlin_i2c_of_match);
+
+static struct i2c_driver goodix_berlin_i2c_driver = {
+ .driver = {
+ .name = "goodix-berlin-i2c",
+ .of_match_table = goodix_berlin_i2c_of_match,
+ .pm = pm_sleep_ptr(&goodix_berlin_pm_ops),
+ },
+ .probe = goodix_berlin_i2c_probe,
+ .id_table = goodix_berlin_i2c_id,
+};
+module_i2c_driver(goodix_berlin_i2c_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Goodix Berlin I2C Touchscreen driver");
+MODULE_AUTHOR("Neil Armstrong <neil.armstrong@linaro.org>");
--
2.34.1
^ permalink raw reply related
* [PATCH v9 2/4] Input: add core support for Goodix Berlin Touchscreen IC
From: Neil Armstrong @ 2023-10-21 11:09 UTC (permalink / raw)
To: Dmitry Torokhov, linux-input
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bastien Nocera,
Hans de Goede, Henrik Rydberg, Jeff LaBundy, linux-arm-msm,
devicetree, linux-kernel, Neil Armstrong
In-Reply-To: <20231021-topic-goodix-berlin-upstream-initial-v9-0-13fb4e887156@linaro.org>
Add initial support for the new Goodix "Berlin" touchscreen ICs.
These touchscreen ICs support SPI, I2C and I3C interface, up to
10 finger touch, stylus and gestures events.
This initial driver is derived from the Goodix goodix_ts_berlin
available at [1] and [2] and only supports the GT9916 IC
present on the Qualcomm SM8550 MTP & QRD touch panel.
The current implementation only supports BerlinD, aka GT9916.
Support for advanced features like:
- Firmware & config update
- Stylus events
- Gestures events
- Previous revisions support (BerlinA or BerlinB)
is not included in current version.
The current support will work with currently flashed firmware
and config, and bail out if firmware or config aren't flashed yet.
[1] https://github.com/goodix/goodix_ts_berlin
[2] https://git.codelinaro.org/clo/la/platform/vendor/opensource/touch-drivers
Reviewed-by: Jeff LaBundy <jeff@labundy.com>
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
drivers/input/touchscreen/Kconfig | 3 +
drivers/input/touchscreen/Makefile | 1 +
drivers/input/touchscreen/goodix_berlin.h | 159 +++++++
drivers/input/touchscreen/goodix_berlin_core.c | 582 +++++++++++++++++++++++++
4 files changed, 745 insertions(+)
diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index e3e2324547b9..950da599ae1a 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -416,6 +416,9 @@ config TOUCHSCREEN_GOODIX
To compile this driver as a module, choose M here: the
module will be called goodix.
+config TOUCHSCREEN_GOODIX_BERLIN_CORE
+ tristate
+
config TOUCHSCREEN_HIDEEP
tristate "HiDeep Touch IC"
depends on I2C
diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
index 62bd24f3ac8e..2e2f3e70cd2c 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -47,6 +47,7 @@ obj-$(CONFIG_TOUCHSCREEN_EGALAX_SERIAL) += egalax_ts_serial.o
obj-$(CONFIG_TOUCHSCREEN_EXC3000) += exc3000.o
obj-$(CONFIG_TOUCHSCREEN_FUJITSU) += fujitsu_ts.o
obj-$(CONFIG_TOUCHSCREEN_GOODIX) += goodix_ts.o
+obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_CORE) += goodix_berlin_core.o
obj-$(CONFIG_TOUCHSCREEN_HIDEEP) += hideep.o
obj-$(CONFIG_TOUCHSCREEN_HYNITRON_CSTXXX) += hynitron_cstxxx.o
obj-$(CONFIG_TOUCHSCREEN_ILI210X) += ili210x.o
diff --git a/drivers/input/touchscreen/goodix_berlin.h b/drivers/input/touchscreen/goodix_berlin.h
new file mode 100644
index 000000000000..235f44947a28
--- /dev/null
+++ b/drivers/input/touchscreen/goodix_berlin.h
@@ -0,0 +1,159 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Goodix Touchscreen Driver
+ * Copyright (C) 2020 - 2021 Goodix, Inc.
+ * Copyright (C) 2023 Linaro Ltd.
+ *
+ * Based on goodix_berlin_berlin driver.
+ */
+
+#ifndef __GOODIX_BERLIN_H_
+#define __GOODIX_BERLIN_H_
+
+#include <linux/gpio/consumer.h>
+#include <linux/input.h>
+#include <linux/input/touchscreen.h>
+#include <linux/regulator/consumer.h>
+#include <linux/sizes.h>
+
+#define GOODIX_BERLIN_MAX_TOUCH 10
+
+#define GOODIX_BERLIN_NORMAL_RESET_DELAY_MS 100
+
+#define GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN 8
+#define GOODIX_BERLIN_STATUS_OFFSET 0
+#define GOODIX_BERLIN_REQUEST_TYPE_OFFSET 2
+
+#define GOODIX_BERLIN_BYTES_PER_POINT 8
+#define GOODIX_BERLIN_COOR_DATA_CHECKSUM_SIZE 2
+#define GOODIX_BERLIN_COOR_DATA_CHECKSUM_MASK GENMASK(15, 0)
+
+/* Read n finger events */
+#define GOODIX_BERLIN_IRQ_READ_LEN(n) (GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN + \
+ (GOODIX_BERLIN_BYTES_PER_POINT * (n)) + \
+ GOODIX_BERLIN_COOR_DATA_CHECKSUM_SIZE)
+
+#define GOODIX_BERLIN_TOUCH_EVENT BIT(7)
+#define GOODIX_BERLIN_REQUEST_EVENT BIT(6)
+#define GOODIX_BERLIN_TOUCH_COUNT_MASK GENMASK(3, 0)
+
+#define GOODIX_BERLIN_REQUEST_CODE_RESET 3
+
+#define GOODIX_BERLIN_POINT_TYPE_MASK GENMASK(3, 0)
+#define GOODIX_BERLIN_POINT_TYPE_STYLUS_HOVER 1
+#define GOODIX_BERLIN_POINT_TYPE_STYLUS 3
+
+#define GOODIX_BERLIN_TOUCH_ID_MASK GENMASK(7, 4)
+
+#define GOODIX_BERLIN_DEV_CONFIRM_VAL 0xAA
+#define GOODIX_BERLIN_BOOTOPTION_ADDR 0x10000
+#define GOODIX_BERLIN_FW_VERSION_INFO_ADDR 0x10014
+
+#define GOODIX_BERLIN_IC_INFO_MAX_LEN SZ_1K
+#define GOODIX_BERLIN_IC_INFO_ADDR 0x10070
+
+struct goodix_berlin_fw_version {
+ u8 rom_pid[6];
+ u8 rom_vid[3];
+ u8 rom_vid_reserved;
+ u8 patch_pid[8];
+ u8 patch_vid[4];
+ u8 patch_vid_reserved;
+ u8 sensor_id;
+ u8 reserved[2];
+ __le16 checksum;
+} __packed;
+
+struct goodix_berlin_ic_info_version {
+ u8 info_customer_id;
+ u8 info_version_id;
+ u8 ic_die_id;
+ u8 ic_version_id;
+ __le32 config_id;
+ u8 config_version;
+ u8 frame_data_customer_id;
+ u8 frame_data_version_id;
+ u8 touch_data_customer_id;
+ u8 touch_data_version_id;
+ u8 reserved[3];
+} __packed;
+
+struct goodix_berlin_ic_info_feature {
+ __le16 freqhop_feature;
+ __le16 calibration_feature;
+ __le16 gesture_feature;
+ __le16 side_touch_feature;
+ __le16 stylus_feature;
+} __packed;
+
+struct goodix_berlin_ic_info_misc {
+ __le32 cmd_addr;
+ __le16 cmd_max_len;
+ __le32 cmd_reply_addr;
+ __le16 cmd_reply_len;
+ __le32 fw_state_addr;
+ __le16 fw_state_len;
+ __le32 fw_buffer_addr;
+ __le16 fw_buffer_max_len;
+ __le32 frame_data_addr;
+ __le16 frame_data_head_len;
+ __le16 fw_attr_len;
+ __le16 fw_log_len;
+ u8 pack_max_num;
+ u8 pack_compress_version;
+ __le16 stylus_struct_len;
+ __le16 mutual_struct_len;
+ __le16 self_struct_len;
+ __le16 noise_struct_len;
+ __le32 touch_data_addr;
+ __le16 touch_data_head_len;
+ __le16 point_struct_len;
+ __le16 reserved1;
+ __le16 reserved2;
+ __le32 mutual_rawdata_addr;
+ __le32 mutual_diffdata_addr;
+ __le32 mutual_refdata_addr;
+ __le32 self_rawdata_addr;
+ __le32 self_diffdata_addr;
+ __le32 self_refdata_addr;
+ __le32 iq_rawdata_addr;
+ __le32 iq_refdata_addr;
+ __le32 im_rawdata_addr;
+ __le16 im_readata_len;
+ __le32 noise_rawdata_addr;
+ __le16 noise_rawdata_len;
+ __le32 stylus_rawdata_addr;
+ __le16 stylus_rawdata_len;
+ __le32 noise_data_addr;
+ __le32 esd_addr;
+} __packed;
+
+struct goodix_berlin_touch_data {
+ u8 id;
+ u8 unused;
+ __le16 x;
+ __le16 y;
+ __le16 w;
+} __packed;
+
+struct goodix_berlin_core {
+ struct device *dev;
+ struct regmap *regmap;
+ struct regulator *avdd;
+ struct regulator *iovdd;
+ struct gpio_desc *reset_gpio;
+ struct touchscreen_properties props;
+ struct goodix_berlin_fw_version fw_version;
+ struct input_dev *input_dev;
+ int irq;
+
+ /* Runtime parameters extracted from IC_INFO buffer */
+ u32 touch_data_addr;
+};
+
+int goodix_berlin_probe(struct device *dev, int irq, const struct input_id *id,
+ struct regmap *regmap);
+
+extern const struct dev_pm_ops goodix_berlin_pm_ops;
+
+#endif
diff --git a/drivers/input/touchscreen/goodix_berlin_core.c b/drivers/input/touchscreen/goodix_berlin_core.c
new file mode 100644
index 000000000000..caf364b8c223
--- /dev/null
+++ b/drivers/input/touchscreen/goodix_berlin_core.c
@@ -0,0 +1,582 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Goodix Touchscreen Driver
+ * Copyright (C) 2020 - 2021 Goodix, Inc.
+ * Copyright (C) 2023 Linaro Ltd.
+ *
+ * Based on goodix_ts_berlin driver.
+ */
+#include <asm/unaligned.h>
+#include <linux/bitfield.h>
+#include <linux/input/mt.h>
+#include <linux/input/touchscreen.h>
+#include <linux/regmap.h>
+
+#include "goodix_berlin.h"
+
+/*
+ * Goodix "Berlin" Touchscreen ID driver
+ *
+ * This driver is distinct from goodix.c since hardware interface
+ * is different enough to require a new driver.
+ * None of the register address or data structure are close enough
+ * to the previous generations.
+ *
+ * Currently only handles Multitouch events with already
+ * programmed firmware and "config" for "Revision D" Berlin IC.
+ *
+ * Support is missing for:
+ * - ESD Management
+ * - Firmware update/flashing
+ * - "Config" update/flashing
+ * - Stylus Events
+ * - Gesture Events
+ * - Support for older revisions (A & B)
+ */
+
+static bool goodix_berlin_checksum_valid(const u8 *data, int size)
+{
+ u32 cal_checksum = 0;
+ u16 r_checksum;
+ u32 i;
+
+ if (size < GOODIX_BERLIN_COOR_DATA_CHECKSUM_SIZE)
+ return false;
+
+ for (i = 0; i < size - GOODIX_BERLIN_COOR_DATA_CHECKSUM_SIZE; i++)
+ cal_checksum += data[i];
+
+ r_checksum = get_unaligned_le16(&data[i]);
+
+ return FIELD_GET(GOODIX_BERLIN_COOR_DATA_CHECKSUM_MASK, cal_checksum) == r_checksum;
+}
+
+static bool goodix_berlin_is_dummy_data(struct goodix_berlin_core *cd,
+ const u8 *data, int size)
+{
+ int i;
+
+ /*
+ * If the device is missing or doesn't respond the buffer
+ * could be filled with bus default line state, 0x00 or 0xff,
+ * so declare success the first time we encounter neither.
+ */
+ for (i = 0; i < size; i++)
+ if (data[i] > 0 && data[i] < 0xff)
+ return false;
+
+ return true;
+}
+
+static int goodix_berlin_dev_confirm(struct goodix_berlin_core *cd)
+{
+ u8 tx_buf[8], rx_buf[8];
+ int retry = 3;
+ int error;
+
+ memset(tx_buf, GOODIX_BERLIN_DEV_CONFIRM_VAL, sizeof(tx_buf));
+ while (retry--) {
+ error = regmap_raw_write(cd->regmap, GOODIX_BERLIN_BOOTOPTION_ADDR, tx_buf,
+ sizeof(tx_buf));
+ if (error)
+ return error;
+
+ error = regmap_raw_read(cd->regmap, GOODIX_BERLIN_BOOTOPTION_ADDR, rx_buf,
+ sizeof(rx_buf));
+ if (error)
+ return error;
+
+ if (!memcmp(tx_buf, rx_buf, sizeof(tx_buf)))
+ return 0;
+
+ usleep_range(5000, 5100);
+ }
+
+ dev_err(cd->dev, "device confirm failed, rx_buf: %*ph\n", 8, rx_buf);
+
+ return -EINVAL;
+}
+
+static int goodix_berlin_power_on(struct goodix_berlin_core *cd, bool on)
+{
+ int error = 0;
+
+ if (on) {
+ error = regulator_enable(cd->iovdd);
+ if (error) {
+ dev_err(cd->dev, "Failed to enable iovdd: %d\n", error);
+ return error;
+ }
+
+ /* Vendor waits 3ms for IOVDD to settle */
+ usleep_range(3000, 3100);
+
+ error = regulator_enable(cd->avdd);
+ if (error) {
+ dev_err(cd->dev, "Failed to enable avdd: %d\n", error);
+ goto err_iovdd_disable;
+ }
+
+ /* Vendor waits 15ms for IOVDD to settle */
+ usleep_range(15000, 15100);
+
+ gpiod_set_value(cd->reset_gpio, 0);
+
+ /* Vendor waits 4ms for Firmware to initialize */
+ usleep_range(4000, 4100);
+
+ error = goodix_berlin_dev_confirm(cd);
+ if (error)
+ goto err_dev_reset;
+
+ /* Vendor waits 100ms for Firmware to fully boot */
+ msleep(GOODIX_BERLIN_NORMAL_RESET_DELAY_MS);
+
+ return 0;
+ }
+
+err_dev_reset:
+ gpiod_set_value(cd->reset_gpio, 1);
+
+ regulator_disable(cd->avdd);
+
+err_iovdd_disable:
+ regulator_disable(cd->iovdd);
+
+ return error;
+}
+
+static int goodix_berlin_read_version(struct goodix_berlin_core *cd)
+{
+ u8 buf[sizeof(struct goodix_berlin_fw_version)];
+ int error;
+
+ error = regmap_raw_read(cd->regmap, GOODIX_BERLIN_FW_VERSION_INFO_ADDR, buf, sizeof(buf));
+ if (error) {
+ dev_err(cd->dev, "error reading fw version, %d\n", error);
+ return error;
+ }
+
+ if (!goodix_berlin_checksum_valid(buf, sizeof(buf))) {
+ dev_err(cd->dev, "invalid fw version: checksum error\n");
+ return -EINVAL;
+ }
+
+ memcpy(&cd->fw_version, buf, sizeof(cd->fw_version));
+
+ return 0;
+}
+
+/* Only extract necessary data for runtime */
+static int goodix_berlin_convert_ic_info(struct goodix_berlin_core *cd,
+ const u8 *data, u16 length)
+{
+ struct goodix_berlin_ic_info_misc misc;
+ unsigned int offset = 0;
+ u8 param_num;
+
+ offset += sizeof(__le16); /* length */
+ offset += sizeof(struct goodix_berlin_ic_info_version);
+ offset += sizeof(struct goodix_berlin_ic_info_feature);
+
+ /* IC_INFO Parameters, variable width structure */
+ offset += 4 * sizeof(u8); /* drv_num, sen_num, button_num, force_num */
+
+ if (offset >= length)
+ goto invalid_offset;
+
+ param_num = data[offset++]; /* active_scan_rate_num */
+
+ offset += param_num * sizeof(__le16);
+
+ if (offset >= length)
+ goto invalid_offset;
+
+ param_num = data[offset++]; /* mutual_freq_num*/
+
+ offset += param_num * sizeof(__le16);
+
+ if (offset >= length)
+ goto invalid_offset;
+
+ param_num = data[offset++]; /* self_tx_freq_num */
+
+ offset += param_num * sizeof(__le16);
+
+ if (offset >= length)
+ goto invalid_offset;
+
+ param_num = data[offset++]; /* self_rx_freq_num */
+
+ offset += param_num * sizeof(__le16);
+
+ if (offset >= length)
+ goto invalid_offset;
+
+ param_num = data[offset++]; /* stylus_freq_num */
+
+ offset += param_num * sizeof(__le16);
+
+ if (offset + sizeof(misc) > length)
+ goto invalid_offset;
+
+ /* goodix_berlin_ic_info_misc */
+ memcpy(&misc, &data[offset], sizeof(misc));
+
+ cd->touch_data_addr = le32_to_cpu(misc.touch_data_addr);
+
+ return 0;
+
+invalid_offset:
+ dev_err(cd->dev, "ic_info length is invalid (offset %d length %d)\n",
+ offset, length);
+ return -EINVAL;
+}
+
+static int goodix_berlin_get_ic_info(struct goodix_berlin_core *cd)
+{
+ u8 afe_data[GOODIX_BERLIN_IC_INFO_MAX_LEN];
+ __le16 length_raw;
+ u16 length;
+ int error;
+
+ error = regmap_raw_read(cd->regmap, GOODIX_BERLIN_IC_INFO_ADDR,
+ &length_raw, sizeof(length_raw));
+ if (error) {
+ dev_info(cd->dev, "failed get ic info length, %d\n", error);
+ return error;
+ }
+
+ length = le16_to_cpu(length_raw);
+ if (length >= GOODIX_BERLIN_IC_INFO_MAX_LEN) {
+ dev_info(cd->dev, "invalid ic info length %d\n", length);
+ return -EINVAL;
+ }
+
+ error = regmap_raw_read(cd->regmap, GOODIX_BERLIN_IC_INFO_ADDR,
+ afe_data, length);
+ if (error) {
+ dev_info(cd->dev, "failed get ic info data, %d\n", error);
+ return error;
+ }
+
+ /* check whether the data is valid (ex. bus default values) */
+ if (goodix_berlin_is_dummy_data(cd, (const uint8_t *)afe_data, length)) {
+ dev_err(cd->dev, "fw info data invalid\n");
+ return -EINVAL;
+ }
+
+ if (!goodix_berlin_checksum_valid((const uint8_t *)afe_data, length)) {
+ dev_info(cd->dev, "fw info checksum error\n");
+ return -EINVAL;
+ }
+
+ error = goodix_berlin_convert_ic_info(cd, afe_data, length);
+ if (error)
+ return error;
+
+ /* check some key info */
+ if (!cd->touch_data_addr) {
+ dev_err(cd->dev, "touch_data_addr is null\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static void goodix_berlin_parse_finger(struct goodix_berlin_core *cd,
+ const void *buf, int touch_num)
+{
+ const struct goodix_berlin_touch_data *touch_data = buf;
+ int i;
+
+ /* Check for data validity */
+ for (i = 0; i < touch_num; i++) {
+ unsigned int id;
+
+ id = FIELD_GET(GOODIX_BERLIN_TOUCH_ID_MASK, touch_data[i].id);
+
+ if (id >= GOODIX_BERLIN_MAX_TOUCH) {
+ dev_warn(cd->dev, "invalid finger id %d\n", id);
+ return;
+ }
+ }
+
+ /* Report finger touches */
+ for (i = 0; i < touch_num; i++) {
+ input_mt_slot(cd->input_dev,
+ FIELD_GET(GOODIX_BERLIN_TOUCH_ID_MASK,
+ touch_data[i].id));
+ input_mt_report_slot_state(cd->input_dev, MT_TOOL_FINGER, true);
+
+ touchscreen_report_pos(cd->input_dev, &cd->props,
+ __le16_to_cpu(touch_data[i].x),
+ __le16_to_cpu(touch_data[i].y),
+ true);
+ input_report_abs(cd->input_dev, ABS_MT_TOUCH_MAJOR,
+ __le16_to_cpu(touch_data[i].w));
+ }
+
+ input_mt_sync_frame(cd->input_dev);
+ input_sync(cd->input_dev);
+}
+
+static void goodix_berlin_touch_handler(struct goodix_berlin_core *cd,
+ const void *pre_buf, u32 pre_buf_len)
+{
+ static u8 buffer[GOODIX_BERLIN_IRQ_READ_LEN(GOODIX_BERLIN_MAX_TOUCH)];
+ u8 point_type, touch_num;
+ int error;
+
+ /* copy pre-data to buffer */
+ memcpy(buffer, pre_buf, pre_buf_len);
+
+ touch_num = FIELD_GET(GOODIX_BERLIN_TOUCH_COUNT_MASK,
+ buffer[GOODIX_BERLIN_REQUEST_TYPE_OFFSET]);
+
+ if (touch_num > GOODIX_BERLIN_MAX_TOUCH) {
+ dev_warn(cd->dev, "invalid touch num %d\n", touch_num);
+ return;
+ }
+
+ if (touch_num) {
+ /* read more data if more than 2 touch events */
+ if (unlikely(touch_num > 2)) {
+ error = regmap_raw_read(cd->regmap,
+ cd->touch_data_addr + pre_buf_len,
+ &buffer[pre_buf_len],
+ (touch_num - 2) * GOODIX_BERLIN_BYTES_PER_POINT);
+ if (error) {
+ dev_err_ratelimited(cd->dev, "failed to get touch data, %d\n",
+ error);
+ return;
+ }
+ }
+
+ point_type = FIELD_GET(GOODIX_BERLIN_POINT_TYPE_MASK,
+ buffer[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN]);
+
+ if (point_type == GOODIX_BERLIN_POINT_TYPE_STYLUS ||
+ point_type == GOODIX_BERLIN_POINT_TYPE_STYLUS_HOVER) {
+ dev_warn_once(cd->dev, "Stylus event type not handled\n");
+ return;
+ }
+
+ if (!goodix_berlin_checksum_valid(&buffer[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN],
+ touch_num * GOODIX_BERLIN_BYTES_PER_POINT + 2)) {
+ dev_err(cd->dev, "touch data checksum error, data: %*ph\n",
+ touch_num * GOODIX_BERLIN_BYTES_PER_POINT + 2,
+ &buffer[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN]);
+ return;
+ }
+ }
+
+ goodix_berlin_parse_finger(cd, &buffer[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN],
+ touch_num);
+}
+
+static int goodix_berlin_request_handle_reset(struct goodix_berlin_core *cd)
+{
+ gpiod_set_value(cd->reset_gpio, 1);
+ usleep_range(2000, 2100);
+ gpiod_set_value(cd->reset_gpio, 0);
+
+ msleep(GOODIX_BERLIN_NORMAL_RESET_DELAY_MS);
+
+ return 0;
+}
+
+static irqreturn_t goodix_berlin_threadirq_func(int irq, void *data)
+{
+ struct goodix_berlin_core *cd = data;
+ u8 buf[GOODIX_BERLIN_IRQ_READ_LEN(2)];
+ u8 event_status;
+ int error;
+
+ /* First, read buffer with space for 2 touch events */
+ error = regmap_raw_read(cd->regmap, cd->touch_data_addr, buf,
+ GOODIX_BERLIN_IRQ_READ_LEN(2));
+ if (error) {
+ dev_err_ratelimited(cd->dev, "failed get event head data, %d\n", error);
+ return IRQ_HANDLED;
+ }
+
+ if (buf[GOODIX_BERLIN_STATUS_OFFSET] == 0)
+ return IRQ_HANDLED;
+
+ if (!goodix_berlin_checksum_valid(buf, GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN)) {
+ dev_warn_ratelimited(cd->dev, "touch head checksum err : %*ph\n",
+ GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN, buf);
+ return IRQ_HANDLED;
+ }
+
+ event_status = buf[GOODIX_BERLIN_STATUS_OFFSET];
+
+ if (event_status & GOODIX_BERLIN_TOUCH_EVENT)
+ goodix_berlin_touch_handler(cd, buf, GOODIX_BERLIN_IRQ_READ_LEN(2));
+
+ if (event_status & GOODIX_BERLIN_REQUEST_EVENT) {
+ switch (buf[GOODIX_BERLIN_REQUEST_TYPE_OFFSET]) {
+ case GOODIX_BERLIN_REQUEST_CODE_RESET:
+ goodix_berlin_request_handle_reset(cd);
+ break;
+
+ default:
+ dev_warn(cd->dev, "unsupported request code 0x%x\n",
+ buf[GOODIX_BERLIN_REQUEST_TYPE_OFFSET]);
+ }
+ }
+
+ /* Clear up status field */
+ regmap_write(cd->regmap, cd->touch_data_addr, 0);
+
+ return IRQ_HANDLED;
+}
+
+static int goodix_berlin_input_dev_config(struct goodix_berlin_core *cd,
+ const struct input_id *id)
+{
+ struct input_dev *input_dev;
+ int error;
+
+ input_dev = devm_input_allocate_device(cd->dev);
+ if (!input_dev)
+ return -ENOMEM;
+
+ cd->input_dev = input_dev;
+ input_set_drvdata(input_dev, cd);
+
+ input_dev->name = "Goodix Berlin Capacitive TouchScreen";
+ input_dev->phys = "input/ts";
+
+ input_dev->id = *id;
+
+ input_set_abs_params(cd->input_dev, ABS_MT_POSITION_X, 0, SZ_64K - 1, 0, 0);
+ input_set_abs_params(cd->input_dev, ABS_MT_POSITION_Y, 0, SZ_64K - 1, 0, 0);
+ input_set_abs_params(cd->input_dev, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
+
+ touchscreen_parse_properties(cd->input_dev, true, &cd->props);
+
+ error = input_mt_init_slots(cd->input_dev, GOODIX_BERLIN_MAX_TOUCH,
+ INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
+ if (error)
+ return error;
+
+ return input_register_device(cd->input_dev);
+}
+
+static int goodix_berlin_pm_suspend(struct device *dev)
+{
+ struct goodix_berlin_core *cd = dev_get_drvdata(dev);
+
+ disable_irq(cd->irq);
+
+ return goodix_berlin_power_on(cd, false);
+}
+
+static int goodix_berlin_pm_resume(struct device *dev)
+{
+ struct goodix_berlin_core *cd = dev_get_drvdata(dev);
+ int error;
+
+ error = goodix_berlin_power_on(cd, true);
+ if (error)
+ return error;
+
+ enable_irq(cd->irq);
+
+ return 0;
+}
+
+EXPORT_GPL_SIMPLE_DEV_PM_OPS(goodix_berlin_pm_ops,
+ goodix_berlin_pm_suspend,
+ goodix_berlin_pm_resume);
+
+static void goodix_berlin_power_off(void *data)
+{
+ struct goodix_berlin_core *cd = data;
+
+ goodix_berlin_power_on(cd, false);
+}
+
+int goodix_berlin_probe(struct device *dev, int irq, const struct input_id *id,
+ struct regmap *regmap)
+{
+ struct goodix_berlin_core *cd;
+ int error;
+
+ if (irq <= 0) {
+ dev_err(dev, "Missing interrupt number\n");
+ return -EINVAL;
+ }
+
+ cd = devm_kzalloc(dev, sizeof(*cd), GFP_KERNEL);
+ if (!cd)
+ return -ENOMEM;
+
+ cd->dev = dev;
+ cd->regmap = regmap;
+ cd->irq = irq;
+
+ cd->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+ if (IS_ERR(cd->reset_gpio))
+ return dev_err_probe(dev, PTR_ERR(cd->reset_gpio),
+ "Failed to request reset gpio\n");
+
+ cd->avdd = devm_regulator_get(dev, "avdd");
+ if (IS_ERR(cd->avdd))
+ return dev_err_probe(dev, PTR_ERR(cd->avdd),
+ "Failed to request avdd regulator\n");
+
+ cd->iovdd = devm_regulator_get(dev, "iovdd");
+ if (IS_ERR(cd->iovdd))
+ return dev_err_probe(dev, PTR_ERR(cd->iovdd),
+ "Failed to request iovdd regulator\n");
+
+ error = goodix_berlin_power_on(cd, true);
+ if (error) {
+ dev_err(dev, "failed power on");
+ return error;
+ }
+
+ error = devm_add_action_or_reset(dev, goodix_berlin_power_off, cd);
+ if (error)
+ return error;
+
+ error = goodix_berlin_read_version(cd);
+ if (error) {
+ dev_err(dev, "failed to get version info");
+ return error;
+ }
+
+ error = goodix_berlin_get_ic_info(cd);
+ if (error) {
+ dev_err(dev, "invalid ic info, abort");
+ return error;
+ }
+
+ error = goodix_berlin_input_dev_config(cd, id);
+ if (error) {
+ dev_err(dev, "failed set input device");
+ return error;
+ }
+
+ error = devm_request_threaded_irq(dev, irq, NULL,
+ goodix_berlin_threadirq_func,
+ IRQF_ONESHOT, "goodix-berlin", cd);
+ if (error) {
+ dev_err(dev, "request threaded irq failed: %d\n", error);
+ return error;
+ }
+
+ dev_set_drvdata(dev, cd);
+
+ dev_dbg(dev, "Goodix Berlin %s Touchscreen Controller", cd->fw_version.patch_pid);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(goodix_berlin_probe);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Goodix Berlin Core Touchscreen driver");
+MODULE_AUTHOR("Neil Armstrong <neil.armstrong@linaro.org>");
--
2.34.1
^ permalink raw reply related
* [PATCH v9 1/4] dt-bindings: input: document Goodix Berlin Touchscreen IC
From: Neil Armstrong @ 2023-10-21 11:09 UTC (permalink / raw)
To: Dmitry Torokhov, linux-input
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bastien Nocera,
Hans de Goede, Henrik Rydberg, Jeff LaBundy, linux-arm-msm,
devicetree, linux-kernel, Neil Armstrong, Rob Herring
In-Reply-To: <20231021-topic-goodix-berlin-upstream-initial-v9-0-13fb4e887156@linaro.org>
Document the Goodix GT9916 wich is part of the "Berlin" serie
of Touchscreen controllers IC from Goodix.
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
.../bindings/input/touchscreen/goodix,gt9916.yaml | 95 ++++++++++++++++++++++
1 file changed, 95 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix,gt9916.yaml b/Documentation/devicetree/bindings/input/touchscreen/goodix,gt9916.yaml
new file mode 100644
index 000000000000..d90f045ac06c
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/touchscreen/goodix,gt9916.yaml
@@ -0,0 +1,95 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/touchscreen/goodix,gt9916.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Goodix Berlin series touchscreen controller
+
+description: The Goodix Berlin series of touchscreen controllers
+ be connected to either I2C or SPI buses.
+
+maintainers:
+ - Neil Armstrong <neil.armstrong@linaro.org>
+
+allOf:
+ - $ref: touchscreen.yaml#
+ - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+properties:
+ compatible:
+ enum:
+ - goodix,gt9916
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ reset-gpios:
+ maxItems: 1
+
+ avdd-supply:
+ description: Analog power supply regulator on AVDD pin
+
+ vddio-supply:
+ description: power supply regulator on VDDIO pin
+
+ spi-max-frequency: true
+ touchscreen-inverted-x: true
+ touchscreen-inverted-y: true
+ touchscreen-size-x: true
+ touchscreen-size-y: true
+ touchscreen-swapped-x-y: true
+
+additionalProperties: false
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - avdd-supply
+ - touchscreen-size-x
+ - touchscreen-size-y
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+ #include <dt-bindings/gpio/gpio.h>
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ touchscreen@5d {
+ compatible = "goodix,gt9916";
+ reg = <0x5d>;
+ interrupt-parent = <&gpio>;
+ interrupts = <25 IRQ_TYPE_LEVEL_LOW>;
+ reset-gpios = <&gpio1 1 GPIO_ACTIVE_LOW>;
+ avdd-supply = <&ts_avdd>;
+ touchscreen-size-x = <1024>;
+ touchscreen-size-y = <768>;
+ };
+ };
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+ #include <dt-bindings/gpio/gpio.h>
+ spi {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ num-cs = <1>;
+ cs-gpios = <&gpio 2 GPIO_ACTIVE_HIGH>;
+ touchscreen@0 {
+ compatible = "goodix,gt9916";
+ reg = <0>;
+ interrupt-parent = <&gpio>;
+ interrupts = <25 IRQ_TYPE_LEVEL_LOW>;
+ reset-gpios = <&gpio1 1 GPIO_ACTIVE_LOW>;
+ avdd-supply = <&ts_avdd>;
+ spi-max-frequency = <1000000>;
+ touchscreen-size-x = <1024>;
+ touchscreen-size-y = <768>;
+ };
+ };
+
+...
--
2.34.1
^ permalink raw reply related
* [PATCH v9 0/4] Input: add initial support for Goodix Berlin touchscreen IC
From: Neil Armstrong @ 2023-10-21 11:09 UTC (permalink / raw)
To: Dmitry Torokhov, linux-input
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bastien Nocera,
Hans de Goede, Henrik Rydberg, Jeff LaBundy, linux-arm-msm,
devicetree, linux-kernel, Neil Armstrong, Rob Herring
These touchscreen ICs support SPI, I2C and I3C interface, up to
10 finger touch, stylus and gestures events.
This initial driver is derived from the Goodix goodix_ts_berlin
available at [1] and [2] and only supports the GT9916 IC
present on the Qualcomm SM8550 MTP & QRD touch panel.
The current implementation only supports BerlinD, aka GT9916.
Support for advanced features like:
- Firmware & config update
- Stylus events
- Gestures events
- Previous revisions support (BerlinA or BerlinB)
is not included in current version.
The current support will work with currently flashed firmware
and config, and bail out if firmware or config aren't flashed yet.
[1] https://github.com/goodix/goodix_ts_berlin
[2] https://git.codelinaro.org/clo/la/platform/vendor/opensource/touch-drivers
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
Changes in v9:
- Rebased on next-20231020
- Link to v8: https://lore.kernel.org/r/20231003-topic-goodix-berlin-upstream-initial-v8-0-171606102ed6@linaro.org
Changes in v8:
- Add missing bitfield.h include in core
- Link to v7: https://lore.kernel.org/r/20231002-topic-goodix-berlin-upstream-initial-v7-0-792fb91f5e88@linaro.org
Changes in v7:
- rebased on v6.6-rc3
- Link to v6: https://lore.kernel.org/r/20230912-topic-goodix-berlin-upstream-initial-v6-0-b4ecfa49fb9d@linaro.org
Changes in v6:
- rebased on v6.6-rc1
- changed commit message prefix to match the other Input commits
- Link to v5: https://lore.kernel.org/r/20230801-topic-goodix-berlin-upstream-initial-v5-0-079252935593@linaro.org
Changes in v5:
- rebased on next-20230801
- Link to v4: https://lore.kernel.org/r/20230606-topic-goodix-berlin-upstream-initial-v4-0-0947c489be17@linaro.org
Changes in v4:
- Core updates:
- drop kconfig depends, deps will be handled by _SPI and _I2C
- change power_on() error labels
- print errors on all dev_err() prints
- remove useless default variable initialization
- switch irq touch checksum error to dev_err()
- add Jeff's review tag
- I2C changes
- change REGMAP_I2C Kconfig from depends to select
- add Jeff's review tag
- SPI changes
- add select REGMAP to Kconfig
- added GOODIX_BERLIN_ prefix to defines
- switched from ret to error
- add Jeff's review tag
- Link to v3: https://lore.kernel.org/r/20230606-topic-goodix-berlin-upstream-initial-v3-0-f0577cead709@linaro.org
Changes in v3:
- Another guge cleanups after Jeff's review:
- appended goodix_berlin_ before all defines
- removed some unused defines
- removed retries on most of read functions, can be added back later
- added __le to ic_info structures
- reworked and simplified irq handling, dropped enum and ts_event structs
- added struct for touch data
- simplified and cleaned goodix_berlin_check_checksum & goodix_berlin_is_dummy_data
- moved touch_data_addr to the end of the main code_data
- reworked probe to get_irq last and right before setip input device
- cleaned probe by removing the "cd->dev"
- added short paragraph to justify new driver for berlin devices
- defined all offsets & masks
- Added bindings review tag
- Link to v2: https://lore.kernel.org/r/20230606-topic-goodix-berlin-upstream-initial-v2-0-26bc8fe1e90e@linaro.org
Changes in v2:
- Huge cleanups after Jeff's review:
- switch to error instead of ret
- drop dummy vendor/product ids
- drop unused defined/enums
- drop unused ic_info and only keep needes values
- cleanup namings and use goodix_berlin_ everywhere
- fix regulator setup
- fix default variables value when assigned afterwars
- removed indirections
- dropped debugfs
- cleaned input_dev setup
- dropped _remove()
- sync'ed i2c and spi drivers
- fixed yaml bindings
- Link to v1: https://lore.kernel.org/r/20230606-topic-goodix-berlin-upstream-initial-v1-0-4a0741b8aefd@linaro.org
---
Neil Armstrong (4):
dt-bindings: input: document Goodix Berlin Touchscreen IC
Input: add core support for Goodix Berlin Touchscreen IC
Input: goodix-berlin - add I2C support for Goodix Berlin Touchscreen IC
Input: goodix-berlin - add SPI support for Goodix Berlin Touchscreen IC
.../bindings/input/touchscreen/goodix,gt9916.yaml | 95 ++++
drivers/input/touchscreen/Kconfig | 31 ++
drivers/input/touchscreen/Makefile | 3 +
drivers/input/touchscreen/goodix_berlin.h | 159 ++++++
drivers/input/touchscreen/goodix_berlin_core.c | 582 +++++++++++++++++++++
drivers/input/touchscreen/goodix_berlin_i2c.c | 69 +++
drivers/input/touchscreen/goodix_berlin_spi.c | 173 ++++++
7 files changed, 1112 insertions(+)
---
base-commit: 2030579113a1b1b5bfd7ff24c0852847836d8fd1
change-id: 20230606-topic-goodix-berlin-upstream-initial-ba97e8ec8f4c
Best regards,
--
Neil Armstrong <neil.armstrong@linaro.org>
^ permalink raw reply
* Re: [PATCH v3 1/3] pwm: make it possible to apply pwm changes in atomic context
From: Hans de Goede @ 2023-10-21 9:08 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Sean Young, linux-media, linux-pwm, Ivaylo Dimitrov,
Thierry Reding, Jonathan Corbet, Jani Nikula, Joonas Lahtinen,
Rodrigo Vivi, Tvrtko Ursulin, David Airlie, Daniel Vetter,
Javier Martinez Canillas, Jean Delvare, Guenter Roeck,
Support Opensource, Dmitry Torokhov, Pavel Machek, Lee Jones,
Mauro Carvalho Chehab, Ilpo Järvinen, Mark Gross,
Liam Girdwood, Mark Brown, Daniel Thompson, Jingoo Han,
Helge Deller, linux-doc, linux-kernel, intel-gfx, dri-devel,
linux-hwmon, linux-input, linux-leds, platform-driver-x86,
linux-arm-kernel, linux-fbdev
In-Reply-To: <20231019105118.64gdzzixwqrztjir@pengutronix.de>
Hi Uwe,
On 10/19/23 12:51, Uwe Kleine-König wrote:
> On Wed, Oct 18, 2023 at 03:57:48PM +0200, Hans de Goede wrote:
>> Hi Sean,
>>
>> On 10/17/23 11:17, Sean Young wrote:
>>> Some drivers require sleeping, for example if the pwm device is connected
>>> over i2c. The pwm-ir-tx requires precise timing, and sleeping causes havoc
>>> with the generated IR signal when sleeping occurs.
>>>
>>> This patch makes it possible to use pwm when the driver does not sleep,
>>> by introducing the pwm_can_sleep() function.
>>>
>>> Signed-off-by: Sean Young <sean@mess.org>
>>
>> I have no objection to this patch by itself, but it seems a bit
>> of unnecessary churn to change all current callers of pwm_apply_state()
>> to a new API.
>
> The idea is to improve the semantic of the function name, see
> https://lore.kernel.org/linux-pwm/20231013180449.mcdmklbsz2rlymzz@pengutronix.de
> for more context.
Hmm, so the argument here is that the GPIO API has this, but GPIOs
generally speaking can be set atomically, so there not being able
to set it atomically is special.
OTOH we have many many many other kernel functions which may sleep
and we don't all postfix them with _can_sleep.
And for PWM controllers pwm_apply_state is IMHO sorta expected to
sleep. Many of these are attached over I2C so things will sleep,
others have a handshake to wait for the current dutycycle to
end before you can apply a second change on top of an earlier
change during the current dutycycle which often also involves
sleeping.
So the natural/expeected thing for pwm_apply_state() is to sleep
and thus it does not need a postfix for this IMHO.
> I think it's very subjective if you consider this
> churn or not.
I consider it churn because I don't think adding a postfix
for what is the default/expected behavior is a good idea
(with GPIOs not sleeping is the expected behavior).
I agree that this is very subjective and very much goes
into the territory of bikeshedding. So please consider
the above my 2 cents on this and lets leave it at that.
> While it's nice to have every caller converted in a single
> step, I'd go for
>
> #define pwm_apply_state(pwm, state) pwm_apply_cansleep(pwm, state)
>
> , keep that macro for a while and convert all users step by step. This
> way we don't needlessly break oot code and the changes to convert to the
> new API can go via their usual trees without time pressure.
I don't think there are enough users of pwm_apply_state() to warrant
such an exercise.
So if people want to move ahead with the _can_sleep postfix addition
(still not a fan) here is my acked-by for the drivers/platform/x86
changes, for merging this through the PWM tree in a single commit:
Acked-by: Hans de Goede <hdegoede@redhat.com>
Regards,
Hans
^ permalink raw reply
* [syzbot] [input?] WARNING in cm109_input_open/usb_submit_urb (2)
From: syzbot @ 2023-10-21 6:32 UTC (permalink / raw)
To: dmitry.torokhov, linux-input, linux-kernel, syzkaller-bugs
Hello,
syzbot found the following issue on:
HEAD commit: 213f891525c2 Merge tag 'probes-fixes-v6.6-rc6' of git://gi..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=1645d5c5680000
kernel config: https://syzkaller.appspot.com/x/.config?x=3c2b0838e2a16cba
dashboard link: https://syzkaller.appspot.com/bug?extid=2e305789579d76b5c253
compiler: gcc (Debian 12.2.0-14) 12.2.0, GNU ld (GNU Binutils for Debian) 2.40
Unfortunately, I don't have any reproducer for this issue yet.
Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/b408820be5ac/disk-213f8915.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/4d1614ab03cf/vmlinux-213f8915.xz
kernel image: https://storage.googleapis.com/syzbot-assets/0405348b5203/bzImage-213f8915.xz
IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+2e305789579d76b5c253@syzkaller.appspotmail.com
usb 5-1: New USB device found, idVendor=0d8c, idProduct=000e, bcdDevice=8e.8f
usb 5-1: New USB device strings: Mfr=0, Product=24, SerialNumber=3
usb 5-1: Product: syz
usb 5-1: SerialNumber: syz
usb 5-1: config 0 descriptor??
cm109 5-1:0.8: invalid payload size 0, expected 4
input: CM109 USB driver as /devices/platform/dummy_hcd.4/usb5/5-1/5-1:0.8/input/input7
------------[ cut here ]------------
URB ffff88814239df00 submitted while active
WARNING: CPU: 1 PID: 5140 at drivers/usb/core/urb.c:379 usb_submit_urb+0x14cb/0x1720 drivers/usb/core/urb.c:379
Modules linked in:
CPU: 1 PID: 5140 Comm: kworker/1:4 Not tainted 6.6.0-rc6-syzkaller-00029-g213f891525c2 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 09/06/2023
Workqueue: usb_hub_wq hub_event
RIP: 0010:usb_submit_urb+0x14cb/0x1720 drivers/usb/core/urb.c:379
Code: bf 0e fe eb cb bb fe ff ff ff e9 ca f3 ff ff e8 3b 43 42 fb 48 89 de 48 c7 c7 40 5e 40 8b c6 05 ae e5 72 08 01 e8 05 68 08 fb <0f> 0b e9 ba fe ff ff bb f8 ff ff ff e9 9e f3 ff ff 48 89 ef e8 3c
RSP: 0018:ffffc900043eef30 EFLAGS: 00010282
RAX: 0000000000000000 RBX: ffff88814239df00 RCX: ffffc90013d1f000
RDX: 0000000000040000 RSI: ffffffff814df0c6 RDI: 0000000000000001
RBP: 0000000000000000 R08: 0000000000000001 R09: 0000000000000000
R10: 0000000000000000 R11: 0a65766974636120 R12: ffff88801d85b810
R13: ffff88801d85b8a0 R14: ffff88801d85b850 R15: ffff88802f7af2e8
FS: 0000000000000000(0000) GS:ffff8880b9900000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f691b620d58 CR3: 000000002b3be000 CR4: 00000000003506e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
<TASK>
cm109_input_open+0x271/0x460 drivers/input/misc/cm109.c:572
input_open_device+0x1c9/0x310 drivers/input/input.c:654
kbd_connect+0xff/0x150 drivers/tty/vt/keyboard.c:1593
input_attach_handler.isra.0+0x17c/0x250 drivers/input/input.c:1064
input_register_device+0xb1e/0x1130 drivers/input/input.c:2396
cm109_usb_probe+0x1225/0x17b0 drivers/input/misc/cm109.c:806
usb_probe_interface+0x307/0x930 drivers/usb/core/driver.c:396
call_driver_probe drivers/base/dd.c:579 [inline]
really_probe+0x234/0xc90 drivers/base/dd.c:658
__driver_probe_device+0x1de/0x4b0 drivers/base/dd.c:800
driver_probe_device+0x4c/0x1a0 drivers/base/dd.c:830
__device_attach_driver+0x1d4/0x300 drivers/base/dd.c:958
bus_for_each_drv+0x157/0x1d0 drivers/base/bus.c:457
__device_attach+0x1e8/0x4b0 drivers/base/dd.c:1030
bus_probe_device+0x17c/0x1c0 drivers/base/bus.c:532
device_add+0x117e/0x1aa0 drivers/base/core.c:3624
usb_set_configuration+0x10cb/0x1c40 drivers/usb/core/message.c:2207
usb_generic_driver_probe+0xca/0x130 drivers/usb/core/generic.c:238
usb_probe_device+0xda/0x2c0 drivers/usb/core/driver.c:293
call_driver_probe drivers/base/dd.c:579 [inline]
really_probe+0x234/0xc90 drivers/base/dd.c:658
__driver_probe_device+0x1de/0x4b0 drivers/base/dd.c:800
driver_probe_device+0x4c/0x1a0 drivers/base/dd.c:830
__device_attach_driver+0x1d4/0x300 drivers/base/dd.c:958
bus_for_each_drv+0x157/0x1d0 drivers/base/bus.c:457
__device_attach+0x1e8/0x4b0 drivers/base/dd.c:1030
bus_probe_device+0x17c/0x1c0 drivers/base/bus.c:532
device_add+0x117e/0x1aa0 drivers/base/core.c:3624
usb_new_device+0xd80/0x1960 drivers/usb/core/hub.c:2597
hub_port_connect drivers/usb/core/hub.c:5459 [inline]
hub_port_connect_change drivers/usb/core/hub.c:5599 [inline]
port_event drivers/usb/core/hub.c:5759 [inline]
hub_event+0x2dac/0x4e10 drivers/usb/core/hub.c:5841
process_one_work+0x884/0x15c0 kernel/workqueue.c:2630
process_scheduled_works kernel/workqueue.c:2703 [inline]
worker_thread+0x8b9/0x1290 kernel/workqueue.c:2784
kthread+0x33c/0x440 kernel/kthread.c:388
ret_from_fork+0x45/0x80 arch/x86/kernel/process.c:147
ret_from_fork_asm+0x11/0x20 arch/x86/entry/entry_64.S:304
</TASK>
---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.
syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
If the bug is already fixed, let syzbot know by replying with:
#syz fix: exact-commit-title
If you want to overwrite bug's subsystems, reply with:
#syz set subsystems: new-subsystem
(See the list of subsystem names on the web dashboard)
If the bug is a duplicate of another bug, reply with:
#syz dup: exact-subject-of-another-report
If you want to undo deduplication, reply with:
#syz undup
^ permalink raw reply
* Re: [PATCH v4 3/4] mfd: cs40l50: Add support for CS40L50 core driver
From: James Ogletree @ 2023-10-20 15:39 UTC (permalink / raw)
To: Lee Jones
Cc: James Ogletree, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Fred Treven, Ben Bright, linux-input@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <20231019162359.GF2424087@google.com>
Thank you for your thorough review. Anything not replied to below will be
incorporated in the next version.
>> +/*
>> + * CS40L50 Advanced Haptic Driver with waveform memory,
>
> s/Driver/device/
CS40L50 is a “haptic driver”, like a "motor driver" in a car. It is an
unfortunate name in this context, but it is straight from the datasheet.
>> +static const struct mfd_cell cs40l50_devs[] = {
>> + {
>> + .name = "cs40l50-vibra",
>> + },
>
>
> Where are the other devices? Without them, it's not an MFD.
The driver will need to support I2S streaming to the device at some point
in the future, for which a codec driver will be added. I thought it better to
submit this as an MFD driver now, rather than as an Input driver, so as
not to have to move everything later.
Should I add the “cs40l50-codec” mfd_cell now, even though it does not
exist yet?
>> +static int cs40l50_handle_redc_est_done(struct cs40l50_private *cs40l50)
>> +{
>> + int error, fractional, integer, stored;
>
> err or ret is traditional.
We received feedback to change from “ret” to “error” in the input
subsystem, and now the opposite in MFD. I have no problem adopting
“err” here, but is it understood that styles will be mixed across
components?
>> +static irqreturn_t cs40l50_process_mbox(int irq, void *data)
>> +{
>> + struct cs40l50_private *cs40l50 = data;
>> + int error = 0;
>> + u32 val;
>> +
>> + mutex_lock(&cs40l50->lock);
>> +
>> + while (!cs40l50_mailbox_read_next(cs40l50, &val)) {
>> + switch (val) {
>> + case 0:
>> + mutex_unlock(&cs40l50->lock);
>> + dev_dbg(cs40l50->dev, "Reached end of queue\n");
>> + return IRQ_HANDLED;
>> + case CS40L50_MBOX_HAPTIC_TRIGGER_GPIO:
>> + dev_dbg(cs40l50->dev, "Mailbox: TRIGGER_GPIO\n");
>
> These all appear to be no-ops?
Correct.
>> + case CS40L50_MBOX_RUNTIME_SHORT:
>> + dev_err(cs40l50->dev, "Runtime short detected\n");
>> + error = cs40l50_error_release(cs40l50);
>> + if (error)
>> + goto out_mutex;
>> + break;
>> + default:
>> + dev_err(cs40l50->dev, "Payload %#X not recognized\n", val);
>> + error = -EINVAL;
>> + goto out_mutex;
>> + }
>> + }
>> +
>> + error = -EIO;
>> +
>> +out_mutex:
>> + mutex_unlock(&cs40l50->lock);
>> +
>> + return IRQ_RETVAL(!error);
>> +}
>
> Should the last two drivers live in drivers/mailbox?
Adopting the mailbox framework seems like an excessive amount
of overhead for our requirements.
>> +static irqreturn_t cs40l50_error(int irq, void *data);
>
> Why is this being forward declared?
>
>> +static const struct cs40l50_irq cs40l50_irqs[] = {
>> + CS40L50_IRQ(AMP_SHORT, "Amp short", error),
>
> I assume that last parameter is half of a function name.
>
> Better to have 2 different structures and do 2 requests I feel.
I think I will combine the two handler functions into one, so as not
to need the struct handler parameter, or the forward declaration.
>> +{
>> + struct device *dev = cs40l50->dev;
>> + int error;
>> +
>> + mutex_init(&cs40l50->lock);
>
> Don't you need to destroy this in the error path?
My understanding based on past feedback is that mutex_destroy()
is an empty function unless mutex debugging is enabled, and there
is no need cleanup the mutex explicitly. I will change this if you
disagree with that feedback.
>
>> +struct cs40l50_irq {
>> + const char *name;
>> + int irq;
>> + irqreturn_t (*handler)(int irq, void *data);
>> +};
>> +
>> +struct cs40l50_private {
>> + struct device *dev;
>> + struct regmap *regmap;
>> + struct cs_dsp dsp;
>> + struct mutex lock;
>> + struct gpio_desc *reset_gpio;
>> + struct regmap_irq_chip_data *irq_data;
>> + struct input_dev *input;
>
> Where is this used?
>
>> + const struct firmware *wmfw;
>
> Or this.
>
>> + struct cs_hap haptics;
>
> Or this?
>
>> + u32 devid;
>> + u32 revid;
>
> Are these used after they're set?
These are all used in the input driver, patch 4/4 of this series. If
this is not acceptable in some way, I will change it per your
suggestions.
Best,
James
^ permalink raw reply
* Re: [PATCH v4 4/4] Input: cs40l50 - Add support for the CS40L50 haptic driver
From: kernel test robot @ 2023-10-20 15:30 UTC (permalink / raw)
To: James Ogletree
Cc: oe-kbuild-all, James Ogletree, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, Lee Jones, Fred Treven, Ben Bright,
linux-input, devicetree, linux-kernel
In-Reply-To: <20231018175726.3879955-5-james.ogletree@opensource.cirrus.com>
Hi James,
kernel test robot noticed the following build warnings:
[auto build test WARNING on dtor-input/next]
[also build test WARNING on dtor-input/for-linus lee-mfd/for-mfd-next robh/for-next linus/master lee-mfd/for-mfd-fixes v6.6-rc6 next-20231020]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/James-Ogletree/dt-bindings-input-cirrus-cs40l50-Add-initial-DT-binding/20231019-015950
base: https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next
patch link: https://lore.kernel.org/r/20231018175726.3879955-5-james.ogletree%40opensource.cirrus.com
patch subject: [PATCH v4 4/4] Input: cs40l50 - Add support for the CS40L50 haptic driver
config: sparc-allyesconfig (https://download.01.org/0day-ci/archive/20231020/202310202344.LreohGFP-lkp@intel.com/config)
compiler: sparc64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231020/202310202344.LreohGFP-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310202344.LreohGFP-lkp@intel.com/
All warnings (new ones prefixed by >>):
drivers/input/misc/cs40l50-vibra.c: In function 'cs40l50_vibra_remove':
>> drivers/input/misc/cs40l50-vibra.c:329:13: warning: the comparison will always evaluate as 'true' for the address of 'dsp' will never be NULL [-Waddress]
329 | if (&cs40l50->dsp)
| ^
In file included from drivers/input/misc/cs40l50-vibra.c:11:
include/linux/mfd/cs40l50.h:180:23: note: 'dsp' declared here
180 | struct cs_dsp dsp;
| ^~~
vim +329 drivers/input/misc/cs40l50-vibra.c
319
320 static int cs40l50_vibra_remove(struct platform_device *pdev)
321 {
322 struct cs40l50_private *cs40l50 = dev_get_drvdata(pdev->dev.parent);
323
324 input_unregister_device(cs40l50->input);
325 cs_hap_remove(&cs40l50->haptics);
326
327 if (cs40l50->dsp.booted)
328 cs_dsp_power_down(&cs40l50->dsp);
> 329 if (&cs40l50->dsp)
330 cs_dsp_remove(&cs40l50->dsp);
331
332 return 0;
333 }
334
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCH v3 3/3] Input: Add TouchNetix axiom i2c touchscreen driver
From: Marco Felsch @ 2023-10-20 12:03 UTC (permalink / raw)
To: Kamel Bouhara
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Henrik Rydberg, linux-input, linux-kernel, devicetree,
mark.satterthwaite, bartp, hannah.rossiter, Thomas Petazzoni,
Gregory Clement, bsp-development.geo
In-Reply-To: <20231012074034.1090436-4-kamel.bouhara@bootlin.com>
Hi Kamel,
just a rough review.
On 23-10-12, Kamel Bouhara wrote:
> Add a new driver for the TouchNetix's axiom family of
> touchscreen controllers. This driver only supports i2c
> and can be later adapted for SPI and USB support.
>
> Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
> ---
> MAINTAINERS | 1 +
> drivers/input/touchscreen/Kconfig | 13 +
> drivers/input/touchscreen/Makefile | 1 +
> .../input/touchscreen/touchnetix_axiom_i2c.c | 740 ++++++++++++++++++
> 4 files changed, 755 insertions(+)
> create mode 100644 drivers/input/touchscreen/touchnetix_axiom_i2c.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 12ae8bc6b8cf..2d1e0b025e89 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -21415,6 +21415,7 @@ M: Kamel Bouhara <kamel.bouhara@bootlin.com>
> L: linux-input@vger.kernel.org
> S: Maintained
> F: Documentation/devicetree/bindings/input/touchscreen/touchnetix,axiom-ax54a.yaml
> +F: drivers/input/touchscreen/touchnetix_axiom_i2c.c
>
> THUNDERBOLT DMA TRAFFIC TEST DRIVER
> M: Isaac Hazan <isaac.hazan@intel.com>
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index e3e2324547b9..58665ccbe077 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -803,6 +803,19 @@ config TOUCHSCREEN_MIGOR
> To compile this driver as a module, choose M here: the
> module will be called migor_ts.
>
> +config TOUCHSCREEN_TOUCHNETIX_AXIOM_I2C
> + tristate "TouchNetix AXIOM based touchscreen controllers"
> + depends on I2C
> + depends on GPIOLIB || COMPILE_TEST
> + help
> + Say Y here if you have a axiom touchscreen connected to
> + your system.
> +
> + If unsure, say N.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called axiom_i2c.
> +
> config TOUCHSCREEN_TOUCHRIGHT
> tristate "Touchright serial touchscreen"
> select SERIO
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index 62bd24f3ac8e..23b6fb8864b0 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -88,6 +88,7 @@ obj-$(CONFIG_TOUCHSCREEN_SUR40) += sur40.o
> obj-$(CONFIG_TOUCHSCREEN_SURFACE3_SPI) += surface3_spi.o
> obj-$(CONFIG_TOUCHSCREEN_TI_AM335X_TSC) += ti_am335x_tsc.o
> obj-$(CONFIG_TOUCHSCREEN_TOUCHIT213) += touchit213.o
> +obj-$(CONFIG_TOUCHSCREEN_TOUCHNETIX_AXIOM_I2C) += touchnetix_axiom_i2c.o
> obj-$(CONFIG_TOUCHSCREEN_TOUCHRIGHT) += touchright.o
> obj-$(CONFIG_TOUCHSCREEN_TOUCHWIN) += touchwin.o
> obj-$(CONFIG_TOUCHSCREEN_TS4800) += ts4800-ts.o
> diff --git a/drivers/input/touchscreen/touchnetix_axiom_i2c.c b/drivers/input/touchscreen/touchnetix_axiom_i2c.c
> new file mode 100644
> index 000000000000..fb6239a87341
> --- /dev/null
> +++ b/drivers/input/touchscreen/touchnetix_axiom_i2c.c
> @@ -0,0 +1,740 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * TouchNetix aXiom Touchscreen Driver
> + *
> + * Copyright (C) 2020-2023 TouchNetix Ltd.
> + *
> + * Author(s): Bart Prescott <bartp@baasheep.co.uk>
> + * Pedro Torruella <pedro.torruella@touchnetix.com>
> + * Mark Satterthwaite <mark.satterthwaite@touchnetix.com>
> + * Hannah Rossiter <hannah.rossiter@touchnetix.com>
> + * Kamel Bouhara <kamel.bouhara@bootlin.com>
> + *
> + */
> +
> +#include <linux/crc16.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/input.h>
> +#include <linux/input/mt.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/pm.h>
> +#include <linux/slab.h>
> +#include <linux/string.h>
Can you please check if all headers are required e.g. sting.h
seems a bit suspicious here.
> +/*
> + * Runtime TCP mode: device is executing normal code and is
> + * accessible via the Touch Controller Mode
> + */
> +#define BOOT_TCP 0
> +/*
> + * Bootloader BLP mode: device is executing bootloader and is
> + * accessible via the Boot Loader Protocol.
> + */
> +#define BOOT_BLP 1
Both defines are not used.
> +#define AXIOM_PROX_LEVEL -128
> +/*
> + * Register group u31 has 2 pages for usage table entries.
> + * (2 * AXIOM_COMMS_PAGE_SIZE) / AXIOM_U31_BYTES_PER_USAGE = 85
> + */
> +#define AXIOM_U31_MAX_USAGES 85
The programmer's guid describe the usage as hexadecimal number prefixed
with an 'u'. The current range is from u00 till uFF, so the max. usages
should be 0xff.
> +#define AXIOM_U31_BYTES_PER_USAGE 6
> +#define AXIOM_U31_PAGE0_LENGTH 0x0C
> +#define AXIOM_U31_BOOTMODE_MASK BIT(7)
> +#define AXIOM_U31_FW_INFO_VARIANT_MASK GENMASK(6, 0)
> +#define AXIOM_U31_FW_INFO_STATUS_MASK BIT(7)
> +
> +#define AXIOM_U41_MAX_TARGETS 10
> +
> +#define AXIOM_U46_AUX_CHANNELS 4
> +#define AXIOM_U46_AUX_MASK GENMASK(11, 0)
I'm still not very happy with the decoding, since the so called
'protocol' is clear and versioned we can add the all required protocols
as struct which has far less magic offsets.
> +
> +#define AXIOM_COMMS_MAX_USAGE_PAGES 3
> +#define AXIOM_COMMS_PAGE_SIZE 256
> +#define AXIOM_COMMS_OVERFLOW_MASK BIT(7)
> +#define AXIOM_COMMS_REPORT_LEN_MASK GENMASK(7, 0)
> +
> +#define AXIOM_REBASELINE_CMD 0x03
> +
> +#define AXIOM_REPORT_USAGE_ID 0x34
> +#define AXIOM_DEVINFO_USAGE_ID 0x31
> +#define AXIOM_USAGE_2HB_REPORT_ID 0x01
> +#define AXIOM_REBASELINE_USAGE_ID 0x02
> +#define AXIOM_USAGE_2AUX_REPORT_ID 0x46
> +#define AXIOM_USAGE_2DCTS_REPORT_ID 0x41
> +
> +#define AXIOM_PAGE_MASK GENMASK(15, 8)
Unused
> +#define AXIOM_PAGE_OFFSET_MASK GENMASK(7, 0)
> +
> +struct axiom_devinfo {
> + char bootloader_fw_major;
> + char bootloader_fw_minor;
> + char bootmode;
> + u16 device_id;
> + char fw_major;
> + char fw_minor;
> + char fw_info_extra;
> + char tcp_revision;
> + u16 jedec_id;
> + char num_usages;
> + char silicon_revision;
> +};
> +
> +/*
> + * Describes parameters of a specific usage, essenstially a single element of
> + * the "Usage Table"
> + */
> +struct usage_entry {
> + char id;
> + char is_report;
> + char start_page;
> + char num_pages;
> +};
> +
> +/*
> + * Holds state of a touch or target when detected prior a touch (eg.
> + * hover or proximity events).
> + */
> +enum axiom_target_state {
> + TARGET_STATE_NOT_PRESENT = 0,
> + TARGET_STATE_PROX = 1,
> + TARGET_STATE_HOVER = 2,
> + TARGET_STATE_TOUCHING = 3,
> + TARGET_STATE_MIN = TARGET_STATE_NOT_PRESENT,
> + TARGET_STATE_MAX = TARGET_STATE_TOUCHING,
STATE_MIN/MAX not used.
> +};
> +
> +struct u41_target {
> + enum axiom_target_state state;
> + u16 x;
> + u16 y;
> + s8 z;
> + bool insert;
> + bool touch;
> +};
> +
> +struct axiom_target_report {
> + u8 index;
> + u8 present;
> + u16 x;
> + u16 y;
> + s8 z;
> +};
> +
> +struct axiom_cmd_header {
> + u16 target_address;
> + u16 length:15;
> + u16 read:1;
> +} __packed;
> +
> +struct axiom_data {
> + struct axiom_devinfo devinfo;
> + struct device *dev;
> + struct gpio_desc *reset_gpio;
> + struct gpio_desc *irq_gpio;
No need to store the irq_gpio here.
> + struct i2c_client *client;
> + struct input_dev *input_dev;
> + u32 max_report_len;
> + u32 report_overflow_counter;
> + u32 report_counter;
> + char rx_buf[AXIOM_COMMS_MAX_USAGE_PAGES * AXIOM_COMMS_PAGE_SIZE];
> + struct u41_target targets[AXIOM_U41_MAX_TARGETS];
> + struct usage_entry usage_table[AXIOM_U31_MAX_USAGES];
> + bool usage_table_populated;
> +};
> +
> +/*
> + * aXiom devices are typically configured to report
> + * touches at a rate of 100Hz (10ms). For systems
> + * that require polling for reports, 100ms seems like
> + * an acceptable polling rate.
> + * When reports are polled, it will be expected to
> + * occasionally observe the overflow bit being set
> + * in the reports. This indicates that reports are not
> + * being read fast enough.
> + */
> +#define POLL_INTERVAL_DEFAULT_MS 100
Above you describe that the touch-rate is ~10ms why do we configure it
10-times lower here? Also 100ms is huge if you think about user respone
time.
> +/* Translate usage/page/offset triplet into physical address. */
> +static u16
> +usage_to_target_address(struct axiom_data *ts, char usage, char page,
> + char offset)
> +{
> + struct axiom_devinfo *device_info;
> + struct usage_entry *usage_table;
> + u32 i;
> +
> + device_info = &ts->devinfo;
> + usage_table = ts->usage_table;
> +
> + /* At the moment the convention is that u31 is always at physical address 0x0 */
> + if (!ts->usage_table_populated) {
> + if (usage == AXIOM_DEVINFO_USAGE_ID)
> + return ((page << 8) + offset);
> + else
> + return 0xffff;
> + }
> +
> + for (i = 0; i < device_info->num_usages; i++) {
> + if (usage_table[i].id != usage)
> + continue;
> +
> + if (page >= usage_table[i].num_pages) {
> + dev_err(ts->dev, "Invalid usage table! usage: %u, page: %u, offset: %u\n",
> + usage, page, offset);
> + return 0xffff;
> + }
> + }
We can avoid this loop if we store the usage table exactly as it is,
e.g.:
usage_table[0x31] = u31;
usage_table[0x41] = u41;
> + return ((usage_table[i].start_page + page) << 8) + offset;
> +}
> +
> +static int
> +axiom_i2c_read(struct i2c_client *client, u8 usage, u8 page, u8 *buf, u16 len)
> +{
> + struct axiom_data *ts = i2c_get_clientdata(client);
> + struct axiom_cmd_header cmd_header;
> + struct i2c_msg msg[2];
> + int ret;
> +
> + cmd_header.target_address = cpu_to_le16(usage_to_target_address(ts, usage, page, 0));
> + cmd_header.length = cpu_to_le16(len);
> + cmd_header.read = 1;
> +
> + msg[0].addr = client->addr;
> + msg[0].flags = 0;
> + msg[0].len = sizeof(cmd_header);
> + msg[0].buf = (u8 *)&cmd_header;
> +
> + msg[1].addr = client->addr;
> + msg[1].flags = I2C_M_RD;
> + msg[1].len = len;
> + msg[1].buf = (char *)buf;
> +
> + ret = i2c_transfer(client->adapter, msg, 2);
> + if (ret != ARRAY_SIZE(msg)) {
> + dev_err(&client->dev,
> + "Failed reading usage %#x page %#x, error=%d\n",
> + usage, page, ret);
> + return -EIO;
> + }
> +
> + return 0;
> +}
> +
> +static int
> +axiom_i2c_write(struct i2c_client *client, u8 usage, u8 page, u8 *buf, u16 len)
> +{
> + struct axiom_data *ts = i2c_get_clientdata(client);
> + struct axiom_cmd_header cmd_header;
> + struct i2c_msg msg[2];
> + int ret;
> +
> + cmd_header.target_address = cpu_to_le16(usage_to_target_address(ts, usage, page, 0));
> + cmd_header.length = cpu_to_le16(len);
> + cmd_header.read = 0;
> +
> + msg[0].addr = client->addr;
> + msg[0].flags = 0;
> + msg[0].len = sizeof(cmd_header);
> + msg[0].buf = (u8 *)&cmd_header;
> +
> + msg[1].addr = client->addr;
> + msg[1].flags = 0;
> + msg[1].len = len;
> + msg[1].buf = (char *)buf;
Please check the "comms protocol app note", for write it is not allowed
to send a stop, so the whole data must be send in one i2c_msg.
> +
> + ret = i2c_transfer(client->adapter, msg, 2);
> + if (ret < 0) {
> + dev_err(&client->dev,
> + "Failed to write usage %#x page %#x, error=%d\n", usage,
> + page, ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * Decodes and populates the local Usage Table.
> + * Given a buffer of data read from page 1 onwards of u31 from an aXiom
> + * device.
> + */
> +static u32 axiom_populate_usage_table(struct axiom_data *ts, char *rx_data)
> +{
> + u32 usage_id = 0;
> + u32 max_report_len = 0;
> + struct axiom_devinfo *device_info;
> + struct usage_entry *usage_table;
> +
> + device_info = &ts->devinfo;
> + usage_table = ts->usage_table;
> +
> + for (usage_id = 0; usage_id < device_info->num_usages; usage_id++) {
> + u16 offset = (usage_id * AXIOM_U31_BYTES_PER_USAGE);
> + char id = rx_data[offset + 0];
> + char start_page = rx_data[offset + 1];
> + char num_pages = rx_data[offset + 2];
> + u32 max_offset = ((rx_data[offset + 3] & AXIOM_PAGE_OFFSET_MASK) + 1) * 2;
> +
> + if (!num_pages)
> + usage_table[usage_id].is_report = true;
> +
> + /* Store the entry into the usage table */
> + usage_table[usage_id].id = id;
> + usage_table[usage_id].start_page = start_page;
> + usage_table[usage_id].num_pages = num_pages;
> +
> + dev_dbg(ts->dev, "Usage %2u Info: %*ph\n", usage_id,
> + AXIOM_U31_BYTES_PER_USAGE,
> + &rx_data[offset]);
> +
> + /* Identify the max report length the module will receive */
> + if (usage_table[usage_id].is_report && max_offset > max_report_len)
> + max_report_len = max_offset;
> + }
As said, the sorting can be really easy:
usage_table[0x01] = u01;
usage_table[0x31] = u31;
> + ts->usage_table_populated = true;
> +
> + return max_report_len;
> +}
> +
> +/* Retrieve, store and print the axiom device information */
> +static int axiom_discover(struct axiom_data *ts)
> +{
> + struct axiom_devinfo *devinfo = &ts->devinfo;
> + struct device *dev = ts->dev;
> + char *rx_data = ts->rx_buf;
> + int ret;
> +
> + /*
> + * Fetch the first page of usage u31 to get the
> + * device information and the number of usages
> + */
> + ret = axiom_i2c_read(ts->client, AXIOM_DEVINFO_USAGE_ID, 0, rx_data,
> + AXIOM_U31_PAGE0_LENGTH);
> + if (ret)
> + return ret;
> +
> + devinfo->bootmode = (rx_data[0] & AXIOM_U31_BOOTMODE_MASK);
> + devinfo->device_id = ((rx_data[1] & AXIOM_PAGE_OFFSET_MASK) << 8) | rx_data[0];
> + devinfo->fw_minor = rx_data[2];
> + devinfo->fw_major = rx_data[3];
> + devinfo->fw_info_extra = rx_data[4];
> + devinfo->bootloader_fw_minor = rx_data[6];
> + devinfo->bootloader_fw_major = rx_data[7];
> + devinfo->jedec_id = (rx_data[8]) | (rx_data[9] << 8);
> + devinfo->num_usages = rx_data[10];
> + devinfo->silicon_revision = rx_data[11];
> +
> + dev_dbg(dev, " Boot Mode: %s\n", ts->devinfo.bootmode ? "BLP" : "TCP");
> + dev_dbg(dev, " Device ID : %04x\n", ts->devinfo.device_id);
> + dev_dbg(dev, " Firmware Rev : %02x.%02x\n", ts->devinfo.fw_major,
> + ts->devinfo.fw_minor);
> + dev_dbg(dev, " Bootloader Rev : %02x.%02x\n",
> + ts->devinfo.bootloader_fw_major,
> + ts->devinfo.bootloader_fw_minor);
> + dev_dbg(dev, " FW Extra Info : %04x\n", ts->devinfo.fw_info_extra);
> + dev_dbg(dev, " Silicon : %02x\n", ts->devinfo.jedec_id);
> + dev_dbg(dev, " Num Usages : %04x\n", ts->devinfo.num_usages);
> +
> + /* Read the second page of usage u31 to get the usage table */
> + ret = axiom_i2c_read(ts->client, AXIOM_DEVINFO_USAGE_ID, 1, rx_data,
> + (AXIOM_U31_BYTES_PER_USAGE * ts->devinfo.num_usages));
> + if (ret)
> + return ret;
> +
> + ts->max_report_len = axiom_populate_usage_table(ts, rx_data);
> + dev_dbg(dev, "Max Report Length: %u\n", ts->max_report_len);
> +
> + return 0;
> +}
> +
> +/*
> + * Support function to axiom_process_u41_report.
> + * Generates input-subsystem events for every target.
> + * After calling this function the caller shall issue
> + * a Sync to the input sub-system.
> + */
> +static bool
> +axiom_process_u41_report_target(struct axiom_data *ts,
> + struct axiom_target_report *target)
> +{
> + struct input_dev *input_dev = ts->input_dev;
> + enum axiom_target_state current_state;
> + struct u41_target *target_prev_state;
> + struct device *dev = ts->dev;
> + bool update = false;
> + int slot;
> +
> + /* Verify the target index */
> + if (target->index >= AXIOM_U41_MAX_TARGETS) {
> + dev_dbg(dev, "Invalid target index! %u\n", target->index);
> + return false;
> + }
> +
> + target_prev_state = &ts->targets[target->index];
> +
> + current_state = TARGET_STATE_NOT_PRESENT;
> +
> + if (target->present) {
> + if (target->z >= 0)
> + current_state = TARGET_STATE_TOUCHING;
> + else if (target->z > AXIOM_PROX_LEVEL && target->z < 0)
> + current_state = TARGET_STATE_HOVER;
> + else if (target->z AXIOM_PROX_LEVEL)
> + current_state = TARGET_STATE_PROX;
> + }
> +
> + if (target_prev_state->state == current_state &&
> + target_prev_state->x == target->x &&
> + target_prev_state->y == target->y &&
> + target_prev_state->z == target->z) {
> + return false;
> + }
> +
> + slot = target->index;
> +
> + dev_dbg(dev, "U41 Target T%u, slot:%u present:%u, x:%u, y:%u, z:%d\n",
> + target->index, slot, target->present,
> + target->x, target->y, target->z);
> +
> + switch (current_state) {
> + case TARGET_STATE_NOT_PRESENT:
> + case TARGET_STATE_PROX:
> + if (target_prev_state->insert)
> + break;
> + update = true;
> + target_prev_state->insert = false;
> + input_mt_slot(input_dev, slot);
> +
> + if (!slot)
> + input_report_key(input_dev, BTN_LEFT, 0);
> +
> + input_mt_report_slot_inactive(input_dev);
> + /*
> + * make sure the previous coordinates are
> + * all off screen when the finger comes back
> + */
> + target->x = 65535;
> + target->y = 65535;
> + target->z = AXIOM_PROX_LEVEL;
> + break;
> + case TARGET_STATE_HOVER:
> + case TARGET_STATE_TOUCHING:
> + target_prev_state->insert = true;
> + update = true;
> + input_mt_slot(input_dev, slot);
> + input_report_abs(input_dev, ABS_MT_TRACKING_ID, slot);
> + input_report_abs(input_dev, ABS_MT_POSITION_X, target->x);
> + input_report_abs(input_dev, ABS_X, target->x);
> + input_report_abs(input_dev, ABS_MT_POSITION_Y, target->y);
> + input_report_abs(input_dev, ABS_Y, target->y);
> +
> + if (current_state == TARGET_STATE_TOUCHING) {
> + input_report_abs(input_dev, ABS_MT_DISTANCE, 0);
> + input_report_abs(input_dev, ABS_DISTANCE, 0);
> + input_report_abs(input_dev, ABS_MT_PRESSURE, target->z);
> + input_report_abs(input_dev, ABS_PRESSURE, target->z);
> + } else {
> + input_report_abs(input_dev, ABS_MT_DISTANCE, -target->z);
> + input_report_abs(input_dev, ABS_DISTANCE, -target->z);
^
Is this valid?
> + input_report_abs(input_dev, ABS_MT_PRESSURE, 0);
> + input_report_abs(input_dev, ABS_PRESSURE, 0);
> + }
> +
> + if (!slot)
> + input_report_key(input_dev, BTN_LEFT, (current_state ==
> + TARGET_STATE_TOUCHING));
> + break;
> + }
> +
> + target_prev_state->state = current_state;
> + target_prev_state->x = target->x;
> + target_prev_state->y = target->y;
> + target_prev_state->z = target->z;
> +
> + if (update)
> + input_mt_sync_frame(input_dev);
> +
> + return update;
> +}
> +
> +/*
> + * Take a raw buffer with u41 report data and decode it.
> + * Also generate input events if needed.
> + * rx_buf: ptr to a byte array [0]: Usage number [1]: Status LSB [2]: Status MSB
> + */
> +static void axiom_process_u41_report(struct axiom_data *ts, char *rx_buf)
> +{
> + struct input_dev *input_dev = ts->input_dev;
> + struct axiom_target_report target;
> + bool update_done = false;
> + u16 target_status;
> + u32 i;
> +
> + target_status = ((rx_buf[1]) | (rx_buf[2] << 8));
> +
> + for (i = 0; i < AXIOM_U41_MAX_TARGETS; i++) {
> + char target_step = rx_buf[(i * 4)];
> +
> + target.index = i;
> + target.present = ((target_status & (1 << i)) != 0) ? 1 : 0;
> + target.x = ((target_step + 3) | ((target_step + 4) << 8));
> + target.y = ((target_step + 5) | ((target_step + 6) << 8));
> + target.z = (s8)(rx_buf[i + 43]);
> + update_done |= axiom_process_u41_report_target(ts, &target);
> + }
> +
> + if (update_done)
> + input_sync(input_dev);
> +}
> +
> +static void axiom_process_u46_report(struct axiom_data *ts, char *rx_buf)
> +{
> + struct input_dev *input_dev = ts->input_dev;
> + u32 event_value;
> + u16 aux_value;
> + u32 i = 0;
> +
> + for (i = 0; i < AXIOM_U46_AUX_CHANNELS; i++) {
> + char target_step = rx_buf[(i * 2)];
> +
> + aux_value = (((target_step + 2) << 8) | (target_step + 1)) & AXIOM_U46_AUX_MASK;
> + event_value = (i << 16) | (aux_value);
> + input_event(input_dev, EV_MSC, MSC_RAW, event_value);
> + }
> +
> + input_mt_sync(input_dev);
> + input_sync(input_dev);
> +}
> +
> +/*
> + * Validates the crc and demultiplexes the axiom reports to the appropriate
> + * report handler
> + */
> +static void axiom_handle_events(struct axiom_data *ts)
> +{
> + char *report_data = ts->rx_buf;
> + struct device *dev = ts->dev;
> + char usage = report_data[1];
> + u16 crc_report;
> + u16 crc_calc;
> + char len;
> +
> + axiom_i2c_read(ts->client, AXIOM_REPORT_USAGE_ID, 0, report_data, ts->max_report_len);
> +
> + if ((report_data[0] & AXIOM_COMMS_OVERFLOW_MASK) != 0)
> + ts->report_overflow_counter++;
> +
> + len = (report_data[0] & AXIOM_COMMS_REPORT_LEN_MASK) * 2;
> + if (!len) {
> + dev_err(dev, "Zero length report discarded.\n");
> + return;
> + }
> +
> + dev_dbg(dev, "Payload Data %*ph\n", len, report_data);
> +
> + /* Validate the report CRC */
> + crc_report = (report_data[len - 1] << 8) | (report_data[len - 2]);
> + /* Length is in 16 bit words and remove the size of the CRC16 itself */
> + crc_calc = crc16(0, report_data, (len - 2));
> +
> + if (crc_calc != crc_report) {
> + dev_err(dev,
> + "CRC mismatch! Expected: %#x, Calculated CRC: %#x.\n",
> + crc_report, crc_calc);
> + return;
> + }
> +
> + switch (usage) {
> + case AXIOM_USAGE_2DCTS_REPORT_ID:
> + axiom_process_u41_report(ts, &report_data[1]);
> + break;
> +
> + case AXIOM_USAGE_2AUX_REPORT_ID:
> + /* This is an aux report (force) */
> + axiom_process_u46_report(ts, &report_data[1]);
> + break;
> +
> + case AXIOM_USAGE_2HB_REPORT_ID:
> + /* This is a heartbeat report */
> + break;
> + }
> +
> + ts->report_counter++;
> +}
> +
> +static void axiom_i2c_poll(struct input_dev *input_dev)
> +{
> + struct axiom_data *ts = input_get_drvdata(input_dev);
> +
> + axiom_handle_events(ts);
> +}
> +
> +static irqreturn_t axiom_irq(int irq, void *dev_id)
> +{
> + struct axiom_data *ts = dev_id;
> +
> + axiom_handle_events(ts);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static void axiom_reset(struct gpio_desc *reset_gpio)
> +{
> + gpiod_set_value_cansleep(reset_gpio, 1);
> + usleep_range(1000, 2000);
> + gpiod_set_value_cansleep(reset_gpio, 0);
> + msleep(100);
> +}
> +
> +/* Rebaseline the touchscreen, effectively zero-ing it */
> +static int axiom_rebaseline(struct axiom_data *ts)
> +{
> + char buffer[8] = {};
> +
> + buffer[0] = AXIOM_REBASELINE_CMD;
> +
> + return axiom_i2c_write(ts->client, AXIOM_REPORT_USAGE_ID, 0, buffer, sizeof(buffer));
> +}
> +
> +static int axiom_i2c_probe(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + struct input_dev *input_dev;
> + struct axiom_data *ts;
> + int ret;
> + int target;
> +
> + ts = devm_kzalloc(dev, sizeof(*ts), GFP_KERNEL);
> + if (!ts)
> + return -ENOMEM;
> +
> + ts->client = client;
> + i2c_set_clientdata(client, ts);
> + ts->dev = dev;
> +
> + ts->irq_gpio = devm_gpiod_get_optional(dev, "irq", GPIOD_IN);
> + if (IS_ERR(ts->irq_gpio))
> + return dev_err_probe(dev, PTR_ERR(ts->irq_gpio), "failed to get irq GPIO");
Nope you removed this from the binding.
> + ts->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> + if (IS_ERR(ts->reset_gpio))
> + return dev_err_probe(dev, PTR_ERR(ts->reset_gpio), "failed to get reset GPIO\n");
Please also add a regulator for the VDDI/VDDA which is required for the
device to work properly.
> + axiom_reset(ts->reset_gpio);
> +
> + if (ts->irq_gpio) {
Nope, please drop the ts->irq_gpio check.
> + ret = devm_request_threaded_irq(dev, client->irq, NULL,
> + axiom_irq, 0, dev_name(dev), ts);
> + if (ret < 0)
If the threaded_irq does fail you can fallback to the polling mode.
> + return dev_err_probe(dev, ret, "Failed to request threaded IRQ\n");
> + }
> +
> + ret = axiom_discover(ts);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed touchscreen discover\n");
> +
> + ret = axiom_rebaseline(ts);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed touchscreen re-baselining\n");
> +
> + input_dev = devm_input_allocate_device(ts->dev);
> + if (!input_dev)
> + return -ENOMEM;
> +
> + input_dev->name = "TouchNetix aXiom Touchscreen";
> + input_dev->phys = "input/axiom_ts";
> +
> + /* Single Touch */
> + input_set_abs_params(input_dev, ABS_X, 0, 65535, 0, 0);
> + input_set_abs_params(input_dev, ABS_Y, 0, 65535, 0, 0);
> +
> + /* Multi Touch */
> + /* Min, Max, Fuzz (expected noise in px, try 4?) and Flat */
> + input_set_abs_params(input_dev, ABS_MT_POSITION_X, 0, 65535, 0, 0);
> + /* Min, Max, Fuzz (expected noise in px, try 4?) and Flat */
> + input_set_abs_params(input_dev, ABS_MT_POSITION_Y, 0, 65535, 0, 0);
> + input_set_abs_params(input_dev, ABS_MT_TOOL_TYPE, 0, MT_TOOL_MAX, 0, 0);
> + input_set_abs_params(input_dev, ABS_MT_DISTANCE, 0, 127, 0, 0);
> + input_set_abs_params(input_dev, ABS_MT_PRESSURE, 0, 127, 0, 0);
> +
> + /* Registers the axiom device as a touchscreen instead of as a mouse pointer */
> + input_mt_init_slots(input_dev, AXIOM_U41_MAX_TARGETS, INPUT_MT_DIRECT);
> +
> + input_set_capability(input_dev, EV_KEY, BTN_LEFT);
> +
> + /* Enables the raw data for up to 4 force channels to be sent to the input subsystem */
> + set_bit(EV_REL, input_dev->evbit);
> + set_bit(EV_MSC, input_dev->evbit);
> + /* Declare that we support "RAW" Miscellaneous events */
> + set_bit(MSC_RAW, input_dev->mscbit);
> +
> + if (!ts->irq_gpio) {
> + ret = input_setup_polling(input_dev, axiom_i2c_poll);
> + if (ret)
> + return dev_err_probe(ts->dev, ret, "Unable to set up polling mode\n");
> + input_set_poll_interval(input_dev, POLL_INTERVAL_DEFAULT_MS);
> + }
> +
> + ts->input_dev = input_dev;
> + input_set_drvdata(ts->input_dev, ts);
> +
> + /* Ensure that all reports are initialised to not be present. */
> + for (target = 0; target < AXIOM_U41_MAX_TARGETS; target++)
> + ts->targets[target].state = TARGET_STATE_NOT_PRESENT;
> +
> + ret = input_register_device(input_dev);
> +
> + if (ret)
> + return dev_err_probe(ts->dev, ret,
> + "Could not register with Input Sub-system.\n");
> +
> + return 0;
> +}
> +
> +static void axiom_i2c_remove(struct i2c_client *client)
> +{
> + struct axiom_data *ts = i2c_get_clientdata(client);
> +
> + input_unregister_device(ts->input_dev);
This can be part of devm_add_action_or_reset() and we could remove the
.remove() callback for this driver.
> +}
> +
> +static const struct i2c_device_id axiom_i2c_id_table[] = {
> + { "axiom-ax54a" },
Albeit the datasheet says: "axiom ax54a" I think ax stands for axiom. So
the name should be "ax54a" only?
> + {},
Nit: { },
> +};
> +
Please drop the unnecessary newline.
> +MODULE_DEVICE_TABLE(i2c, axiom_i2c_id_table);
> +
> +static const struct of_device_id axiom_i2c_of_match[] = {
> + { .compatible = "touchnetix,axiom-ax54a", },
> + {}
same here.
> +};
> +
same here.
> +MODULE_DEVICE_TABLE(of, axiom_i2c_of_match);
> +
> +static struct i2c_driver axiom_i2c_driver = {
> + .driver = {
> + .name = "axiom",
> + .of_match_table = axiom_i2c_of_match,
> + },
> + .id_table = axiom_i2c_id_table,
> + .probe = axiom_i2c_probe,
> + .remove = axiom_i2c_remove,
> +};
> +
same here.
> +module_i2c_driver(axiom_i2c_driver);
> +
> +MODULE_AUTHOR("Bart Prescott <bartp@baasheep.co.uk>");
> +MODULE_AUTHOR("Pedro Torruella <pedro.torruella@touchnetix.com>");
> +MODULE_AUTHOR("Mark Satterthwaite <mark.satterthwaite@touchnetix.com>");
> +MODULE_AUTHOR("Hannah Rossiter <hannah.rossiter@touchnetix.com>");
> +MODULE_AUTHOR("Kamel Bouhara <kamel.bouhara@bootlin.com>");
> +MODULE_DESCRIPTION("TouchNetix aXiom touchscreen I2C bus driver");
> +MODULE_LICENSE("GPL");
> --
> 2.25.1
>
>
^ permalink raw reply
* [PATCH] HID: wacom_sys: add error code check in wacom_feature_mapping
From: Su Hui @ 2023-10-20 9:02 UTC (permalink / raw)
To: ping.cheng, jason.gerecke, jikos, benjamin.tissoires
Cc: Su Hui, linux-input, linux-kernel, kernel-janitors
hid_report_raw_event() can return error code like '-ENOMEM' if
failed, so check 'ret' to make sure all things work fine.
Signed-off-by: Su Hui <suhui@nfschina.com>
---
drivers/hid/wacom_sys.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 3f704b8072e8..1f898d4ee708 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -320,6 +320,8 @@ static void wacom_feature_mapping(struct hid_device *hdev,
if (ret == n && features->type == HID_GENERIC) {
ret = hid_report_raw_event(hdev,
HID_FEATURE_REPORT, data, n, 0);
+ if (ret)
+ hid_warn(hdev, "failed to report feature\n");
} else if (ret == 2 && features->type != HID_GENERIC) {
features->touch_max = data[1];
} else {
@@ -381,6 +383,8 @@ static void wacom_feature_mapping(struct hid_device *hdev,
if (ret == n) {
ret = hid_report_raw_event(hdev, HID_FEATURE_REPORT,
data, n, 0);
+ if (ret)
+ hid_warn(hdev, "failed to report feature\n");
} else {
hid_warn(hdev, "%s: could not retrieve sensor offsets\n",
__func__);
--
2.30.2
^ permalink raw reply related
* Re: [PATCH v4 4/5] clk: twl: add clock driver for TWL6032
From: Stephen Boyd @ 2023-10-19 23:43 UTC (permalink / raw)
To: andreas, bcousson, conor+dt, devicetree, dmitry.torokhov,
krzysztof.kozlowski+dt, lee, linux-clk, linux-input, linux-kernel,
linux-omap, mturquette, robh+dt, tony
In-Reply-To: <20230916100515.1650336-5-andreas@kemnade.info>
Quoting Andreas Kemnade (2023-09-16 03:05:14)
> The TWL6032 has some clock outputs which are controlled like
> fixed-voltage regulators, in some drivers for these chips
> found in the wild, just the regulator api is abused for controlling
> them, so simply use something similar to the regulator functions.
> Due to a lack of hardware available for testing, leave out the
> TWL6030-specific part of those functions.
>
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
Applied to clk-next
^ permalink raw reply
* Re: [PATCH v4 3/4] mfd: cs40l50: Add support for CS40L50 core driver
From: Lee Jones @ 2023-10-19 16:23 UTC (permalink / raw)
To: James Ogletree
Cc: James Ogletree, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Fred Treven, Ben Bright, linux-input, devicetree, linux-kernel
In-Reply-To: <20231018175726.3879955-4-james.ogletree@opensource.cirrus.com>
On Wed, 18 Oct 2023, James Ogletree wrote:
> From: James Ogletree <james.ogletree@cirrus.com>
>
> Introduce support for Cirrus Logic Device CS40L50: a
> haptic driver with waveform memory, integrated DSP,
> and closed-loop algorithms.
>
> The MFD component registers and initializes the device.
>
> Signed-off-by: James Ogletree <james.ogletree@cirrus.com>
> ---
> MAINTAINERS | 2 +
> drivers/mfd/Kconfig | 30 +++
> drivers/mfd/Makefile | 4 +
> drivers/mfd/cs40l50-core.c | 443 ++++++++++++++++++++++++++++++++++++
> drivers/mfd/cs40l50-i2c.c | 69 ++++++
> drivers/mfd/cs40l50-spi.c | 68 ++++++
> include/linux/mfd/cs40l50.h | 198 ++++++++++++++++
> 7 files changed, 814 insertions(+)
> create mode 100644 drivers/mfd/cs40l50-core.c
> create mode 100644 drivers/mfd/cs40l50-i2c.c
> create mode 100644 drivers/mfd/cs40l50-spi.c
> create mode 100644 include/linux/mfd/cs40l50.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 57daf77bf550..08e1e9695d49 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4971,7 +4971,9 @@ L: patches@opensource.cirrus.com
> S: Supported
> F: Documentation/devicetree/bindings/input/cirrus,cs40l50.yaml
> F: drivers/input/misc/cirrus*
> +F: drivers/mfd/cs40l*
> F: include/linux/input/cirrus*
> +F: include/linux/mfd/cs40l*
>
> CIRRUS LOGIC DSP FIRMWARE DRIVER
> M: Simon Trimmer <simont@opensource.cirrus.com>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 8b93856de432..a133d04a7859 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -2187,6 +2187,36 @@ config MCP_UCB1200_TS
>
> endmenu
>
> +config MFD_CS40L50_CORE
> + tristate
> + select MFD_CORE
> + select CS_DSP
> + select REGMAP_IRQ
> +
> +config MFD_CS40L50_I2C
> + tristate "Cirrus Logic CS40L50 (I2C)"
> + select REGMAP_I2C
> + select MFD_CS40L50_CORE
> + depends on I2C
> + help
> + Select this to support the Cirrus Logic CS40L50 Haptic
> + Driver over I2C.
> +
> + This driver can be built as a module. If built as a module it will be
> + called "cs40l50-i2c".
> +
> +config MFD_CS40L50_SPI
> + tristate "Cirrus Logic CS40L50 (SPI)"
> + select REGMAP_SPI
> + select MFD_CS40L50_CORE
> + depends on SPI
> + help
> + Select this to support the Cirrus Logic CS40L50 Haptic
> + Driver over SPI.
> +
> + This driver can be built as a module. If built as a module it will be
> + called "cs40l50-spi".
> +
> config MFD_VEXPRESS_SYSREG
> tristate "Versatile Express System Registers"
> depends on VEXPRESS_CONFIG && GPIOLIB
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 7ed3ef4a698c..3b1a43b3acaf 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -95,6 +95,10 @@ obj-$(CONFIG_MFD_MADERA) += madera.o
> obj-$(CONFIG_MFD_MADERA_I2C) += madera-i2c.o
> obj-$(CONFIG_MFD_MADERA_SPI) += madera-spi.o
>
> +obj-$(CONFIG_MFD_CS40L50_CORE) += cs40l50-core.o
> +obj-$(CONFIG_MFD_CS40L50_I2C) += cs40l50-i2c.o
> +obj-$(CONFIG_MFD_CS40L50_SPI) += cs40l50-spi.o
> +
> obj-$(CONFIG_TPS6105X) += tps6105x.o
> obj-$(CONFIG_TPS65010) += tps65010.o
> obj-$(CONFIG_TPS6507X) += tps6507x.o
> diff --git a/drivers/mfd/cs40l50-core.c b/drivers/mfd/cs40l50-core.c
> new file mode 100644
> index 000000000000..f1eadd80203a
> --- /dev/null
> +++ b/drivers/mfd/cs40l50-core.c
> @@ -0,0 +1,443 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * CS40L50 Advanced Haptic Driver with waveform memory,
s/Driver/device/
> + * integrated DSP, and closed-loop algorithms
> + *
> + * Copyright 2023 Cirrus Logic, Inc.
> + *
Remove this line.
No Author?
> + */
> +
> +#include <linux/gpio/consumer.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/cs40l50.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regulator/consumer.h>
> +
> +static const struct mfd_cell cs40l50_devs[] = {
> + {
> + .name = "cs40l50-vibra",
> + },
This should be on one line.
Where are the other devices? Without them, it's not an MFD.
> +};
> +
> +const struct regmap_config cs40l50_regmap = {
> + .reg_bits = 32,
> + .reg_stride = 4,
> + .val_bits = 32,
> + .reg_format_endian = REGMAP_ENDIAN_BIG,
> + .val_format_endian = REGMAP_ENDIAN_BIG,
> +};
> +EXPORT_SYMBOL_GPL(cs40l50_regmap);
> +
> +static struct regulator_bulk_data cs40l50_supplies[] = {
> + {
> + .supply = "vp",
> + },
> + {
> + .supply = "vio",
> + },
One line each please.
> +};
> +
> +static int cs40l50_handle_f0_est_done(struct cs40l50_private *cs40l50)
> +{
> + u32 f_zero;
> + int error;
> +
> + error = regmap_read(cs40l50->regmap, CS40L50_F0_ESTIMATION, &f_zero);
> + if (error)
> + return error;
> +
> + return regmap_write(cs40l50->regmap, CS40L50_F0_STORED, f_zero);
> +}
I have no idea what this is doing (probably goes for the following
functions too. Either give the function a friendly name or provide
commentary so people can read it.
> +static int cs40l50_handle_redc_est_done(struct cs40l50_private *cs40l50)
> +{
> + int error, fractional, integer, stored;
err or ret is traditional.
The other variables need better nomenclature.
> + u32 redc;
This one too.
I won't mention this again, but is likely to apply throughout.
> +
> + error = regmap_read(cs40l50->regmap, CS40L50_RE_EST_STATUS, &redc);
> + if (error)
> + return error;
> +
> + error = regmap_write(cs40l50->regmap, CS40L50_REDC_ESTIMATION, redc);
> + if (error)
> + return error;
> +
> + /* Convert from Q8.15 to (Q7.17 * 29/240) */
I have no idea what this is supposed to be telling me.
> + integer = ((redc >> 15) & 0xFF) << 17;
> + fractional = (redc & 0x7FFF) * 4;
> + stored = (integer | fractional) * 29 / 240;
No magic numbers. Define them all please.
> + return regmap_write(cs40l50->regmap, CS40L50_REDC_STORED, stored);
> +}
> +
> +static int cs40l50_error_release(struct cs40l50_private *cs40l50)
> +{
> + int error;
> +
> + error = regmap_write(cs40l50->regmap, CS40L50_ERR_RLS,
> + CS40L50_GLOBAL_ERR_RLS);
> + if (error)
> + return error;
> +
> + return regmap_write(cs40l50->regmap, CS40L50_ERR_RLS, 0);
> +}
> +
> +static int cs40l50_mailbox_read_next(struct cs40l50_private *cs40l50, u32 *val)
> +{
> + u32 rd_ptr, wt_ptr;
> + int error;
> +
> + error = regmap_read(cs40l50->regmap, CS40L50_MBOX_QUEUE_WT, &wt_ptr);
> + if (error)
> + return error;
> +
> + error = regmap_read(cs40l50->regmap, CS40L50_MBOX_QUEUE_RD, &rd_ptr);
> + if (error)
> + return error;
> +
> + if (wt_ptr == rd_ptr) {
> + *val = 0;
> + return 0;
> + }
> +
> + error = regmap_read(cs40l50->regmap, rd_ptr, val);
> + if (error)
> + return error;
> +
> + rd_ptr += sizeof(u32);
> + if (rd_ptr > CS40L50_MBOX_QUEUE_END)
> + rd_ptr = CS40L50_MBOX_QUEUE_BASE;
> +
> + return regmap_write(cs40l50->regmap, CS40L50_MBOX_QUEUE_RD, rd_ptr);
> +}
> +
> +static irqreturn_t cs40l50_process_mbox(int irq, void *data)
> +{
> + struct cs40l50_private *cs40l50 = data;
> + int error = 0;
> + u32 val;
> +
> + mutex_lock(&cs40l50->lock);
> +
> + while (!cs40l50_mailbox_read_next(cs40l50, &val)) {
> + switch (val) {
> + case 0:
> + mutex_unlock(&cs40l50->lock);
> + dev_dbg(cs40l50->dev, "Reached end of queue\n");
> + return IRQ_HANDLED;
> + case CS40L50_MBOX_HAPTIC_TRIGGER_GPIO:
> + dev_dbg(cs40l50->dev, "Mailbox: TRIGGER_GPIO\n");
These all appear to be no-ops?
> + break;
> + case CS40L50_MBOX_HAPTIC_TRIGGER_MBOX:
> + dev_dbg(cs40l50->dev, "Mailbox: TRIGGER_MBOX\n");
> + break;
> + case CS40L50_MBOX_HAPTIC_TRIGGER_I2S:
> + dev_dbg(cs40l50->dev, "Mailbox: TRIGGER_I2S\n");
> + break;
> + case CS40L50_MBOX_HAPTIC_COMPLETE_MBOX:
> + dev_dbg(cs40l50->dev, "Mailbox: COMPLETE_MBOX\n");
> + break;
> + case CS40L50_MBOX_HAPTIC_COMPLETE_GPIO:
> + dev_dbg(cs40l50->dev, "Mailbox: COMPLETE_GPIO\n");
> + break;
> + case CS40L50_MBOX_HAPTIC_COMPLETE_I2S:
> + dev_dbg(cs40l50->dev, "Mailbox: COMPLETE_I2S\n");
> + break;
> + case CS40L50_MBOX_F0_EST_START:
> + dev_dbg(cs40l50->dev, "Mailbox: F0_EST_START\n");
> + break;
> + case CS40L50_MBOX_F0_EST_DONE:
> + dev_dbg(cs40l50->dev, "Mailbox: F0_EST_DONE\n");
> + error = cs40l50_handle_f0_est_done(cs40l50);
> + if (error)
> + goto out_mutex;
> + break;
> + case CS40L50_MBOX_REDC_EST_START:
> + dev_dbg(cs40l50->dev, "Mailbox: REDC_EST_START\n");
> + break;
> + case CS40L50_MBOX_REDC_EST_DONE:
> + dev_dbg(cs40l50->dev, "Mailbox: REDC_EST_DONE\n");
> + error = cs40l50_handle_redc_est_done(cs40l50);
> + if (error)
> + goto out_mutex;
> + break;
> + case CS40L50_MBOX_LE_EST_START:
> + dev_dbg(cs40l50->dev, "Mailbox: LE_EST_START\n");
> + break;
> + case CS40L50_MBOX_LE_EST_DONE:
> + dev_dbg(cs40l50->dev, "Mailbox: LE_EST_DONE\n");
> + break;
> + case CS40L50_MBOX_AWAKE:
> + dev_dbg(cs40l50->dev, "Mailbox: AWAKE\n");
> + break;
> + case CS40L50_MBOX_INIT:
> + dev_dbg(cs40l50->dev, "Mailbox: INIT\n");
> + break;
> + case CS40L50_MBOX_ACK:
> + dev_dbg(cs40l50->dev, "Mailbox: ACK\n");
> + break;
> + case CS40L50_MBOX_ERR_EVENT_UNMAPPED:
> + dev_err(cs40l50->dev, "Unmapped event\n");
> + break;
> + case CS40L50_MBOX_ERR_EVENT_MODIFY:
> + dev_err(cs40l50->dev, "Failed to modify event index\n");
> + break;
> + case CS40L50_MBOX_ERR_NULLPTR:
> + dev_err(cs40l50->dev, "Null pointer\n");
> + break;
> + case CS40L50_MBOX_ERR_BRAKING:
> + dev_err(cs40l50->dev, "Braking not in progress\n");
> + break;
> + case CS40L50_MBOX_ERR_INVAL_SRC:
> + dev_err(cs40l50->dev, "Suspend/resume invalid source\n");
> + break;
> + case CS40L50_MBOX_ERR_ENABLE_RANGE:
> + dev_err(cs40l50->dev, "GPIO enable out of range\n");
> + break;
> + case CS40L50_MBOX_ERR_GPIO_UNMAPPED:
> + dev_err(cs40l50->dev, "GPIO not mapped\n");
> + break;
> + case CS40L50_MBOX_ERR_ISR_RANGE:
> + dev_err(cs40l50->dev, "GPIO ISR out of range\n");
> + break;
> + case CS40L50_MBOX_PERMANENT_SHORT:
> + dev_crit(cs40l50->dev, "Permanent short detected\n");
> + break;
> + case CS40L50_MBOX_RUNTIME_SHORT:
> + dev_err(cs40l50->dev, "Runtime short detected\n");
> + error = cs40l50_error_release(cs40l50);
> + if (error)
> + goto out_mutex;
> + break;
> + default:
> + dev_err(cs40l50->dev, "Payload %#X not recognized\n", val);
> + error = -EINVAL;
> + goto out_mutex;
> + }
> + }
> +
> + error = -EIO;
> +
> +out_mutex:
> + mutex_unlock(&cs40l50->lock);
> +
> + return IRQ_RETVAL(!error);
> +}
Should the last two drivers live in drivers/mailbox?
> +static irqreturn_t cs40l50_error(int irq, void *data);
Why is this being forward declared?
> +static const struct cs40l50_irq cs40l50_irqs[] = {
> + CS40L50_IRQ(AMP_SHORT, "Amp short", error),
I assume that last parameter is half of a function name.
Better to have 2 different structures and do 2 requests I feel.
> + CS40L50_IRQ(VIRT2_MBOX, "Mailbox", process_mbox),
> + CS40L50_IRQ(TEMP_ERR, "Overtemperature", error),
> + CS40L50_IRQ(BST_UVP, "Boost undervoltage", error),
> + CS40L50_IRQ(BST_SHORT, "Boost short", error),
> + CS40L50_IRQ(BST_ILIMIT, "Boost current limit", error),
> + CS40L50_IRQ(UVLO_VDDBATT, "Boost UVLO", error),
> + CS40L50_IRQ(GLOBAL_ERROR, "Global", error),
> +};
> +
> +static irqreturn_t cs40l50_error(int irq, void *data)
> +{
> + struct cs40l50_private *cs40l50 = data;
> +
> + dev_err(cs40l50->dev, "%s error\n", cs40l50_irqs[irq].name);
> + return IRQ_RETVAL(!cs40l50_error_release(cs40l50));
Please break the function out of the parentheses.
We don't tend to put functions in if()s either.
> +}
> +
> +static const struct regmap_irq cs40l50_reg_irqs[] = {
> + CS40L50_REG_IRQ(IRQ1_INT_1, AMP_SHORT),
I commented on these where you defined them - see below.
> + CS40L50_REG_IRQ(IRQ1_INT_2, VIRT2_MBOX),
> + CS40L50_REG_IRQ(IRQ1_INT_8, TEMP_ERR),
> + CS40L50_REG_IRQ(IRQ1_INT_9, BST_UVP),
> + CS40L50_REG_IRQ(IRQ1_INT_9, BST_SHORT),
> + CS40L50_REG_IRQ(IRQ1_INT_9, BST_ILIMIT),
> + CS40L50_REG_IRQ(IRQ1_INT_10, UVLO_VDDBATT),
> + CS40L50_REG_IRQ(IRQ1_INT_18, GLOBAL_ERROR),
> +};
> +
> +static struct regmap_irq_chip cs40l50_irq_chip = {
> + .name = "CS40L50 IRQ Controller",
> +
> + .status_base = CS40L50_IRQ1_INT_1,
> + .mask_base = CS40L50_IRQ1_MASK_1,
> + .ack_base = CS40L50_IRQ1_INT_1,
> + .num_regs = 22,
> +
> + .irqs = cs40l50_reg_irqs,
> + .num_irqs = ARRAY_SIZE(cs40l50_reg_irqs),
> +
> + .runtime_pm = true,
> +};
> +
> +static int cs40l50_irq_init(struct cs40l50_private *cs40l50)
> +{
> + struct device *dev = cs40l50->dev;
> + int error, i, irq;
> +
> + error = devm_regmap_add_irq_chip(dev, cs40l50->regmap, cs40l50->irq,
> + IRQF_ONESHOT | IRQF_SHARED, 0,
> + &cs40l50_irq_chip, &cs40l50->irq_data);
> + if (error)
> + return error;
> +
> + for (i = 0; i < ARRAY_SIZE(cs40l50_irqs); i++) {
> + irq = regmap_irq_get_virq(cs40l50->irq_data, cs40l50_irqs[i].irq);
> + if (irq < 0) {
> + dev_err(dev, "Failed getting %s\n", cs40l50_irqs[i].name);
> + return irq;
> + }
> +
> + error = devm_request_threaded_irq(dev, irq, NULL,
> + cs40l50_irqs[i].handler,
> + IRQF_ONESHOT | IRQF_SHARED,
> + cs40l50_irqs[i].name, cs40l50);
> + if (error) {
> + dev_err(dev, "Failed requesting %s\n", cs40l50_irqs[i].name);
> + return error;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int cs40l50_part_num_resolve(struct cs40l50_private *cs40l50)
> +{
> + struct device *dev = cs40l50->dev;
> + int error;
> +
> + error = regmap_read(cs40l50->regmap, CS40L50_DEVID, &cs40l50->devid);
> + if (error)
> + return error;
> +
> + if (cs40l50->devid != CS40L50_DEVID_A) {
> + dev_err(dev, "Invalid device ID: %#010X\n", cs40l50->devid);
> + return -EINVAL;
> + }
> +
> + error = regmap_read(cs40l50->regmap, CS40L50_REVID, &cs40l50->revid);
> + if (error)
> + return error;
> +
> + if (cs40l50->revid != CS40L50_REVID_B0) {
> + dev_err(dev, "Invalid revision: %#04X\n", cs40l50->revid);
> + return -EINVAL;
> + }
> +
> + dev_info(dev, "Cirrus Logic CS40L50 revision %02X\n", cs40l50->revid);
> +
> + return 0;
> +}
> +
> +int cs40l50_probe(struct cs40l50_private *cs40l50)
Previous Cirrus drivers have omitted the "_private" part, which I think
is better. "_ddata" is also common and acceptable.
> +{
> + struct device *dev = cs40l50->dev;
> + int error;
> +
> + mutex_init(&cs40l50->lock);
Don't you need to destroy this in the error path?
> + cs40l50->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> + if (IS_ERR(cs40l50->reset_gpio))
> + return dev_err_probe(dev, PTR_ERR(cs40l50->reset_gpio),
> + "Failed getting reset GPIO\n");
> +
> + error = devm_regulator_bulk_get(dev, ARRAY_SIZE(cs40l50_supplies),
> + cs40l50_supplies);
> + if (error)
> + goto err_reset;
> +
> + error = regulator_bulk_enable(ARRAY_SIZE(cs40l50_supplies),
> + cs40l50_supplies);
> + if (error)
> + goto err_reset;
> +
> + usleep_range(CS40L50_CP_READY_US, CS40L50_CP_READY_US + 100);
A comment for why this is required please.
And why 100us is appropriate.
> + gpiod_set_value_cansleep(cs40l50->reset_gpio, 1);
> +
> + usleep_range(CS40L50_CP_READY_US, CS40L50_CP_READY_US + 1000);
As above.
> + pm_runtime_set_autosuspend_delay(cs40l50->dev, CS40L50_AUTOSUSPEND_MS);
> + pm_runtime_use_autosuspend(cs40l50->dev);
> + pm_runtime_set_active(cs40l50->dev);
> + pm_runtime_get_noresume(cs40l50->dev);
> + devm_pm_runtime_enable(cs40l50->dev);
> +
> + error = cs40l50_part_num_resolve(cs40l50);
*_get_model()?
> + if (error)
> + goto err_supplies;
> +
> + error = cs40l50_irq_init(cs40l50);
> + if (error)
> + goto err_supplies;
> +
> + error = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE, cs40l50_devs,
> + ARRAY_SIZE(cs40l50_devs), NULL, 0, NULL);
> + if (error)
> + goto err_supplies;
> +
> + pm_runtime_mark_last_busy(cs40l50->dev);
> + pm_runtime_put_autosuspend(cs40l50->dev);
> +
> + return 0;
> +
> +err_supplies:
> + regulator_bulk_disable(ARRAY_SIZE(cs40l50_supplies), cs40l50_supplies);
> +err_reset:
> + gpiod_set_value_cansleep(cs40l50->reset_gpio, 0);
> +
> + return error;
> +}
> +EXPORT_SYMBOL_GPL(cs40l50_probe);
> +
> +int cs40l50_remove(struct cs40l50_private *cs40l50)
> +{
> + regulator_bulk_disable(ARRAY_SIZE(cs40l50_supplies), cs40l50_supplies);
> + gpiod_set_value_cansleep(cs40l50->reset_gpio, 1);
mutex_destroy()?
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(cs40l50_remove);
> +
> +static int cs40l50_runtime_suspend(struct device *dev)
> +{
> + struct cs40l50_private *cs40l50 = dev_get_drvdata(dev);
> +
> + return regmap_write(cs40l50->regmap, CS40L50_DSP_MBOX, CS40L50_ALLOW_HIBER);
> +}
> +
> +static int cs40l50_runtime_resume(struct device *dev)
> +{
> + struct cs40l50_private *cs40l50 = dev_get_drvdata(dev);
> + int error, i;
> + u32 val;
> +
> + /* Device NAKs when exiting hibernation, so optionally retry here. */
A comment - hoorah!
> + for (i = 0; i < CS40L50_DSP_TIMEOUT_COUNT; i++) {
> + error = regmap_write(cs40l50->regmap, CS40L50_DSP_MBOX,
> + CS40L50_PREVENT_HIBER);
> + if (!error)
> + break;
> +
> + usleep_range(CS40L50_DSP_POLL_US, CS40L50_DSP_POLL_US + 100);
> + }
> +
> + for (; i < CS40L50_DSP_TIMEOUT_COUNT; i++) {
So, the amount of time this section is given is entirely based on how
well the previous section did. Is that intentional?
Perhaps some comments to help straighten out your intentions.
> + error = regmap_read(cs40l50->regmap, CS40L50_DSP_MBOX, &val);
> + if (!error && val == 0)
> + return 0;
> +
> + usleep_range(CS40L50_DSP_POLL_US, CS40L50_DSP_POLL_US + 100);
> + }
> +
> + return error ? error : -ETIMEDOUT;
return error ?: -ETIMEDOUT;
> +}
> +
> +EXPORT_GPL_DEV_PM_OPS(cs40l50_pm_ops) = {
> + RUNTIME_PM_OPS(cs40l50_runtime_suspend, cs40l50_runtime_resume, NULL)
> +};
> +
> +MODULE_DESCRIPTION("CS40L50 Advanced Haptic Driver");
> +MODULE_AUTHOR("James Ogletree, Cirrus Logic Inc. <james.ogletree@cirrus.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/mfd/cs40l50-i2c.c b/drivers/mfd/cs40l50-i2c.c
> new file mode 100644
> index 000000000000..be1b130eb94b
> --- /dev/null
> +++ b/drivers/mfd/cs40l50-i2c.c
> @@ -0,0 +1,69 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * CS40L50 I2C Driver
This is not an I2C driver.
Best to describe the hardware rather that the "driver".
> + *
> + * Copyright 2023 Cirrus Logic, Inc.
> + *
Remove please.
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/mfd/cs40l50.h>
> +
> +static int cs40l50_i2c_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct device *dev = &client->dev;
> + struct cs40l50_private *cs40l50;
> +
> + cs40l50 = devm_kzalloc(dev, sizeof(*cs40l50), GFP_KERNEL);
> + if (!cs40l50)
> + return -ENOMEM;
> +
> + i2c_set_clientdata(client, cs40l50);
> +
> + cs40l50->regmap = devm_regmap_init_i2c(client, &cs40l50_regmap);
> + if (IS_ERR(cs40l50->regmap))
> + return dev_err_probe(cs40l50->dev, PTR_ERR(cs40l50->regmap),
> + "Failed to initialize register map\n");
> +
> + cs40l50->dev = dev;
> + cs40l50->irq = client->irq;
> +
> + return cs40l50_probe(cs40l50);
> +}
> +
> +static void cs40l50_i2c_remove(struct i2c_client *client)
> +{
> + struct cs40l50_private *cs40l50 = i2c_get_clientdata(client);
> +
> + cs40l50_remove(cs40l50);
> +}
> +
> +static const struct i2c_device_id cs40l50_id_i2c[] = {
> + {"cs40l50", 0},
This second parameter shouldn't be required now.
> + {}
> +};
> +MODULE_DEVICE_TABLE(i2c, cs40l50_id_i2c);
> +
> +static const struct of_device_id cs40l50_of_match[] = {
> + { .compatible = "cirrus,cs40l50" },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, cs40l50_of_match);
> +
> +static struct i2c_driver cs40l50_i2c_driver = {
> + .driver = {
> + .name = "cs40l50",
> + .of_match_table = cs40l50_of_match,
> + .pm = pm_ptr(&cs40l50_pm_ops),
> + },
> + .id_table = cs40l50_id_i2c,
> + .probe = cs40l50_i2c_probe,
> + .remove = cs40l50_i2c_remove,
> +};
> +
Remove this line.
> +module_i2c_driver(cs40l50_i2c_driver);
> +
> +MODULE_DESCRIPTION("CS40L50 I2C Driver");
> +MODULE_AUTHOR("James Ogletree, Cirrus Logic Inc. <james.ogletree@cirrus.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/mfd/cs40l50-spi.c b/drivers/mfd/cs40l50-spi.c
> new file mode 100644
> index 000000000000..8311d48efedf
> --- /dev/null
> +++ b/drivers/mfd/cs40l50-spi.c
> @@ -0,0 +1,68 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * CS40L50 SPI Driver
> + *
> + * Copyright 2023 Cirrus Logic, Inc.
> + *
Same comments as before.
> + */
> +
> +#include <linux/input/cs40l50.h>
> +#include <linux/mfd/spi.h>
> +
> +static int cs40l50_spi_probe(struct spi_device *spi)
> +{
> + struct cs40l50_private *cs40l50;
> + struct device *dev = &spi->dev;
> +
> + cs40l50 = devm_kzalloc(dev, sizeof(*cs40l50), GFP_KERNEL);
> + if (!cs40l50)
> + return -ENOMEM;
> +
> + spi_set_drvdata(spi, cs40l50);
> +
> + cs40l50->regmap = devm_regmap_init_spi(spi, &cs40l50_regmap);
> + if (IS_ERR(cs40l50->regmap))
> + return dev_err_probe(cs40l50->dev, PTR_ERR(cs40l50->regmap),
> + "Failed to initialize register map\n");
> +
> + cs40l50->dev = dev;
> + cs40l50->irq = spi->irq;
> +
> + return cs40l50_probe(cs40l50);
> +}
> +
> +static void cs40l50_spi_remove(struct spi_device *spi)
> +{
> + struct cs40l50_private *cs40l50 = spi_get_drvdata(spi);
> +
> + cs40l50_remove(cs40l50);
> +}
> +
> +static const struct spi_device_id cs40l50_id_spi[] = {
> + {"cs40l50", 0},
> + {}
> +};
> +MODULE_DEVICE_TABLE(spi, cs40l50_id_spi);
> +
> +static const struct of_device_id cs40l50_of_match[] = {
> + { .compatible = "cirrus,cs40l50" },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, cs40l50_of_match);
> +
> +static struct spi_driver cs40l50_spi_driver = {
> + .driver = {
> + .name = "cs40l50",
> + .of_match_table = cs40l50_of_match,
> + .pm = pm_ptr(&cs40l50_pm_ops),
> + },
> + .id_table = cs40l50_id_spi,
> + .probe = cs40l50_spi_probe,
> + .remove = cs40l50_spi_remove,
> +};
> +
> +module_spi_driver(cs40l50_spi_driver);
> +
> +MODULE_DESCRIPTION("CS40L50 SPI Driver");
> +MODULE_AUTHOR("James Ogletree, Cirrus Logic Inc. <james.ogletree@cirrus.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/mfd/cs40l50.h b/include/linux/mfd/cs40l50.h
> new file mode 100644
> index 000000000000..c625a999a5ae
> --- /dev/null
> +++ b/include/linux/mfd/cs40l50.h
> @@ -0,0 +1,198 @@
> +/* SPDX-License-Identifier: GPL-2.0
> + *
> + * CS40L50 Advanced Haptic Driver with waveform memory,
> + * integrated DSP, and closed-loop algorithms
> + *
> + * Copyright 2023 Cirrus Logic, Inc.
> + *
> + */
> +
> +#ifndef __CS40L50_H__
> +#define __CS40L50_H__
> +
> +#include <linux/firmware/cirrus/cs_dsp.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/input.h>
> +#include <linux/input/cirrus_haptics.h>
> +#include <linux/interrupt.h>
> +#include <linux/pm.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +
> +/* Power Supply Configuration */
> +#define CS40L50_BLOCK_ENABLES2 0x201C
> +#define CS40L50_ERR_RLS 0x2034
> +#define CS40L50_PWRMGT_CTL 0x2900
> +#define CS40L50_BST_LPMODE_SEL 0x3810
> +#define CS40L50_DCM_LOW_POWER 0x1
> +#define CS40L50_OVERTEMP_WARN 0x4000010
> +
> +/* Interrupts */
> +#define CS40L50_IRQ1_INT_1 0xE010
> +#define CS40L50_IRQ1_INT_2 0xE014
> +#define CS40L50_IRQ1_INT_8 0xE02C
> +#define CS40L50_IRQ1_INT_9 0xE030
> +#define CS40L50_IRQ1_INT_10 0xE034
> +#define CS40L50_IRQ1_INT_18 0xE054
> +#define CS40L50_IRQ1_MASK_1 0xE090
> +#define CS40L50_IRQ1_MASK_2 0xE094
> +#define CS40L50_IRQ1_MASK_20 0xE0DC
> +#define CS40L50_IRQ_MASK_2_OVERRIDE 0xFFDF7FFF
> +#define CS40L50_IRQ_MASK_20_OVERRIDE 0x15C01000
> +#define CS40L50_AMP_SHORT_MASK BIT(31)
> +#define CS40L50_VIRT2_MBOX_MASK BIT(21)
> +#define CS40L50_TEMP_ERR_MASK BIT(31)
> +#define CS40L50_BST_UVP_MASK BIT(6)
> +#define CS40L50_BST_SHORT_MASK BIT(7)
> +#define CS40L50_BST_ILIMIT_MASK BIT(8)
> +#define CS40L50_UVLO_VDDBATT_MASK BIT(16)
> +#define CS40L50_GLOBAL_ERROR_MASK BIT(15)
> +#define CS40L50_GLOBAL_ERR_RLS BIT(11)
> +#define CS40L50_IRQ(_irq, _name, _hand) \
> + { \
> + .irq = CS40L50_ ## _irq ## _IRQ,\
> + .name = _name, \
> + .handler = cs40l50_ ## _hand, \
> + }
> +#define CS40L50_REG_IRQ(_reg, _irq) \
Please don't reinvent the wheel:
REGMAP_IRQ_REG_LINE()
> + [CS40L50_ ## _irq ## _IRQ] = { \
> + .reg_offset = (CS40L50_ ## _reg) - CS40L50_IRQ1_INT_1, \
> + .mask = CS40L50_ ## _irq ## _MASK \
> + }
> +
> +/* Mailbox Inbound Commands */
> +#define CS40L50_RAM_BANK_INDEX_START 0x1000000
> +#define CS40L50_RTH_INDEX_START 0x1400000
> +#define CS40L50_RTH_INDEX_END 0x1400001
> +#define CS40L50_ROM_BANK_INDEX_START 0x1800000
> +#define CS40L50_ROM_BANK_INDEX_END 0x180001A
> +#define CS40L50_PREVENT_HIBER 0x2000003
> +#define CS40L50_ALLOW_HIBER 0x2000004
> +#define CS40L50_OWT_PUSH 0x3000008
> +#define CS40L50_STOP_PLAYBACK 0x5000000
> +#define CS40L50_OWT_DELETE 0xD000000
> +
> +/* Mailbox Outbound Commands */
> +#define CS40L50_MBOX_QUEUE_BASE 0x11004
> +#define CS40L50_MBOX_QUEUE_END 0x1101C
> +#define CS40L50_DSP_MBOX 0x11020
> +#define CS40L50_MBOX_QUEUE_WT 0x28042C8
> +#define CS40L50_MBOX_QUEUE_RD 0x28042CC
> +#define CS40L50_MBOX_HAPTIC_COMPLETE_MBOX 0x1000000
> +#define CS40L50_MBOX_HAPTIC_COMPLETE_GPIO 0x1000001
> +#define CS40L50_MBOX_HAPTIC_COMPLETE_I2S 0x1000002
> +#define CS40L50_MBOX_HAPTIC_TRIGGER_MBOX 0x1000010
> +#define CS40L50_MBOX_HAPTIC_TRIGGER_GPIO 0x1000011
> +#define CS40L50_MBOX_HAPTIC_TRIGGER_I2S 0x1000012
> +#define CS40L50_MBOX_INIT 0x2000000
> +#define CS40L50_MBOX_AWAKE 0x2000002
> +#define CS40L50_MBOX_F0_EST_START 0x7000011
> +#define CS40L50_MBOX_F0_EST_DONE 0x7000021
> +#define CS40L50_MBOX_REDC_EST_START 0x7000012
> +#define CS40L50_MBOX_REDC_EST_DONE 0x7000022
> +#define CS40L50_MBOX_LE_EST_START 0x7000014
> +#define CS40L50_MBOX_LE_EST_DONE 0x7000024
> +#define CS40L50_MBOX_ACK 0xA000000
> +#define CS40L50_MBOX_PANIC 0xC000000
> +#define CS40L50_MBOX_WATERMARK 0xD000000
> +#define CS40L50_MBOX_ERR_EVENT_UNMAPPED 0xC0004B3
> +#define CS40L50_MBOX_ERR_EVENT_MODIFY 0xC0004B4
> +#define CS40L50_MBOX_ERR_NULLPTR 0xC0006A5
> +#define CS40L50_MBOX_ERR_BRAKING 0xC0006A8
> +#define CS40L50_MBOX_ERR_INVAL_SRC 0xC0006AC
> +#define CS40L50_MBOX_ERR_ENABLE_RANGE 0xC000836
> +#define CS40L50_MBOX_ERR_GPIO_UNMAPPED 0xC000837
> +#define CS40L50_MBOX_ERR_ISR_RANGE 0xC000838
> +#define CS40L50_MBOX_PERMANENT_SHORT 0xC000C1C
> +#define CS40L50_MBOX_RUNTIME_SHORT 0xC000C1D
> +
> +/* DSP */
> +#define CS40L50_DSP1_XMEM_PACKED_0 0x2000000
> +#define CS40L50_DSP1_XMEM_UNPACKED32_0 0x2400000
> +#define CS40L50_SYS_INFO_ID 0x25E0000
> +#define CS40L50_DSP1_XMEM_UNPACKED24_0 0x2800000
> +#define CS40L50_RAM_INIT 0x28021DC
> +#define CS40L50_POWER_ON_SEQ 0x2804320
> +#define CS40L50_OWT_BASE 0x2805C34
> +#define CS40L50_NUM_OF_WAVES 0x280CB4C
> +#define CS40L50_CORE_BASE 0x2B80000
> +#define CS40L50_CCM_CORE_CONTROL 0x2BC1000
> +#define CS40L50_DSP1_YMEM_PACKED_0 0x2C00000
> +#define CS40L50_DSP1_YMEM_UNPACKED32_0 0x3000000
> +#define CS40L50_DSP1_YMEM_UNPACKED24_0 0x3400000
> +#define CS40L50_DSP1_PMEM_0 0x3800000
> +#define CS40L50_DSP1_PMEM_5114 0x3804FE8
> +#define CS40L50_MEM_RDY_HW 0x2
> +#define CS40L50_RAM_INIT_FLAG 0x1
> +#define CS40L50_CLOCK_DISABLE 0x80
> +#define CS40L50_CLOCK_ENABLE 0x281
> +#define CS40L50_DSP_POLL_US 1000
> +#define CS40L50_DSP_TIMEOUT_COUNT 100
> +#define CS40L50_CP_READY_US 2200
> +#define CS40L50_PSEQ_SIZE 200
> +#define CS40L50_AUTOSUSPEND_MS 2000
> +
> +/* Firmware */
> +#define CS40L50_FW "cs40l50.wmfw"
> +#define CS40L50_WT "cs40l50.bin"
> +
> +/* Calibration */
> +#define CS40L50_REDC_ESTIMATION 0x2802F7C
> +#define CS40L50_F0_ESTIMATION 0x2802F84
> +#define CS40L50_F0_STORED 0x2805C00
> +#define CS40L50_REDC_STORED 0x2805C04
> +#define CS40L50_RE_EST_STATUS 0x3401B40
> +
> +/* Open wavetable */
> +#define CS40L50_OWT_SIZE 0x2805C38
> +#define CS40L50_OWT_NEXT 0x2805C3C
> +#define CS40L50_NUM_OF_OWT_WAVES 0x2805C40
> +
> +/* GPIO */
> +#define CS40L50_GPIO_BASE 0x2804140
> +
> +/* Device */
> +#define CS40L50_DEVID 0x0
> +#define CS40L50_REVID 0x4
> +#define CS40L50_DEVID_A 0x40A50
> +#define CS40L50_REVID_B0 0xB0
> +
> +enum cs40l50_irq_list {
> + CS40L50_GLOBAL_ERROR_IRQ,
> + CS40L50_UVLO_VDDBATT_IRQ,
> + CS40L50_BST_ILIMIT_IRQ,
> + CS40L50_BST_SHORT_IRQ,
> + CS40L50_BST_UVP_IRQ,
> + CS40L50_TEMP_ERR_IRQ,
> + CS40L50_VIRT2_MBOX_IRQ,
> + CS40L50_AMP_SHORT_IRQ,
> +};
> +
> +struct cs40l50_irq {
> + const char *name;
> + int irq;
> + irqreturn_t (*handler)(int irq, void *data);
> +};
> +
> +struct cs40l50_private {
> + struct device *dev;
> + struct regmap *regmap;
> + struct cs_dsp dsp;
> + struct mutex lock;
> + struct gpio_desc *reset_gpio;
> + struct regmap_irq_chip_data *irq_data;
> + struct input_dev *input;
Where is this used?
> + const struct firmware *wmfw;
Or this.
> + struct cs_hap haptics;
Or this?
> + u32 devid;
> + u32 revid;
Are these used after they're set?
> + int irq;
> +};
> +
> +int cs40l50_probe(struct cs40l50_private *cs40l50);
> +int cs40l50_remove(struct cs40l50_private *cs40l50);
> +
> +extern const struct regmap_config cs40l50_regmap;
> +extern const struct dev_pm_ops cs40l50_pm_ops;
> +
> +#endif /* __CS40L50_H__ */
> --
> 2.25.1
>
--
0)
Lee Jones [李琼斯]
^ permalink raw reply
* Re: [v0001,1/1] input/ts: ili210x: send abs-mt-pressure for untouched
From: gerz-1 @ 2023-10-19 13:18 UTC (permalink / raw)
To: linux-kernel; +Cc: markus.burri, rydberg, linux-input
In-Reply-To: <20231017131057.1034956-1-markus.burri@mt.com>
Tested-by: Burak Gerz burak.gerz@mt.com
^ permalink raw reply
* Re: [PATCH v3 1/3] pwm: make it possible to apply pwm changes in atomic context
From: Uwe Kleine-König @ 2023-10-19 10:51 UTC (permalink / raw)
To: Hans de Goede
Cc: Sean Young, linux-media, linux-pwm, Ivaylo Dimitrov,
Thierry Reding, Jonathan Corbet, Jani Nikula, Joonas Lahtinen,
Rodrigo Vivi, Tvrtko Ursulin, David Airlie, Daniel Vetter,
Javier Martinez Canillas, Jean Delvare, Guenter Roeck,
Support Opensource, Dmitry Torokhov, Pavel Machek, Lee Jones,
Mauro Carvalho Chehab, Ilpo Järvinen, Mark Gross,
Liam Girdwood, Mark Brown, Daniel Thompson, Jingoo Han,
Helge Deller, linux-doc, linux-kernel, intel-gfx, dri-devel,
linux-hwmon, linux-input, linux-leds, platform-driver-x86,
linux-arm-kernel, linux-fbdev
In-Reply-To: <90728c06-4c6c-b3d2-4723-c24711be2fa5@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 2548 bytes --]
On Wed, Oct 18, 2023 at 03:57:48PM +0200, Hans de Goede wrote:
> Hi Sean,
>
> On 10/17/23 11:17, Sean Young wrote:
> > Some drivers require sleeping, for example if the pwm device is connected
> > over i2c. The pwm-ir-tx requires precise timing, and sleeping causes havoc
> > with the generated IR signal when sleeping occurs.
> >
> > This patch makes it possible to use pwm when the driver does not sleep,
> > by introducing the pwm_can_sleep() function.
> >
> > Signed-off-by: Sean Young <sean@mess.org>
>
> I have no objection to this patch by itself, but it seems a bit
> of unnecessary churn to change all current callers of pwm_apply_state()
> to a new API.
The idea is to improve the semantic of the function name, see
https://lore.kernel.org/linux-pwm/20231013180449.mcdmklbsz2rlymzz@pengutronix.de
for more context. I think it's very subjective if you consider this
churn or not. While it's nice to have every caller converted in a single
step, I'd go for
#define pwm_apply_state(pwm, state) pwm_apply_cansleep(pwm, state)
, keep that macro for a while and convert all users step by step. This
way we don't needlessly break oot code and the changes to convert to the
new API can go via their usual trees without time pressure.
> Why not just keep pwm_apply_state() as is and introduce a new
> pwm_apply_state_atomic() for callers which want to apply state
> in a case where sleeping is not allowed ?
There is a big spontaneous growth of function name patterns. I didn't
claim having done a complete research, but not using a suffix for the
fast variant and _cansleep for the sleeping one at least aligns to how
the gpio subsystem names things.
Grepping a bit more:
- regmap has: regmap_might_sleep() and struct regmap::can_sleep
The actual API doesn't have different functions to differentiate
sleeping and non-sleeping calls. (Though there is
regmap_read_poll_timeout_atomic().)
- kmap() + kmap_atomic()
- set_pte() + set_pte_atomic()
- There is scmi_is_transport_atomic() and scmi_handle::is_transport_atomic()
(Is this also about sleeping?)
- There are quite a lot more symbols ending in _atomic and in
_cansleep, but several of them don't exists to differentiate a slow
and a fast procedure. (e.g. drm_mode_atomic)
Not entirely sure what to read out of that.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v4 13/17] platform/x86/amd/pmf: Add PMF-AMDGPU get interface
From: Christian König @ 2023-10-19 9:01 UTC (permalink / raw)
To: Shyam Sundar S K, Mario Limonciello, Ilpo Järvinen,
Daniel Vetter, Dave Airlie
Cc: Hans de Goede, markgross, basavaraj.natikar, jikos,
benjamin.tissoires, alexander.deucher, Xinhui.Pan, airlied,
daniel, Patil.Reddy, platform-driver-x86, linux-input, amd-gfx,
dri-devel
In-Reply-To: <80fcdcb2-1a22-4002-9bfd-d6cf15d5a57b@amd.com>
Am 18.10.23 um 19:05 schrieb Shyam Sundar S K:
>
> On 10/18/2023 9:37 PM, Christian König wrote:
>> Am 18.10.23 um 17:47 schrieb Mario Limonciello:
>>> On 10/18/2023 08:40, Christian König wrote:
>>>>
>>>> Am 18.10.23 um 11:28 schrieb Shyam Sundar S K:
>>>>> On 10/18/2023 2:50 PM, Ilpo Järvinen wrote:
>>>>>> On Wed, 18 Oct 2023, Shyam Sundar S K wrote:
>>>>>>
>>>>>>> In order to provide GPU inputs to TA for the Smart PC solution
>>>>>>> to work, we
>>>>>>> need to have interface between the PMF driver and the AMDGPU
>>>>>>> driver.
>>>>>>>
>>>>>>> Add the initial code path for get interface from AMDGPU.
>>>>>>>
>>>>>>> Co-developed-by: Mario Limonciello <mario.limonciello@amd.com>
>>>>>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>>>>>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>>>>>>> ---
>>>>>>> drivers/gpu/drm/amd/amdgpu/Makefile | 2 +
>>>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 +
>>>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c | 138
>>>>>>> ++++++++++++++++++++++++
>>>>>>> drivers/platform/x86/amd/pmf/Kconfig | 1 +
>>>>>>> drivers/platform/x86/amd/pmf/core.c | 1 +
>>>>>>> drivers/platform/x86/amd/pmf/pmf.h | 3 +
>>>>>>> drivers/platform/x86/amd/pmf/spc.c | 13 +++
>>>>>>> drivers/platform/x86/amd/pmf/tee-if.c | 26 +++++
>>>>>>> include/linux/amd-pmf-io.h | 35 ++++++
>>>>>>> 9 files changed, 220 insertions(+)
>>>>>>> create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c
>>>>>>> create mode 100644 include/linux/amd-pmf-io.h
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile
>>>>>>> b/drivers/gpu/drm/amd/amdgpu/Makefile
>>>>>>> index 384b798a9bad..7fafccefbd7a 100644
>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/Makefile
>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/Makefile
>>>>>>> @@ -86,6 +86,8 @@ amdgpu-$(CONFIG_PROC_FS) += amdgpu_fdinfo.o
>>>>>>> amdgpu-$(CONFIG_PERF_EVENTS) += amdgpu_pmu.o
>>>>>>> +amdgpu-$(CONFIG_AMD_PMF) += amdgpu_pmf.o
>>>>>>> +
>>>>>>> # add asic specific block
>>>>>>> amdgpu-$(CONFIG_DRM_AMDGPU_CIK)+= cik.o cik_ih.o \
>>>>>>> dce_v8_0.o gfx_v7_0.o cik_sdma.o uvd_v4_2.o vce_v2_0.o
>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>>>>> index a79d53bdbe13..26ffa1b4fe57 100644
>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>>>>> @@ -50,6 +50,7 @@
>>>>>>> #include <linux/hashtable.h>
>>>>>>> #include <linux/dma-fence.h>
>>>>>>> #include <linux/pci.h>
>>>>>>> +#include <linux/amd-pmf-io.h>
>>>>>>> #include <drm/ttm/ttm_bo.h>
>>>>>>> #include <drm/ttm/ttm_placement.h>
>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c
>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c
>>>>>>> new file mode 100644
>>>>>>> index 000000000000..ac981848df50
>>>>>>> --- /dev/null
>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c
>>>>>>> @@ -0,0 +1,138 @@
>>>>>>> +/*
>>>>>>> + * Copyright 2023 Advanced Micro Devices, Inc.
>>>>>>> + *
>>>>>>> + * Permission is hereby granted, free of charge, to any person
>>>>>>> obtaining a
>>>>>>> + * copy of this software and associated documentation files
>>>>>>> (the "Software"),
>>>>>>> + * to deal in the Software without restriction, including
>>>>>>> without limitation
>>>>>>> + * the rights to use, copy, modify, merge, publish, distribute,
>>>>>>> sublicense,
>>>>>>> + * and/or sell copies of the Software, and to permit persons to
>>>>>>> whom the
>>>>>>> + * Software is furnished to do so, subject to the following
>>>>>>> conditions:
>>>>>>> + *
>>>>>>> + * The above copyright notice and this permission notice shall
>>>>>>> be included in
>>>>>>> + * all copies or substantial portions of the Software.
>>>>>>> + *
>>>>>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY
>>>>>>> KIND, EXPRESS OR
>>>>>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>>>>>>> MERCHANTABILITY,
>>>>>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO
>>>>>>> EVENT SHALL
>>>>>>> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY
>>>>>>> CLAIM, DAMAGES OR
>>>>>>> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
>>>>>>> OTHERWISE,
>>>>>>> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR
>>>>>>> THE USE OR
>>>>>>> + * OTHER DEALINGS IN THE SOFTWARE.
>>>>>> This is MIT, right? Add the required SPDX-License-Identifier line
>>>>>> for it
>>>>>> at the top of the file, thank you.
>>>>>>
>>>>> all the files in drivers/gpu/drm/amd/amdgpu/* carry the same license
>>>>> text. So, have retained it to maintain uniformity.
>>>> Please add the SPDX License Identifier for any file you add.
> OK. I thought to keep it same like the other files. I will change this
> to SPDX in the next revision.
>
>>>> Apart from that the whole approach of attaching this directly to
>>>> amdgpu looks extremely questionable to me.
>>>>
>>> What's the long term outlook for things that are needed directly
>>> from amdgpu? Is there going to be more besides the backlight and
>>> the display count/type?
>> Yeah, that goes into the same direction as my concern.
> PMF is an APU only feature and will need inputs from multiple
> subsystems (in this case its GPU) to send changing system information
> to PMF TA (Trusted Application, running on ASP/PSP) as input.
>
> Its not only about the display count/type and backlight, there are
> many others things in pipe like the GPU engine running time,
> utilization percentage etc, that guide the TA to make better decisions.
>
> This series is only targeted to build the initial plubming with the
> subsystems inside the kernel and later keep adding changes to get more
> information from other subsystems.
Yeah, and that's what I strongly disagree on. Don't come up with such an
approach without consulting with upstream maintainers first.
What you propose here is a system wide framework for improving power
management, that's fine. The problem is that we already have something
very similar in the thermal framework.
See for example the Intel solution here
https://docs.kernel.org/driver-api/thermal/intel_dptf.html
From the general design the job of the amdgpu driver is to drive the
display hardware. So this should in general be completely decoupled from
this driver. As Mario suggested as well you can iterate over the
connected displays independently. Same thing for the backlight.
When it comes to hardware state like GPU engine utilization then we
don't have that inside amdgpu either.
Regards,
Christian.
>
> that is the reason you see that, this patch proposes amd-pmf-io.h as
> the interface to talk to other subsystems. For the initial path, I
> have opted to get information from SFH and GPU drivers. But this is
> meant to grow in future.
>
>>> At least for the display count I suppose one way that it could be
>>> "decoupled" from amdgpu is to use drm_for_each_connector_iter to
>>> iterate all the connectors and then count the connected ones.
>> And what if the number of connected displays change? How is amdgpu
>> supposed to signal events like that?
> PMF driver polls for the information based on a configurable sampling
> frequency.
>
> you can look at amd_pmf_get_gpu_info(), that gets called from
> amd_pmf_populate_ta_inputs() in spc.c in this proposed series.
>
> Thanks,
> Shyam
>
>> This whole solution doesn't looks well thought through.
>>
>> Regards,
>> Christian.
>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>>>> + *
>>>>>>> + * Author: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>>>>>>> + *
>>>>>>> + */
>>>>>> Remove the extra empty line at the end of the comment.
>>>>>>
>>>>> I just took the standard template for all the gpu files. Is that
>>>>> OK to
>>>>> retain the blank line?
>>>>>
>>>>> If not, I can remove it in the next version.
>>>>>
>>>>> Rest all remarks I will address.
>>>>>
>>>>> Thanks,
>>>>> Shyam
>>>>>
>>>>>>> +
>>>>>>> +#include <linux/backlight.h>
>>>>>>> +#include "amdgpu.h"
>>>>>>> +
>>>>>>> +int amd_pmf_get_gfx_data(struct amd_gpu_pmf_data *pmf)
>>>>>>> +{
>>>>>>> + struct drm_device *drm_dev = pci_get_drvdata(pmf->gpu_dev);
>>>>>>> + struct drm_mode_config *mode_config = &drm_dev->mode_config;
>>>>>>> + struct amdgpu_device *adev = drm_to_adev(drm_dev);
>>>>>>> + struct drm_connector_list_iter iter;
>>>>>>> + struct drm_connector *connector;
>>>>>>> + int i = 0;
>>>>>>> +
>>>>>>> + /* Reset the count to zero */
>>>>>>> + pmf->display_count = 0;
>>>>>>> + if (!(adev->flags & AMD_IS_APU)) {
>>>>>>> + DRM_ERROR("PMF-AMDGPU interface not supported\n");
>>>>>>> + return -ENODEV;
>>>>>>> + }
>>>>>>> +
>>>>>>> + mutex_lock(&mode_config->mutex);
>>>>>>> + drm_connector_list_iter_begin(drm_dev, &iter);
>>>>>>> + drm_for_each_connector_iter(connector, &iter) {
>>>>>>> + if (connector->status == connector_status_connected)
>>>>>>> + pmf->display_count++;
>>>>>>> + if (connector->status != pmf->con_status[i])
>>>>>>> + pmf->con_status[i] = connector->status;
>>>>>>> + if (connector->connector_type != pmf->connector_type[i])
>>>>>>> + pmf->connector_type[i] = connector->connector_type;
>>>>>>> +
>>>>>>> + i++;
>>>>>>> + if (i >= MAX_SUPPORTED)
>>>>>>> + break;
>>>>>>> + }
>>>>>>> + drm_connector_list_iter_end(&iter);
>>>>>>> + mutex_unlock(&mode_config->mutex);
>>>>>>> +
>>>>>>> + return 0;
>>>>>>> +}
>>>>>>> +EXPORT_SYMBOL_GPL(amd_pmf_get_gfx_data);
>>>>>>> +
>>>>>>> +static int amd_pmf_gpu_get_cur_state(struct
>>>>>>> thermal_cooling_device *cooling_dev,
>>>>>>> + unsigned long *state)
>>>>>>> +{
>>>>>>> + struct backlight_device *bd;
>>>>>>> +
>>>>>>> + if (!acpi_video_backlight_use_native())
>>>>>>> + return -ENODEV;
>>>>>>> +
>>>>>>> + bd = backlight_device_get_by_type(BACKLIGHT_RAW);
>>>>>>> + if (!bd)
>>>>>>> + return -ENODEV;
>>>>>>> +
>>>>>>> + *state = backlight_get_brightness(bd);
>>>>>>> +
>>>>>>> + return 0;
>>>>>>> +}
>>>>>>> +
>>>>>>> +static int amd_pmf_gpu_get_max_state(struct
>>>>>>> thermal_cooling_device *cooling_dev,
>>>>>>> + unsigned long *state)
>>>>>>> +{
>>>>>>> + struct backlight_device *bd;
>>>>>>> +
>>>>>>> + if (!acpi_video_backlight_use_native())
>>>>>>> + return -ENODEV;
>>>>>>> +
>>>>>>> + bd = backlight_device_get_by_type(BACKLIGHT_RAW);
>>>>>>> + if (!bd)
>>>>>>> + return -ENODEV;
>>>>>>> +
>>>>>>> + if (backlight_is_blank(bd))
>>>>>>> + *state = 0;
>>>>>>> + else
>>>>>>> + *state = bd->props.max_brightness;
>>>>>>> +
>>>>>>> + return 0;
>>>>>>> +}
>>>>>>> +
>>>>>>> +static const struct thermal_cooling_device_ops bd_cooling_ops = {
>>>>>>> + .get_max_state = amd_pmf_gpu_get_max_state,
>>>>>>> + .get_cur_state = amd_pmf_gpu_get_cur_state,
>>>>>>> +};
>>>>>>> +
>>>>>>> +int amd_pmf_gpu_init(struct amd_gpu_pmf_data *pmf)
>>>>>>> +{
>>>>>>> + struct drm_device *drm_dev = pci_get_drvdata(pmf->gpu_dev);
>>>>>>> + struct amdgpu_device *adev = drm_to_adev(drm_dev);
>>>>>>> +
>>>>>>> + if (!(adev->flags & AMD_IS_APU)) {
>>>>>>> + DRM_ERROR("PMF-AMDGPU interface not supported\n");
>>>>>>> + return -ENODEV;
>>>>>>> + }
>>>>>>> +
>>>>>>> + if (!acpi_video_backlight_use_native())
>>>>>>> + return -ENODEV;
>>>>>>> +
>>>>>>> + pmf->raw_bd = backlight_device_get_by_type(BACKLIGHT_RAW);
>>>>>>> + if (!pmf->raw_bd)
>>>>>>> + return -ENODEV;
>>>>>>> +
>>>>>>> + pmf->cooling_dev =
>>>>>>> thermal_cooling_device_register("pmf_gpu_bd",
>>>>>>> + pmf, &bd_cooling_ops);
>>>>>>> + if (IS_ERR(pmf->cooling_dev))
>>>>>>> + return -ENODEV;
>>>>>>> +
>>>>>>> + return 0;
>>>>>>> +}
>>>>>>> +EXPORT_SYMBOL_GPL(amd_pmf_gpu_init);
>>>>>>> +
>>>>>>> +void amd_pmf_gpu_deinit(struct amd_gpu_pmf_data *pmf)
>>>>>>> +{
>>>>>>> + thermal_cooling_device_unregister(pmf->cooling_dev);
>>>>>>> +}
>>>>>>> +EXPORT_SYMBOL_GPL(amd_pmf_gpu_deinit);
>>>>>>> diff --git a/drivers/platform/x86/amd/pmf/Kconfig
>>>>>>> b/drivers/platform/x86/amd/pmf/Kconfig
>>>>>>> index f246252bddd8..7f430de7af44 100644
>>>>>>> --- a/drivers/platform/x86/amd/pmf/Kconfig
>>>>>>> +++ b/drivers/platform/x86/amd/pmf/Kconfig
>>>>>>> @@ -10,6 +10,7 @@ config AMD_PMF
>>>>>>> depends on AMD_NB
>>>>>>> select ACPI_PLATFORM_PROFILE
>>>>>>> depends on TEE && AMDTEE
>>>>>>> + depends on DRM_AMDGPU
>>>>>>> help
>>>>>>> This driver provides support for the AMD Platform
>>>>>>> Management Framework.
>>>>>>> The goal is to enhance end user experience by making AMD
>>>>>>> PCs smarter,
>>>>>>> diff --git a/drivers/platform/x86/amd/pmf/core.c
>>>>>>> b/drivers/platform/x86/amd/pmf/core.c
>>>>>>> index 4b8156033fa6..c59ba527ff49 100644
>>>>>>> --- a/drivers/platform/x86/amd/pmf/core.c
>>>>>>> +++ b/drivers/platform/x86/amd/pmf/core.c
>>>>>>> @@ -411,6 +411,7 @@ static int amd_pmf_probe(struct
>>>>>>> platform_device *pdev)
>>>>>>> }
>>>>>>> dev->cpu_id = rdev->device;
>>>>>>> + dev->root = rdev;
>>>>>>> err = amd_smn_read(0, AMD_PMF_BASE_ADDR_LO, &val);
>>>>>>> if (err) {
>>>>>>> diff --git a/drivers/platform/x86/amd/pmf/pmf.h
>>>>>>> b/drivers/platform/x86/amd/pmf/pmf.h
>>>>>>> index 8712299ad52b..615cd3a31986 100644
>>>>>>> --- a/drivers/platform/x86/amd/pmf/pmf.h
>>>>>>> +++ b/drivers/platform/x86/amd/pmf/pmf.h
>>>>>>> @@ -13,6 +13,7 @@
>>>>>>> #include <linux/acpi.h>
>>>>>>> #include <linux/platform_profile.h>
>>>>>>> +#include <linux/amd-pmf-io.h>
>>>>>>> #define POLICY_BUF_MAX_SZ 0x4b000
>>>>>>> #define POLICY_SIGN_COOKIE 0x31535024
>>>>>>> @@ -228,9 +229,11 @@ struct amd_pmf_dev {
>>>>>>> void *shbuf;
>>>>>>> struct delayed_work pb_work;
>>>>>>> struct pmf_action_table *prev_data;
>>>>>>> + struct amd_gpu_pmf_data gfx_data;
>>>>>>> u64 policy_addr;
>>>>>>> void *policy_base;
>>>>>>> bool smart_pc_enabled;
>>>>>>> + struct pci_dev *root;
>>>>>>> };
>>>>>>> struct apmf_sps_prop_granular {
>>>>>>> diff --git a/drivers/platform/x86/amd/pmf/spc.c
>>>>>>> b/drivers/platform/x86/amd/pmf/spc.c
>>>>>>> index 512e0c66efdc..cf4962ef97c2 100644
>>>>>>> --- a/drivers/platform/x86/amd/pmf/spc.c
>>>>>>> +++ b/drivers/platform/x86/amd/pmf/spc.c
>>>>>>> @@ -44,6 +44,10 @@ void amd_pmf_dump_ta_inputs(struct
>>>>>>> amd_pmf_dev *dev, struct ta_pmf_enact_table *
>>>>>>> dev_dbg(dev->dev, "Max C0 Residency : %u\n",
>>>>>>> in->ev_info.max_c0residency);
>>>>>>> dev_dbg(dev->dev, "GFX Busy : %u\n", in->ev_info.gfx_busy);
>>>>>>> dev_dbg(dev->dev, "Connected Display Count : %u\n",
>>>>>>> in->ev_info.monitor_count);
>>>>>>> + dev_dbg(dev->dev, "Primary Display Type : %s\n",
>>>>>>> + drm_get_connector_type_name(in->ev_info.display_type));
>>>>>>> + dev_dbg(dev->dev, "Primary Display State : %s\n",
>>>>>>> in->ev_info.display_state ?
>>>>>>> + "Connected" : "disconnected/unknown");
>>>>>>> dev_dbg(dev->dev, "LID State : %s\n",
>>>>>>> in->ev_info.lid_state ? "Close" : "Open");
>>>>>>> dev_dbg(dev->dev, "==== TA inputs END ====\n");
>>>>>>> }
>>>>>>> @@ -146,6 +150,14 @@ static int amd_pmf_get_slider_info(struct
>>>>>>> amd_pmf_dev *dev, struct ta_pmf_enact_
>>>>>>> return 0;
>>>>>>> }
>>>>>>> +static void amd_pmf_get_gpu_info(struct amd_pmf_dev *dev,
>>>>>>> struct ta_pmf_enact_table *in)
>>>>>>> +{
>>>>>>> + amd_pmf_get_gfx_data(&dev->gfx_data);
>>>>>>> + in->ev_info.monitor_count = dev->gfx_data.display_count;
>>>>>>> + in->ev_info.display_type = dev->gfx_data.connector_type[0];
>>>>>>> + in->ev_info.display_state = dev->gfx_data.con_status[0];
>>>>>>> +}
>>>>>>> +
>>>>>>> void amd_pmf_populate_ta_inputs(struct amd_pmf_dev *dev,
>>>>>>> struct ta_pmf_enact_table *in)
>>>>>>> {
>>>>>>> /* TA side lid open is 1 and close is 0, hence the ! here */
>>>>>>> @@ -154,4 +166,5 @@ void amd_pmf_populate_ta_inputs(struct
>>>>>>> amd_pmf_dev *dev, struct ta_pmf_enact_tab
>>>>>>> amd_pmf_get_smu_info(dev, in);
>>>>>>> amd_pmf_get_battery_info(dev, in);
>>>>>>> amd_pmf_get_slider_info(dev, in);
>>>>>>> + amd_pmf_get_gpu_info(dev, in);
>>>>>>> }
>>>>>>> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c
>>>>>>> b/drivers/platform/x86/amd/pmf/tee-if.c
>>>>>>> index 2f5fb8623c20..956e66b78605 100644
>>>>>>> --- a/drivers/platform/x86/amd/pmf/tee-if.c
>>>>>>> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
>>>>>>> @@ -9,6 +9,7 @@
>>>>>>> */
>>>>>>> #include <linux/debugfs.h>
>>>>>>> +#include <linux/pci.h>
>>>>>>> #include <linux/tee_drv.h>
>>>>>>> #include <linux/uuid.h>
>>>>>>> #include "pmf.h"
>>>>>>> @@ -357,6 +358,19 @@ static int amd_pmf_get_bios_buffer(struct
>>>>>>> amd_pmf_dev *dev)
>>>>>>> return amd_pmf_start_policy_engine(dev);
>>>>>>> }
>>>>>>> +static int amd_pmf_get_gpu_handle(struct pci_dev *pdev, void
>>>>>>> *data)
>>>>>>> +{
>>>>>>> + struct amd_pmf_dev *dev = data;
>>>>>>> +
>>>>>>> + if (pdev->vendor == PCI_VENDOR_ID_ATI && pdev->devfn == 0) {
>>>>>>> + /* Found the amdgpu handle from the pci root after
>>>>>>> walking through the pci bus */
>>>>>> If you insist on having this comment, make it a function comment
>>>>>> instead
>>>>>> (with appropriate modifications into the content of it) but I
>>>>>> personally
>>>>>> don't find it that useful so it could be just dropped as well, IMO.
>>>>>>
>>>>>>> + dev->gfx_data.gpu_dev = pdev;
>>>>>>> + return 1; /* Stop walking */
>>>>>>> + }
>>>>>>> +
>>>>>>> + return 0; /* Continue walking */
>>>>>>> +}
>>>>>>> +
>>>>>>> static int amd_pmf_amdtee_ta_match(struct
>>>>>>> tee_ioctl_version_data *ver, const void *data)
>>>>>>> {
>>>>>>> return ver->impl_id == TEE_IMPL_ID_AMDTEE;
>>>>>>> @@ -446,6 +460,15 @@ int amd_pmf_init_smart_pc(struct
>>>>>>> amd_pmf_dev *dev)
>>>>>>> INIT_DELAYED_WORK(&dev->pb_work, amd_pmf_invoke_cmd);
>>>>>>> amd_pmf_set_dram_addr(dev);
>>>>>>> amd_pmf_get_bios_buffer(dev);
>>>>>>> +
>>>>>>> + /* Get amdgpu handle */
>>>>>> Useless comment.
>>>>>>
>>>>>>> + pci_walk_bus(dev->root->bus, amd_pmf_get_gpu_handle, dev);
>>>>>>> + if (!dev->gfx_data.gpu_dev)
>>>>>>> + dev_err(dev->dev, "GPU handle not found!\n");
>>>>>>> +
>>>>>>> + if (!amd_pmf_gpu_init(&dev->gfx_data))
>>>>>>> + dev->gfx_data.gpu_dev_en = true;
>>>>>>> +
>>>>>>> dev->prev_data = kzalloc(sizeof(*dev->prev_data),
>>>>>>> GFP_KERNEL);
>>>>>>> if (!dev->prev_data)
>>>>>>> return -ENOMEM;
>>>>>>> @@ -461,5 +484,8 @@ void amd_pmf_deinit_smart_pc(struct
>>>>>>> amd_pmf_dev *dev)
>>>>>>> kfree(dev->prev_data);
>>>>>>> kfree(dev->policy_buf);
>>>>>>> cancel_delayed_work_sync(&dev->pb_work);
>>>>>>> + if (dev->gfx_data.gpu_dev_en)
>>>>>>> + amd_pmf_gpu_deinit(&dev->gfx_data);
>>>>>>> + pci_dev_put(dev->gfx_data.gpu_dev);
>>>>>>> amd_pmf_tee_deinit(dev);
>>>>>>> }
>>>>>>> diff --git a/include/linux/amd-pmf-io.h
>>>>>>> b/include/linux/amd-pmf-io.h
>>>>>>> new file mode 100644
>>>>>>> index 000000000000..5f79e66a41b3
>>>>>>> --- /dev/null
>>>>>>> +++ b/include/linux/amd-pmf-io.h
>>>>>>> @@ -0,0 +1,35 @@
>>>>>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>>>>>> +/*
>>>>>>> + * AMD Platform Management Framework Interface
>>>>>>> + *
>>>>>>> + * Copyright (c) 2023, Advanced Micro Devices, Inc.
>>>>>>> + * All Rights Reserved.
>>>>>>> + *
>>>>>>> + * Author: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>>>>>>> + */
>>>>>>> +
>>>>>>> +#ifndef AMD_PMF_IO_H
>>>>>>> +#define AMD_PMF_IO_H
>>>>>>> +
>>>>>>> +#include <acpi/video.h>
>>>>>>> +#include <drm/drm_connector.h>
>>>>>>> +#include <linux/backlight.h>
>>>>>>> +#include <linux/thermal.h>
>>>>>>> +
>>>>>>> +#define MAX_SUPPORTED 4
>>>>>>> +
>>>>>>> +/* amdgpu */
>>>>>> Document the structure properly with kerneldoc instead of an
>>>>>> unhelpful
>>>>>> comment like above :-). Please also check if you add any other
>>>>>> structs
>>>>>> into kernel-wide headers that you didn't document yet. Or fields
>>>>>> into
>>>>>> existing structs.
>>>>>>
>>>>>>> +struct amd_gpu_pmf_data {
>>>>>>> + struct pci_dev *gpu_dev;
>>>>>>> + struct backlight_device *raw_bd;
>>>>>>> + struct thermal_cooling_device *cooling_dev;
>>>>>>> + enum drm_connector_status con_status[MAX_SUPPORTED];
>>>>>>> + int display_count;
>>>>>>> + int connector_type[MAX_SUPPORTED];
>>>>>>> + bool gpu_dev_en;
>>>>>>> +};
>>>>>>> +
>>>>>>> +int amd_pmf_get_gfx_data(struct amd_gpu_pmf_data *pmf);
>>>>>>> +int amd_pmf_gpu_init(struct amd_gpu_pmf_data *pmf);
>>>>>>> +void amd_pmf_gpu_deinit(struct amd_gpu_pmf_data *pmf);
>>>>>>> +#endif
>>>>>>>
^ permalink raw reply
* Re: [PATCH v4 4/5] clk: twl: add clock driver for TWL6032
From: Andreas Kemnade @ 2023-10-19 7:18 UTC (permalink / raw)
To: Stephen Boyd
Cc: bcousson, conor+dt, devicetree, dmitry.torokhov,
krzysztof.kozlowski+dt, lee, linux-clk, linux-input, linux-kernel,
linux-omap, mturquette, robh+dt, tony
In-Reply-To: <8c63372175266d42efbfca0104e19a14.sboyd@kernel.org>
Am Wed, 18 Oct 2023 18:20:22 -0700
schrieb Stephen Boyd <sboyd@kernel.org>:
> Quoting Andreas Kemnade (2023-09-16 03:05:14)
> > The TWL6032 has some clock outputs which are controlled like
> > fixed-voltage regulators, in some drivers for these chips
> > found in the wild, just the regulator api is abused for controlling
> > them, so simply use something similar to the regulator functions.
> > Due to a lack of hardware available for testing, leave out the
> > TWL6030-specific part of those functions.
> >
> > Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> > ---
>
> Did you want me to pick this up in clk tree?
>
> Acked-by: Stephen Boyd <sboyd@kernel.org>
>
Well, Lee has picked up 1-3 of this series, so I think it is a good idea
if you take it. I do not think it will make things any easier if it goes
through Tony's tree because part 5 is a bit tricky.
If picked up independently (of the whole set and not only part 4), there
will be a range of commits were WLAN will not work at all instead of
basically working randomly by the clock is enabled for some reason (can
happen if warm-booted from the vendor kernel).
So IMHO for part 5 we have to decide wether to decide if above is
considered to break bisectability and take the patch for 6.8 or
ignore that and take it for 6.7.
But no matter how we decide, that should not affect the route for this
part 4. So it is a good idea to have it through the clk tree.
Regrads,
Andreas
^ permalink raw reply
* Re: [PATCH v3 4/4] HID: touchscreen: Add initial support for Himax HID-over-SPI
From: kernel test robot @ 2023-10-19 6:47 UTC (permalink / raw)
To: Tylor Yang, dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt,
conor+dt, jikos, benjamin.tissoires, linux-input, devicetree,
linux-kernel
Cc: oe-kbuild-all, poyuan_chang, jingyliang, hbarnor, wuxy23, luolm1,
poyu_hung, Tylor Yang
In-Reply-To: <20231017091900.801989-5-tylor_yang@himax.corp-partner.google.com>
Hi Tylor,
kernel test robot noticed the following build errors:
[auto build test ERROR on hid/for-next]
[also build test ERROR on dtor-input/next dtor-input/for-linus robh/for-next linus/master v6.6-rc6 next-20231018]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Tylor-Yang/dt-bindings-input-Introduce-Himax-HID-over-SPI-device/20231017-172156
base: https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git for-next
patch link: https://lore.kernel.org/r/20231017091900.801989-5-tylor_yang%40himax.corp-partner.google.com
patch subject: [PATCH v3 4/4] HID: touchscreen: Add initial support for Himax HID-over-SPI
config: powerpc-allmodconfig (https://download.01.org/0day-ci/archive/20231019/202310191454.v9qp5FPx-lkp@intel.com/config)
compiler: powerpc64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231019/202310191454.v9qp5FPx-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310191454.v9qp5FPx-lkp@intel.com/
All errors (new ones prefixed by >>):
drivers/hid/hx-hid/hx_core.c: In function 'himax_boot_upgrade':
drivers/hid/hx-hid/hx_core.c:701:14: warning: variable 'fw_load_status' set but not used [-Wunused-but-set-variable]
701 | bool fw_load_status = false;
| ^~~~~~~~~~~~~~
drivers/hid/hx-hid/hx_core.c: At top level:
drivers/hid/hx-hid/hx_core.c:831:6: warning: no previous prototype for 'hx_hid_update' [-Wmissing-prototypes]
831 | void hx_hid_update(struct work_struct *work)
| ^~~~~~~~~~~~~
drivers/hid/hx-hid/hx_core.c:890:6: warning: no previous prototype for 'himax_report_data_deinit' [-Wmissing-prototypes]
890 | void himax_report_data_deinit(struct himax_ts_data *ts)
| ^~~~~~~~~~~~~~~~~~~~~~~~
drivers/hid/hx-hid/hx_core.c:940:5: warning: no previous prototype for 'himax_chip_suspend' [-Wmissing-prototypes]
940 | int himax_chip_suspend(struct himax_ts_data *ts)
| ^~~~~~~~~~~~~~~~~~
drivers/hid/hx-hid/hx_core.c: In function 'himax_chip_suspend':
drivers/hid/hx-hid/hx_core.c:942:13: warning: variable 'ret' set but not used [-Wunused-but-set-variable]
942 | int ret = 0;
| ^~~
drivers/hid/hx-hid/hx_core.c: At top level:
drivers/hid/hx-hid/hx_core.c:983:5: warning: no previous prototype for 'himax_chip_resume' [-Wmissing-prototypes]
983 | int himax_chip_resume(struct himax_ts_data *ts)
| ^~~~~~~~~~~~~~~~~
drivers/hid/hx-hid/hx_core.c: In function 'himax_resume':
>> drivers/hid/hx-hid/hx_core.c:1047:21: error: too few arguments to function 'himax_chip_init'
1047 | if (himax_chip_init())
| ^~~~~~~~~~~~~~~
In file included from drivers/hid/hx-hid/hx_ic_core.h:6,
from drivers/hid/hx-hid/hx_core.c:16:
drivers/hid/hx-hid/hx_core.h:485:5: note: declared here
485 | int himax_chip_init(struct himax_ts_data *ts);
| ^~~~~~~~~~~~~~~
drivers/hid/hx-hid/hx_core.c: At top level:
drivers/hid/hx-hid/hx_core.c:1212:6: warning: no previous prototype for 'himax_chip_deinit' [-Wmissing-prototypes]
1212 | void himax_chip_deinit(struct himax_ts_data *ts)
| ^~~~~~~~~~~~~~~~~
drivers/hid/hx-hid/hx_core.c: In function 'himax_platform_init':
drivers/hid/hx-hid/hx_core.c:1271:13: warning: variable 'err' set but not used [-Wunused-but-set-variable]
1271 | int err = PROBE_FAIL;
| ^~~
drivers/hid/hx-hid/hx_core.c: At top level:
drivers/hid/hx-hid/hx_core.c:1353:5: warning: no previous prototype for 'himax_spi_drv_probe' [-Wmissing-prototypes]
1353 | int himax_spi_drv_probe(struct spi_device *spi)
| ^~~~~~~~~~~~~~~~~~~
vim +/himax_chip_init +1047 drivers/hid/hx-hid/hx_core.c
66a3d0692ad03f Tylor Yang 2023-10-17 1034
66a3d0692ad03f Tylor Yang 2023-10-17 1035 int himax_resume(struct device *dev)
66a3d0692ad03f Tylor Yang 2023-10-17 1036 {
66a3d0692ad03f Tylor Yang 2023-10-17 1037 int ret = 0;
66a3d0692ad03f Tylor Yang 2023-10-17 1038 struct himax_ts_data *ts = dev_get_drvdata(dev);
66a3d0692ad03f Tylor Yang 2023-10-17 1039
66a3d0692ad03f Tylor Yang 2023-10-17 1040 I("enter");
66a3d0692ad03f Tylor Yang 2023-10-17 1041 /*
66a3d0692ad03f Tylor Yang 2023-10-17 1042 * wait until device resume for TDDI
66a3d0692ad03f Tylor Yang 2023-10-17 1043 * TDDI: Touch and display Driver IC
66a3d0692ad03f Tylor Yang 2023-10-17 1044 */
66a3d0692ad03f Tylor Yang 2023-10-17 1045 if (!ts->initialized) {
66a3d0692ad03f Tylor Yang 2023-10-17 1046 #if !defined(CONFIG_FB)
66a3d0692ad03f Tylor Yang 2023-10-17 @1047 if (himax_chip_init())
66a3d0692ad03f Tylor Yang 2023-10-17 1048 return -ECANCELED;
66a3d0692ad03f Tylor Yang 2023-10-17 1049 #else
66a3d0692ad03f Tylor Yang 2023-10-17 1050 E("init not ready, skip!");
66a3d0692ad03f Tylor Yang 2023-10-17 1051 return -ECANCELED;
66a3d0692ad03f Tylor Yang 2023-10-17 1052 #endif
66a3d0692ad03f Tylor Yang 2023-10-17 1053 }
66a3d0692ad03f Tylor Yang 2023-10-17 1054 ret = himax_chip_resume(ts);
66a3d0692ad03f Tylor Yang 2023-10-17 1055 if (ret < 0) {
66a3d0692ad03f Tylor Yang 2023-10-17 1056 E("resume failed!");
66a3d0692ad03f Tylor Yang 2023-10-17 1057 I("retry resume");
66a3d0692ad03f Tylor Yang 2023-10-17 1058 schedule_delayed_work(&ts->work_resume_delayed_work,
66a3d0692ad03f Tylor Yang 2023-10-17 1059 msecs_to_jiffies(ts->pdata->ic_resume_delay));
66a3d0692ad03f Tylor Yang 2023-10-17 1060 // I("try int rescue");
66a3d0692ad03f Tylor Yang 2023-10-17 1061 // himax_int_enable(ts, 1);
66a3d0692ad03f Tylor Yang 2023-10-17 1062 }
66a3d0692ad03f Tylor Yang 2023-10-17 1063
66a3d0692ad03f Tylor Yang 2023-10-17 1064 return ret;
66a3d0692ad03f Tylor Yang 2023-10-17 1065 }
66a3d0692ad03f Tylor Yang 2023-10-17 1066
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox