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