Re: [PATCH 1/5] mfd:rtsx: Read vendor setting from config space

[Dan Carpenter]

This code has a lot of undocumented magic flags and terrible naming
in it.

> +static void rtl8411_init_settings(struct rtsx_pcr *pcr)
> +{
> +	u32 val1;
> +	u8 val2;

These names are 100% useless.  "val" means nothing.  "1" means
nothing.  "2" means nothing.  In fact, val1 is REG1 and val2 is
REG3 as opposed to REG2 as one would expect from the name so it is
misleading.

> +
> +	rtsx_pci_read_config_dword(pcr, PCR_SETTING_REG1, &val1);
> +	dev_dbg(&(pcr->pci->dev), "Cfg 0x%x: 0x%x\n", PCR_SETTING_REG1, val1);
> +
> +	if (!(val1 & 0x1000000)) {

What is 0x1000000?  We do this test over and over so it should be
an inline function.

> +		pcr->aspm_val = (u8)((val1 >> 28) & 0x03);

The "_val" in "apsm_val" is useless.
What does "aspm" stand for?
No need to cast.  0-3 is less than 255 so casting to u8 is just
noise.
These shift masks are used over and over so they should be defined
as macros or inline functions in a header file:

#define rtl8411_reg_to_XXX(reg)		((reg >> 25) & 0x01)
#define rtl8411_reg_to_sdXXX(reg)	((reg >> 26) & 0x03)
#define rtl8411_reg_to_aspm(reg)	((reg >> 28) & 0x03)

(Choose your own names which make sense).

> +		pcr->sd30_drive_sel_1v8 = map_sd_drive((val1 >> 26) & 0x03);
> +		pcr->card_drive_sel &= 0x3F;
> +		pcr->card_drive_sel |= (u8)((val1 >> 25) & 0x01) << 6;

Again, no need to cast.  This should be a macro or an inline
function.

Pretty much the same comments apply throughout.  I'm not going to
comment on it line by line.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/