RE: [PATCH] ASoC: rt766: add RT766/RT767 SDCA driver

["Shuming [范書銘]" <shumingf@realtek.com>]

> > +static struct sdca_entity *rt766_find_entity_by_label(struct
> sdca_function_data *func,
> > +     const char *label)
> > +{
> > +     struct sdca_entity *entity = NULL;
> > +     int idx;
> > +
> > +     for (idx = 0; idx < func->num_entities; idx++) {
> > +             entity = &func->entities[idx];
> > +
> > +             if (!strcmp(entity->label, label))
> > +                     return entity;
> > +     }
> > +
> > +     return NULL;
> > +}
> 
> Probably better to export find_sdca_entity_by_label() if you need this from the
> driver.

Thanks for the review. 
I will try to export find_sdca_entity_by_label() and populate_rate_format().

> > +static int rt766_sdca_irq_ctl(struct rt766_sdca_priv *rt766,
> > +                                                       struct
> sdca_function_data *function,
> > +                                                       struct
> snd_soc_component *component,
> > +                                                       struct
> sdca_interrupt_info *info,
> > +                                                       bool
> enabled)
> > +{
> > +     struct device *dev = &rt766->slave->dev;
> > +     struct sdca_interrupt *interrupt;
> > +     struct sdca_control *control;
> > +     struct sdca_entity *entity;
> > +     irq_handler_t handler;
> > +     int i, j, irq, ret;
> > +
> > +     for (i = 0; i < function->num_entities; i++) {
> > +             entity = &function->entities[i];
> > +
> > +             for (j = 0; j < entity->num_controls; j++) {
> > +                     control = &entity->controls[j];
> > +                     irq = control->interrupt_position;
> > +
> > +                     switch (SDCA_CTL_TYPE(entity->type,
> control->sel)) {
> > +                     case SDCA_CTL_TYPE_S(GE, DETECTED_MODE):
> > +                             handler = rt766_sdca_irq_jd_handler;
> > +                             break;
> > +                     case SDCA_CTL_TYPE_S(HIDE,
> HIDTX_CURRENTOWNER):
> > +                             handler = rt766_sdca_irq_btn_handler;
> > +                             break;
> > +                     default:
> > +                             continue;
> > +                     }
> > +
> > +                     interrupt = &info->irqs[irq];
> > +
> > +                     if (enabled) {
> > +                             ret = sdca_irq_data_populate(dev,
> rt766->regmap, component,
> > +
> function, entity, control,
> > +
> interrupt);
> > +                             if (ret)
> > +                                     return ret;
> > +
> > +                             interrupt->priv = rt766;
> > +                             ret = sdca_irq_request(dev, info, irq,
> interrupt->name,
> > +
> handler, interrupt);
> > +                             if (ret) {
> > +                                     dev_err(dev, "failed to
> request irq %s: %d\n",
> > +                                             interrupt->name,
> ret);
> > +                                     return ret;
> > +                             }
> > +                             dev_dbg(dev, "Requesting IRQ %d
> InterruptName=%s\n", irq, interrupt->name);
> > +                     } else {
> > +                             sdca_irq_free(dev, info, irq,
> interrupt->name, interrupt);
> > +                             dev_dbg(dev, "Freeing IRQ %d\n", irq);
> > +                     }
> > +             }
> > +     }
> > +
> > +     return 0;
> > +}
> 
> Hmm... yeah this is moving in a slightly different direction from where I was
> going with the core code. Let me think about that and update tomorrow.

Sorry, I don't quite follow the point.
Do you mean that it will not work with this patch (ASoC: SDCA: Populate IRQ data earlier)?

> > +static int rt766_sdca_pde47_event(struct snd_soc_dapm_widget *w,
> > +     struct snd_kcontrol *kcontrol, int event) {
> > +     struct snd_soc_component *component =
> snd_soc_dapm_to_component(w->dapm);
> > +     struct rt766_sdca_priv *rt766 =
> snd_soc_component_get_drvdata(component);
> > +     const struct sdca_entity *entity = NULL;
> > +     unsigned char ps0 = 0x0, ps3 = 0x3;
> > +     int from_ps, to_ps;
> > +     int ret;
> > +
> > +     switch (event) {
> > +     case SND_SOC_DAPM_POST_PMU:
> > +             regmap_write(rt766->regmap, RT766_PDE_REQ_REG(UAJ,
> PDE47), ps0);
> > +             from_ps = ps3;
> > +             to_ps = ps0;
> > +             break;
> > +     case SND_SOC_DAPM_PRE_PMD:
> > +             regmap_write(rt766->regmap, RT766_PDE_REQ_REG(UAJ,
> PDE47), ps3);
> > +             from_ps = ps0;
> > +             to_ps = ps3;
> > +             break;
> > +     }
> > +
> > +     entity = rt766_find_entity_by_label(rt766->uaj_func_data, "PDE 47");
> > +     ret = sdca_asoc_pde_poll_actual_ps(component->dev,
> rt766->regmap,
> > +                                RT766_FUNC_NUM_UAJ,
> > +                                RT766_SDCA_ENT_PDE47,
> > +                                from_ps, to_ps,
> > +                                entity ? entity->pde.max_delay :
> NULL,
> > +                                entity ?
> entity->pde.num_max_delay : 0);
> > +     if (ret)
> > +             dev_err(component->dev, "%s: PDE transition %x -> %x
> failed, err=%d\n",
> > +                     __func__, from_ps, to_ps, ret);
> 
> Does this actually run in cases where the entity doesn't exist? I would be
> inclined to error out if you didn't find the entity.

Ok, will fix

> > +static int rt766_sdca_pde11_event(struct snd_soc_dapm_widget *w,
> > +     struct snd_kcontrol *kcontrol, int event) {
> > +     struct snd_soc_component *component =
> snd_soc_dapm_to_component(w->dapm);
> > +     struct rt766_sdca_priv *rt766 =
> snd_soc_component_get_drvdata(component);
> > +     const struct sdca_entity *entity = NULL;
> > +     unsigned char ps0 = 0x0, ps3 = 0x3;
> > +     int from_ps, to_ps;
> > +     int ret;
> > +
> > +     switch (event) {
> > +     case SND_SOC_DAPM_POST_PMU:
> > +             regmap_write(rt766->regmap, RT766_PDE_REQ_REG(MIC,
> PDE11), ps0);
> > +             from_ps = ps3;
> > +             to_ps = ps0;
> > +             break;
> > +     case SND_SOC_DAPM_PRE_PMD:
> > +             regmap_write(rt766->regmap, RT766_PDE_REQ_REG(MIC,
> PDE11), ps3);
> > +             from_ps = ps0;
> > +             to_ps = ps3;
> > +             break;
> > +     }
> > +
> > +     entity = rt766_find_entity_by_label(rt766->sm_func_data, "PDE 11");
> > +     ret = sdca_asoc_pde_poll_actual_ps(component->dev,
> rt766->regmap,
> > +                                RT766_FUNC_NUM_MIC,
> > +                                RT766_SDCA_ENT_PDE11,
> > +                                from_ps, to_ps,
> > +                                entity ? entity->pde.max_delay :
> NULL,
> > +                                entity ?
> entity->pde.num_max_delay : 0);
> > +     if (ret)
> > +             dev_err(component->dev, "%s: PDE transition %x -> %x
> failed, err=%d\n",
> > +                     __func__, from_ps, to_ps, ret);
> > +
> > +     return ret;
> > +}
> 
> I feel like lot of these put/get and event helpers would benefit from a
> parameterised helper. You have loads of functions doing the same thing,
> would it be nicer to have a helper that takes the required registers etc.

Sure, will create helper functions for put/get and event.

> > +static int rt766_sdca_pcm_hw_params(struct snd_pcm_substream
> *substream,
> > +                             struct snd_pcm_hw_params *params,
> > +                             struct snd_soc_dai *dai) {
> > +     struct snd_soc_component *component = dai->component;
> > +     struct rt766_sdca_priv *rt766 =
> snd_soc_component_get_drvdata(component);
> > +     struct sdw_stream_config stream_config;
> > +     struct sdw_port_config port_config;
> > +     enum sdw_data_direction direction;
> > +     struct sdw_stream_runtime *sdw_stream;
> > +     int retval, port, num_channels;
> > +     unsigned int sampling_rate;
> > +
> > +     dev_dbg(dai->dev, "%s %s id %d", __func__, dai->name, dai->id);
> > +     sdw_stream = snd_soc_dai_get_dma_data(dai, substream);
> > +
> > +     if (!sdw_stream)
> > +             return -EINVAL;
> > +
> > +     if (!rt766->slave)
> > +             return -EINVAL;
> > +
> > +     /* SoundWire specific configuration */
> > +     if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> > +             direction = SDW_DATA_DIR_RX;
> > +             if (dai->id == RT766_AIF1)
> > +                     port = 3;
> > +             else if (dai->id == RT766_AIF2)
> > +                     port = 1;
> > +             else
> > +                     return -EINVAL;
> > +     } else {
> > +             direction = SDW_DATA_DIR_TX;
> > +             if (dai->id == RT766_AIF1)
> > +                     port = 12;
> > +             else if (dai->id == RT766_AIF3)
> > +                     port = 8;
> > +             else
> > +                     return -EINVAL;
> > +     }
> > +
> > +     stream_config.frame_rate = params_rate(params);
> > +     stream_config.ch_count = params_channels(params);
> > +     stream_config.bps =
> snd_pcm_format_width(params_format(params));
> > +     stream_config.direction = direction;
> > +
> > +     num_channels = params_channels(params);
> > +     port_config.ch_mask = GENMASK(num_channels - 1, 0);
> > +     port_config.num = port;
> 
> Can you use snd_sdw_params_to_config here?

OK, will fix

> > +static int rt766_parse_rates(struct device *dev,
> > +                             struct sdca_function_data *function,
> > +                             struct sdca_entity *entity,
> > +                             unsigned int *out_rates) {
> > +     struct sdca_control_range *range;
> > +     unsigned int sample_rate;
> > +     unsigned int clock_rates = 0;
> > +     unsigned int rates = 0;
> > +     int sel, i;
> > +
> > +     switch (entity->type) {
> > +     case SDCA_ENTITY_TYPE_IT:
> > +             sel = SDCA_CTL_IT_USAGE;
> > +             break;
> > +     case SDCA_ENTITY_TYPE_OT:
> > +             sel = SDCA_CTL_OT_USAGE;
> > +             break;
> > +     default:
> > +             dev_err(dev, "%s: entity type has no usage control\n",
> > +                     entity->label);
> > +             return -EINVAL;
> > +     }
> > +
> > +     if (entity->iot.clock) {
> > +             range = sdca_selector_find_range(dev, entity->iot.clock,
> > +
> SDCA_CTL_CS_SAMPLERATEINDEX,
> > +
> SDCA_SAMPLERATEINDEX_NCOLS, 0);
> > +             if (!range)
> > +                     return -EINVAL;
> > +
> > +             for (i = 0; i < range->rows; i++) {
> > +                     sample_rate = sdca_range(range,
> SDCA_SAMPLERATEINDEX_RATE, i);
> > +                     clock_rates |= rate_find_mask(dev,
> sample_rate);
> > +             }
> > +     } else {
> > +             clock_rates = UINT_MAX;
> > +     }
> > +
> > +     range = sdca_selector_find_range(dev, entity, sel,
> SDCA_USAGE_NCOLS, 0);
> > +     if (!range)
> > +             return -EINVAL;
> > +
> > +     for (i = 0; i < range->rows; i++) {
> > +             sample_rate = sdca_range(range,
> SDCA_USAGE_SAMPLE_RATE, i);
> > +             sample_rate = rate_find_mask(dev, sample_rate);
> > +
> > +             if (sample_rate & clock_rates)
> > +                     rates |= sample_rate;
> > +     }
> > +
> > +     *out_rates = rates;
> > +
> > +     dev_dbg(dev, "%s: entity %s supports rates 0x%08x,
> clock_rates=0x%08x\n",
> > +             __func__, entity->label, rates, clock_rates);
> > +
> > +     return 0;
> > +}
> 
> This is mostly just populate_rate_format(), the values end up in the
> snd_soc_pcm_stream, can we export that and use it here?

Sure, will try to export populate_rate_format() and use it here.