Am 24.03.2017 um 23:52 schrieb Kevin Hilman: > Heiner Kallweit <hkallweit1@xxxxxxxxx> writes: > >> Use GENMASK consistently for all bit masks and switch to using the >> bitfield macros GET_FIELD and PREP_FIELD. This hides parts of the >> complexity of dealing with bit fields. >> >> Signed-off-by: Heiner Kallweit <hkallweit1@xxxxxxxxx> > > Very nice. I should've used these from the beginning. > > Some comments below... > >> --- >> drivers/mmc/host/meson-gx-mmc.c | 84 +++++++++++++++++++---------------------- >> 1 file changed, 38 insertions(+), 46 deletions(-) >> >> diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c >> index b917765c..cf2ccc67 100644 >> --- a/drivers/mmc/host/meson-gx-mmc.c >> +++ b/drivers/mmc/host/meson-gx-mmc.c >> @@ -36,23 +36,25 @@ >> #include <linux/clk-provider.h> >> #include <linux/regulator/consumer.h> >> #include <linux/interrupt.h> >> +#include <linux/bitfield.h> >> >> #define DRIVER_NAME "meson-gx-mmc" >> >> #define SD_EMMC_CLOCK 0x0 >> #define CLK_DIV_SHIFT 0 >> #define CLK_DIV_WIDTH 6 >> -#define CLK_DIV_MASK 0x3f >> +#define CLK_DIV_MASK GENMASK(5, 0) >> #define CLK_DIV_MAX 63 >> #define CLK_SRC_SHIFT 6 >> #define CLK_SRC_WIDTH 2 > > Shouldn't you get rid of the shift/width here too? > Just had a look, yes, that's possible too. We just need some built-in compiler magic to derive these value from the mask in meson_mmc_clk_init. >> -#define CLK_SRC_MASK 0x3 >> +#define CLK_SRC_MASK GENMASK(7, 6) >> #define CLK_SRC_XTAL 0 /* external crystal */ >> #define CLK_SRC_XTAL_RATE 24000000 >> #define CLK_SRC_PLL 1 /* FCLK_DIV2 */ >> #define CLK_SRC_PLL_RATE 1000000000 >> -#define CLK_PHASE_SHIFT 8 >> -#define CLK_PHASE_MASK 0x3 >> +#define CLK_CORE_PHASE_MASK GENMASK(9, 8) >> +#define CLK_TX_PHASE_MASK GENMASK(11, 10) >> +#define CLK_RX_PHASE_MASK GENMASK(13, 12) > > The latter 2 aren't used anywhere yet. I prefer to keep this #defines > to only fields that are actually used. > Right, the latter two are used in a later patch only. So I will insert them when needed. >> #define CLK_PHASE_0 0 >> #define CLK_PHASE_90 1 >> #define CLK_PHASE_180 2 > > Otherwise, looks good to me. > > Reviewed-by: Kevin Hilman <khilman@xxxxxxxxxxxx> > Thanks for the quick review, Heiner > Kevin > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
