diff mbox

[v2] ims-pcu: Add commands supported by the new version of the FW

Message ID 20140104013957.GA19702@core.coreip.homeip.net (mailing list archive)
State New, archived
Headers show

Commit Message

Dmitry Torokhov Jan. 4, 2014, 1:39 a.m. UTC
Hi Andrey,

On Wed, Jan 01, 2014 at 04:47:01PM -0800, Andrey Smirnov wrote:
> New version of the PCU firmware supports two new commands:
>   - IMS_PCU_CMD_OFN_SET_CONFIG which allows to write data to the
>   registers of one finger navigation(OFN) chip present on the device
>   - IMS_PCU_CMD_OFN_GET_CONFIG which allows to read data form the
>   registers of said chip.
> 
> This commit adds two helper functions to use those commands and sysfs
> attributes to use them. It also exposes some OFN configuration
> parameters via sysfs.

Thank you for making the changes. I do not still quite like the games we
play with the OFN attributes, how about the patch below (on top of
yours)?

Thanks.

Comments

Andrey Smirnov Jan. 4, 2014, 4:24 a.m. UTC | #1
On Fri, Jan 3, 2014 at 5:39 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> Hi Andrey,
>
> On Wed, Jan 01, 2014 at 04:47:01PM -0800, Andrey Smirnov wrote:
>> New version of the PCU firmware supports two new commands:
>>   - IMS_PCU_CMD_OFN_SET_CONFIG which allows to write data to the
>>   registers of one finger navigation(OFN) chip present on the device
>>   - IMS_PCU_CMD_OFN_GET_CONFIG which allows to read data form the
>>   registers of said chip.
>>
>> This commit adds two helper functions to use those commands and sysfs
>> attributes to use them. It also exposes some OFN configuration
>> parameters via sysfs.
>
> Thank you for making the changes. I do not still quite like the games we
> play with the OFN attributes, how about the patch below (on top of
> yours)?
>

Yeah, I agree I like it the "two separate sysfs groups" group approach
better. The only small nitpick about your patch is that I think we
should use "get_unaligned_le16" instead of "le16_to_cpup"(In case
anyone decides to run the driver on SuperH or C6x DSPs from TI :-)).
Let me test it and if everything works as expected I'll apply you
patch, convert it to "get_unaligned_le16", squash and send v3 of the
patch.

Thanks,
Andrey
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dmitry Torokhov Jan. 4, 2014, 4:44 a.m. UTC | #2
On Fri, Jan 03, 2014 at 08:24:17PM -0800, Andrey Smirnov wrote:
> On Fri, Jan 3, 2014 at 5:39 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > Hi Andrey,
> >
> > On Wed, Jan 01, 2014 at 04:47:01PM -0800, Andrey Smirnov wrote:
> >> New version of the PCU firmware supports two new commands:
> >>   - IMS_PCU_CMD_OFN_SET_CONFIG which allows to write data to the
> >>   registers of one finger navigation(OFN) chip present on the device
> >>   - IMS_PCU_CMD_OFN_GET_CONFIG which allows to read data form the
> >>   registers of said chip.
> >>
> >> This commit adds two helper functions to use those commands and sysfs
> >> attributes to use them. It also exposes some OFN configuration
> >> parameters via sysfs.
> >
> > Thank you for making the changes. I do not still quite like the games we
> > play with the OFN attributes, how about the patch below (on top of
> > yours)?
> >
> 
> Yeah, I agree I like it the "two separate sysfs groups" group approach
> better. The only small nitpick about your patch is that I think we
> should use "get_unaligned_le16" instead of "le16_to_cpup"(In case
> anyone decides to run the driver on SuperH or C6x DSPs from TI :-)).
> Let me test it and if everything works as expected I'll apply you
> patch, convert it to "get_unaligned_le16", squash and send v3 of the
> patch.

Why do we need get_unaligned_le16()? As far as I can see pcu->cmd_buf is
aligned and therefore pcu->cmd_buf[2] is also aligned on word boundary.

