| Message ID | 20200917141242.9081-10-cezary.rojewski@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ASoC: Intel: Catpt - Lynx and Wildcat point | expand |
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); > +} >
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
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
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.
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. > >>> > >>> 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? Why are you putting random driver-specific attributes on a device owned by a different bus? That can cause problems if you are not careful. Does the SoC core not provide you with a sound device to do this for instead? > > >>> 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. The bus should provide you with the ability to do this, so look into that for your driver. But why are you creating a binary sysfs file? That's only for passing data raw through the kernel to/from the hardware, with no parsing/massaging possible. Is that what is happening here? I guess if there was documentation, it would be easier to review :) thanks, greg k-h
On 2020-09-21 10:35 AM, gregkh@linuxfoundation.org wrote: > 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: ... >> >> 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? > > Why are you putting random driver-specific attributes on a device owned > by a different bus? That can cause problems if you are not careful. > > Does the SoC core not provide you with a sound device to do this for > instead? > I know not about any device that ASoC core provides for me for such tasks. And the confusion about ADSP device location stems from incomplete description coming from ACPI tables. I've had a quite lengthy discussion about this with Andy. Re: [PATCH v4 02/13] ASoC: Intel: catpt: Define DSP operations https://www.spinics.net/lists/alsa-devel/msg114651.html >> ... >>> >>> 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. > > The bus should provide you with the ability to do this, so look into > that for your driver. > > But why are you creating a binary sysfs file? That's only for passing > data raw through the kernel to/from the hardware, with no > parsing/massaging possible. Is that what is happening here? > > I guess if there was documentation, it would be easier to review :) > > thanks, > > greg k-h > In v7 I've removed the binary sysfs file, plus the organization has been simplified - made use of struct device_driver's 'dev_groups' field to automate creation/ removal process of sysfs files as requested. Documentation has too been added so things should be clearer. Thanks, Czarek
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); +}
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