Date:   Tue, 07 Sep 2021 09:01:17 +0000
From:   Yassine Oudjana <y.oudjana@...tonmail.com>
To:     Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc:     Rob Herring <robh+dt@...nel.org>, linux-input@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        phone-devel@...r.kernel.org, ~postmarketos/upstreaming@...ts.sr.ht
Subject: Re: [PATCH v3 1/2] Input: cypress-sf - Add Cypress StreetFighter touchkey driver



On Mon, Sep 6 2021 at 10:31:42 +0400, Dmitry Torokhov 
<dmitry.torokhov@...il.com> 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@...tonmail.com>
> 
> 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@...tonmail.com>
>>  + */
>>  +
>>  +#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@...tonmail.com>");
>>  +MODULE_DESCRIPTION("Cypress StreetFighter Touchkey Driver");
>>  +MODULE_LICENSE("GPL v2");
>>  --
>>  2.33.0
>> 
>> 
> 
> Thanks.
> 
> --
> Dmitry

Thanks for the review!
Yassine

Powered by blists - more mailing lists