Thanks.
Andrey Smirnov Jan. 4, 2014, 5:03 a.m. UTC | #3
On Fri, Jan 3, 2014 at 8:44 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Fri, Jan 03, 2014 at 08:24:17PM -0800, Andrey Smirnov wrote:
>> On Fri, Jan 3, 2014 at 5:39 PM, Dmitry Torokhov
>> <dmitry.torokhov@gmail.com> wrote:
>> > Hi Andrey,
>> >
>> > On Wed, Jan 01, 2014 at 04:47:01PM -0800, Andrey Smirnov wrote:
>> >> New version of the PCU firmware supports two new commands:
>> >>   - IMS_PCU_CMD_OFN_SET_CONFIG which allows to write data to the
>> >>   registers of one finger navigation(OFN) chip present on the device
>> >>   - IMS_PCU_CMD_OFN_GET_CONFIG which allows to read data form the
>> >>   registers of said chip.
>> >>
>> >> This commit adds two helper functions to use those commands and sysfs
>> >> attributes to use them. It also exposes some OFN configuration
>> >> parameters via sysfs.
>> >
>> > Thank you for making the changes. I do not still quite like the games we
>> > play with the OFN attributes, how about the patch below (on top of
>> > yours)?
>> >
>>
>> Yeah, I agree I like it the "two separate sysfs groups" group approach
>> better. The only small nitpick about your patch is that I think we
>> should use "get_unaligned_le16" instead of "le16_to_cpup"(In case
>> anyone decides to run the driver on SuperH or C6x DSPs from TI :-)).
>> Let me test it and if everything works as expected I'll apply you
>> patch, convert it to "get_unaligned_le16", squash and send v3 of the
>> patch.
>
> Why do we need get_unaligned_le16()? As far as I can see pcu->cmd_buf is
> aligned and therefore pcu->cmd_buf[2] is also aligned on word boundary.

* The "pcu" structure is allocated with kmalloc which doesn't give any
guarantees about address alignment.
* I am not sure if the cmd_buf field in that structure is aligned, and
even if it is, any future changes to that structure may shift its
offset.
* Also even if the data we are interested in is aligned on 2-byte
border, I think all those architectures require 4-byte border
alignment.

>
> Thanks.
>
> --
> Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dmitry Torokhov Jan. 4, 2014, 5:41 a.m. UTC | #4
On Fri, Jan 03, 2014 at 09:03:11PM -0800, Andrey Smirnov wrote:
> On Fri, Jan 3, 2014 at 8:44 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > On Fri, Jan 03, 2014 at 08:24:17PM -0800, Andrey Smirnov wrote:
> >> On Fri, Jan 3, 2014 at 5:39 PM, Dmitry Torokhov
> >> <dmitry.torokhov@gmail.com> wrote:
> >> > Hi Andrey,
> >> >
> >> > On Wed, Jan 01, 2014 at 04:47:01PM -0800, Andrey Smirnov wrote:
> >> >> New version of the PCU firmware supports two new commands:
> >> >>   - IMS_PCU_CMD_OFN_SET_CONFIG which allows to write data to the
> >> >>   registers of one finger navigation(OFN) chip present on the device
> >> >>   - IMS_PCU_CMD_OFN_GET_CONFIG which allows to read data form the
> >> >>   registers of said chip.
> >> >>
> >> >> This commit adds two helper functions to use those commands and sysfs
> >> >> attributes to use them. It also exposes some OFN configuration
> >> >> parameters via sysfs.
> >> >
> >> > Thank you for making the changes. I do not still quite like the games we
> >> > play with the OFN attributes, how about the patch below (on top of
> >> > yours)?
> >> >
> >>
> >> Yeah, I agree I like it the "two separate sysfs groups" group approach
> >> better. The only small nitpick about your patch is that I think we
> >> should use "get_unaligned_le16" instead of "le16_to_cpup"(In case
> >> anyone decides to run the driver on SuperH or C6x DSPs from TI :-)).
> >> Let me test it and if everything works as expected I'll apply you
> >> patch, convert it to "get_unaligned_le16", squash and send v3 of the
> >> patch.
> >
> > Why do we need get_unaligned_le16()? As far as I can see pcu->cmd_buf is
> > aligned and therefore pcu->cmd_buf[2] is also aligned on word boundary.
> 
> * The "pcu" structure is allocated with kmalloc which doesn't give any
> guarantees about address alignment.
> * I am not sure if the cmd_buf field in that structure is aligned, and
> even if it is, any future changes to that structure may shift its
> offset.
> * Also even if the data we are interested in is aligned on 2-byte
> border, I think all those architectures require 4-byte border
> alignment.

As far as I know word access only requires word alignment. Please see
the other patch I just posted that adds alignment check in balcklight
handling code.

