ASoC: simple-card: simplify code
PATCH 1/1 ASoC: simple-card: simplify code
This patch
- removes the fields of the platform data which are of no use to the non-DT platform callers,
- uses a new private structure to handle all the sound card information,
- simplifies the code and make easier a possible multi-DAI links extension.
---
Xiubo, I also removed 'of_device_is_available' which seems really
useless: the module is not probed when the DT status is not "okay".
--- include/sound/simple_card.h | 4 -- sound/soc/generic/simple-card.c | 178 ++++++++++----------- 2 files changed, 88 insertions(+), 94 deletions(-)
include/sound/simple_card.h +0 -4
@@ -29,10 +29,6 @@ struct asoc_simple_card_info {unsigned int daifmt;struct asoc_simple_dai cpu_dai; struct asoc_simple_dai codec_dai; - -/* used in simple-card.c */ -struct snd_soc_dai_link snd_link; - struct snd_soc_card snd_card; };#endif /* __SIMPLE_CARD_H */
sound/soc/generic/simple-card.c +88 -90
266 lines not on this page · raw message
Signature · 2 lines
Ken ar c'hentañ | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/
Mark Brown
maintainerQuoted 10 lines · Jean-Francois Moine
This patch
- removes the fields of the platform data which are of no use to the non-DT platform callers,
- uses a new private structure to handle all the sound card information,
- simplifies the code and make easier a possible multi-DAI links extension.Signed-off-by Jean-Francois Moine <[email protected]>---
Xiubo, I also removed 'of_device_is_available' which seems really
useless: the module is not probed when the DT status is not "okay".
Please send this as a patch series to aid review, one patch doing four different changes is much harder to review.
sound/soc/generic/simple-card.c static int asoc_simple_card_parse_of(struct device_node *node, · line 156
Quoted 7 lines
sound/soc/generic/simple-card.c +4
-2ret =asoc_simple_card_sub_parse_of (np, - &info->cpu_dai, - of_cpu); + &priv->cpu_dai, + (struct device_node **) + &dai_link->cpu_of_node, + &dai_link->cpu_dai_name);
What's this cast here for? That code doesn't look at all safe.
Quoted 7 lines · On Tue, 14 Jan 2014 14:20:14 +0000 Mark Brown <[email protected]> wrote:
Quoted 6 lines · Jean-Francois Moine
This patch
- removes the fields of the platform data which are of no use to the non-DT platform callers,
- uses a new private structure to handle all the sound card information,
- simplifies the code and make easier a possible multi-DAI links extension.
Quoted 7 lines
Quoted 4 lines
Signed-off-by Jean-Francois Moine <[email protected]>---
Xiubo, I also removed 'of_device_is_available' which seems really
useless: the module is not probed when the DT status is not "okay".Please send this as a patch series to aid review, one patch doing four different changes is much harder to review.
As there are other bugs to fix, I may put back the 'of_device_is_available', but there are not 3 different changes: I just explain the visible effects of the patch. The patch itself is, as the subject says, 'simplify code', that is, 'have a simpler code with no change in the logic'.
Quoted 10 lines
sound/soc/generic/simple-card.c static int asoc_simple_card_parse_of(struct device_node *node, · line 156
Quoted 7 lines
sound/soc/generic/simple-card.c +4
-2ret =asoc_simple_card_sub_parse_of (np, - &info->cpu_dai, - of_cpu); + &priv->cpu_dai, + (struct device_node **) + &dai_link->cpu_of_node, + &dai_link->cpu_dai_name);What's this cast here for? That code doesn't look at all safe.
dai_link->cpu_of_node is 'const struct device_node *' and both of_clk_get() and of_node_put() want 'struct device_node *'. So, there must be a cast somewhere.
Do you prefer I put these ones when calling the 'of_xx' functions?
Signature · 6 lines
Ken ar c'hentañ | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/ _______________________________________________ Alsa-devel mailing list [email protected] http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Mark Brown
maintainerQuoted 10 lines · Jean-Francois Moine
Quoted 2 lines · Mark Brown
Please send this as a patch series to aid review, one patch doing four different changes is much harder to review.
As there are other bugs to fix, I may put back the '
of_device_is_available', but there are not 3 different changes: I just explain the visible effects of the patch. The patch itself is, as the subject says, 'simplify code', that is, 'have a simpler code with no change in the logic'.
There's several different simplifications going on here, or at least it sounded that way. For example creating a private data struct doesn't seem obviously related to deleting unused fields from the platform data. The larger a change is the more benefit there is from a series of mechanical individual updates rather than several at once.
Quoted 10 lines
Quoted 9 lines
sound/soc/generic/simple-card.c static int asoc_simple_card_parse_of(struct device_node *node, · line 156
Quoted 7 lines
sound/soc/generic/simple-card.c +4
-2ret =asoc_simple_card_sub_parse_of (np, - &info->cpu_dai, - of_cpu); + &priv->cpu_dai, + (struct device_node **) + &dai_link->cpu_of_node, + &dai_link->cpu_dai_name);What's this cast here for? That code doesn't look at all safe.
Quoted 5 lines
dai_link->cpu_of_nodeis 'const structdevice_node*' and bothof_clk_get()andof_node_put()want 'structdevice_node*'. So, there must be a cast somewhere.Do you prefer I put these ones when calling the '
of_xx' functions?
No, I think this stuff needs to actually be type safe with no dodgy casts. I'm not sure why we're doing an of_node_put(), for the of_clk_get() it's not immediately obvious why it's not taking a const clk. Or perhaps the pointer shouldn't be stored as const.