[PATCH 0/4] mfd: rtsx: Decrease driver size and add new device

[PATCH 0/4] mfd: rtsx: Decrease driver size and add new device

[Lee Jones]

With the recent added support request of yet another device, the burden
of duplicated Ops and Voltage Switch code was becoming a little messy.
To rectify is, we're creating a generic initialisation call, which in
turn uses a generic Ops structure. The individual initialisation routines
fill in the small differences in call-backs and device data after
invoking the generic call.

 drivers/mfd/rtl8411.c        | 144 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------------------------------------------------------
 drivers/mfd/rtsx_pcr.c       |   8 ++++++-
 drivers/mfd/rtsx_pcr.h       |   1 +
 include/linux/mfd/rtsx_pci.h |   1 +
 4 files changed, 81 insertions(+), 73 deletions(-)

[PATCH 1/4] mfd: rtsx: Generify call-back operations in order to reduce code duplication

[Lee Jones]

With only a few variations between devices, it seems silly to keep bolting
on entirely new and mostly duplicated operation structs when a generic one
could be defined and the small differences could be handled in the init
functions.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mfd/rtl8411.c        | 118 ++++++++++++++++++-------------------------
 include/linux/mfd/rtsx_pci.h |   2 +-
 2 files changed, 50 insertions(+), 70 deletions(-)

diff --git a/drivers/mfd/rtl8411.c b/drivers/mfd/rtl8411.c
index 5280135..92e9bf6 100644
--- a/drivers/mfd/rtl8411.c
+++ b/drivers/mfd/rtl8411.c
@@ -279,38 +279,6 @@ static int rtl8411_conv_clk_and_div_n(int input, int dir)
 	return output;
 }
 
