On Tue, 12 Jul 2022 at 19:52, Krzysztof Kozlowski
<krzysztof.kozlowski@xxxxxxxxxx> wrote:
[snip]
> > +static void sysmmu_write(struct sysmmu_drvdata *data, size_t idx, u32 val)
> > +{
> > + writel(val, data->sfrbase + data->regs[idx]);
>
> Beside what Robin wrote, I also don't think these wrappers actually
> help, because you reverse arguments comparing to writel.
>
> How about having a per-variant structure with offsets and using it like:
>
> #define SYSMMU_REG(data, reg) ((data)->sfrbase + (data)->variant->reg)
> writel(CTRL_ENABLE, SYSMMU_REG(data, mmu_ctrl_reg))
>
> Would that be more readable?
>
Yes, this looks better for my taste too. I tend to get a tunnel vision
when working with downstream code for a while. But I noticed that that
approach is used sometimes as well, e.g. in
drivers/pinctrl/samsung/pinctrl-exynos-arm64.c (in struct
samsung_pin_bank_type). Anyway, I've reworked it exactly as you
suggested, will send v2 soon. Thanks!
>
> Best regards,
> Krzysztof
