PCI: brcmstb: Use GENMASK() as __GENMASK() is for internal use only
Subsystems (5)
PATCH 1/1 PCI: brcmstb: Use GENMASK() as __GENMASK() is for internal use only
Use GENMASK() as __GENMASK() is for internal use only.
--- drivers/pci/controller/pcie-brcmstb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/pci/controller/pcie-brcmstb.c +1 -1
@@ -619,7 +619,7 @@ static void brcm_msi_remove(struct brcm_pcie *pcie)static void brcm_msi_set_regs (struct brcm_msi *msi) { -u32 val =__GENMASK (31 , msi->legacy_shift); +u32 val =GENMASK (31 , msi->legacy_shift);writel (val, msi->intr_base +MSI_INT_MASK_CLR );writel (val, msi->intr_base +MSI_INT_CLR );
Signature · 1 line
2.33.0
Quoted 1 line · Andy Shevchenko
Use
GENMASK()as__GENMASK()is for internal use only.
Note, it's the only user of __GENMASK() in the kernel.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Hi Andy,
Quoted 1 line
Use
GENMASK()as__GENMASK()is for internal use only.
To add, for posterity, that using __GENMASK() bypasses the GENMASK_INPUT_CHECK() macro that adds extra validation.
Quoted 11 lines
Fixes 3baec684a531 ("PCI: brcmstb: Accommodate MSI for older chips")Signed-off-by Andy Shevchenko <[email protected]>--- drivers/pci/controller/pcie-brcmstb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/pci/controller/pcie-brcmstb.c +0
-0
drivers/pci/controller/pcie-brcmstb.c static void brcm_msi_remove(struct brcm_pcie *pcie) · line 622
Quoted 5 lines
drivers/pci/controller/pcie-brcmstb.c +1
-1@@ -619,7 +619,7 @@ static void brcm_msi_remove(struct brcm_pcie *pcie)static void brcm_msi_set_regs (struct brcm_msi *msi) { -u32 val =__GENMASK (31 , msi->legacy_shift); +u32 val =GENMASK (31 , msi->legacy_shift);
Thank you!
Krzysztof
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Quoted 4 lines · Krzysztof Wilczyński
Quoted 1 line
Use
GENMASK()as__GENMASK()is for internal use only.To add, for posterity, that using
__GENMASK()bypasses theGENMASK_INPUT_CHECK()macro that adds extra validation.
In general, yes, but here we have a variable...
Quoted 2 lines
drivers/pci/controller/pcie-brcmstb.c static void brcm_msi_remove(struct brcm_pcie *pcie) · line 622
Quoted 2 lines
drivers/pci/controller/pcie-brcmstb.c +1
-1-u32 val =__GENMASK (31 , msi->legacy_shift); +u32 val =GENMASK (31 , msi->legacy_shift);
...which make me thing that the whole construction is ugly (and I truly believe the code is very ugly here, because
the idea behind GENMASK() is to be used with constants).
So, what about
u32 val = ~(BIT(msi->legacy_shift) - 1);
instead?
Quoted 3 lines
Thank you!
Reviewed-by Krzysztof Wilczyński <[email protected]>
Thank you!
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 1 line · Andy Shevchenko
On Wed, Oct 27, 2021 at 12:00:16PM +0200, Krzysztof Wilczyński wrote:
...
Quoted 1 line
(and I truly believe the code is very ugly here, because
s/code/generated code/
Quoted 1 line
the idea behind
GENMASK()is to be used with constants).
Signature · 2 lines
With Best Regards, Andy Shevchenko
Hi Andy,
Quoted 15 lines · Krzysztof Wilczyński
Quoted 4 lines
Quoted 1 line
Use
GENMASK()as__GENMASK()is for internal use only.To add, for posterity, that using
__GENMASK()bypasses theGENMASK_INPUT_CHECK()macro that adds extra validation.In general, yes, but here we have a variable...
Quoted 2 lines
Quoted 2 lines
- u32 val =
__GENMASK(31, msi->legacy_shift);
+ u32 val =GENMASK(31, msi->legacy_shift);...which make me thing that the whole construction is ugly (and I truly believe the code is very ugly here, because
the idea behindGENMASK()is to be used with constants).
Quoted 5 lines
So, what about
u32 val = ~(BIT(msi->legacy_shift) - 1);instead?
Sorry for late reply! Thankfully, you've sent a v2 using the BIT() macro already. Thank you!
Krzysztof