-static const struct pcr_ops rtl8411_pcr_ops = {
-	.fetch_vendor_settings = rtl8411_fetch_vendor_settings,
-	.extra_init_hw = rtl8411_extra_init_hw,
-	.optimize_phy = NULL,
-	.turn_on_led = rtl8411_turn_on_led,
-	.turn_off_led = rtl8411_turn_off_led,
-	.enable_auto_blink = rtl8411_enable_auto_blink,
-	.disable_auto_blink = rtl8411_disable_auto_blink,
-	.card_power_on = rtl8411_card_power_on,
-	.card_power_off = rtl8411_card_power_off,
-	.switch_output_voltage = rtl8411_switch_output_voltage,
-	.cd_deglitch = rtl8411_cd_deglitch,
-	.conv_clk_and_div_n = rtl8411_conv_clk_and_div_n,
-	.force_power_down = rtl8411_force_power_down,
-};
-
-static const struct pcr_ops rtl8411b_pcr_ops = {
-	.fetch_vendor_settings = rtl8411b_fetch_vendor_settings,
-	.extra_init_hw = rtl8411b_extra_init_hw,
-	.optimize_phy = NULL,
-	.turn_on_led = rtl8411_turn_on_led,
-	.turn_off_led = rtl8411_turn_off_led,
-	.enable_auto_blink = rtl8411_enable_auto_blink,
-	.disable_auto_blink = rtl8411_disable_auto_blink,
-	.card_power_on = rtl8411_card_power_on,
-	.card_power_off = rtl8411_card_power_off,
-	.switch_output_voltage = rtl8411_switch_output_voltage,
-	.cd_deglitch = rtl8411_cd_deglitch,
-	.conv_clk_and_div_n = rtl8411_conv_clk_and_div_n,
-	.force_power_down = rtl8411_force_power_down,
-};
-
 /* SD Pull Control Enable:
  *     SD_DAT[3:0] ==> pull up
  *     SD_CD       ==> pull up
@@ -441,60 +409,72 @@ static const u32 rtl8411b_qfn48_ms_pull_ctl_disable_tbl[] = {
 	0,
 };
 
-void rtl8411_init_params(struct rtsx_pcr *pcr)
+#define set_pull_ctrl_tables(__device) \
+	pcr->sd_pull_ctl_enable_tbl  = __device##_sd_pull_ctl_enable_tbl;  \
+	pcr->sd_pull_ctl_disable_tbl = __device##_sd_pull_ctl_disable_tbl; \
+	pcr->ms_pull_ctl_enable_tbl  = __device##_ms_pull_ctl_enable_tbl;  \
+	pcr->ms_pull_ctl_disable_tbl = __device##_ms_pull_ctl_disable_tbl;
+
+static struct pcr_ops rtl84xx_pcr_generic_ops = {
+	.fetch_vendor_settings = NULL, /* Dynamic */
+	.extra_init_hw = NULL,         /* Dynamic */
+	.optimize_phy = NULL,
+	.turn_on_led = rtl8411_turn_on_led,
+	.turn_off_led = rtl8411_turn_off_led,
+	.enable_auto_blink = rtl8411_enable_auto_blink,
+	.disable_auto_blink = rtl8411_disable_auto_blink,
+	.card_power_on = rtl8411_card_power_on,
+	.card_power_off = rtl8411_card_power_off,
+	.switch_output_voltage = rtl84xx_switch_output_voltage,
+	.cd_deglitch = rtl8411_cd_deglitch,
+	.conv_clk_and_div_n = rtl8411_conv_clk_and_div_n,
+	.force_power_down = rtl8411_force_power_down,
+};
+
+void rtl84xx_generic_params(struct rtsx_pcr *pcr)
 {
+	pcr->ops = &rtl84xx_pcr_generic_ops;
+
 	pcr->extra_caps = EXTRA_CAPS_SD_SDR50 | EXTRA_CAPS_SD_SDR104;
 	pcr->num_slots = 2;
-	pcr->ops = &rtl8411_pcr_ops;
 
 	pcr->flags = 0;
 	pcr->card_drive_sel = RTL8411_CARD_DRIVE_DEFAULT;
 	pcr->sd30_drive_sel_1v8 = DRIVER_TYPE_B;
 	pcr->sd30_drive_sel_3v3 = DRIVER_TYPE_D;
 	pcr->aspm_en = ASPM_L1_EN;
-	pcr->tx_initial_phase = SET_CLOCK_PHASE(23, 7, 14);
-	pcr->rx_initial_phase = SET_CLOCK_PHASE(4, 3, 10);
 
 	pcr->ic_version = rtl8411_get_ic_version(pcr);
-	pcr->sd_pull_ctl_enable_tbl = rtl8411_sd_pull_ctl_enable_tbl;
-	pcr->sd_pull_ctl_disable_tbl = rtl8411_sd_pull_ctl_disable_tbl;
-	pcr->ms_pull_ctl_enable_tbl = rtl8411_ms_pull_ctl_enable_tbl;
-	pcr->ms_pull_ctl_disable_tbl = rtl8411_ms_pull_ctl_disable_tbl;
+}
+
+void rtl8411_init_params(struct rtsx_pcr *pcr)
+{
+	/* Fill in common parameters. */
+	rtl84xx_generic_params(pcr);
+
+	/* Device Ops variation. */
+	pcr->ops->fetch_vendor_settings = rtl8411_fetch_vendor_settings;
+	pcr->ops->extra_init_hw = rtl8411_extra_init_hw;
+
+	pcr->rx_initial_phase = SET_CLOCK_PHASE(4, 3, 10);
+
+	set_pull_ctrl_tables(rtl8411);
 }
 
 void rtl8411b_init_params(struct rtsx_pcr *pcr)
 {
-	pcr->extra_caps = EXTRA_CAPS_SD_SDR50 | EXTRA_CAPS_SD_SDR104;
-	pcr->num_slots = 2;
-	pcr->ops = &rtl8411b_pcr_ops;
+	/* Fill in common parameters. */
+	rtl84xx_generic_params(pcr);
+
+	/* Device Ops variation. */
+	pcr->ops->fetch_vendor_settings = rtl8411b_fetch_vendor_settings;
+	pcr->ops->extra_init_hw = rtl8411b_extra_init_hw;
 
-	pcr->flags = 0;
-	pcr->card_drive_sel = RTL8411_CARD_DRIVE_DEFAULT;
-	pcr->sd30_drive_sel_1v8 = DRIVER_TYPE_B;
-	pcr->sd30_drive_sel_3v3 = DRIVER_TYPE_D;
-	pcr->aspm_en = ASPM_L1_EN;
 	pcr->tx_initial_phase = SET_CLOCK_PHASE(23, 7, 14);
 	pcr->rx_initial_phase = SET_CLOCK_PHASE(4, 3, 10);
 
-	pcr->ic_version = rtl8411_get_ic_version(pcr);
-
-	if (rtl8411b_is_qfn48(pcr)) {
-		pcr->sd_pull_ctl_enable_tbl =
-			rtl8411b_qfn48_sd_pull_ctl_enable_tbl;
-		pcr->sd_pull_ctl_disable_tbl =
-			rtl8411b_qfn48_sd_pull_ctl_disable_tbl;
-		pcr->ms_pull_ctl_enable_tbl =
-			rtl8411b_qfn48_ms_pull_ctl_enable_tbl;
-		pcr->ms_pull_ctl_disable_tbl =
-			rtl8411b_qfn48_ms_pull_ctl_disable_tbl;
-	} else {
-		pcr->sd_pull_ctl_enable_tbl =
-			rtl8411b_qfn64_sd_pull_ctl_enable_tbl;
-		pcr->sd_pull_ctl_disable_tbl =
-			rtl8411b_qfn64_sd_pull_ctl_disable_tbl;
-		pcr->ms_pull_ctl_enable_tbl =
-			rtl8411b_qfn64_ms_pull_ctl_enable_tbl;
-		pcr->ms_pull_ctl_disable_tbl =
-			rtl8411b_qfn64_ms_pull_ctl_disable_tbl;
-	}
+	if (rtl8411b_is_qfn48(pcr))
+		set_pull_ctrl_tables(rtl8411b_qfn48);
+	else
+		set_pull_ctrl_tables(rtl8411b_qfn64);
 }
