diff mbox series

[v6,09/14] ASoC: Intel: catpt: Simple sysfs attributes

Message ID 20200917141242.9081-10-cezary.rojewski@intel.com
State New
Headers show
Series ASoC: Intel: Catpt - Lynx and Wildcat point | expand

Commit Message

Cezary Rojewski Sept. 17, 2020, 2:12 p.m. UTC
Add sysfs entries for displaying version of FW currently in use as well
as binary dump of entire version info, including build and log providers
hashes.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---

Changes in v6:
- functions declaration and usage now part of this patch instead of
  being separated from it

Changes in v2:
- fixed size provided to memcpy() in fw_build_read() as reported by Mark

 sound/soc/intel/catpt/core.h   |  3 ++
 sound/soc/intel/catpt/device.c |  6 +++
 sound/soc/intel/catpt/fs.c     | 79 ++++++++++++++++++++++++++++++++++
 3 files changed, 88 insertions(+)
 create mode 100644 sound/soc/intel/catpt/fs.c

Comments

Cezary Rojewski Sept. 18, 2020, 3:22 p.m. UTC | #1
On 2020-09-17 4:12 PM, Cezary Rojewski wrote:
> Add sysfs entries for displaying version of FW currently in use as well
> as binary dump of entire version info, including build and log providers
> hashes.
> 
> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
> ---
> 
> Changes in v6:
> - functions declaration and usage now part of this patch instead of
>    being separated from it
> 
> Changes in v2:
> - fixed size provided to memcpy() in fw_build_read() as reported by Mark
> 

+Greg KH

Greg, would you mind taking a look at these sysfs entries added for new
catpt driver (Audio DSP driver for Haswell and Broadwell machines)?

Link to opening post for the series:
[PATCH v6 00/14] ASoC: Intel: Catpt - Lynx and Wildcat point
https://www.spinics.net/lists/alsa-devel/msg115765.html

Let me give you a quick introduction to the catpt's fs code:
During power-up sequence a handshake is made between host (kernel device
driver) and DSP (firmware) side. Two sysfs entries are generated which
expose running DSP firmware version and its build info - information
obtained during said handshake.

Much like devices (such as those of PCI-type) expose sysfs entries for
their easy identification, catpt provides entries to identify DSP FW it
is dealing with.

Thanks,
Czarek

