[PATCH 1/2] Input: ims-pcu - use driver core to instantiate device attributes

[PATCH 1/2] Input: ims-pcu - use driver core to instantiate device attributes

[Dmitry Torokhov]

Instead of manually creating driver-specific device attributes
set struct usb_driver->dev_groups pointer to have the driver core
do it.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/misc/ims-pcu.c | 53 +++++++++++++++++++-----------------
 1 file changed, 28 insertions(+), 25 deletions(-)

diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
index 408a586f8c36..0e61d969662f 100644
--- a/drivers/input/misc/ims-pcu.c
+++ b/drivers/input/misc/ims-pcu.c
@@ -1466,9 +1466,27 @@ static struct attribute *ims_pcu_ofn_attrs[] = {
 	NULL
 };
 
+static umode_t ims_pcu_ofn_is_attr_visible(struct kobject *kobj,
+					   struct attribute *attr, int n)
+{
+	struct device *dev = kobj_to_dev(kobj);
+	struct usb_interface *intf = to_usb_interface(dev);
+	struct ims_pcu *pcu = usb_get_intfdata(intf);
+	umode_t mode = attr->mode;
+
+	/*
+	 * PCU-B devices, both GEN_1 and GEN_2 do not have OFN sensor.
+	 */
+	if (pcu->bootloader_mode || pcu->device_id == IMS_PCU_PCU_B_DEVICE_ID)
+		mode = 0;
+
+	return mode;
+}
+
 static const struct attribute_group ims_pcu_ofn_attr_group = {
-	.name	= "ofn",
-	.attrs	= ims_pcu_ofn_attrs,
+	.name		= "ofn",
+	.is_visible	= ims_pcu_ofn_is_attr_visible,
+	.attrs		= ims_pcu_ofn_attrs,
 };
 
 static void ims_pcu_irq(struct urb *urb)
@@ -1890,16 +1908,6 @@ static int ims_pcu_init_application_mode(struct ims_pcu *pcu)
 	/* Device appears to be operable, complete initialization */
 	pcu->device_no = atomic_inc_return(&device_no);
 
-	/*
-	 * PCU-B devices, both GEN_1 and GEN_2 do not have OFN sensor
-	 */
-	if (pcu->device_id != IMS_PCU_PCU_B_DEVICE_ID) {
-		error = sysfs_create_group(&pcu->dev->kobj,
-					   &ims_pcu_ofn_attr_group);
-		if (error)
-			return error;
-	}
-
 	error = ims_pcu_setup_backlight(pcu);
 	if (error)
 		return error;
@@ -1936,10 +1944,6 @@ static void ims_pcu_destroy_application_mode(struct ims_pcu *pcu)
 			ims_pcu_destroy_gamepad(pcu);
 		ims_pcu_destroy_buttons(pcu);
 		ims_pcu_destroy_backlight(pcu);
-
-		if (pcu->device_id != IMS_PCU_PCU_B_DEVICE_ID)
-			sysfs_remove_group(&pcu->dev->kobj,
-					   &ims_pcu_ofn_attr_group);
 	}
 }
 