diff --git a/include/linux/mfd/rtsx_pci.h b/include/linux/mfd/rtsx_pci.h
index d1382df..f5f3ffc 100644
--- a/include/linux/mfd/rtsx_pci.h
+++ b/include/linux/mfd/rtsx_pci.h
@@ -855,7 +855,7 @@ struct rtsx_pcr {
 	const u32			*ms_pull_ctl_enable_tbl;
 	const u32			*ms_pull_ctl_disable_tbl;
 
-	const struct pcr_ops		*ops;
+	struct pcr_ops			*ops;
 	enum PDEV_STAT			state;
 
 	int				num_slots;
-- 
1.8.1.2

[PATCH 2/4] mfd: rtsx: Generify the switch voltage routine

[Lee Jones]

Other, upcoming devices will want to change the shift value when using
the switch voltage call-back routine. In this patch we will ensure that
unnecessary code duplication shall not occur.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mfd/rtl8411.c        | 12 ++++++++----
 drivers/mfd/rtsx_pcr.c       |  3 ++-
 include/linux/mfd/rtsx_pci.h |  3 ++-
 3 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/mfd/rtl8411.c b/drivers/mfd/rtl8411.c
index 92e9bf6..9bba4e2 100644
--- a/drivers/mfd/rtl8411.c
+++ b/drivers/mfd/rtl8411.c
@@ -191,24 +191,25 @@ static int rtl8411_card_power_off(struct rtsx_pcr *pcr, int card)
 			BPP_LDO_POWB, BPP_LDO_SUSPEND);
 }
 
