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