Re: [PATCH v4 01/13] ASoC: Intel: Add catpt device

From: Andy Shevchenko <andriy.shevchenko@intel.com>
To: Cezary Rojewski <cezary.rojewski@intel.com>
Cc: pierre-louis.bossart@linux.intel.com,
	alsa-devel@alsa-project.org, filip.kaczmarski@intel.com,
	harshapriya.n@intel.com, marcin.barlik@intel.com,
	zwisler@google.com, lgirdwood@gmail.com, tiwai@suse.com,
	filip.proborszcz@intel.com, broonie@kernel.org,
	amadeuszx.slawinski@linux.intel.com, michal.wasko@intel.com,
	cujomalainey@chromium.org, krzysztof.hejmowski@intel.com,
	ppapierkowski@habana.ai, vamshi.krishna.gopal@intel.com
Subject: Re: [PATCH v4 01/13] ASoC: Intel: Add catpt device
Date: Wed, 19 Aug 2020 16:43:23 +0300	[thread overview]
Message-ID: <20200819134323.GM1891694@smile.fi.intel.com> (raw)
In-Reply-To: <7cd5fa73-797e-17c3-4b7c-7635a18c59af@intel.com>

On Wed, Aug 19, 2020 at 03:26:04PM +0200, Cezary Rojewski wrote:
> On 2020-08-18 12:07 PM, Andy Shevchenko wrote:
> > On Mon, Aug 17, 2020 at 12:02:39PM +0200, Cezary Rojewski wrote:
> > > On 2020-08-13 8:29 PM, Andy Shevchenko wrote:
> > > > On Wed, Aug 12, 2020 at 10:57:41PM +0200, Cezary Rojewski wrote:

...

> And this is a very good approach. Thought device_get_match_data is 'heavier'
> than the devm_kzalloc.

Memory allocation in 90% cases are heavier then the rest (because of page fault
exceptions).

...

> I did some testing today and indeed very simple approach suffices:
> - devm_platform_get_and_ioremap_resource for DSP bar
> - devm_platform_ioremap_resource for PCI bar
> - instead of doing devm_ioremap_resource() separately for dmac in
> catpt_dmac_probe(), just do:
> 
> dmac->regs = cdev->lpe_ba + cdev->spec->host_dma_offset[<engine id>]

Yes, something like this. Basically what you have is particular case of MFD
(see drivers/mfd for other uses).

...

> > > > > +#define CATPT_SSP_SSC0		0x0
> > > > > +#define CATPT_SSP_SSC1		0x4
> > > > > +#define CATPT_SSP_SSS		0x8
> > > > > +#define CATPT_SSP_SSIT		0xC
> > > > > +#define CATPT_SSP_SSD		0x10 and
> > > > > +#define CATPT_SSP_SSTO		0x28
> > > > > +#define CATPT_SSP_SSPSP		0x2C
> > > > > +#define CATPT_SSP_SSTSA		0x30
> > > > > +#define CATPT_SSP_SSRSA		0x34
> > > > > +#define CATPT_SSP_SSTSS		0x38
> > > > > +#define CATPT_SSP_SSC2		0x40
> > > > > +#define CATPT_SSP_SSPSP2	0x44
> > > > 
> > > > Isn't it PXA2xx register set? Can you use their definitions?
> > > 
> > > Could you be more specific? Wasn't able to find anything useful in /include.
> > 
> > include/linux/pxa2xx_ssp.h
> 
> Did some reconnaissance and it while this registers are shared, LPT/WPT are
> equipped with a newer version of said ssp device with some old
> functionalities have been removed too. So the question comes down to: do I
> re-use PXA2XX registers due to historical background (inheritance) -or-
> leave it explicit as is. Honestly, I don't really mind either of these. Got
> surprised by short register names in mentioned header though.

Short names are for historical reasons, but you may change them over the
kernel, if you wish (I think you won't spend time on this).

My vision is to extend that header to cover changes and use it in your code.
It might, though, require some cleanups to be done against pxa2xx_ssp.h.

-- 
With Best Regards,
Andy Shevchenko