>   sound/soc/intel/catpt/core.h   |  3 ++
>   sound/soc/intel/catpt/device.c |  6 +++
>   sound/soc/intel/catpt/fs.c     | 79 ++++++++++++++++++++++++++++++++++
>   3 files changed, 88 insertions(+)
>   create mode 100644 sound/soc/intel/catpt/fs.c
> 
> diff --git a/sound/soc/intel/catpt/core.h b/sound/soc/intel/catpt/core.h
> index a29b4c0232cb..1f0f1ac92341 100644
> --- a/sound/soc/intel/catpt/core.h
> +++ b/sound/soc/intel/catpt/core.h
> @@ -155,6 +155,9 @@ int catpt_store_module_states(struct catpt_dev *cdev, struct dma_chan *chan);
>   int catpt_store_memdumps(struct catpt_dev *cdev, struct dma_chan *chan);
>   int catpt_coredump(struct catpt_dev *cdev);
>   
> +int catpt_sysfs_create(struct catpt_dev *cdev);
> +void catpt_sysfs_remove(struct catpt_dev *cdev);
> +
>   #include <sound/memalloc.h>
>   #include <uapi/sound/asound.h>
>   
> diff --git a/sound/soc/intel/catpt/device.c b/sound/soc/intel/catpt/device.c
> index 7c7ddbabaf55..e9b7c1f474e0 100644
> --- a/sound/soc/intel/catpt/device.c
> +++ b/sound/soc/intel/catpt/device.c
> @@ -184,6 +184,10 @@ static int catpt_probe_components(struct catpt_dev *cdev)
>   		goto board_err;
>   	}
>   
> +	ret = catpt_sysfs_create(cdev);
> +	if (ret)
> +		goto board_err;
> +
>   	/* reflect actual ADSP state in pm_runtime */
>   	pm_runtime_set_active(cdev->dev);
>   
> @@ -292,6 +296,8 @@ static int catpt_acpi_remove(struct platform_device *pdev)
>   	catpt_sram_free(&cdev->iram);
>   	catpt_sram_free(&cdev->dram);
>   
> +	catpt_sysfs_remove(cdev);
> +
>   	return 0;
>   }
>   
> diff --git a/sound/soc/intel/catpt/fs.c b/sound/soc/intel/catpt/fs.c
> new file mode 100644
> index 000000000000..d73493687f4a
> --- /dev/null
> +++ b/sound/soc/intel/catpt/fs.c
> @@ -0,0 +1,79 @@
> +// SPDX-License-Identifier: GPL-2.0-pcm
> +//
> +// Copyright(c) 2020 Intel Corporation. All rights reserved.
> +//
> +// Author: Cezary Rojewski <cezary.rojewski@intel.com>
> +//
> +
> +#include <linux/pm_runtime.h>
> +#include "core.h"
> +
> +static ssize_t fw_version_show(struct device *dev,
> +			       struct device_attribute *attr, char *buf)
> +{
> +	struct catpt_dev *cdev = dev_get_drvdata(dev);
> +	struct catpt_fw_version version;
> +	int ret;
> +
> +	pm_runtime_get_sync(cdev->dev);
> +
> +	ret = catpt_ipc_get_fw_version(cdev, &version);
> +
> +	pm_runtime_mark_last_busy(cdev->dev);
> +	pm_runtime_put_autosuspend(cdev->dev);
> +
> +	if (ret)
> +		return CATPT_IPC_ERROR(ret);
> +
> +	return sprintf(buf, "%d.%d.%d.%d\n", version.type, version.major,
> +		       version.minor, version.build);
> +}
> +
> +static DEVICE_ATTR_RO(fw_version);
> +
> +static ssize_t fw_build_read(struct file *filp, struct kobject *kobj,
> +			     struct bin_attribute *bin_attr, char *buf,
> +			     loff_t off, size_t count)
> +{
> +	struct catpt_dev *cdev = dev_get_drvdata(kobj_to_dev(kobj));
> +	struct catpt_fw_version version;
> +	int ret;
> +
> +	pm_runtime_get_sync(cdev->dev);
> +
> +	ret = catpt_ipc_get_fw_version(cdev, &version);
> +
> +	pm_runtime_mark_last_busy(cdev->dev);
> +	pm_runtime_put_autosuspend(cdev->dev);
> +
> +	if (ret)
> +		return CATPT_IPC_ERROR(ret);
> +
> +	memcpy(buf, &version, sizeof(version));
> +	return count;
> +}
> +
> +static BIN_ATTR_RO(fw_build, sizeof(struct catpt_fw_version));
> +
> +int catpt_sysfs_create(struct catpt_dev *cdev)
> +{
> +	int ret;
> +
> +	ret = sysfs_create_file(&cdev->dev->kobj, &dev_attr_fw_version.attr);
> +	if (ret)
> +		return ret;
> +
> +	ret = sysfs_create_bin_file(&cdev->dev->kobj, &bin_attr_fw_build);
> +	if (ret) {
> +		sysfs_remove_file(&cdev->dev->kobj, &dev_attr_fw_version.attr);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +void catpt_sysfs_remove(struct catpt_dev *cdev)
> +{
> +	sysfs_remove_bin_file(&cdev->dev->kobj, &bin_attr_fw_build);
> +	sysfs_remove_file(&cdev->dev->kobj, &dev_attr_fw_version.attr);
> +}
>
Greg KH Sept. 19, 2020, 2:42 p.m. UTC | #2
On Fri, Sep 18, 2020 at 03:22:13PM +0000, Rojewski, Cezary wrote:
> On 2020-09-17 4:12 PM, Cezary Rojewski wrote:
> > Add sysfs entries for displaying version of FW currently in use as well
> > as binary dump of entire version info, including build and log providers
> > hashes.
> > 
> > Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
> > ---
> > 
> > Changes in v6:
> > - functions declaration and usage now part of this patch instead of
> >    being separated from it
> > 
> > Changes in v2:
> > - fixed size provided to memcpy() in fw_build_read() as reported by Mark
> > 
> 
> +Greg KH
> 
> Greg, would you mind taking a look at these sysfs entries added for new
> catpt driver (Audio DSP driver for Haswell and Broadwell machines)?

Why me?

> Link to opening post for the series:
> [PATCH v6 00/14] ASoC: Intel: Catpt - Lynx and Wildcat point
> https://www.spinics.net/lists/alsa-devel/msg115765.html

Does lore.kernel.org handle alsa-devel yet?

> 
> Let me give you a quick introduction to the catpt's fs code:
> During power-up sequence a handshake is made between host (kernel device
> driver) and DSP (firmware) side. Two sysfs entries are generated which
> expose running DSP firmware version and its build info - information
> obtained during said handshake.
> 
> Much like devices (such as those of PCI-type) expose sysfs entries for
> their easy identification, catpt provides entries to identify DSP FW it
> is dealing with.

No Documentation/ABI/ entry for these new devices explaining what they
do and are for?  That would be a good first step, and has always been a
requirement for sysfs files.  Do that and resend the series and cc: me
and ask for my review and I will be glad to give it.

Oh, a few notes below:

> >   sound/soc/intel/catpt/core.h   |  3 ++
> >   sound/soc/intel/catpt/device.c |  6 +++
> >   sound/soc/intel/catpt/fs.c     | 79 ++++++++++++++++++++++++++++++++++
> >   3 files changed, 88 insertions(+)
> >   create mode 100644 sound/soc/intel/catpt/fs.c
> > 
> > diff --git a/sound/soc/intel/catpt/core.h b/sound/soc/intel/catpt/core.h
> > index a29b4c0232cb..1f0f1ac92341 100644
> > --- a/sound/soc/intel/catpt/core.h
> > +++ b/sound/soc/intel/catpt/core.h
> > @@ -155,6 +155,9 @@ int catpt_store_module_states(struct catpt_dev *cdev, struct dma_chan *chan);
> >   int catpt_store_memdumps(struct catpt_dev *cdev, struct dma_chan *chan);
> >   int catpt_coredump(struct catpt_dev *cdev);
> >   
> > +int catpt_sysfs_create(struct catpt_dev *cdev);
> > +void catpt_sysfs_remove(struct catpt_dev *cdev);
> > +
> >   #include <sound/memalloc.h>
> >   #include <uapi/sound/asound.h>
> >   
> > diff --git a/sound/soc/intel/catpt/device.c b/sound/soc/intel/catpt/device.c
> > index 7c7ddbabaf55..e9b7c1f474e0 100644
> > --- a/sound/soc/intel/catpt/device.c
> > +++ b/sound/soc/intel/catpt/device.c
> > @@ -184,6 +184,10 @@ static int catpt_probe_components(struct catpt_dev *cdev)
> >   		goto board_err;
> >   	}
> >   
> > +	ret = catpt_sysfs_create(cdev);
> > +	if (ret)
> > +		goto board_err;

Why are you calling a specific function to do all of this?  Why not
provide a default_groups pointer which allows the driver core to
automatically create/destroy the sysfs files for you in a race-free
manner with userspace?

That's the recommended way, you should never have to manually create
files.


> > +
> >   	/* reflect actual ADSP state in pm_runtime */
> >   	pm_runtime_set_active(cdev->dev);
> >   
> > @@ -292,6 +296,8 @@ static int catpt_acpi_remove(struct platform_device *pdev)
> >   	catpt_sram_free(&cdev->iram);
> >   	catpt_sram_free(&cdev->dram);
> >   
> > +	catpt_sysfs_remove(cdev);
> > +
> >   	return 0;
> >   }
> >   
> > diff --git a/sound/soc/intel/catpt/fs.c b/sound/soc/intel/catpt/fs.c
> > new file mode 100644
> > index 000000000000..d73493687f4a
> > --- /dev/null
> > +++ b/sound/soc/intel/catpt/fs.c
> > @@ -0,0 +1,79 @@
> > +// SPDX-License-Identifier: GPL-2.0-pcm