-static int rtl8411_switch_output_voltage(struct rtsx_pcr *pcr, u8 voltage)
+static int rtl84xx_switch_output_voltage(struct rtsx_pcr *pcr,
+					 u8 voltage, int shift)
 {
 	u8 mask, val;
 	int err;
 
-	mask = (BPP_REG_TUNED18 << BPP_TUNED18_SHIFT_8411) | BPP_PAD_MASK;
+	mask = (BPP_REG_TUNED18 << shift) | BPP_PAD_MASK;
 	if (voltage == OUTPUT_3V3) {
 		err = rtsx_pci_write_register(pcr,
 				SD30_DRIVE_SEL, 0x07, pcr->sd30_drive_sel_3v3);
 		if (err < 0)
 			return err;
-		val = (BPP_ASIC_3V3 << BPP_TUNED18_SHIFT_8411) | BPP_PAD_3V3;
+		val = (BPP_ASIC_3V3 << shift) | BPP_PAD_3V3;
 	} else if (voltage == OUTPUT_1V8) {
 		err = rtsx_pci_write_register(pcr,
 				SD30_DRIVE_SEL, 0x07, pcr->sd30_drive_sel_1v8);
 		if (err < 0)
 			return err;
-		val = (BPP_ASIC_1V8 << BPP_TUNED18_SHIFT_8411) | BPP_PAD_1V8;
+		val = (BPP_ASIC_1V8 << shift) | BPP_PAD_1V8;
 	} else {
 		return -EINVAL;
 	}
@@ -426,6 +427,7 @@ static struct pcr_ops rtl84xx_pcr_generic_ops = {
 	.card_power_on = rtl8411_card_power_on,
 	.card_power_off = rtl8411_card_power_off,
 	.switch_output_voltage = rtl84xx_switch_output_voltage,
+	.voltage_reg_shift = 0,        /* Dynamic */
 	.cd_deglitch = rtl8411_cd_deglitch,
 	.conv_clk_and_div_n = rtl8411_conv_clk_and_div_n,
 	.force_power_down = rtl8411_force_power_down,
@@ -455,6 +457,7 @@ void rtl8411_init_params(struct rtsx_pcr *pcr)
 	/* Device Ops variation. */
 	pcr->ops->fetch_vendor_settings = rtl8411_fetch_vendor_settings;
 	pcr->ops->extra_init_hw = rtl8411_extra_init_hw;
+	pcr->ops->voltage_reg_shift = BPP_TUNED18_SHIFT_8411;
 
 	pcr->rx_initial_phase = SET_CLOCK_PHASE(4, 3, 10);
 
@@ -469,6 +472,7 @@ void rtl8411b_init_params(struct rtsx_pcr *pcr)
 	/* Device Ops variation. */
 	pcr->ops->fetch_vendor_settings = rtl8411b_fetch_vendor_settings;
 	pcr->ops->extra_init_hw = rtl8411b_extra_init_hw;
+	pcr->ops->voltage_reg_shift = BPP_TUNED18_SHIFT_8411;
 
 	pcr->tx_initial_phase = SET_CLOCK_PHASE(23, 7, 14);
 	pcr->rx_initial_phase = SET_CLOCK_PHASE(4, 3, 10);
diff --git a/drivers/mfd/rtsx_pcr.c b/drivers/mfd/rtsx_pcr.c
index e6ae772..f41ee8f 100644
--- a/drivers/mfd/rtsx_pcr.c
+++ b/drivers/mfd/rtsx_pcr.c
@@ -734,7 +734,8 @@ EXPORT_SYMBOL_GPL(rtsx_pci_card_exclusive_check);
 int rtsx_pci_switch_output_voltage(struct rtsx_pcr *pcr, u8 voltage)
 {
 	if (pcr->ops->switch_output_voltage)
-		return pcr->ops->switch_output_voltage(pcr, voltage);
+		return pcr->ops->switch_output_voltage(pcr, voltage,
+					pcr->ops->voltage_reg_shift);
 
 	return 0;
 }
diff --git a/include/linux/mfd/rtsx_pci.h b/include/linux/mfd/rtsx_pci.h
index f5f3ffc..5afc448 100644
--- a/include/linux/mfd/rtsx_pci.h
+++ b/include/linux/mfd/rtsx_pci.h
@@ -774,11 +774,12 @@ struct pcr_ops {
 	int		(*card_power_on)(struct rtsx_pcr *pcr, int card);
 	int		(*card_power_off)(struct rtsx_pcr *pcr, int card);
 	int		(*switch_output_voltage)(struct rtsx_pcr *pcr,
-						u8 voltage);
+						 u8 voltage, int shift);
 	unsigned int	(*cd_deglitch)(struct rtsx_pcr *pcr);
 	int		(*conv_clk_and_div_n)(int clk, int dir);
 	void		(*fetch_vendor_settings)(struct rtsx_pcr *pcr);
 	void		(*force_power_down)(struct rtsx_pcr *pcr, u8 pm_state);
+	int		voltage_reg_shift;
 };
 
 enum PDEV_STAT  {PDEV_STAT_IDLE, PDEV_STAT_RUN};
-- 
1.8.1.2

[PATCH 3/4] mfd: rtsx: Add rtl8402 card reader

[Lee Jones]

This patch adds the new card reader 'rtl8402' to the two already supported
by this driver.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mfd/rtl8411.c  | 16 ++++++++++++++++
 drivers/mfd/rtsx_pcr.c |  5 +++++
 drivers/mfd/rtsx_pcr.h |  1 +
 3 files changed, 22 insertions(+)

diff --git a/drivers/mfd/rtl8411.c b/drivers/mfd/rtl8411.c
index 9bba4e2..0771796 100644
--- a/drivers/mfd/rtl8411.c
+++ b/drivers/mfd/rtl8411.c
@@ -482,3 +482,19 @@ void rtl8411b_init_params(struct rtsx_pcr *pcr)
 	else
 		set_pull_ctrl_tables(rtl8411b_qfn64);
 }
