Re: [PATCH v1 2/4] ASoC: simple-card-utils: create jack inputs for aux_devs

[Astrid Rost]

On 1/18/23 14:39, Amadeusz Sławiński wrote:
> On 1/18/2023 1:52 PM, Astrid Rost wrote:
> > Add a generic way to create jack inputs for auxiliary jack detection
> > drivers (e.g. via i2c, spi), which are not part of any real codec.
> > The simple-card can be used as combining card driver to add the jacks,
> > no new one is required.
> >
> > Create a jack (for input-events) for jack devices in the auxiliary
> > device list (aux_devs). A device which has the functions set_jack and
> > get_jack_supported_type counts as jack device.
> >
> > Add a devicetree entry, in case not all input types are required.
> >   simple-card,aux-jack-types:
> > Array of snd_jack_type to create jack-input-event for jack devices in
> > aux-devs. If the setting is 0, the supported type of the device is used.
> >
> > Signed-off-by: Astrid Rost <astrid.rost@xxxxxxxx>
> > ---
> >   include/sound/simple_card_utils.h     |  3 ++
> >   sound/soc/generic/simple-card-utils.c | 63 +++++++++++++++++++++++++++
> >   sound/soc/generic/simple-card.c       |  4 ++
> >   3 files changed, 70 insertions(+)
> >
> > diff --git a/include/sound/simple_card_utils.h 
> > b/include/sound/simple_card_utils.h
> > index 38590f1ae9ee..a3f3f3aa9e6e 100644
> > --- a/include/sound/simple_card_utils.h
> > +++ b/include/sound/simple_card_utils.h
> > @@ -69,6 +69,7 @@ struct asoc_simple_priv {
> >       } *dai_props;
> >       struct asoc_simple_jack hp_jack;
> >       struct asoc_simple_jack mic_jack;
> > +    struct snd_soc_jack *aux_jacks;
> >       struct snd_soc_dai_link *dai_link;
> >       struct asoc_simple_dai *dais;
> >       struct snd_soc_dai_link_component *dlcs;
> > @@ -187,6 +188,8 @@ int asoc_simple_parse_pin_switches(struct 
> > snd_soc_card *card,
> >   int asoc_simple_init_jack(struct snd_soc_card *card,
> >                      struct asoc_simple_jack *sjack,
> >                      int is_hp, char *prefix, char *pin);
> > +int asoc_simple_init_aux_jacks(struct asoc_simple_priv *priv,
> > +                char *prefix);
> >   int asoc_simple_init_priv(struct asoc_simple_priv *priv,
> >                      struct link_info *li);
> >   int asoc_simple_remove(struct platform_device *pdev);
> > diff --git a/sound/soc/generic/simple-card-utils.c 
> > b/sound/soc/generic/simple-card-utils.c
> > index e35becce9635..668123549fbf 100644
> > --- a/sound/soc/generic/simple-card-utils.c
> > +++ b/sound/soc/generic/simple-card-utils.c
> > @@ -786,6 +786,69 @@ int asoc_simple_init_jack(struct snd_soc_card *card,
> >   }
> >   EXPORT_SYMBOL_GPL(asoc_simple_init_jack);
> > +int asoc_simple_init_aux_jacks(struct asoc_simple_priv *priv, char 
> > *prefix)
> > +{
> > +    struct snd_soc_card *card = simple_priv_to_card(priv);
> > +    struct device *dev = card->dev;
> > +    struct snd_soc_component *component;
> > +    char prop[128];
> > +    int found_jack_index = 0;
> > +    int num = 0;
> > +    int ret;
> > +
> > +    if (priv->aux_jacks)
> > +        return 0;
> > +
> > +    snprintf(prop, sizeof(prop), "%saux-jack-types", prefix);
> > +    num = of_property_count_u32_elems(dev->of_node, prop);
> > +    if (num < 1)
> > +        return 0;
> > +
> > +    priv->aux_jacks = devm_kcalloc(card->dev, num,
> > +                       sizeof(struct snd_soc_jack), GFP_KERNEL);
> > +    if (!priv->aux_jacks)
> > +        return -ENOMEM;
> > +
> > +    for_each_card_auxs(card, component) {
> > +        if (found_jack_index >= num)
> > +            break;
> > +
> > +        if (component->driver->set_jack &&
> > +            component->driver->get_jack_supported_type) {
>
> This check is really weird, you are checking if callbacks exist at all, 
> which should be unnecessary as it duplicates the work oh 
> snd_soc_component_ functions. I would just try to call 
> snd_soc_component_get_jack_supported_type() directly and if it returns 
> -ENOTSUPP, just go to next iteration.
> I guess your main problem is snd_soc_component_set_jack(), but you can 
> just call it with NULL jack to check if it returns -ENOTSUPP, but I 
> guess the overall asumption would be that if someone implements 
> .get_jack_supported_type, they also implemented .set_jack, so maybe it 
> is just unnecessary?

Thank you!
Yes, it works fine without it. I will remove it.

> > +            char id[128];
> > +            int type = 0;
> > +            struct snd_soc_jack *jack =
> > +                &(priv->aux_jacks[found_jack_index]);
> > +            int type_supp_mask =
> > +                snd_soc_component_get_jack_supported_type(
> > +                    component);
> > +
> > +            ret = of_property_read_u32_index(
> > +                dev->of_node, prop, found_jack_index++, &type);
> > +            if (ret)
> > +                continue;
> > +
> > +            if (type)
> > +                type &= type_supp_mask; /* use only supported types */
> > +            else
> > +                type = type_supp_mask; /* use all supported types */
> > +
> > +            if (!type)
> > +                continue;
> > +
> > +            /* create jack */
> > +            snprintf(id, sizeof(id), "%s-jack", component->name);
> > +            ret = snd_soc_card_jack_new(card, id, type, jack);
> > +            if (ret)
> > +                continue;
> > +
> > +            (void)snd_soc_component_set_jack(component, jack, NULL);
> > +        }
> > +    }
> > +    return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(asoc_simple_init_aux_jacks);
> > +
> >   int asoc_simple_init_priv(struct asoc_simple_priv *priv,
> >                 struct link_info *li)
> >   {
> > diff --git a/sound/soc/generic/simple-card.c 
> > b/sound/soc/generic/simple-card.c
> > index feb55b66239b..e98932c16754 100644
> > --- a/sound/soc/generic/simple-card.c
> > +++ b/sound/soc/generic/simple-card.c
> > @@ -623,6 +623,10 @@ static int simple_soc_probe(struct snd_soc_card 
> > *card)
> >       if (ret < 0)
> >           return ret;
> > +    ret = asoc_simple_init_aux_jacks(priv, PREFIX);
> > +    if (ret < 0)
> > +        return ret;
> > +
> >       return 0;
> >   }