There are both tegra_smmu_swgroup and tegra_smmu_group structs
using "group" for their pointer instances. This gets confusing
to read the driver sometimes.
So this patch renames "group" of struct tegra_smmu_swgroup to
"swgrp" as a cleanup. Also renames its "find" function.
Note that we already have "swgroup" being used for an unsigned
int type variable that is inside struct tegra_smmu_swgroup, so
it's not able to use "swgroup" but only something like "swgrp".
Signed-off-by: Nicolin Chen <[email protected]>
---
drivers/iommu/tegra-smmu.c | 34 +++++++++++++++++-----------------
1 file changed, 17 insertions(+), 17 deletions(-)
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index a32ed347e25d..0f3883045ffa 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -334,35 +334,35 @@ static void tegra_smmu_domain_free(struct iommu_domain *domain)
}
static const struct tegra_smmu_swgroup *
-tegra_smmu_find_swgroup(struct tegra_smmu *smmu, unsigned int swgroup)
+tegra_smmu_find_swgrp(struct tegra_smmu *smmu, unsigned int swgroup)
{
- const struct tegra_smmu_swgroup *group = NULL;
+ const struct tegra_smmu_swgroup *swgrp = NULL;
unsigned int i;
for (i = 0; i < smmu->soc->num_swgroups; i++) {
if (smmu->soc->swgroups[i].swgroup == swgroup) {
- group = &smmu->soc->swgroups[i];
+ swgrp = &smmu->soc->swgroups[i];
break;
}
}
- return group;
+ return swgrp;
}
static void tegra_smmu_enable(struct tegra_smmu *smmu, unsigned int swgroup,
unsigned int asid)
{
- const struct tegra_smmu_swgroup *group;
+ const struct tegra_smmu_swgroup *swgrp;
unsigned int i;
u32 value;
- group = tegra_smmu_find_swgroup(smmu, swgroup);
- if (group) {
- value = smmu_readl(smmu, group->reg);
+ swgrp = tegra_smmu_find_swgrp(smmu, swgroup);
+ if (swgrp) {
+ value = smmu_readl(smmu, swgrp->reg);
value &= ~SMMU_ASID_MASK;
value |= SMMU_ASID_VALUE(asid);
value |= SMMU_ASID_ENABLE;
- smmu_writel(smmu, value, group->reg);
+ smmu_writel(smmu, value, swgrp->reg);
} else {
pr_warn("%s group from swgroup %u not found\n", __func__,
swgroup);
@@ -385,17 +385,17 @@ static void tegra_smmu_enable(struct tegra_smmu *smmu, unsigned int swgroup,
static void tegra_smmu_disable(struct tegra_smmu *smmu, unsigned int swgroup,
unsigned int asid)
{
- const struct tegra_smmu_swgroup *group;
+ const struct tegra_smmu_swgroup *swgrp;
unsigned int i;
u32 value;
- group = tegra_smmu_find_swgroup(smmu, swgroup);
- if (group) {
- value = smmu_readl(smmu, group->reg);
+ swgrp = tegra_smmu_find_swgrp(smmu, swgroup);
+ if (swgrp) {
+ value = smmu_readl(smmu, swgrp->reg);
value &= ~SMMU_ASID_MASK;
value |= SMMU_ASID_VALUE(asid);
value &= ~SMMU_ASID_ENABLE;
- smmu_writel(smmu, value, group->reg);
+ smmu_writel(smmu, value, swgrp->reg);
}
for (i = 0; i < smmu->soc->num_clients; i++) {
@@ -1008,11 +1008,11 @@ static int tegra_smmu_swgroups_show(struct seq_file *s, void *data)
seq_printf(s, "------------------------\n");
for (i = 0; i < smmu->soc->num_swgroups; i++) {
- const struct tegra_smmu_swgroup *group = &smmu->soc->swgroups[i];
+ const struct tegra_smmu_swgroup *swgrp = &smmu->soc->swgroups[i];
const char *status;
unsigned int asid;
- value = smmu_readl(smmu, group->reg);
+ value = smmu_readl(smmu, swgrp->reg);
if (value & SMMU_ASID_ENABLE)
status = "yes";
@@ -1021,7 +1021,7 @@ static int tegra_smmu_swgroups_show(struct seq_file *s, void *data)
asid = value & SMMU_ASID_MASK;
- seq_printf(s, "%-9s %-7s %#04x\n", group->name, status,
+ seq_printf(s, "%-9s %-7s %#04x\n", swgrp->name, status,
asid);
}
--
2.17.1
On Mon, Sep 13, 2021 at 06:38:55PM -0700, Nicolin Chen wrote:
> There are both tegra_smmu_swgroup and tegra_smmu_group structs
> using "group" for their pointer instances. This gets confusing
> to read the driver sometimes.
>
> So this patch renames "group" of struct tegra_smmu_swgroup to
> "swgrp" as a cleanup. Also renames its "find" function.
>
> Note that we already have "swgroup" being used for an unsigned
> int type variable that is inside struct tegra_smmu_swgroup, so
> it's not able to use "swgroup" but only something like "swgrp".
>
> Signed-off-by: Nicolin Chen <[email protected]>
> ---
> drivers/iommu/tegra-smmu.c | 34 +++++++++++++++++-----------------
> 1 file changed, 17 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> index a32ed347e25d..0f3883045ffa 100644
> --- a/drivers/iommu/tegra-smmu.c
> +++ b/drivers/iommu/tegra-smmu.c
> @@ -334,35 +334,35 @@ static void tegra_smmu_domain_free(struct iommu_domain *domain)
> }
>
> static const struct tegra_smmu_swgroup *
> -tegra_smmu_find_swgroup(struct tegra_smmu *smmu, unsigned int swgroup)
> +tegra_smmu_find_swgrp(struct tegra_smmu *smmu, unsigned int swgroup)
This makes things inconsistent now. The tegra_smmu_find_swgroup() name
indicates that we're looking for some "swgroup" entity within an "smmu"
object. The entity that we're looking for is a struct tegra_smmu_swgroup
so I think it makes sense to use that full name in the function name.
> {
> - const struct tegra_smmu_swgroup *group = NULL;
> + const struct tegra_smmu_swgroup *swgrp = NULL;
I don't think the existing naming is confusing. The variable name
"group" is consistently used for tegra_smmu_swgroup structures and there
are no cases where we would confuse them with struct tegra_smmu_group
instances.
However, I don't feel strongly about it, so I'm fine with changing the
variable names to "swgrp" if you think that makes things less confusing.
Thierry
On Thu, Oct 07, 2021 at 06:57:31PM +0200, Thierry Reding wrote:
> On Mon, Sep 13, 2021 at 06:38:55PM -0700, Nicolin Chen wrote:
> > There are both tegra_smmu_swgroup and tegra_smmu_group structs
> > using "group" for their pointer instances. This gets confusing
> > to read the driver sometimes.
> >
> > So this patch renames "group" of struct tegra_smmu_swgroup to
> > "swgrp" as a cleanup. Also renames its "find" function.
> >
> > Note that we already have "swgroup" being used for an unsigned
> > int type variable that is inside struct tegra_smmu_swgroup, so
> > it's not able to use "swgroup" but only something like "swgrp".
> >
> > Signed-off-by: Nicolin Chen <[email protected]>
> > ---
> > drivers/iommu/tegra-smmu.c | 34 +++++++++++++++++-----------------
> > 1 file changed, 17 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> > index a32ed347e25d..0f3883045ffa 100644
> > --- a/drivers/iommu/tegra-smmu.c
> > +++ b/drivers/iommu/tegra-smmu.c
> > @@ -334,35 +334,35 @@ static void tegra_smmu_domain_free(struct iommu_domain *domain)
> > }
> >
> > static const struct tegra_smmu_swgroup *
> > -tegra_smmu_find_swgroup(struct tegra_smmu *smmu, unsigned int swgroup)
> > +tegra_smmu_find_swgrp(struct tegra_smmu *smmu, unsigned int swgroup)
>
> This makes things inconsistent now. The tegra_smmu_find_swgroup() name
> indicates that we're looking for some "swgroup" entity within an "smmu"
> object. The entity that we're looking for is a struct tegra_smmu_swgroup
> so I think it makes sense to use that full name in the function name.
This is more like an indirect change to keep consistency between
function name and pointer name.
> > {
> > - const struct tegra_smmu_swgroup *group = NULL;
> > + const struct tegra_smmu_swgroup *swgrp = NULL;
>
> I don't think the existing naming is confusing. The variable name
> "group" is consistently used for tegra_smmu_swgroup structures and there
> are no cases where we would confuse them with struct tegra_smmu_group
> instances.
If we don't rename it, then PATCH-4 adds to struct tegra_smmu_group
a "struct tegra_smmu_swgroup *group", which results in a confusing
group->group...
> However, I don't feel strongly about it, so I'm fine with changing the
> variable names to "swgrp" if you think that makes things less confusing.
Yea, I'd like to keep this change. I will respin it in next version
after fixing other comments.