RE: [PATCH v5 01/13] ASoC: Intel: Add catpt device

["Rojewski, Cezary" <cezary.rojewski@intel.com>]

> On Wed, Sep 16, 2020 at 06:30:27PM +0000, Rojewski, Cezary wrote:
> > > On Wed, Sep 16, 2020 at 06:24:56PM +0300, Andy Shevchenko wrote:
> > > > On Tue, Sep 15, 2020 at 06:29:32PM +0200, Cezary Rojewski wrote:
> > > > > Declare base structures, registers and device routines for the catpt
> > > > > solution. Catpt deprecates and is a direct replacement for
> > > > > sound/soc/intel/haswell. Supports Lynxpoint and Wildcat Point both.
> > > >
> > > > Few nit-picks below. Overall looks good, FWIW,
> > > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > >
> > > Actually hold on. See below.
> > >
> > > > > +void catpt_sram_init(struct resource *sram, u32 start, u32 size);
> > > > > +void catpt_sram_free(struct resource *sram);
> > > > > +struct resource *
> > > > > +catpt_request_region(struct resource *root, resource_size_t size);
> > >
> > > These seems dangling declarations that has to be moved to the
> > > corresponding
> > > patch. Please, revisit entire series to be sure that:
> > >
> > > - each patch doesn't add any warnings on W=1
> > > - each patch doesn't have dangling stuff
> > > - each patch is bisectable for compilation and run-time
> > >
> >
> > TLDR: you want patches:
> > 6/13 ASoC: Intel: catpt: PCM operations
> > 5/13 ASoC: Intel: catpt: Add IPC messages
> > 4/13 ASoC: Intel: catpt: Implement IPC protocol
> > 3/13 ASoC: Intel: catpt: Firmware loading and context restore
> > 2/13 ASoC: Intel: catpt: Define DSP operations
> > 1/13 ASoC: Intel: Add catpt device
> >
> > squashed. There is no other way to achieve that without combining
> > all the core-code together. fs and traces can be provided separately,
> > but not the first 6.
> 
> No. My point is introduce header (declaration) with definition (c-file)
> together. Like those three of four functions.
> 

Problem is that all of these are intertwined. I'll end up creating
patches which will be constantly updating files added by the opening
patch e.g.: core.h. Even if I'm to let's say, separate "just" pcm
operations, then the following:

int catpt_register_plat_component(struct catpt_dev *cdev);
void catpt_stream_update_position(struct catpt_dev *cdev,
                                  struct catpt_stream_runtime *stream,
                                  struct catpt_notify_position *pos);
struct catpt_stream_runtime *
catpt_stream_find(struct catpt_dev *cdev, u8 stream_hw_id);
int catpt_arm_stream_templates(struct catpt_dev *cdev);

have to removed initially from core.h. These, however, are part of
standard ipc flow and device probing. In consequence I'll have to update
the programming flow later on for these files.

I'll dig again and see what and where could be split.
Hope I'm wrong and the results won't be as ugly as I think they're
going to be.

Thanks for your input Andy, once again!
Czarek