+
+void rtl8402_init_params(struct rtsx_pcr *pcr)
+{
+	/* Fill in common parameters. */
+	rtl84xx_generic_params(pcr);
+
+	/* Device Ops variation. */
+	pcr->ops->fetch_vendor_settings = rtl8411_fetch_vendor_settings;
+	pcr->ops->extra_init_hw = rtl8411_extra_init_hw;
+	pcr->ops->voltage_reg_shift = BPP_TUNED18_SHIFT_8402;
+
+	pcr->tx_initial_phase = SET_CLOCK_PHASE(23, 7, 14);
+	pcr->rx_initial_phase = SET_CLOCK_PHASE(4, 3, 10);
+
+	set_pull_ctrl_tables(rtl8411);
+}
diff --git a/drivers/mfd/rtsx_pcr.c b/drivers/mfd/rtsx_pcr.c
index f41ee8f..e5dbe67 100644
--- a/drivers/mfd/rtsx_pcr.c
+++ b/drivers/mfd/rtsx_pcr.c
@@ -56,6 +56,7 @@ static DEFINE_PCI_DEVICE_TABLE(rtsx_pci_ids) = {
 	{ PCI_DEVICE(0x10EC, 0x5289), PCI_CLASS_OTHERS << 16, 0xFF0000 },
 	{ PCI_DEVICE(0x10EC, 0x5227), PCI_CLASS_OTHERS << 16, 0xFF0000 },
 	{ PCI_DEVICE(0x10EC, 0x5249), PCI_CLASS_OTHERS << 16, 0xFF0000 },
+	{ PCI_DEVICE(0x10EC, 0x5286), PCI_CLASS_OTHERS << 16, 0xFF0000 },
 	{ PCI_DEVICE(0x10EC, 0x5287), PCI_CLASS_OTHERS << 16, 0xFF0000 },
 	{ 0, }
 };
@@ -1047,6 +1048,10 @@ static int rtsx_pci_init_chip(struct rtsx_pcr *pcr)
 		rts5229_init_params(pcr);
 		break;
 
+	case 0x5286:
+		rtl8402_init_params(pcr);
+		break;
+
 	case 0x5289:
 		rtl8411_init_params(pcr);
 		break;
