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

["gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>]

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