What is "GPL-2.0-pcm" for a SPDX identifier?  We don't have that listed
in LICENSES, do we?

Did this pass the spdxcheck tool in scripts?

thanks,

greg k-h
Cezary Rojewski Sept. 20, 2020, 5:03 p.m. UTC | #3
On 2020-09-19 4:42 PM, gregkh@linuxfoundation.org wrote:
> On Fri, Sep 18, 2020 at 03:22:13PM +0000, Rojewski, Cezary wrote:
>> On 2020-09-17 4:12 PM, Cezary Rojewski wrote:
>>> Add sysfs entries for displaying version of FW currently in use as well
>>> as binary dump of entire version info, including build and log providers
>>> hashes.
>>>
>>> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
>>> ---
>>>
>>> Changes in v6:
>>> - functions declaration and usage now part of this patch instead of
>>>     being separated from it
>>>
>>> Changes in v2:
>>> - fixed size provided to memcpy() in fw_build_read() as reported by Mark
>>>
>>
>> +Greg KH
>>
>> Greg, would you mind taking a look at these sysfs entries added for new
>> catpt driver (Audio DSP driver for Haswell and Broadwell machines)?
> 
> Why me?
> 

Andy (CC'ed) suggested that it's best if sysfs code is routed through you.
Given your input, I believe he was right.

>> Link to opening post for the series:
>> [PATCH v6 00/14] ASoC: Intel: Catpt - Lynx and Wildcat point
>> https://www.spinics.net/lists/alsa-devel/msg115765.html
> 
> Does lore.kernel.org handle alsa-devel yet?
> 

Believe it does as alsa-devel archive can be found on lore.kernel.org.
Not really the guy to answer integration questions, though.

>>
>> Let me give you a quick introduction to the catpt's fs code:
>> During power-up sequence a handshake is made between host (kernel device
>> driver) and DSP (firmware) side. Two sysfs entries are generated which
>> expose running DSP firmware version and its build info - information
>> obtained during said handshake.
>>
>> Much like devices (such as those of PCI-type) expose sysfs entries for
>> their easy identification, catpt provides entries to identify DSP FW it
>> is dealing with.
> 
> No Documentation/ABI/ entry for these new devices explaining what they
> do and are for?  That would be a good first step, and has always been a
> requirement for sysfs files.  Do that and resend the series and cc: me
> and ask for my review and I will be glad to give it.
> 
> Oh, a few notes below:
> 

Well, that's just one device driver targeting basically single device
available in two flavors. Lack of Documentation/ABI/<sysfs-doc> for
solution has been noted though. Will add in v7. As this device is
available on /sys/bus/pci0000:00/<dev> is the name for upcoming file:
sysfs-bus-pci-devices-catpt ok? Or, would you prevent a different, more
explicit one?

>>>    sound/soc/intel/catpt/core.h   |  3 ++
>>>    sound/soc/intel/catpt/device.c |  6 +++
>>>    sound/soc/intel/catpt/fs.c     | 79 ++++++++++++++++++++++++++++++++++
>>>    3 files changed, 88 insertions(+)
>>>    create mode 100644 sound/soc/intel/catpt/fs.c
>>>
>>> diff --git a/sound/soc/intel/catpt/core.h b/sound/soc/intel/catpt/core.h
>>> index a29b4c0232cb..1f0f1ac92341 100644
>>> --- a/sound/soc/intel/catpt/core.h
>>> +++ b/sound/soc/intel/catpt/core.h
>>> @@ -155,6 +155,9 @@ int catpt_store_module_states(struct catpt_dev *cdev, struct dma_chan *chan);
>>>    int catpt_store_memdumps(struct catpt_dev *cdev, struct dma_chan *chan);
>>>    int catpt_coredump(struct catpt_dev *cdev);
>>>    
>>> +int catpt_sysfs_create(struct catpt_dev *cdev);
>>> +void catpt_sysfs_remove(struct catpt_dev *cdev);
>>> +
>>>    #include <sound/memalloc.h>
>>>    #include <uapi/sound/asound.h>
>>>    
>>> diff --git a/sound/soc/intel/catpt/device.c b/sound/soc/intel/catpt/device.c
>>> index 7c7ddbabaf55..e9b7c1f474e0 100644
>>> --- a/sound/soc/intel/catpt/device.c
>>> +++ b/sound/soc/intel/catpt/device.c
>>> @@ -184,6 +184,10 @@ static int catpt_probe_components(struct catpt_dev *cdev)
>>>    		goto board_err;
>>>    	}
>>>    
>>> +	ret = catpt_sysfs_create(cdev);
>>> +	if (ret)
>>> +		goto board_err;
> 
> Why are you calling a specific function to do all of this?  Why not
> provide a default_groups pointer which allows the driver core to
> automatically create/destroy the sysfs files for you in a race-free
> manner with userspace?
> 
> That's the recommended way, you should never have to manually create
> files.
> 
> 