Thanks.
Dmitry Torokhov Jan. 7, 2014, 7:14 a.m. UTC | #5
On Fri, Jan 03, 2014 at 09:41:33PM -0800, Dmitry Torokhov wrote:
> On Fri, Jan 03, 2014 at 09:03:11PM -0800, Andrey Smirnov wrote:
> > On Fri, Jan 3, 2014 at 8:44 PM, Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> > > On Fri, Jan 03, 2014 at 08:24:17PM -0800, Andrey Smirnov wrote:
> > >> On Fri, Jan 3, 2014 at 5:39 PM, Dmitry Torokhov
> > >> <dmitry.torokhov@gmail.com> wrote:
> > >> > Hi Andrey,
> > >> >
> > >> > On Wed, Jan 01, 2014 at 04:47:01PM -0800, Andrey Smirnov wrote:
> > >> >> New version of the PCU firmware supports two new commands:
> > >> >>   - IMS_PCU_CMD_OFN_SET_CONFIG which allows to write data to the
> > >> >>   registers of one finger navigation(OFN) chip present on the device
> > >> >>   - IMS_PCU_CMD_OFN_GET_CONFIG which allows to read data form the
> > >> >>   registers of said chip.
> > >> >>
> > >> >> This commit adds two helper functions to use those commands and sysfs
> > >> >> attributes to use them. It also exposes some OFN configuration
> > >> >> parameters via sysfs.
> > >> >
> > >> > Thank you for making the changes. I do not still quite like the games we
> > >> > play with the OFN attributes, how about the patch below (on top of
> > >> > yours)?
> > >> >
> > >>
> > >> Yeah, I agree I like it the "two separate sysfs groups" group approach
> > >> better. The only small nitpick about your patch is that I think we
> > >> should use "get_unaligned_le16" instead of "le16_to_cpup"(In case
> > >> anyone decides to run the driver on SuperH or C6x DSPs from TI :-)).
> > >> Let me test it and if everything works as expected I'll apply you
> > >> patch, convert it to "get_unaligned_le16", squash and send v3 of the
> > >> patch.
> > >
> > > Why do we need get_unaligned_le16()? As far as I can see pcu->cmd_buf is
> > > aligned and therefore pcu->cmd_buf[2] is also aligned on word boundary.
> > 
> > * The "pcu" structure is allocated with kmalloc which doesn't give any
> > guarantees about address alignment.
> > * I am not sure if the cmd_buf field in that structure is aligned, and
> > even if it is, any future changes to that structure may shift its
> > offset.
> > * Also even if the data we are interested in is aligned on 2-byte
> > border, I think all those architectures require 4-byte border
> > alignment.
> 
> As far as I know word access only requires word alignment. Please see
> the other patch I just posted that adds alignment check in balcklight
> handling code.

Andrey, were you able to test the patch?

Thanks.
Andrey Smirnov Jan. 7, 2014, 7:14 p.m. UTC | #6
Sorry, haven't had the chance yet. I'll try to do this and hopefully
send v3 of the patch by the end of this week

