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

From: "Rojewski, Cezary" <cezary.rojewski@intel.com>
To: "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>
Cc: "pierre-louis.bossart@linux.intel.com"
	<pierre-louis.bossart@linux.intel.com>,
	"alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>,
	"andriy.shevchenko@linux.intel.com"
	<andriy.shevchenko@linux.intel.com>,
	"Kaczmarski, Filip" <filip.kaczmarski@intel.com>,
	"N, Harshapriya" <harshapriya.n@intel.com>,
	"Barlik, Marcin" <marcin.barlik@intel.com>,
	"zwisler@google.com" <zwisler@google.com>,
	"lgirdwood@gmail.com" <lgirdwood@gmail.com>,
	"tiwai@suse.com" <tiwai@suse.com>,
	"Proborszcz, Filip" <filip.proborszcz@intel.com>,
	"broonie@kernel.org" <broonie@kernel.org>,
	"amadeuszx.slawinski@linux.intel.com"
	<amadeuszx.slawinski@linux.intel.com>,
	"Wasko, Michal" <michal.wasko@intel.com>,
	"cujomalainey@chromium.org" <cujomalainey@chromium.org>,
	"Hejmowski, Krzysztof" <krzysztof.hejmowski@intel.com>,
	"Papierkowski, Piotr \(Habana\)" <ppapierkowski@habana.ai>,
	"Gopal,  Vamshi Krishna" <vamshi.krishna.gopal@intel.com>
Subject: RE: [PATCH v6 09/14] ASoC: Intel: catpt: Simple sysfs attributes
Date: Mon, 21 Sep 2020 21:13:04 +0000	[thread overview]
Message-ID: <8c5e5cec31aa4fefb6efa63a3caace0a@intel.com> (raw)
In-Reply-To: <20200921083514.GA3151537@kroah.com>

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