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

[Alexandre Courbot <gnurou@xxxxxxxxx>]

On Mon, Mar 2, 2015 at 5:54 PM, Vince Hsu <vinceh@xxxxxxxxxx> 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);


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?
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html