On Mon, Jan 6, 2014 at 11:14 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Fri, Jan 03, 2014 at 09:41:33PM -0800, Dmitry Torokhov wrote:
>> On Fri, Jan 03, 2014 at 09:03:11PM -0800, Andrey Smirnov wrote:
>> > On Fri, Jan 3, 2014 at 8:44 PM, Dmitry Torokhov
>> > <dmitry.torokhov@gmail.com> wrote:
>> > > On Fri, Jan 03, 2014 at 08:24:17PM -0800, Andrey Smirnov wrote:
>> > >> On Fri, Jan 3, 2014 at 5:39 PM, Dmitry Torokhov
>> > >> <dmitry.torokhov@gmail.com> wrote:
>> > >> > Hi Andrey,
>> > >> >
>> > >> > On Wed, Jan 01, 2014 at 04:47:01PM -0800, Andrey Smirnov wrote:
>> > >> >> New version of the PCU firmware supports two new commands:
>> > >> >>   - IMS_PCU_CMD_OFN_SET_CONFIG which allows to write data to the
>> > >> >>   registers of one finger navigation(OFN) chip present on the device
>> > >> >>   - IMS_PCU_CMD_OFN_GET_CONFIG which allows to read data form the
>> > >> >>   registers of said chip.
>> > >> >>
>> > >> >> This commit adds two helper functions to use those commands and sysfs
>> > >> >> attributes to use them. It also exposes some OFN configuration
>> > >> >> parameters via sysfs.
>> > >> >
>> > >> > Thank you for making the changes. I do not still quite like the games we
>> > >> > play with the OFN attributes, how about the patch below (on top of
>> > >> > yours)?
>> > >> >
>> > >>
>> > >> Yeah, I agree I like it the "two separate sysfs groups" group approach
>> > >> better. The only small nitpick about your patch is that I think we
>> > >> should use "get_unaligned_le16" instead of "le16_to_cpup"(In case
>> > >> anyone decides to run the driver on SuperH or C6x DSPs from TI :-)).
>> > >> Let me test it and if everything works as expected I'll apply you
>> > >> patch, convert it to "get_unaligned_le16", squash and send v3 of the
>> > >> patch.
>> > >
>> > > Why do we need get_unaligned_le16()? As far as I can see pcu->cmd_buf is
>> > > aligned and therefore pcu->cmd_buf[2] is also aligned on word boundary.
>> >
>> > * The "pcu" structure is allocated with kmalloc which doesn't give any
>> > guarantees about address alignment.
>> > * I am not sure if the cmd_buf field in that structure is aligned, and
>> > even if it is, any future changes to that structure may shift its
>> > offset.
>> > * Also even if the data we are interested in is aligned on 2-byte
>> > border, I think all those architectures require 4-byte border
>> > alignment.
>>
>> As far as I know word access only requires word alignment. Please see
>> the other patch I just posted that adds alignment check in balcklight
>> handling code.
>
> Andrey, were you able to test the patch?
>
> Thanks.
>
> --
> Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andrey Smirnov Jan. 9, 2014, 5:57 a.m. UTC | #7
On Mon, Jan 6, 2014 at 11:14 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> Andrey, were you able to test the patch?

Hi Dmitry,

I tested the patch and with exception of a trivial to fix typo/bug it
worked fine.

I see that you moved the OFN attributes to a dedicated directory,
which I personally like better, but I am not sure if IMS would not
want to push back on it since they might have started to use the API
already. I'll send a new version to Chris so that he can do some
additional testing and see if they can incorporate that naming change.
I'll e-mail you v3 of the patch as soon as that matter becomes more
clear.

Thanks,
Andrey
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dmitry Torokhov Jan. 9, 2014, 6:43 a.m. UTC | #8
On Wed, Jan 08, 2014 at 09:57:20PM -0800, Andrey Smirnov wrote:
> On Mon, Jan 6, 2014 at 11:14 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > Andrey, were you able to test the patch?
> 
> Hi Dmitry,
> 
> I tested the patch and with exception of a trivial to fix typo/bug it
> worked fine.
> 
> I see that you moved the OFN attributes to a dedicated directory,
> which I personally like better, but I am not sure if IMS would not
> want to push back on it since they might have started to use the API
> already. I'll send a new version to Chris so that he can do some
> additional testing and see if they can incorporate that naming change.
> I'll e-mail you v3 of the patch as soon as that matter becomes more
> clear.

Andrey,

I am pretty sure that the new sysfs layout with OFN group split makes
most sense. Luckily this is brand new functionality so IMS should be
able to adjust their userspace to accommodate it.

Thanks.
diff mbox

Patch

diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
index 4244f47..bd25a78 100644
--- a/drivers/input/misc/ims-pcu.c
+++ b/drivers/input/misc/ims-pcu.c
@@ -1224,11 +1224,87 @@  ims_pcu_update_firmware_status_show(struct device *dev,
 static DEVICE_ATTR(update_firmware_status, S_IRUGO,
 		   ims_pcu_update_firmware_status_show, NULL);
 
-enum pcu_ofn_offsets {
-	OFN_REG_RESULT_LSB_OFFSET = 2,
-	OFN_REG_RESULT_MSB_OFFSET = 3,
+static struct attribute *ims_pcu_attrs[] = {
+	&ims_pcu_attr_part_number.dattr.attr,
+	&ims_pcu_attr_serial_number.dattr.attr,
+	&ims_pcu_attr_date_of_manufacturing.dattr.attr,
+	&ims_pcu_attr_fw_version.dattr.attr,
+	&ims_pcu_attr_bl_version.dattr.attr,
+	&ims_pcu_attr_reset_reason.dattr.attr,
+	&dev_attr_reset_device.attr,
+	&dev_attr_update_firmware.attr,
+	&dev_attr_update_firmware_status.attr,
+	NULL,
+};
+
+static umode_t ims_pcu_is_attr_visible(struct kobject *kobj,
+				       struct attribute *attr, int n)
+{
+	struct device *dev = container_of(kobj, struct device, kobj);
+	struct usb_interface *intf = to_usb_interface(dev);
+	struct ims_pcu *pcu = usb_get_intfdata(intf);
+	umode_t mode = attr->mode;
+
+	if (pcu->bootloader_mode) {
+		if (attr != &dev_attr_update_firmware_status.attr &&
+		    attr != &dev_attr_update_firmware.attr &&
+		    attr != &dev_attr_reset_device.attr) {
+			mode = 0;
+		}
+	} else {
+		if (attr == &dev_attr_update_firmware_status.attr)
+			mode = 0;
+	}
+
+	return mode;
+}
+
+static struct attribute_group ims_pcu_attr_group = {
+	.is_visible	= ims_pcu_is_attr_visible,
+	.attrs		= ims_pcu_attrs,
 };
 
+/* Support for a separate OFN attribute group */
+
+#define OFN_REG_RESULT_OFFSET	2
+
+static int ims_pcu_read_ofn_config(struct ims_pcu *pcu, u8 addr, u8 *data)
+{
+	int error;
+	u16 result;
+
+	error = ims_pcu_execute_command(pcu, OFN_GET_CONFIG,
+					&addr, sizeof(addr));
+	if (error)
+		return error;
+
+	result = le16_to_cpup((__le16 *)&pcu->cmd_buf[OFN_REG_RESULT_OFFSET]);
+	if ((s16)result < 0)
+		return -EIO;
+
+	/* We only need LSB */
+	*data = pcu->cmd_buf[OFN_REG_RESULT_OFFSET];
+	return 0;
+}
+
+static int ims_pcu_write_ofn_config(struct ims_pcu *pcu, u8 addr, u8 data)
+{
+	u8 buffer[] = { addr, data };
+	int error;
+	u16 result;
+
+	error = ims_pcu_execute_command(pcu, OFN_SET_CONFIG,
+					&buffer, sizeof(buffer));
+	if (error)
+		return error;
+
+	result = le16_to_cpup((__le16 *)&pcu->cmd_buf[OFN_REG_RESULT_OFFSET]);
+	if ((s16)result < 0)
+		return -EIO;
+
+	return 0;
+}
+
 static ssize_t ims_pcu_ofn_reg_data_show(struct device *dev,
 					 struct device_attribute *dattr,
 					 char *buf)
@@ -1236,24 +1312,16 @@  static ssize_t ims_pcu_ofn_reg_data_show(struct device *dev,
 	struct usb_interface *intf = to_usb_interface(dev);
 	struct ims_pcu *pcu = usb_get_intfdata(intf);
 	int error;
+	u8 data;
 
 	mutex_lock(&pcu->cmd_mutex);
-
-	error = ims_pcu_execute_command(pcu, OFN_GET_CONFIG,
-					&pcu->ofn_reg_addr,
-					sizeof(pcu->ofn_reg_addr));
-	if (error >= 0) {
-		const s16 result = pcu->cmd_buf[OFN_REG_RESULT_MSB_OFFSET] << 8 |
-			           pcu->cmd_buf[OFN_REG_RESULT_LSB_OFFSET];
-		if (result < 0)
-			error = result;
-		else
-			error = scnprintf(buf, PAGE_SIZE, "%x\n", result);
-	}
-
+	error = ims_pcu_read_ofn_config(pcu, pcu->ofn_reg_addr, &data);
 	mutex_unlock(&pcu->cmd_mutex);
 
-	return error;
+	if (error)
+		return error;
+
+	return scnprintf(buf, PAGE_SIZE, "%x\n", data);
 }
 
 static ssize_t ims_pcu_ofn_reg_data_store(struct device *dev,
@@ -1264,33 +1332,19 @@  static ssize_t ims_pcu_ofn_reg_data_store(struct device *dev,
 	struct ims_pcu *pcu = usb_get_intfdata(intf);
 	int error;
 	u8 value;
-	u8 buffer[2];
-	s16 result;
 
 	error = kstrtou8(buf, 0, &value);
 	if (error)
 		return error;
 
-	buffer[0] = pcu->ofn_reg_addr;
-	buffer[1] = value;
-
 	mutex_lock(&pcu->cmd_mutex);
-
-	error = ims_pcu_execute_command(pcu, OFN_SET_CONFIG,
-					&buffer, sizeof(buffer));
-
-	result = pcu->cmd_buf[OFN_REG_RESULT_MSB_OFFSET] << 8 |
-  		 pcu->cmd_buf[OFN_REG_RESULT_LSB_OFFSET];
-
+	error = ims_pcu_write_ofn_config(pcu, pcu->ofn_reg_addr, value);
 	mutex_unlock(&pcu->cmd_mutex);
 
-	if (!error && result < 0)
-		error = -ENOENT;
-
 	return error ?: count;
 }
 
-static DEVICE_ATTR(ofn_reg_data, S_IRUGO | S_IWUSR,
+static DEVICE_ATTR(reg_data, S_IRUGO | S_IWUSR,
 		   ims_pcu_ofn_reg_data_show, ims_pcu_ofn_reg_data_store);
 
 static ssize_t ims_pcu_ofn_reg_addr_show(struct device *dev,
@@ -1328,47 +1382,47 @@  static ssize_t ims_pcu_ofn_reg_addr_store(struct device *dev,
 	return error ?: count;
 }
 
-static DEVICE_ATTR(ofn_reg_addr, S_IRUGO | S_IWUSR,
+static DEVICE_ATTR(reg_addr, S_IRUGO | S_IWUSR,
 		   ims_pcu_ofn_reg_addr_show, ims_pcu_ofn_reg_addr_store);
 
-static ssize_t ims_pcu_ofn_bit_show(u8 addr, u8 nr,
-				    struct device *dev,
+struct ims_pcu_ofn_bit_attribute {
+	struct device_attribute dattr;
+	u8 addr;
+	u8 nr;
+};
+
+static ssize_t ims_pcu_ofn_bit_show(struct device *dev,
 				    struct device_attribute *dattr,
 				    char *buf)
 {
 	struct usb_interface *intf = to_usb_interface(dev);
 	struct ims_pcu *pcu = usb_get_intfdata(intf);
+	struct ims_pcu_ofn_bit_attribute *attr =
+		container_of(dattr, struct ims_pcu_ofn_bit_attribute, dattr);
 	int error;
+	u8 data;
 
 	mutex_lock(&pcu->cmd_mutex);
+	error = ims_pcu_read_ofn_config(pcu, attr->addr, &data);
+	mutex_unlock(&pcu->cmd_mutex);
 
-	error = ims_pcu_execute_command(pcu, OFN_GET_CONFIG,
-					&addr, sizeof(addr));
-	if (error >= 0) {
-		const s16 result = pcu->cmd_buf[OFN_REG_RESULT_MSB_OFFSET] << 8 |
-			           pcu->cmd_buf[OFN_REG_RESULT_LSB_OFFSET];
-		if (result < 0)
-			error = result;
-		else
-			error = scnprintf(buf, PAGE_SIZE, "%d\n",
-					  !!(result & (1 << nr)));
-	}
+	if (error)
+		return error;
 
-	mutex_unlock(&pcu->cmd_mutex);
-	return error;
+	return scnprintf(buf, PAGE_SIZE, "%d\n", !!(data & (1 << attr->nr)));
 }
 
-static ssize_t ims_pcu_ofn_bit_store(u8 addr, u8 nr,
-				     struct device *dev,
+static ssize_t ims_pcu_ofn_bit_store(struct device *dev,
 				     struct device_attribute *dattr,
 				     const char *buf, size_t count)
 {
 	struct usb_interface *intf = to_usb_interface(dev);
 	struct ims_pcu *pcu = usb_get_intfdata(intf);
+	struct ims_pcu_ofn_bit_attribute *attr =
+		container_of(dattr, struct ims_pcu_ofn_bit_attribute, dattr);
 	int error;
 	int value;
-	u8 contents;
-
+	u8 data;
 
 	error = kstrtoint(buf, 0, &value);
 	if (error)
@@ -1379,135 +1433,54 @@  static ssize_t ims_pcu_ofn_bit_store(u8 addr, u8 nr,
 
 	mutex_lock(&pcu->cmd_mutex);
 
-	error = ims_pcu_execute_command(pcu, OFN_GET_CONFIG,
-					&addr, sizeof(addr));
-	if (error < 0)
-		goto exit;
-
-	{
-		const s16 result = pcu->cmd_buf[OFN_REG_RESULT_MSB_OFFSET] << 8 |
-			           pcu->cmd_buf[OFN_REG_RESULT_LSB_OFFSET];
-		if (result < 0) {
-			error = result;
-			goto exit;
-		}
-		contents = (u8) result;
-	}
-
-	if (value)
-		contents |= 1 << nr;
-	else
-		contents &= ~(1 << nr);
+	error = ims_pcu_read_ofn_config(pcu, attr->addr, &data);
+	if (!error) {
+		if (value)
+			data |= 1U << attr->nr;
+		else
+			data &= ~(1U << attr->nr);
 
-	{
-		const u8 buffer[] = { addr, contents };
-		error = ims_pcu_execute_command(pcu, OFN_SET_CONFIG,
-						&buffer, sizeof(buffer));
+		error = ims_pcu_write_ofn_config(pcu, attr->addr, data);
 	}
 
-exit:
 	mutex_unlock(&pcu->cmd_mutex);
 
-	if (!error) {
-		const s16 result = pcu->cmd_buf[OFN_REG_RESULT_MSB_OFFSET] << 8 |
-			           pcu->cmd_buf[OFN_REG_RESULT_LSB_OFFSET];
-		error = result;
-	}
-
 	return error ?: count;
 }
 
+#define IMS_PCU_OFN_BIT_ATTR(_field, _addr, _nr)			\
+struct ims_pcu_ofn_bit_attribute ims_pcu_ofn_attr_##_field = {		\
+	.dattr = __ATTR(_field, S_IWUSR | S_IRUGO,			\
+			ims_pcu_ofn_bit_show, ims_pcu_ofn_bit_store),	\
+	.addr = _addr,							\
+	.nr = _nr,							\
+}
 
-#define IMS_PCU_BIT_ATTR(name, addr, nr)				\
-	static ssize_t ims_pcu_##name##_show(struct device *dev,	\
-					     struct device_attribute *dattr, \
-					     char *buf)			\
-	{								\
-		return ims_pcu_ofn_bit_show(addr, nr, dev, dattr, buf);	\
-	}								\
-									\
-	static ssize_t ims_pcu_##name##_store(struct device *dev,	\
-					      struct device_attribute *dattr, \
-					      const char *buf, size_t count) \
-	{								\
-		return ims_pcu_ofn_bit_store(addr, nr, dev, dattr, buf, count); \
-	}								\
-									\
-	static DEVICE_ATTR(name, S_IRUGO | S_IWUSR,			\
-			   ims_pcu_##name##_show, ims_pcu_##name##_store)
-
-IMS_PCU_BIT_ATTR(ofn_engine_enable,   0x60, 7);
-IMS_PCU_BIT_ATTR(ofn_speed_enable,    0x60, 6);
-IMS_PCU_BIT_ATTR(ofn_assert_enable,   0x60, 5);
-IMS_PCU_BIT_ATTR(ofn_xyquant_enable,  0x60, 4);
-IMS_PCU_BIT_ATTR(ofn_xyscale_enable,  0x60, 1);
-
-IMS_PCU_BIT_ATTR(ofn_scale_x2,        0x63, 6);
-IMS_PCU_BIT_ATTR(ofn_scale_y2,        0x63, 7);
-
-static struct attribute *ims_pcu_attrs[] = {
-	&ims_pcu_attr_part_number.dattr.attr,
-	&ims_pcu_attr_serial_number.dattr.attr,
-	&ims_pcu_attr_date_of_manufacturing.dattr.attr,
-	&ims_pcu_attr_fw_version.dattr.attr,
-	&ims_pcu_attr_bl_version.dattr.attr,
-	&ims_pcu_attr_reset_reason.dattr.attr,
-	&dev_attr_reset_device.attr,
-	&dev_attr_update_firmware.attr,
-	&dev_attr_update_firmware_status.attr,
-
-#define IMS_PCU_ATTRS_OFN_START_OFFSET (9)
-
-	&dev_attr_ofn_reg_data.attr,
-	&dev_attr_ofn_reg_addr.attr,
-	&dev_attr_ofn_engine_enable.attr,
-	&dev_attr_ofn_speed_enable.attr,
-	&dev_attr_ofn_assert_enable.attr,
-	&dev_attr_ofn_xyquant_enable.attr,
-	&dev_attr_ofn_xyscale_enable.attr,
-	&dev_attr_ofn_scale_x2.attr,
-	&dev_attr_ofn_scale_y2.attr,
+static IMS_PCU_OFN_BIT_ATTR(engine_enable,   0x60, 7);
+static IMS_PCU_OFN_BIT_ATTR(speed_enable,    0x60, 6);
+static IMS_PCU_OFN_BIT_ATTR(assert_enable,   0x60, 5);
+static IMS_PCU_OFN_BIT_ATTR(xyquant_enable,  0x60, 4);
+static IMS_PCU_OFN_BIT_ATTR(xyscale_enable,  0x60, 1);
+
+static IMS_PCU_OFN_BIT_ATTR(scale_x2,        0x63, 6);
+static IMS_PCU_OFN_BIT_ATTR(scale_y2,        0x63, 7);
+
+static struct attribute *ims_pcu_ofn_attrs[] = {
+	&dev_attr_reg_data.attr,
+	&dev_attr_reg_addr.attr,
+	&ims_pcu_ofn_attr_engine_enable.dattr.attr,
+	&ims_pcu_ofn_attr_speed_enable.dattr.attr,
+	&ims_pcu_ofn_attr_assert_enable.dattr.attr,
+	&ims_pcu_ofn_attr_xyquant_enable.dattr.attr,
+	&ims_pcu_ofn_attr_xyscale_enable.dattr.attr,
+	&ims_pcu_ofn_attr_scale_x2.dattr.attr,
+	&ims_pcu_ofn_attr_scale_y2.dattr.attr,
 	NULL
 };
 
-static umode_t ims_pcu_is_attr_visible(struct kobject *kobj,
-				       struct attribute *attr, int n)
-{
-	struct device *dev = container_of(kobj, struct device, kobj);
-	struct usb_interface *intf = to_usb_interface(dev);
-	struct ims_pcu *pcu = usb_get_intfdata(intf);
-	umode_t mode = attr->mode;
-
-	if (pcu->bootloader_mode) {
-		if (attr != &dev_attr_update_firmware_status.attr &&
-		    attr != &dev_attr_update_firmware.attr &&
-		    attr != &dev_attr_reset_device.attr) {
-			mode = 0;
-		}
-	} else {
-		if (attr == &dev_attr_update_firmware_status.attr) {
-			mode = 0;
-		} else if (pcu->setup_complete && pcu->device_id == 5) {
-			/*
-			   PCU-B devices, both GEN_1 and GEN_2(device_id == 5),
-			   have no OFN sensor so exposing those attributes
-			   for them does not make any sense
-			*/
-			int i;
-			for (i = IMS_PCU_ATTRS_OFN_START_OFFSET; ims_pcu_attrs[i]; i++)
-				if (attr == ims_pcu_attrs[i]) {
-					mode = 0;
-					break;
-				}
-		}
-	}
-
-	return mode;
-}
-
-static struct attribute_group ims_pcu_attr_group = {
-	.is_visible	= ims_pcu_is_attr_visible,
-	.attrs		= ims_pcu_attrs,
+static struct attribute_group ims_pcu_ofn_attr_group = {
+	.name	= "ofn",
+	.attrs	= ims_pcu_ofn_attrs,
 };
 
 static void ims_pcu_irq(struct urb *urb)
@@ -1908,6 +1881,13 @@  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) - 1;
 
+	if (pcu->device_id != 5) {
+		error = sysfs_create_group(&pcu->dev->kobj,
+					   &ims_pcu_attr_group);
+		if (error)
+			return error;
+	}
+
 	error = ims_pcu_setup_backlight(pcu);
 	if (error)
 		return error;
@@ -1925,14 +1905,12 @@  static int ims_pcu_init_application_mode(struct ims_pcu *pcu)
 
 	pcu->setup_complete = true;
 
-	sysfs_update_group(&pcu->dev->kobj, &ims_pcu_attr_group);
-
 	return 0;
 
-err_destroy_backlight:
-	ims_pcu_destroy_backlight(pcu);
 err_destroy_buttons:
 	ims_pcu_destroy_buttons(pcu);
+err_destroy_backlight:
+	ims_pcu_destroy_backlight(pcu);
 	return error;
 }
 
@@ -1946,6 +1924,10 @@  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 != 5)
+			sysfs_remove_group(&pcu->dev->kobj,
+					   &ims_pcu_ofn_attr_group);
 	}
 }