Thanks, that's something new. As this is simple device-driver, I believe
you meant usage of sysfs_(add|remove)_group() or their "device"
equivalents: device_(add|remove)_group(), is that correct? Haven't found
any usage of default_group within /sound/ subsystem what cannot be said
about the functions I've just mentioned.

Feel free to correct me if I'm wrong about this.

>>> +
>>>    	/* reflect actual ADSP state in pm_runtime */
>>>    	pm_runtime_set_active(cdev->dev);
>>>    
>>> @@ -292,6 +296,8 @@ static int catpt_acpi_remove(struct platform_device *pdev)
>>>    	catpt_sram_free(&cdev->iram);
>>>    	catpt_sram_free(&cdev->dram);
>>>    
>>> +	catpt_sysfs_remove(cdev);
>>> +
>>>    	return 0;
>>>    }
>>>    
>>> diff --git a/sound/soc/intel/catpt/fs.c b/sound/soc/intel/catpt/fs.c
>>> new file mode 100644
>>> index 000000000000..d73493687f4a
>>> --- /dev/null
>>> +++ b/sound/soc/intel/catpt/fs.c
>>> @@ -0,0 +1,79 @@
>>> +// SPDX-License-Identifier: GPL-2.0-pcm
> 
> What is "GPL-2.0-pcm" for a SPDX identifier?  We don't have that listed
> in LICENSES, do we?
> 
> Did this pass the spdxcheck tool in scripts?
> 
> thanks,
> 
> greg k-h
> 

