| Message ID | 87o6x69h4y.wl-kuninori.morimoto.gx@renesas.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ASoC: use snd_soc_dapm_to_component() | expand |
On 2025-04-09 4:57 AM, Kuninori Morimoto wrote: > Now, snd_soc_dapm_to_component() (A) will convert dapm to component by > container_of() (a). > > (A) static inline struct snd_soc_component *snd_soc_dapm_to_component( > struct snd_soc_dapm_context *dapm) > { > (a) return container_of(dapm, struct snd_soc_component, dapm); > } > > Because (a) uses container_of(), dapm of component works, but > dapm of others will be "unknown" pointer (= not NULL). > > OTOH, we will call snd_soc_dapm_init() (X) to initialize dapm, and > it will be called from snd_soc_bind_card() (p) or > soc_probe_component() (q) with pointer (component/NULL). > > (p) static int snd_soc_bind_card(...) > { > ... > (X) snd_soc_dapm_init(..., NULL); > ... ^^^^ > } > > (q) static int soc_probe_component(...) > { > ... > (X) snd_soc_dapm_init(, component); > ... ^^^^^^^^^ > } > > And snd_soc_dapm_init() (X) will fill dapm->component (x) > > (X) void snd_soc_dapm_init(..., component, ...) > { > ... > (x) dapm->component = component; > ... > } > > We can simply use dapm->component in snd_soc_dapm_to_component() (A). > In this case, dapm of others (p) will be just NULL. > > Use dapm->component instead of container_of(). > The picky note can be removed by this patch. > > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > Cc: Benjamin Bara <benjamin.bara@skidata.com> > --- > include/sound/soc-component.h | 6 +----- > 1 file changed, 1 insertion(+), 5 deletions(-) > > diff --git a/include/sound/soc-component.h b/include/sound/soc-component.h > index 61534ac0edd1..63928773df32 100644 > --- a/include/sound/soc-component.h > +++ b/include/sound/soc-component.h > @@ -265,15 +265,11 @@ struct snd_soc_component { > * snd_soc_dapm_to_component() - Casts a DAPM context to the component it is > * embedded in > * @dapm: The DAPM context to cast to the component > - * > - * This function must only be used on DAPM contexts that are known to be part of > - * a component (e.g. in a component driver). Otherwise the behavior is > - * undefined. > */ > static inline struct snd_soc_component *snd_soc_dapm_to_component( > struct snd_soc_dapm_context *dapm) > { > - return container_of(dapm, struct snd_soc_component, dapm); > + return dapm->component; > } > > /** Why not drop the wrapper all along? Personally, I find assignment 'comp = dapm->component' more readable than the wrapper above. We have very long prefixes in sound/soc/ in general, and the devs end up reaching char line-limits. I'd rather focus on 'struct snd_soc_dapm_context' exclusively. A valid pointer to this type is required for many functions found in include/sound/ yet the struct is more of framework-internal entity. Strict separation between dapm-context "for the framework" from dapm-context "for the user" would grant better readability benefits. Kind regards, Czarek
Hi Cezary Thank you for your review. > > static inline struct snd_soc_component *snd_soc_dapm_to_component( > > struct snd_soc_dapm_context *dapm) > > { > > - return container_of(dapm, struct snd_soc_component, dapm); > > + return dapm->component; > > } (snip) > Why not drop the wrapper all along? Personally, I find assignment 'comp > = dapm->component' more readable than the wrapper above. We have very > long prefixes in sound/soc/ in general, and the devs end up reaching > char line-limits. Ah, yes indeed. I didn't notice this patern. > I'd rather focus on 'struct snd_soc_dapm_context' exclusively. A valid > pointer to this type is required for many functions found in > include/sound/ yet the struct is more of framework-internal entity. > Strict separation between dapm-context "for the framework" from > dapm-context "for the user" would grant better readability benefits. I confused about this. If we drop snd_soc_dapm_to_component() function, and directly use dapm->component, isn't it going to be the opposite of separation? If my understand was correct of "strict separation", "user" can use dapm-context pointer, but can't access to its member variable, like struct clk ? Thank you for your help !! Best regards --- Kuninori Morimoto
Hi Cezary, again > > I'd rather focus on 'struct snd_soc_dapm_context' exclusively. A valid > > pointer to this type is required for many functions found in > > include/sound/ yet the struct is more of framework-internal entity. > > Strict separation between dapm-context "for the framework" from > > dapm-context "for the user" would grant better readability benefits. I have checked each drivers which is using struct snd_soc_dapm_context. It seems not only framework but many user drivers need to use/access to it. So separation seems impossible, and dropping wrapper is maybe not good idea. But I can understand that it will be very longer code :) x = dapm->component; x = snd_soc_dapm_to_component(dapm); For me personally, wrapper function/macro is not so bad idea. It is much safer to use function/macro rather than touching them directly. Anyway, I will try to keep current patch-set style. But any comment is very welcome. Thank you for your help !! Best regards --- Kuninori Morimoto
On 2025-04-11 4:04 AM, Kuninori Morimoto wrote: > > Hi Cezary, again > >>> I'd rather focus on 'struct snd_soc_dapm_context' exclusively. A valid >>> pointer to this type is required for many functions found in >>> include/sound/ yet the struct is more of framework-internal entity. >>> Strict separation between dapm-context "for the framework" from >>> dapm-context "for the user" would grant better readability benefits. > > I have checked each drivers which is using struct snd_soc_dapm_context. > It seems not only framework but many user drivers need to use/access to it. > > So separation seems impossible, and dropping wrapper is maybe not good idea. > But I can understand that it will be very longer code :) > > x = dapm->component; > x = snd_soc_dapm_to_component(dapm); > > For me personally, wrapper function/macro is not so bad idea. > It is much safer to use function/macro rather than touching them directly. > > Anyway, I will try to keep current patch-set style. > But any comment is very welcome. > Thank you for your help !! I see two replies, hope it's fine if I just reply to this one. I love the enthusiasm though :D What I meant is that even the name of the type - 'dapm_context' - speaks of framework-internal stuff. The fields that define that struct also are not for the users (drivers) really. Perhaps there is a way to get rid of the dapm_context pointer entirely in the argument lists of public functions found in include/sound/. The pointer could be obtained e.g.: via container_of() by the framework's internal handler when needed. Otherwise, I do not see real benefit of using 'x = snd_soc_dapm_to_component()' over 'x = dapm->component'. In my opinion there's no need to provide wrapper for every field we have. No hard blocks though. Kind regards, Czarek
Hi Cezary Thank you for your feedback > What I meant is that even the name of the type - 'dapm_context' - speaks > of framework-internal stuff. The fields that define that struct also are > not for the users (drivers) really. Perhaps there is a way to get rid of > the dapm_context pointer entirely in the argument lists of public > functions found in include/sound/. The pointer could be obtained e.g.: > via container_of() by the framework's internal handler when needed. Hmm... In my quick check, the reason why user driver directly use dapm->xxx is to get/access dapm->card dapm->component dapm->dev dapm->bias_level dapm->idle_bias_off OTOH, many drivers are already using dapm related functions, like snd_soc_dapm_new_controls() snd_soc_dapm_add_routes() snd_soc_dapm_disable_pin_unlocked() snd_soc_dapm_sync_unlocked() snd_soc_dapm_mutex_lock() ... So, I think it is easy to hide dapm member from user driver, but dapm_context pointer itself is still needed in user driver. I think we can do is below To hide dapm_context details from user driver - define struct dapm_context name only in include/sound. - define struct dapm_context details in soc_dapm.c To use dapm_context related feature in user driver - use snd_soc_dapm_xxx() function in all user driver I think it can be 1st step. We can reconsider about 2nd step ? But what do you think ? Thank you for your help !! Best regards --- Kuninori Morimoto
Hi Cezary, Mark > dapm->card > dapm->component > dapm->dev > dapm->bias_level > dapm->idle_bias_off (snip) > snd_soc_dapm_new_controls() > snd_soc_dapm_add_routes() > snd_soc_dapm_disable_pin_unlocked() > snd_soc_dapm_sync_unlocked() > snd_soc_dapm_mutex_lock() > ... (snip) > To hide dapm_context details from user driver > - define struct dapm_context name only in include/sound. > - define struct dapm_context details in soc_dapm.c > > To use dapm_context related feature in user driver > - use snd_soc_dapm_xxx() function in all user driver We have 2 way to get dapm_context (card vs component). So if soc-dapm has function for these, we can remove dapm_context from header (Not 100% sure for now, but maybe). It will be big patch-set. I'm OK to create it. But what do you think ? --- soc-dapm.c ------- static __snd_soc_dapm_xxx(dapm_context, ...) { ... } int snd_soc_dapm_xxx_for_card(card, xxx) { __snd_soc_dapm_xxx(card->dapm, ...); } int snd_soc_dapm_xxx_for_component(component, xxx) { __snd_soc_dapm_xxx(component->dapm, ...); } --- soc-dapm.h ------- #define snd_soc_dapm_xxx(x) _Generic((x, ...), \ struct snd_soc_card * : snd_soc_dapm_xxx_for_card, \ struct snd_soc_component * : snd_soc_dapm_xxx_for_component)(x, ...) Thank you for your help !! Best regards --- Kuninori Morimoto
diff --git a/include/sound/soc-component.h b/include/sound/soc-component.h index 61534ac0edd1..63928773df32 100644 --- a/include/sound/soc-component.h +++ b/include/sound/soc-component.h @@ -265,15 +265,11 @@ struct snd_soc_component { * snd_soc_dapm_to_component() - Casts a DAPM context to the component it is * embedded in * @dapm: The DAPM context to cast to the component - * - * This function must only be used on DAPM contexts that are known to be part of - * a component (e.g. in a component driver). Otherwise the behavior is - * undefined. */ static inline struct snd_soc_component *snd_soc_dapm_to_component( struct snd_soc_dapm_context *dapm) { - return container_of(dapm, struct snd_soc_component, dapm); + return dapm->component; } /**
Now, snd_soc_dapm_to_component() (A) will convert dapm to component by container_of() (a). (A) static inline struct snd_soc_component *snd_soc_dapm_to_component( struct snd_soc_dapm_context *dapm) { (a) return container_of(dapm, struct snd_soc_component, dapm); } Because (a) uses container_of(), dapm of component works, but dapm of others will be "unknown" pointer (= not NULL). OTOH, we will call snd_soc_dapm_init() (X) to initialize dapm, and it will be called from snd_soc_bind_card() (p) or soc_probe_component() (q) with pointer (component/NULL). (p) static int snd_soc_bind_card(...) { ... (X) snd_soc_dapm_init(..., NULL); ... ^^^^ } (q) static int soc_probe_component(...) { ... (X) snd_soc_dapm_init(, component); ... ^^^^^^^^^ } And snd_soc_dapm_init() (X) will fill dapm->component (x) (X) void snd_soc_dapm_init(..., component, ...) { ... (x) dapm->component = component; ... } We can simply use dapm->component in snd_soc_dapm_to_component() (A). In this case, dapm of others (p) will be just NULL. Use dapm->component instead of container_of(). The picky note can be removed by this patch. Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> Cc: Benjamin Bara <benjamin.bara@skidata.com> --- include/sound/soc-component.h | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)