diff --git a/drivers/mfd/rtsx_pcr.h b/drivers/mfd/rtsx_pcr.h
index 947e79b..8cac8db 100644
--- a/drivers/mfd/rtsx_pcr.h
+++ b/drivers/mfd/rtsx_pcr.h
@@ -29,6 +29,7 @@
 
 void rts5209_init_params(struct rtsx_pcr *pcr);
 void rts5229_init_params(struct rtsx_pcr *pcr);
+void rtl8402_init_params(struct rtsx_pcr *pcr);
 void rtl8411_init_params(struct rtsx_pcr *pcr);
 void rts5227_init_params(struct rtsx_pcr *pcr);
 void rts5249_init_params(struct rtsx_pcr *pcr);
-- 
1.8.1.2

[PATCH 4/4] mfd: rtsx: Prevent 'used uninitialised' warnings

[Lee Jones]

drivers/mfd/rtl8411.c: In function ‘rtl8411_fetch_vendor_settings’:
drivers/mfd/rtl8411.c:58:7: warning: ‘reg1’ is used uninitialized in this function [-Wuninitialized]
drivers/mfd/rtl8411.c: In function ‘rtl8411b_fetch_vendor_settings’:
drivers/mfd/rtl8411.c:79:7: warning: ‘reg’ is used uninitialized in this function [-Wuninitialized]
drivers/mfd/rtl8411.c: In function ‘rtl8411_fetch_vendor_settings’:
drivers/mfd/rtl8411.c:69:26: warning: ‘reg3’ may be used uninitialized in this function [-Wuninitialized]

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mfd/rtl8411.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/mfd/rtl8411.c b/drivers/mfd/rtl8411.c
index 0771796..cb8416c 100644
--- a/drivers/mfd/rtl8411.c
+++ b/drivers/mfd/rtl8411.c
@@ -49,8 +49,8 @@ static int rtl8411b_is_qfn48(struct rtsx_pcr *pcr)
 
 static void rtl8411_fetch_vendor_settings(struct rtsx_pcr *pcr)
 {
-	u32 reg1;
-	u8 reg3;
+	u32 reg1 = 0;
+	u8 reg3 = 0;
 
 	rtsx_pci_read_config_dword(pcr, PCR_SETTING_REG1, &reg1);
 	dev_dbg(&(pcr->pci->dev), "Cfg 0x%x: 0x%x\n", PCR_SETTING_REG1, reg1);
@@ -71,7 +71,7 @@ static void rtl8411_fetch_vendor_settings(struct rtsx_pcr *pcr)
 
 static void rtl8411b_fetch_vendor_settings(struct rtsx_pcr *pcr)
 {
-	u32 reg;
+	u32 reg = 0;
 
 	rtsx_pci_read_config_dword(pcr, PCR_SETTING_REG1, &reg);
 	dev_dbg(&(pcr->pci->dev), "Cfg 0x%x: 0x%x\n", PCR_SETTING_REG1, reg);
-- 
1.8.1.2

Re: [PATCH 2/4] mfd: rtsx: Generify the switch voltage routine

[micky]

Hi Lee

Thank you for your work.

Switch output voltage may have some problem here.
1. modify params of function in pcr_ops may affect other chips, like
rts52xx, and we may add other registers or value in the future, just
add params may not enough to handle this case. So I think different
chip should use a different function here, and modify the function
will not affect others.

2. rtl8402 has another difference with 8411 here, it doesn't use
  BPP_ASIC_1V8, but use BPP_ASIC_2V0.

so, I will send another patch to fix this problem.

> -static int rtl8411_switch_output_voltage(struct rtsx_pcr *pcr, u8 voltage)
> +static int rtl84xx_switch_output_voltage(struct rtsx_pcr *pcr,
> +					 u8 voltage, int shift)
>   {
>   	u8 mask, val;
>   	int err;
>   
> -	mask = (BPP_REG_TUNED18 << BPP_TUNED18_SHIFT_8411) | BPP_PAD_MASK;
> +	mask = (BPP_REG_TUNED18 << shift) | BPP_PAD_MASK;
>   	if (voltage == OUTPUT_3V3) {
>   		err = rtsx_pci_write_register(pcr,
>   				SD30_DRIVE_SEL, 0x07, pcr->sd30_drive_sel_3v3);
>   		if (err < 0)
>   			return err;
> -		val = (BPP_ASIC_3V3 << BPP_TUNED18_SHIFT_8411) | BPP_PAD_3V3;
> +		val = (BPP_ASIC_3V3 << shift) | BPP_PAD_3V3;
>   	} else if (voltage == OUTPUT_1V8) {
>   		err = rtsx_pci_write_register(pcr,
>   				SD30_DRIVE_SEL, 0x07, pcr->sd30_drive_sel_1v8);
>   		if (err < 0)
>   			return err;
> -		val = (BPP_ASIC_1V8 << BPP_TUNED18_SHIFT_8411) | BPP_PAD_1V8;
> +		val = (BPP_ASIC_1V8 << shift) | BPP_PAD_1V8;
>   	} else {
>   		return -EINVAL;
>   	}


-- 
Best Regards
Micky.

Re: [PATCH 1/4] mfd: rtsx: Generify call-back operations in orderto reduce code duplication

[micky]

On 11/14/2013 05:29 PM, Lee Jones wrote:
> +	if (rtl8411b_is_qfn48(pcr))
> +		set_pull_ctrl_tables(rtl8411b_qfn48);
> +	else
> +		set_pull_ctrl_tables(rtl8411b_qfn64);
Here will cause some problem, set_pull_ctrl_tables() include not a single
statements, will make the if-else not work correctly.

-- 
Best Regards
Micky.

Re: [PATCH 2/4] mfd: rtsx: Generify the switch voltage routine

[Lee Jones]

> Switch output voltage may have some problem here.
> 1. modify params of function in pcr_ops may affect other chips, like
> rts52xx, and we may add other registers or value in the future, just
> add params may not enough to handle this case. So I think different
> chip should use a different function here, and modify the function
> will not affect others.

Okay, I agree to a certain extent. I'll drop this patch for now.

Can you please try to find a way to consolidate them. I'm really not
keen on keeping 6 functions around that are mostly identical.

> 2. rtl8402 has another difference with 8411 here, it doesn't use
>  BPP_ASIC_1V8, but use BPP_ASIC_2V0.
> 
> so, I will send another patch to fix this problem.
> 
> >-static int rtl8411_switch_output_voltage(struct rtsx_pcr *pcr, u8 voltage)
> >+static int rtl84xx_switch_output_voltage(struct rtsx_pcr *pcr,
> >+					 u8 voltage, int shift)
> >  {
> >  	u8 mask, val;
> >  	int err;
> >-	mask = (BPP_REG_TUNED18 << BPP_TUNED18_SHIFT_8411) | BPP_PAD_MASK;
> >+	mask = (BPP_REG_TUNED18 << shift) | BPP_PAD_MASK;
> >  	if (voltage == OUTPUT_3V3) {
> >  		err = rtsx_pci_write_register(pcr,
> >  				SD30_DRIVE_SEL, 0x07, pcr->sd30_drive_sel_3v3);
> >  		if (err < 0)
> >  			return err;
> >-		val = (BPP_ASIC_3V3 << BPP_TUNED18_SHIFT_8411) | BPP_PAD_3V3;
> >+		val = (BPP_ASIC_3V3 << shift) | BPP_PAD_3V3;
> >  	} else if (voltage == OUTPUT_1V8) {
> >  		err = rtsx_pci_write_register(pcr,
> >  				SD30_DRIVE_SEL, 0x07, pcr->sd30_drive_sel_1v8);
> >  		if (err < 0)
> >  			return err;
> >-		val = (BPP_ASIC_1V8 << BPP_TUNED18_SHIFT_8411) | BPP_PAD_1V8;
> >+		val = (BPP_ASIC_1V8 << shift) | BPP_PAD_1V8;
> >  	} else {
> >  		return -EINVAL;
> >  	}
> 
> 

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog