Re: [RFC PATCH 2/9] memory: tegra: add mc flush support

[Vince Hsu <vinceh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>]

Hi,

On 03/02/2015 05:29 PM, Alexandre Courbot wrote:
> On Mon, Mar 2, 2015 at 5:54 PM, Vince Hsu <vinceh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>>>> +int tegra_mc_flush(struct tegra_mc_swgroup *sg)
>>>> +{
>>>> +       struct tegra_mc *mc;
>>>> +       const struct tegra_mc_hotreset *client;
>>>> +       int i;
>>>> +
>>>> +       if (!sg || !sg->mc)
>>>> +               return -EINVAL;;
>>>> +
>>>> +       mc = sg->mc;
>>>> +       if (!mc->soc->ops || !mc->soc->ops->flush)
>>>> +               return -EINVAL;;
>>>> +
>>>> +       client = mc->soc->hotresets;
>>>> +
>>>> +       for (i = 0; i < mc->soc->num_hotresets; i++, client++) {
>>>> +               if (sg->id == client->swgroup)
>>>> +                       return mc->soc->ops->flush(mc, client);
>>>> +       }
>>>> +
>>>> +       return -EINVAL;
>>>> +}
>>>> +EXPORT_SYMBOL(tegra_mc_flush);
>>>> +
>>>> +int tegra_mc_flush_done(struct tegra_mc_swgroup *sg)
>>>> +{
>>>> +       struct tegra_mc *mc;
>>>> +       const struct tegra_mc_hotreset *client;
>>>> +       int i;
>>>> +
>>>> +       if (!sg || !sg->mc)
>>>> +               return -EINVAL;;
>>>> +
>>>> +       mc = sg->mc;
>>>> +       if (!mc->soc->ops || !mc->soc->ops->flush)
>>>> +               return -EINVAL;;
>>>> +
>>>> +       client = mc->soc->hotresets;
>>>> +
>>>> +       for (i = 0; i < mc->soc->num_hotresets; i++, client++) {
>>>> +               if (sg->id == client->swgroup)
>>>> +                       return mc->soc->ops->flush_done(mc, client);
>>>> +       }
>>>> +
>>>> +       return -EINVAL;
>>>> +}
>>>> +EXPORT_SYMBOL(tegra_mc_flush_done);
>>> These functions are identical, excepted for the callback they are
>>> invoking. Could you merge the common part into a function that returns
>>> the right client to call the callback on, or ERR_PTR(-EINVAL) in case
>>> of failure?
>> I couldn't think of a clever way to do this. Any ideas? :)
> How about something like this (warning: might now be that great, untested):
>
> /* Have this in your .h and use it in your tegra_mc_ops struct */
> typedef int (*mc_op)(struct tegra_mc *mc,
>               const struct tegra_mc_hotreset *hotreset)
>
> static int __tegra_mc_op(struct tegra_mc_swgroup *sg, mc_op op)
> {
>         struct tegra_mc *mc;
>         const struct tegra_mc_hotreset *client;
>         int i;
>
>         mc = sg->mc;
>         client = mc->soc->hotresets;
>
>         for (i = 0; i < mc->soc->num_hotresets; i++, client++) {
>                 if (sg->id == client->swgroup)
>                         return op(mc, client);
>         }
>
>         return -EINVAL;
> }
>
> #define tegra_mc_op(sg, op)                                               \
>      ((!sg || !sg->mc || !sg->mc->soc->ops || sg->mc->soc->ops->op) ?  \
>          -EINVAL :                                                 \
>          __tegra_mc_op(sg, sg->mc->soc->ops->op));
>
> int tegra_mc_flush(struct tegra_mc_swgroup *sg)
> {
>         return tegra_mc_op(sg, flush);
> }
> EXPORT_SYMBOL(tegra_mc_flush);
>
> int tegra_mc_flush_done(struct tegra_mc_swgroup *sg)
> {
>         return tegra_mc_op(sg, flush_done);
> }
> EXPORT_SYMBOL(tegra_mc_flush_done);
Thanks for the example! Will use this idea and remove the double 
semicolon below.

Thanks,
Vince

>
>
> Actually when writing this code I found two other issues:
>
>>>> +int tegra_mc_flush_done(struct tegra_mc_swgroup *sg)
>>>> +{
>>>> +       struct tegra_mc *mc;
>>>> +       const struct tegra_mc_hotreset *client;
>>>> +       int i;
>>>> +
>>>> +       if (!sg || !sg->mc)
>>>> +               return -EINVAL;;
> Double ; (also in tegra_mc_flush)
>
>>>> +
>>>> +       mc = sg->mc;
>>>> +       if (!mc->soc->ops || !mc->soc->ops->flush)
> Should be ops->flush_done?