Well, after s/pcm/only/ is does : )
Mark already mentioned this mistake, my bad for letting it into v6..

Thanks for your input, much appreciated.

Czarek
Andy Shevchenko Sept. 21, 2020, 8:09 a.m. UTC | #4
On Sun, Sep 20, 2020 at 05:03:00PM +0000, Rojewski, Cezary wrote:
> On 2020-09-19 4:42 PM, gregkh@linuxfoundation.org wrote:
> > On Fri, Sep 18, 2020 at 03:22:13PM +0000, Rojewski, Cezary wrote:
> >> On 2020-09-17 4:12 PM, Cezary Rojewski wrote:
> >>> Add sysfs entries for displaying version of FW currently in use as well
> >>> as binary dump of entire version info, including build and log providers
> >>> hashes.

...

> >> Greg, would you mind taking a look at these sysfs entries added for new
> >> catpt driver (Audio DSP driver for Haswell and Broadwell machines)?
> > 
> > Why me?
> > 
> 
> Andy (CC'ed) suggested that it's best if sysfs code is routed through you.
> Given your input, I believe he was right.

'routed' probably is not what I meant, rather 'reviewed'. Because I remember
that sysfs got new support for attributes thru certain members of the driver
structure. Also I'm not sure I know the policy about binary attributes (it's
possible that my knowledge is interfering with sysctl binary attributes that
are no-no nowadays).

Anyway, thanks for looking into this!

> >> Link to opening post for the series:
> >> [PATCH v6 00/14] ASoC: Intel: Catpt - Lynx and Wildcat point
> >> https://www.spinics.net/lists/alsa-devel/msg115765.html
> > 
> > Does lore.kernel.org handle alsa-devel yet?
> > 
> 
> Believe it does as alsa-devel archive can be found on lore.kernel.org.
> Not really the guy to answer integration questions, though.

...

> >>> +	ret = catpt_sysfs_create(cdev);
> >>> +	if (ret)
> >>> +		goto board_err;
> > 
> > Why are you calling a specific function to do all of this?  Why not
> > provide a default_groups pointer which allows the driver core to
> > automatically create/destroy the sysfs files for you in a race-free
> > manner with userspace?
> > 
> > That's the recommended way, you should never have to manually create
> > files.
> 
> Thanks, that's something new. As this is simple device-driver, I believe
> you meant usage of sysfs_(add|remove)_group() or their "device"
> equivalents: device_(add|remove)_group(), is that correct? Haven't found
> any usage of default_group within /sound/ subsystem what cannot be said
> about the functions I've just mentioned.
> 
> Feel free to correct me if I'm wrong about this.

It's a member of the struct device_driver, if I'm not mistaken.
diff mbox series

Patch

diff --git a/sound/soc/intel/catpt/core.h b/sound/soc/intel/catpt/core.h
index a29b4c0232cb..1f0f1ac92341 100644
--- a/sound/soc/intel/catpt/core.h
+++ b/sound/soc/intel/catpt/core.h
@@ -155,6 +155,9 @@  int catpt_store_module_states(struct catpt_dev *cdev, struct dma_chan *chan);
 int catpt_store_memdumps(struct catpt_dev *cdev, struct dma_chan *chan);
 int catpt_coredump(struct catpt_dev *cdev);
 
