On Mon, Sep 6 2021 at 10:31:42 +0400, Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx> wrote: > Hi Yassine, > > On Wed, Sep 01, 2021 at 04:52:58PM +0000, Yassine Oudjana wrote: >> This adds support for Cypress StreetFighter touchkey controllers >> such as sf3155. >> This driver supports managing regulators and generating input >> events. >> >> Due to lack of documentation, this driver is entirely based on >> information >> gathered from a driver written for an old Android kernel fork[1][2]. >> >> Signed-off-by: Yassine Oudjana <y.oudjana@xxxxxxxxxxxxxx> > > Thank you for the patch. Please run your patches through > checkpatch.pl. Sure. > See additional comments below. > > >> >> [1] >> https://github.com/LineageOS/android_kernel_xiaomi_msm8996/blob/lineage-18.1/drivers/input/touchscreen/cyttsp_button.c >> [2] >> https://github.com/LineageOS/android_kernel_xiaomi_msm8996/blob/lineage-18.1/arch/arm/boot/dts/qcom/a4-msm8996-mtp.dtsi#L291-L314 >> --- >> Changes since v2: >> - Code style fixes. >> - Added copyright statement. >> Changes since v1: >> - Changed version variables in probe to int to allow storing error >> codes. >> >> drivers/input/keyboard/Kconfig | 10 ++ >> drivers/input/keyboard/Makefile | 1 + >> drivers/input/keyboard/cypress-sf.c | 223 >> ++++++++++++++++++++++++++++ >> 3 files changed, 234 insertions(+) >> create mode 100644 drivers/input/keyboard/cypress-sf.c >> >> diff --git a/drivers/input/keyboard/Kconfig >> b/drivers/input/keyboard/Kconfig >> index 40a070a2e7f5..6f3fbea8b803 100644 >> --- a/drivers/input/keyboard/Kconfig >> +++ b/drivers/input/keyboard/Kconfig >> @@ -788,4 +788,14 @@ config KEYBOARD_MTK_PMIC >> To compile this driver as a module, choose M here: the >> module will be called pmic-keys. >> >> +config KEYBOARD_CYPRESS_SF >> + tristate "Cypress StreetFighter touchkey support" >> + depends on I2C >> + help >> + Say Y here if you want to enable support for Cypress >> StreetFighter >> + touchkeys. >> + >> + To compile this driver as a module, choose M here: the >> + module will be called cypress-sf. >> + >> endif >> diff --git a/drivers/input/keyboard/Makefile >> b/drivers/input/keyboard/Makefile >> index 1d689fdd5c00..e3c8648f834e 100644 >> --- a/drivers/input/keyboard/Makefile >> +++ b/drivers/input/keyboard/Makefile >> @@ -17,6 +17,7 @@ obj-$(CONFIG_KEYBOARD_BCM) += bcm-keypad.o >> obj-$(CONFIG_KEYBOARD_CAP11XX) += cap11xx.o >> obj-$(CONFIG_KEYBOARD_CLPS711X) += clps711x-keypad.o >> obj-$(CONFIG_KEYBOARD_CROS_EC) += cros_ec_keyb.o >> +obj-$(CONFIG_KEYBOARD_CYPRESS_SF) += cypress-sf.o >> obj-$(CONFIG_KEYBOARD_DAVINCI) += davinci_keyscan.o >> obj-$(CONFIG_KEYBOARD_DLINK_DIR685) += dlink-dir685-touchkeys.o >> obj-$(CONFIG_KEYBOARD_EP93XX) += ep93xx_keypad.o >> diff --git a/drivers/input/keyboard/cypress-sf.c >> b/drivers/input/keyboard/cypress-sf.c >> new file mode 100644 >> index 000000000000..f2862547f633 >> --- /dev/null >> +++ b/drivers/input/keyboard/cypress-sf.c >> @@ -0,0 +1,223 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * Cypress StreetFighter Touchkey Driver >> + * >> + * Copyright (c) 2021 Yassine Oudjana <y.oudjana@xxxxxxxxxxxxxx> >> + */ >> + >> +#include <linux/bitops.h> >> +#include <linux/device.h> >> +#include <linux/i2c.h> >> +#include <linux/input.h> >> +#include <linux/interrupt.h> >> +#include <linux/module.h> >> +#include <linux/pm.h> >> +#include <linux/regulator/consumer.h> >> + >> +#define CYPRESS_SF_DEV_NAME "cypress-sf" >> + >> +#define CYPRESS_SF_REG_FW_VERSION 0x46 >> +#define CYPRESS_SF_REG_HW_VERSION 0x48 >> +#define CYPRESS_SF_REG_BUTTON_STATUS 0x4a >> + >> +struct cypress_sf_data { >> + struct i2c_client *client; >> + struct input_dev *input_dev; >> + struct regulator_bulk_data regulators[2]; >> + u32 *keycodes; >> + unsigned long keystates; >> + int num_keys; >> +}; >> + >> +static irqreturn_t cypress_sf_irq_handler(int irq, void *devid) >> +{ >> + struct cypress_sf_data *touchkey = devid; >> + unsigned long keystates; >> + bool curr_state, new_state; >> + int key; >> + >> + keystates = i2c_smbus_read_byte_data(touchkey->client, >> + CYPRESS_SF_REG_BUTTON_STATUS); >> + if (keystates < 0) { > > keystates is declared as unsigned, so this condition will never > trigger. Right. I'll change it. > >> + dev_err(&touchkey->client->dev, "Failed to read button status"); >> + return IRQ_NONE; >> + } >> + >> + for(key = 0; key < touchkey->num_keys; ++key) { >> + curr_state = test_bit(key, &touchkey->keystates); >> + new_state = test_bit(key, &keystates); >> + >> + if(curr_state ^ new_state) { > > Instead of this please do > > bitmap_xor(&changed, &keystates, &touchkey->keystates, > touchkey->num_keys); > for_each_set_bit(key, &changed, touchkey->num_keys) { > ... > } > Noted. >> + dev_dbg(&touchkey->client->dev,\ >> + "Key %d changed to %d", key, new_state); >> + input_report_key(touchkey->input_dev, >> + touchkey->keycodes[key], >> + new_state); >> + } >> + } >> + input_sync(touchkey->input_dev); >> + touchkey->keystates = keystates; >> + >> + return IRQ_HANDLED; >> +} >> + >> +static int cypress_sf_probe(struct i2c_client *client, >> + const struct i2c_device_id *id) >> +{ >> + struct cypress_sf_data *touchkey; >> + int hw_version, fw_version; >> + int i, ret; >> + >> + touchkey = devm_kzalloc(&client->dev, sizeof(*touchkey), >> GFP_KERNEL); >> + if(!touchkey) >> + return -ENOMEM; >> + >> + touchkey->client = client; >> + i2c_set_clientdata(client, touchkey); >> + >> + touchkey->regulators[0].supply = "vdd"; >> + touchkey->regulators[1].supply = "avdd"; >> + >> + ret = devm_regulator_bulk_get(&client->dev, >> + ARRAY_SIZE(touchkey->regulators), >> + touchkey->regulators); >> + if(ret) { >> + dev_err(&client->dev, "Failed to get regulators: %d\n", ret); >> + return ret; >> + } >> + >> + touchkey->num_keys = >> of_property_count_elems_of_size(client->dev.of_node, >> + "linux,keycodes", >> + sizeof(u32)); > > Please use device_property_* API instead of of_property_*. Noted. >> + if(touchkey->num_keys < 0) >> + /* Default key count */ >> + touchkey->num_keys = 2; >> + >> + touchkey->keycodes = devm_kzalloc(&client->dev, >> + sizeof(u32) * touchkey->num_keys, GFP_KERNEL); >> + if(!touchkey->keycodes) >> + return -ENOMEM; >> + >> + ret = of_property_read_u32_array(client->dev.of_node, >> "linux,keycodes", >> + touchkey->keycodes, touchkey->num_keys); >> + >> + if(touchkey->num_keys < 0) { >> + /* Default keycodes */ >> + touchkey->keycodes[0] = KEY_BACK; >> + touchkey->keycodes[1] = KEY_MENU; >> + } >> + >> + ret = regulator_bulk_enable(ARRAY_SIZE(touchkey->regulators), >> + touchkey->regulators); > > Please call variables that carry error core or 0 for success 'error'. I'll rename them. > >> + if(ret) { >> + dev_err(&client->dev, "Failed to enable regulators: %d\n", ret); >> + return ret; >> + } >> + >> + touchkey->input_dev = devm_input_allocate_device(&client->dev); >> + if(!touchkey->input_dev) { >> + dev_err(&client->dev, "Failed to allocate input device\n"); >> + return -ENOMEM; >> + } >> + >> + touchkey->input_dev->name = CYPRESS_SF_DEV_NAME; >> + touchkey->input_dev->id.bustype = BUS_I2C; >> + >> + hw_version = i2c_smbus_read_byte_data(touchkey->client, >> + CYPRESS_SF_REG_HW_VERSION); >> + fw_version = i2c_smbus_read_word_data(touchkey->client, >> + CYPRESS_SF_REG_FW_VERSION); >> + if(hw_version < 0 || fw_version < 0) >> + dev_warn(&client->dev, "Failed to read versions\n"); >> + else >> + dev_info(&client->dev, "HW version %d, FW version %d\n", >> + hw_version, fw_version); > > Why is this needed? I was using it as an indicator for success finding the chip, but it isn't strictly needed. I'll remove it. > >> + >> + for(i = 0; i < touchkey->num_keys; ++i) >> + input_set_capability(touchkey->input_dev, EV_KEY, >> + touchkey->keycodes[i]); >> + >> + ret = input_register_device(touchkey->input_dev); >> + if(ret) { >> + dev_err(&client->dev, >> + "Failed to register input device: %d\n", ret); >> + return ret; >> + } >> + >> + ret = devm_request_threaded_irq(&client->dev, client->irq, >> + NULL, cypress_sf_irq_handler, >> + IRQF_ONESHOT, >> + CYPRESS_SF_DEV_NAME, touchkey); >> + if(ret) { >> + dev_err(&client->dev, >> + "Failed to register threaded irq: %d", ret); >> + return ret; >> + } >> + >> + return 0; >> +}; >> + >> +static int __maybe_unused cypress_sf_suspend(struct device *dev) { >> + struct i2c_client *client = to_i2c_client(dev); >> + struct cypress_sf_data *touchkey = i2c_get_clientdata(client); >> + int ret; >> + >> + disable_irq(client->irq); >> + ret = regulator_bulk_disable(ARRAY_SIZE(touchkey->regulators), >> + touchkey->regulators); >> + if(ret) { >> + dev_err(dev, "Failed to disable regulators: %d", ret); >> + enable_irq(client->irq); >> + return ret; >> + } >> + dev_dbg(dev, "Suspended device"); >> + >> + return 0; >> +} >> + >> +static int __maybe_unused cypress_sf_resume(struct device *dev) { >> + struct i2c_client *client = to_i2c_client(dev); >> + struct cypress_sf_data *touchkey = i2c_get_clientdata(client); >> + int ret; >> + >> + ret = regulator_bulk_enable(ARRAY_SIZE(touchkey->regulators), >> + touchkey->regulators); >> + if(ret) { >> + dev_err(dev, "Failed to enable regulators: %d", ret); >> + return ret; >> + } >> + enable_irq(client->irq); >> + dev_dbg(dev, "Resumed device"); >> + >> + return 0; >> +} >> + >> +static SIMPLE_DEV_PM_OPS(cypress_sf_pm_ops, >> + cypress_sf_suspend, cypress_sf_resume); >> + >> +static struct i2c_device_id cypress_sf_id_table[] = { >> + { CYPRESS_SF_DEV_NAME, 0 }, >> + { } >> +}; >> +MODULE_DEVICE_TABLE(i2c, cypress_sf_id_table); >> + >> +static const struct of_device_id cypress_sf_of_match[] = { >> + { .compatible = "cypress,sf3155", }, >> + { }, >> +}; >> +MODULE_DEVICE_TABLE(of, cypress_sf_of_match); >> + >> +static struct i2c_driver cypress_sf_driver = { >> + .driver = { >> + .name = CYPRESS_SF_DEV_NAME, >> + .pm = &cypress_sf_pm_ops, >> + .of_match_table = of_match_ptr(cypress_sf_of_match), >> + }, >> + .id_table = cypress_sf_id_table, >> + .probe = cypress_sf_probe, >> +}; >> +module_i2c_driver(cypress_sf_driver); >> + >> +MODULE_AUTHOR("Yassine Oudjana <y.oudjana@xxxxxxxxxxxxxx>"); >> +MODULE_DESCRIPTION("Cypress StreetFighter Touchkey Driver"); >> +MODULE_LICENSE("GPL v2"); >> -- >> 2.33.0 >> >> > > Thanks. > > -- > Dmitry Thanks for the review! Yassine
