On 2020-09-19 4:42 PM, gregkh@xxxxxxxxxxxxxxxxxxx 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@xxxxxxxxx> >>> --- >>> >>> 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