@@ -2031,20 +2035,14 @@ static int ims_pcu_probe(struct usb_interface *intf,
 	if (error)
 		goto err_stop_io;
 
-	error = sysfs_create_group(&intf->dev.kobj, &ims_pcu_attr_group);
-	if (error)
-		goto err_stop_io;
-
 	error = pcu->bootloader_mode ?
 			ims_pcu_init_bootloader_mode(pcu) :
 			ims_pcu_init_application_mode(pcu);
 	if (error)
-		goto err_remove_sysfs;
+		goto err_stop_io;
 
 	return 0;
 
-err_remove_sysfs:
-	sysfs_remove_group(&intf->dev.kobj, &ims_pcu_attr_group);
 err_stop_io:
 	ims_pcu_stop_io(pcu);
 err_free_buffers:
@@ -2070,8 +2068,6 @@ static void ims_pcu_disconnect(struct usb_interface *intf)
 	if (alt->desc.bInterfaceClass != USB_CLASS_COMM)
 		return;
 
-	sysfs_remove_group(&intf->dev.kobj, &ims_pcu_attr_group);
-
 	ims_pcu_stop_io(pcu);
 
 	if (pcu->bootloader_mode)
@@ -2130,9 +2126,16 @@ static const struct usb_device_id ims_pcu_id_table[] = {
 	{ }
 };
 
+static const struct attribute_group *ims_pcu_sysfs_groups[] = {
+	&ims_pcu_attr_group,
+	&ims_pcu_ofn_attr_group,
+	NULL
+};
+
 static struct usb_driver ims_pcu_driver = {
 	.name			= "ims_pcu",
 	.id_table		= ims_pcu_id_table,
+	.dev_groups		= ims_pcu_sysfs_groups,
 	.probe			= ims_pcu_probe,
 	.disconnect		= ims_pcu_disconnect,
 #ifdef CONFIG_PM
-- 
2.45.2.505.gda0bf45e8d-goog

[PATCH 2/2] Input: ims-pcu - switch to using cleanup functions

[Dmitry Torokhov]

Start using __free() and guard() primitives to simplify the code
and error handling.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/misc/ims-pcu.c | 135 ++++++++++++++++-------------------
 1 file changed, 62 insertions(+), 73 deletions(-)

diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
index 0e61d969662f..e7bd8f9858ac 100644
--- a/drivers/input/misc/ims-pcu.c
+++ b/drivers/input/misc/ims-pcu.c
@@ -928,9 +928,8 @@ static void ims_pcu_process_async_firmware(const struct firmware *fw,
 		goto out;
 	}
 
-	mutex_lock(&pcu->cmd_mutex);
-	ims_pcu_handle_firmware_update(pcu, fw);
-	mutex_unlock(&pcu->cmd_mutex);
+	scoped_guard(mutex, &pcu->cmd_mutex)
+		ims_pcu_handle_firmware_update(pcu, fw);
 
 	release_firmware(fw);
 
@@ -954,7 +953,7 @@ static int ims_pcu_backlight_set_brightness(struct led_classdev *cdev,
 	__le16 br_val = cpu_to_le16(value);
 	int error;
 
-	mutex_lock(&pcu->cmd_mutex);
+	guard(mutex)(&pcu->cmd_mutex);
 
 	error = ims_pcu_execute_command(pcu, SET_BRIGHTNESS,
 					&br_val, sizeof(br_val));
@@ -963,8 +962,6 @@ static int ims_pcu_backlight_set_brightness(struct led_classdev *cdev,
 			 "Failed to set desired brightness %u, error: %d\n",
 			 value, error);
 
-	mutex_unlock(&pcu->cmd_mutex);
-
 	return error;
 }
 
@@ -978,7 +975,7 @@ ims_pcu_backlight_get_brightness(struct led_classdev *cdev)
 	int brightness;
 	int error;
 
-	mutex_lock(&pcu->cmd_mutex);
+	guard(mutex)(&pcu->cmd_mutex);
 
 	error = ims_pcu_execute_query(pcu, GET_BRIGHTNESS);
 	if (error) {
@@ -992,8 +989,6 @@ ims_pcu_backlight_get_brightness(struct led_classdev *cdev)
 			get_unaligned_le16(&pcu->cmd_buf[IMS_PCU_DATA_OFFSET]);
 	}
 
-	mutex_unlock(&pcu->cmd_mutex);
-
 	return brightness;
 }
 
@@ -1073,24 +1068,23 @@ static ssize_t ims_pcu_attribute_store(struct device *dev,
 	if (data_len > attr->field_length)
 		return -EINVAL;
 
-	error = mutex_lock_interruptible(&pcu->cmd_mutex);
-	if (error)
-		return error;
+	scoped_cond_guard(mutex, return -EINTR, &pcu->cmd_mutex) {
+		memset(field, 0, attr->field_length);
+		memcpy(field, buf, data_len);
 
-	memset(field, 0, attr->field_length);
-	memcpy(field, buf, data_len);
+		error = ims_pcu_set_info(pcu);
 
-	error = ims_pcu_set_info(pcu);
-
-	/*
-	 * Even if update failed, let's fetch the info again as we just
-	 * clobbered one of the fields.
-	 */
-	ims_pcu_get_info(pcu);
+		/*
+		 * Even if update failed, let's fetch the info again as we just
+		 * clobbered one of the fields.
+		 */
+		ims_pcu_get_info(pcu);
 
-	mutex_unlock(&pcu->cmd_mutex);
+		if (error)
+			return error;
+	}
 
-	return error < 0 ? error : count;
+	return count;
 }
 
 #define IMS_PCU_ATTR(_field, _mode)					\
@@ -1153,7 +1147,6 @@ static ssize_t ims_pcu_update_firmware_store(struct device *dev,
 {
 	struct usb_interface *intf = to_usb_interface(dev);
 	struct ims_pcu *pcu = usb_get_intfdata(intf);
-	const struct firmware *fw = NULL;
 	int value;
 	int error;
 
@@ -1164,35 +1157,33 @@ static ssize_t ims_pcu_update_firmware_store(struct device *dev,
 	if (value != 1)
 		return -EINVAL;
 
-	error = mutex_lock_interruptible(&pcu->cmd_mutex);
-	if (error)
-		return error;
-
+	const struct firmware *fw __free(firmware) = NULL;
 	error = request_ihex_firmware(&fw, IMS_PCU_FIRMWARE_NAME, pcu->dev);
 	if (error) {
 		dev_err(pcu->dev, "Failed to request firmware %s, error: %d\n",
 			IMS_PCU_FIRMWARE_NAME, error);
-		goto out;
+		return error;
 	}
 
-	/*
-	 * If we are already in bootloader mode we can proceed with
-	 * flashing the firmware.
-	 *
-	 * If we are in application mode, then we need to switch into
-	 * bootloader mode, which will cause the device to disconnect
-	 * and reconnect as different device.
-	 */
-	if (pcu->bootloader_mode)
-		error = ims_pcu_handle_firmware_update(pcu, fw);
-	else
-		error = ims_pcu_switch_to_bootloader(pcu);
+	scoped_cond_guard(mutex_intr, return -EINTR, &pcu->cmd_mutex) {
+		/*
+		 * If we are already in bootloader mode we can proceed with
+		 * flashing the firmware.
+		 *
+		 * If we are in application mode, then we need to switch into
+		 * bootloader mode, which will cause the device to disconnect
+		 * and reconnect as different device.
+		 */
+		if (pcu->bootloader_mode)
+			error = ims_pcu_handle_firmware_update(pcu, fw);
+		else
+			error = ims_pcu_switch_to_bootloader(pcu);
 
-	release_firmware(fw);
+		if (error)
+			return error;
+	}
 
-out:
-	mutex_unlock(&pcu->cmd_mutex);
-	return error ?: count;
+	return count;
 }
 
 static DEVICE_ATTR(update_firmware, S_IWUSR,
@@ -1302,12 +1293,11 @@ static ssize_t ims_pcu_ofn_reg_data_show(struct device *dev,
 	int error;
 	u8 data;
 
-	mutex_lock(&pcu->cmd_mutex);
-	error = ims_pcu_read_ofn_config(pcu, pcu->ofn_reg_addr, &data);
-	mutex_unlock(&pcu->cmd_mutex);
-
-	if (error)
-		return error;
+	scoped_guard(mutex, &pcu->cmd_mutex) {
+		error = ims_pcu_read_ofn_config(pcu, pcu->ofn_reg_addr, &data);
+		if (error)
+			return error;
+	}
 
 	return sysfs_emit(buf, "%x\n", data);
 }
@@ -1325,11 +1315,13 @@ static ssize_t ims_pcu_ofn_reg_data_store(struct device *dev,
 	if (error)
 		return error;
 
-	mutex_lock(&pcu->cmd_mutex);
+	guard(mutex)(&pcu->cmd_mutex);
+
 	error = ims_pcu_write_ofn_config(pcu, pcu->ofn_reg_addr, value);
-	mutex_unlock(&pcu->cmd_mutex);
+	if (error)
+		return error;
 
-	return error ?: count;
+	return count;
 }
 
 static DEVICE_ATTR(reg_data, S_IRUGO | S_IWUSR,
@@ -1341,13 +1333,10 @@ static ssize_t ims_pcu_ofn_reg_addr_show(struct device *dev,
 {
 	struct usb_interface *intf = to_usb_interface(dev);
 	struct ims_pcu *pcu = usb_get_intfdata(intf);
-	int error;
 
-	mutex_lock(&pcu->cmd_mutex);
-	error = sysfs_emit(buf, "%x\n", pcu->ofn_reg_addr);
-	mutex_unlock(&pcu->cmd_mutex);
+	guard(mutex)(&pcu->cmd_mutex);
 
-	return error;
+	return sysfs_emit(buf, "%x\n", pcu->ofn_reg_addr);
 }
 
 static ssize_t ims_pcu_ofn_reg_addr_store(struct device *dev,
@@ -1363,9 +1352,9 @@ static ssize_t ims_pcu_ofn_reg_addr_store(struct device *dev,
 	if (error)
 		return error;
 
-	mutex_lock(&pcu->cmd_mutex);
+	guard(mutex)(&pcu->cmd_mutex);
+
 	pcu->ofn_reg_addr = value;
-	mutex_unlock(&pcu->cmd_mutex);
 
 	return count;
 }
@@ -1390,12 +1379,11 @@ static ssize_t ims_pcu_ofn_bit_show(struct device *dev,
 	int error;
 	u8 data;
 
-	mutex_lock(&pcu->cmd_mutex);
-	error = ims_pcu_read_ofn_config(pcu, attr->addr, &data);
-	mutex_unlock(&pcu->cmd_mutex);
-
-	if (error)
-		return error;
+	scoped_guard(mutex, &pcu->cmd_mutex) {
+		error = ims_pcu_read_ofn_config(pcu, attr->addr, &data);
+		if (error)
+			return error;
+	}
 
 	return sysfs_emit(buf, "%d\n", !!(data & (1 << attr->nr)));
 }
@@ -1419,21 +1407,22 @@ static ssize_t ims_pcu_ofn_bit_store(struct device *dev,
 	if (value > 1)
 		return -EINVAL;
 
-	mutex_lock(&pcu->cmd_mutex);
+	scoped_guard(mutex, &pcu->cmd_mutex) {
+		error = ims_pcu_read_ofn_config(pcu, attr->addr, &data);
+		if (error)
+			return error;
 
-	error = ims_pcu_read_ofn_config(pcu, attr->addr, &data);
-	if (!error) {
 		if (value)
 			data |= 1U << attr->nr;
 		else
 			data &= ~(1U << attr->nr);
 
 		error = ims_pcu_write_ofn_config(pcu, attr->addr, data);
+		if (error)
+			return error;
 	}
 
-	mutex_unlock(&pcu->cmd_mutex);
-
-	return error ?: count;
+	return count;
 }
 
 #define IMS_PCU_OFN_BIT_ATTR(_field, _addr, _nr)			\
-- 
2.45.2.505.gda0bf45e8d-goog