+int catpt_sysfs_create(struct catpt_dev *cdev);
+void catpt_sysfs_remove(struct catpt_dev *cdev);
+
 #include <sound/memalloc.h>
 #include <uapi/sound/asound.h>
 
diff --git a/sound/soc/intel/catpt/device.c b/sound/soc/intel/catpt/device.c
index 7c7ddbabaf55..e9b7c1f474e0 100644
--- a/sound/soc/intel/catpt/device.c
+++ b/sound/soc/intel/catpt/device.c
@@ -184,6 +184,10 @@  static int catpt_probe_components(struct catpt_dev *cdev)
 		goto board_err;
 	}
 
+	ret = catpt_sysfs_create(cdev);
+	if (ret)
+		goto board_err;
+
 	/* reflect actual ADSP state in pm_runtime */
 	pm_runtime_set_active(cdev->dev);
 
@@ -292,6 +296,8 @@  static int catpt_acpi_remove(struct platform_device *pdev)
 	catpt_sram_free(&cdev->iram);
 	catpt_sram_free(&cdev->dram);
 
+	catpt_sysfs_remove(cdev);
+
 	return 0;
 }
 
diff --git a/sound/soc/intel/catpt/fs.c b/sound/soc/intel/catpt/fs.c
new file mode 100644
index 000000000000..d73493687f4a
--- /dev/null
+++ b/sound/soc/intel/catpt/fs.c
@@ -0,0 +1,79 @@ 
+// SPDX-License-Identifier: GPL-2.0-pcm
+//
+// Copyright(c) 2020 Intel Corporation. All rights reserved.
+//
+// Author: Cezary Rojewski <cezary.rojewski@intel.com>
+//
+
+#include <linux/pm_runtime.h>
+#include "core.h"
+
+static ssize_t fw_version_show(struct device *dev,
+			       struct device_attribute *attr, char *buf)
+{
+	struct catpt_dev *cdev = dev_get_drvdata(dev);
+	struct catpt_fw_version version;
+	int ret;
+
+	pm_runtime_get_sync(cdev->dev);
+
+	ret = catpt_ipc_get_fw_version(cdev, &version);
+
+	pm_runtime_mark_last_busy(cdev->dev);
+	pm_runtime_put_autosuspend(cdev->dev);
+
+	if (ret)
+		return CATPT_IPC_ERROR(ret);
+
+	return sprintf(buf, "%d.%d.%d.%d\n", version.type, version.major,
+		       version.minor, version.build);
+}
+
+static DEVICE_ATTR_RO(fw_version);
+
+static ssize_t fw_build_read(struct file *filp, struct kobject *kobj,
+			     struct bin_attribute *bin_attr, char *buf,
+			     loff_t off, size_t count)
+{
+	struct catpt_dev *cdev = dev_get_drvdata(kobj_to_dev(kobj));
+	struct catpt_fw_version version;
+	int ret;
+
+	pm_runtime_get_sync(cdev->dev);
+
+	ret = catpt_ipc_get_fw_version(cdev, &version);
+
+	pm_runtime_mark_last_busy(cdev->dev);
+	pm_runtime_put_autosuspend(cdev->dev);
+
+	if (ret)
+		return CATPT_IPC_ERROR(ret);
+
+	memcpy(buf, &version, sizeof(version));
+	return count;
+}
+
+static BIN_ATTR_RO(fw_build, sizeof(struct catpt_fw_version));
+
+int catpt_sysfs_create(struct catpt_dev *cdev)
+{
+	int ret;
+
+	ret = sysfs_create_file(&cdev->dev->kobj, &dev_attr_fw_version.attr);
+	if (ret)
+		return ret;
+
+	ret = sysfs_create_bin_file(&cdev->dev->kobj, &bin_attr_fw_build);
+	if (ret) {
+		sysfs_remove_file(&cdev->dev->kobj, &dev_attr_fw_version.attr);
+		return ret;
+	}
+
+	return 0;
+}
+
+void catpt_sysfs_remove(struct catpt_dev *cdev)
+{
+	sysfs_remove_bin_file(&cdev->dev->kobj, &bin_attr_fw_build);
+	sysfs_remove_file(&cdev->dev->kobj, &dev_attr_fw_version.attr);
+}