Re: [PATCH v4 4/4] Input: cs40l50 - Add support for the CS40L50 haptic driver

[Jeff LaBundy]

Hi James,

On Wed, Nov 01, 2023 at 08:47:07PM +0000, James Ogletree wrote:
> Hi Jeff,
> 
> > On Oct 24, 2023, at 10:03 PM, Jeff LaBundy <jeff@xxxxxxxxxxx> wrote:
> >> 
> >> +static const struct cs_dsp_client_ops cs40l50_cs_dsp_client_ops;
> >> +
> >> +static const struct cs_dsp_region cs40l50_dsp_regions[] = {
> >> + {
> >> + .type = WMFW_HALO_PM_PACKED,
> >> + .base = CS40L50_DSP1_PMEM_0
> >> + },
> >> + {
> >> + .type = WMFW_HALO_XM_PACKED,
> >> + .base = CS40L50_DSP1_XMEM_PACKED_0
> >> + },
> >> + {
> >> + .type = WMFW_HALO_YM_PACKED,
> >> + .base = CS40L50_DSP1_YMEM_PACKED_0
> >> + },
> >> + {
> >> + .type = WMFW_ADSP2_XM,
> >> + .base = CS40L50_DSP1_XMEM_UNPACKED24_0
> >> + },
> >> + {
> >> + .type = WMFW_ADSP2_YM,
> >> + .base = CS40L50_DSP1_YMEM_UNPACKED24_0
> >> + },
> >> +};
> >> +
> >> +static int cs40l50_cs_dsp_init(struct cs40l50_private *cs40l50)
> >> +{
> >> + cs40l50->dsp.num = 1;
> >> + cs40l50->dsp.type = WMFW_HALO;
> >> + cs40l50->dsp.dev = cs40l50->dev;
> >> + cs40l50->dsp.regmap = cs40l50->regmap;
> >> + cs40l50->dsp.base = CS40L50_CORE_BASE;
> >> + cs40l50->dsp.base_sysinfo = CS40L50_SYS_INFO_ID;
> >> + cs40l50->dsp.mem = cs40l50_dsp_regions;
> >> + cs40l50->dsp.num_mems = ARRAY_SIZE(cs40l50_dsp_regions);
> >> + cs40l50->dsp.no_core_startstop = true;
> >> + cs40l50->dsp.client_ops = &cs40l50_cs_dsp_client_ops;
> >> +
> >> + return cs_dsp_halo_init(&cs40l50->dsp);
> >> +}
> >> +
> >> +static struct cs_hap_bank cs40l50_banks[] = {
> >> + {
> >> + .bank = WVFRM_BANK_RAM,
> >> + .base_index = CS40L50_RAM_BANK_INDEX_START,
> >> + .max_index = CS40L50_RAM_BANK_INDEX_START,
> >> + },
> >> + {
> >> + .bank = WVFRM_BANK_ROM,
> >> + .base_index = CS40L50_ROM_BANK_INDEX_START,
> >> + .max_index = CS40L50_ROM_BANK_INDEX_END,
> >> + },
> >> + {
> >> + .bank = WVFRM_BANK_OWT,
> >> + .base_index = CS40L50_RTH_INDEX_START,
> >> + .max_index = CS40L50_RTH_INDEX_END,
> >> + },
> >> +};
> > 
> > These structs describe the DSP, and hence the silicon; they are not
> > specific to the input/FF device. Presumably the DSP could run algorithms
> > that support only the I2S streaming case as well (e.g. A2H); therefore,
> > these seem more appropriately placed in the MFD.
> 
> Acknowledged, but would you consider the last struct “cs40l50_banks” as
> an exception? It would go unused in a codec-only setup.

I agree with you; I should have inserted my comment after cs40l50_cs_dsp_init()
and not cs40l50_banks[].

> 
> Best,
> James
> 
> 

Kind regards,
Jeff LaBundy