Hello Jaechul,

On 01/03/2017 04:57 AM, Jaechul Lee wrote:

This patch adds support for the TM2 touch key and led
functionlity.

s/functionlity/functionality

I would add a comment about the sleep here.

+		msleep(150);
+	} else {
+		int err;
+
+		err = regulator_disable(samsung_touchkey->regulator_vcc);
+		if (err)
+			ret = err;
+
+		err = regulator_disable(samsung_touchkey->regulator_vdd);
+		if (err && !ret)
+			ret = err;
+	}
+	samsung_touchkey->power_onoff = onoff;
+
+	return ret;
+}
+
+static void tm2_touchkey_irq_work(struct work_struct *irq_work)
+{
+	struct tm2_touchkey_data *samsung_touchkey =
+	    container_of(irq_work, struct tm2_touchkey_data, irq_work);
+
+	if (!samsung_touchkey->pressed) {
+		input_report_key(samsung_touchkey->input_dev, KEY_PHONE, 0);
+		input_report_key(samsung_touchkey->input_dev, KEY_BACK, 0);
+	} else {
+		if (samsung_touchkey->keycode_type == TM2_TOUCHKEY_KEY_MENU)
+			input_report_key(samsung_touchkey->input_dev,
+					 KEY_PHONE, 1);
+		else
+			input_report_key(samsung_touchkey->input_dev,
+					 KEY_BACK, 1);
+	}
+	input_sync(samsung_touchkey->input_dev);
+}
+
+static irqreturn_t tm2_touchkey_irq_handler(int irq, void *devid)
+{
+	struct tm2_touchkey_data *samsung_touchkey = devid;
+	u32 data;
+
+	data = i2c_smbus_read_byte_data(samsung_touchkey->client,
+					TM2_TOUCHKEY_KEYCODE_REG);
+
+	if (data < 0) {
+		dev_err(&samsung_touchkey->client->dev, "Failed to read i2c data\n");
+		return IRQ_HANDLED;
+	}
+
+	samsung_touchkey->keycode_type = data & TM2_TOUCHKEY_BIT_KEYCODE;
+	samsung_touchkey->pressed = !(data & TM2_TOUCHKEY_BIT_PRESS_EV);
+
+	if (samsung_touchkey->keycode_type != TM2_TOUCHKEY_KEY_MENU &&
+	    samsung_touchkey->keycode_type != TM2_TOUCHKEY_KEY_BACK)

Shouldn't at least a debug message be printed here so the user can
know that an error occurred and a correct keycode was not received?

+		return IRQ_HANDLED;
+
+	schedule_work(&samsung_touchkey->irq_work);
+
+	return IRQ_HANDLED;
+}
+
+static int tm2_touchkey_probe(struct i2c_client *client,
+				  const struct i2c_device_id *id)
+{
+	struct tm2_touchkey_data *samsung_touchkey;
+	int ret;
+
+	ret = i2c_check_functionality(client->adapter,
+				      I2C_FUNC_SMBUS_BYTE |
+				      I2C_FUNC_SMBUS_BYTE_DATA);
+	if (!ret) {
+		dev_err(&client->dev, "No I2C functionality found\n");
+		return -ENODEV;
+	}
+
+	samsung_touchkey = devm_kzalloc(&client->dev,
+			sizeof(struct tm2_touchkey_data), GFP_KERNEL);
+
+	if (!samsung_touchkey) {
+		dev_err(&client->dev, "Failed to allocate memory.\n");
+		return -ENOMEM;
+	}
+
+	samsung_touchkey->client = client;
+	i2c_set_clientdata(client, samsung_touchkey);
+	INIT_WORK(&samsung_touchkey->irq_work, tm2_touchkey_irq_work);
+
+	/* regulator */
+	samsung_touchkey->regulator_vcc =
+				devm_regulator_get(&client->dev, "vcc");
+	if (IS_ERR(samsung_touchkey->regulator_vcc)) {
+		dev_err(&client->dev, "Failed to get vcc regulator\n");
+		return PTR_ERR(samsung_touchkey->regulator_vcc);
+	}
+
+	samsung_touchkey->regulator_vdd =
+				devm_regulator_get(&client->dev, "vdd");
+	if (IS_ERR(samsung_touchkey->regulator_vdd)) {
+		dev_err(&client->dev, "Failed to get vdd regulator\n");
+		return PTR_ERR(samsung_touchkey->regulator_vcc);
+	}
+
+	/* power */
+	ret = tm2_touchkey_power_enable(samsung_touchkey);
+	if (ret) {
+		dev_err(&client->dev, "Failed to enable power\n");
+		return ret;
+	}
+
+	/* irq */
+	ret = devm_request_threaded_irq(&client->dev,
+					client->irq, NULL,
+					tm2_touchkey_irq_handler,
+					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+					TM2_TOUCHKEY_DEV_NAME,
+					samsung_touchkey);
+	if (ret) {
+		dev_err(&client->dev, "Failed to request threaded irq\n");
+		return ret;
+	}
+
+	/* input device */
+	samsung_touchkey->input_dev = devm_input_allocate_device(&client->dev);
+	if (!samsung_touchkey->input_dev) {
+		dev_err(&client->dev, "Failed to alloc input device.\n");
+		return -ENOMEM;
+	}
+	samsung_touchkey->input_dev->name = TM2_TOUCHKEY_DEV_NAME;
+	samsung_touchkey->input_dev->id.bustype = BUS_I2C;
+	samsung_touchkey->input_dev->dev.parent = &client->dev;
+
+	set_bit(EV_KEY, samsung_touchkey->input_dev->evbit);
+	set_bit(KEY_PHONE, samsung_touchkey->input_dev->keybit);
+	set_bit(KEY_BACK, samsung_touchkey->input_dev->keybit);
+	input_set_drvdata(samsung_touchkey->input_dev, samsung_touchkey);
+
+	ret = input_register_device(samsung_touchkey->input_dev);
+	if (ret) {
+		dev_err(&client->dev, "Failed to register input device.\n");
+		return ret;
+	}
+
+	/* led device */
+	samsung_touchkey->led_dev.name = TM2_TOUCHKEY_DEV_NAME;
+	samsung_touchkey->led_dev.brightness = LED_FULL;
+	samsung_touchkey->led_dev.max_brightness = LED_FULL;
+	samsung_touchkey->led_dev.brightness_set =
+						tm2_touchkey_led_brightness_set;
+
+	ret = devm_led_classdev_register(&client->dev,
+					 &samsung_touchkey->led_dev);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to register touchkey led\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static void tm2_touchkey_shutdown(struct i2c_client *client)
+{
+	struct tm2_touchkey_data *samsung_touchkey =
+						i2c_get_clientdata(client);
+	int ret;
+
+	disable_irq(client->irq);
+	ret = tm2_touchkey_power_disable(samsung_touchkey);
+	if (ret)
+		dev_err(&client->dev, "Failed to disable power\n");
+}
+
+static int tm2_touchkey_suspend(struct device *dev)
+{
+	struct tm2_touchkey_data *samsung_touchkey = dev_get_drvdata(dev);
+	int ret;
+
+	disable_irq(samsung_touchkey->client->irq);
+	ret = tm2_touchkey_power_disable(samsung_touchkey);
+	if (ret)
+		dev_err(dev, "Failed to disable power\n");
+
+	return ret;
+}

These two functions are basically the same, can you factor it out?

+
+static int tm2_touchkey_resume(struct device *dev)
+{
+	struct tm2_touchkey_data *samsung_touchkey = dev_get_drvdata(dev);
+	int ret;
+
+	enable_irq(samsung_touchkey->client->irq);
+	ret = tm2_touchkey_power_enable(samsung_touchkey);
+	if (ret)
+		dev_err(dev, "Failed to enable power\n");
+
+	return ret;
+}
+
+static SIMPLE_DEV_PM_OPS(tm2_touchkey_pm_ops, tm2_touchkey_suspend,
+							tm2_touchkey_resume);
+
+static const struct i2c_device_id tm2_touchkey_id_table[] = {
+	{TM2_TOUCHKEY_DEV_NAME, 0},
+	{},
+};
+

You need a MODULE_DEVICE_TABLE(i2c, tm2_touchkey_id_table) here so the
module can be autoloaded when the device is registered.

+static const struct of_device_id tm2_touchkey_of_match[] = {
+	{.compatible = "samsung,tm2-touchkey",},
+	{},
+};
+

Here a MODULE_DEVICE_TABLE(of, tm2_touchkey_of_match) is not strictly
needed since the I2C core always reports MODALIAS of the form i2c:<dev>
but still is good to have so the I2C core can be fixed at some point.

+static struct i2c_driver tm2_touchkey_driver = {
+	.driver = {
+		.name = TM2_TOUCHKEY_DEV_NAME,
+		.pm = &tm2_touchkey_pm_ops,
+		.of_match_table = of_match_ptr(tm2_touchkey_of_match),
+	},
+	.probe = tm2_touchkey_probe,
+	.shutdown = tm2_touchkey_shutdown,
+	.id_table = tm2_touchkey_id_table,
+};
+
+module_i2c_driver(tm2_touchkey_driver);
+
+MODULE_AUTHOR("Beomho Seo [off-list ref]");
+MODULE_AUTHOR("Jaechul Lee [off-list ref]");
+MODULE_DESCRIPTION("Samsung touchkey driver");
+MODULE_LICENSE("GPL v2");

The rest looks good to me, so after the changes I suggested:

Reviewed-by: Javier Martinez Canillas <redacted>

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America