Messages in this thread | | | Date | Tue, 14 Jan 2014 18:00:05 +0000 | From | Mark Brown <> | Subject | Re: [PATCH] ASoC: simple-card: simplify code |
| |
On Tue, Jan 14, 2014 at 05:12:58PM +0100, Jean-Francois Moine wrote: > Mark Brown <broonie@kernel.org> wrote:
> > 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.
> > > ret = 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?
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. [unhandled content-type:application/pgp-signature] | |