[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....@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

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

[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....@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;

[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(-)

[Lee Jones]

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

Signed-off-by: Lee Jones <lee....@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

[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....@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

[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.

[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.

[micky...@realsil.com.cn]

From: Micky Ching <micky...@realsil.com.cn>

With the recent added support request of yet another device, the

burden of duplicated code was becoming a little messy. To rectify is,
we init rtl8411-like chips to 8411 param first, then modify the
different values according each chip.

Lee Jones (2):
mfd: rtsx: reduce code duplication in rtl8411
mfd: rtsx: Prevent 'used uninitialised' warnings

Micky Ching (1):
mfd: rtsx: add card reader rtl8402

drivers/mfd/rtl8411.c | 111 +++++++++++++++++++++++-------------------------
drivers/mfd/rtsx_pcr.c | 9 ++--
drivers/mfd/rtsx_pcr.h | 9 +++-
3 files changed, 65 insertions(+), 64 deletions(-)

--
1.7.9.5

[micky...@realsil.com.cn]

From: Micky Ching <micky...@realsil.com.cn>

Add card reader rtl8042, rtl8402 is much like rtl8411, so just add it to
rtl8411.c

Signed-off-by: Micky Ching <micky...@realsil.com.cn>
---
drivers/mfd/rtl8411.c | 29 +++++++++++++++++++++++++++++
drivers/mfd/rtsx_pcr.c | 2 ++
2 files changed, 31 insertions(+)

diff --git a/drivers/mfd/rtl8411.c b/drivers/mfd/rtl8411.c
index 4de1045..8e4b546 100644
--- a/drivers/mfd/rtl8411.c
+++ b/drivers/mfd/rtl8411.c
@@ -216,6 +216,31 @@ static int rtl8411_switch_output_voltage(struct rtsx_pcr *pcr, u8 voltage)
return rtsx_pci_write_register(pcr, LDO_CTL, mask, val);
}

+static int rtl8402_switch_output_voltage(struct rtsx_pcr *pcr, u8 voltage)
+{
+ u8 mask, val;
+ int err;
+
+ mask = (BPP_REG_TUNED18 << BPP_TUNED18_SHIFT_8402) | 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_8402) | 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_2V0 << BPP_TUNED18_SHIFT_8402) | BPP_PAD_1V8;
+ } else {
+ return -EINVAL;
+ }
+
+ return rtsx_pci_write_register(pcr, LDO_CTL, mask, val);
+}
+
static unsigned int rtl8411_cd_deglitch(struct rtsx_pcr *pcr)
{
unsigned int card_exist;
@@ -460,6 +485,10 @@ void rtl8411_init_params(struct rtsx_pcr *pcr)
set_pull_ctrl_tables(rtl8411b_qfn64);

break;
+ case 0x5286:
+ rtl8411_pcr_ops.switch_output_voltage =
+ rtl8402_switch_output_voltage;
+ break;
}

pcr->ops = &rtl8411_pcr_ops;
diff --git a/drivers/mfd/rtsx_pcr.c b/drivers/mfd/rtsx_pcr.c
index ecc6852..865d209 100644
--- a/drivers/mfd/rtsx_pcr.c
+++ b/drivers/mfd/rtsx_pcr.c
@@ -57,6 +57,7 @@ static DEFINE_PCI_DEVICE_TABLE(rtsx_pci_ids) = {

{ PCI_DEVICE(0x10EC, 0x5227), PCI_CLASS_OTHERS << 16, 0xFF0000 },
{ PCI_DEVICE(0x10EC, 0x5249), PCI_CLASS_OTHERS << 16, 0xFF0000 },

{ PCI_DEVICE(0x10EC, 0x5287), PCI_CLASS_OTHERS << 16, 0xFF0000 },
+ { PCI_DEVICE(0x10EC, 0x5286), PCI_CLASS_OTHERS << 16, 0xFF0000 },

{ 0, }
};

@@ -1056,6 +1057,7 @@ static int rtsx_pci_init_chip(struct rtsx_pcr *pcr)

case 0x5287:
case 0x5289:
+ case 0x5286:
rtl8411_init_params(pcr);
break;

[micky...@realsil.com.cn]

From: Lee Jones <lee....@linaro.org>

in order to remove duplicated code in rtl8411, we make 8411 as the base
init params, and other like-8411 chips will just change the different
value with 8411, this can save some source code.

Signed-off-by: Lee Jones <lee....@linaro.org>

Signed-off-by: Micky Ching <micky...@realsil.com.cn>
---

drivers/mfd/rtl8411.c | 76 +++++++++++++-----------------------------------
drivers/mfd/rtsx_pcr.c | 7 ++---
drivers/mfd/rtsx_pcr.h | 9 +++++-
3 files changed, 31 insertions(+), 61 deletions(-)

diff --git a/drivers/mfd/rtl8411.c b/drivers/mfd/rtl8411.c
index 5280135..4de1045 100644
--- a/drivers/mfd/rtl8411.c
+++ b/drivers/mfd/rtl8411.c
@@ -279,7 +279,7 @@ static int rtl8411_conv_clk_and_div_n(int input, int dir)

return output;
}

-static const struct pcr_ops rtl8411_pcr_ops = {

+static struct pcr_ops rtl8411_pcr_ops = {
.fetch_vendor_settings = rtl8411_fetch_vendor_settings,
.extra_init_hw = rtl8411_extra_init_hw,
.optimize_phy = NULL,
@@ -295,22 +295,6 @@ static const struct pcr_ops rtl8411_pcr_ops = {
.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,11 +425,10 @@ static const u32 rtl8411b_qfn48_ms_pull_ctl_disable_tbl[] = {

0,
};

-void rtl8411_init_params(struct rtsx_pcr *pcr)

+void rtl8411_init_base_params(struct rtsx_pcr *pcr)
{

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;

@@ -456,45 +439,28 @@ void rtl8411_init_params(struct rtsx_pcr *pcr)

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 rtl8411b_init_params(struct rtsx_pcr *pcr)
+void rtl8411_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;

-

- 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);

+ /* rtl8411 params */
+ rtl8411_init_base_params(pcr);
+ set_pull_ctrl_tables(rtl8411);
+
+ /* different with rtl8411 */
+ switch (PCI_PID(pcr)) {
+ case 0x5287:
+ rtl8411_pcr_ops.fetch_vendor_settings =
+ rtl8411b_fetch_vendor_settings;
+ rtl8411_pcr_ops.extra_init_hw = rtl8411b_extra_init_hw;
+

+ if (rtl8411b_is_qfn48(pcr))
+ set_pull_ctrl_tables(rtl8411b_qfn48);
+ else
+ set_pull_ctrl_tables(rtl8411b_qfn64);

- 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;

+ break;
}
+
+ pcr->ops = &rtl8411_pcr_ops;
}
diff --git a/drivers/mfd/rtsx_pcr.c b/drivers/mfd/rtsx_pcr.c
index 11e20af..ecc6852 100644
--- a/drivers/mfd/rtsx_pcr.c
+++ b/drivers/mfd/rtsx_pcr.c
@@ -1046,10 +1046,6 @@ static int rtsx_pci_init_chip(struct rtsx_pcr *pcr)
rts5229_init_params(pcr);
break;

- case 0x5289:
- rtl8411_init_params(pcr);
- break;
-
case 0x5227:
rts5227_init_params(pcr);
break;
@@ -1059,7 +1055,8 @@ static int rtsx_pci_init_chip(struct rtsx_pcr *pcr)
break;

case 0x5287:
- rtl8411b_init_params(pcr);
+ case 0x5289:
+ rtl8411_init_params(pcr);

break;
}

diff --git a/drivers/mfd/rtsx_pcr.h b/drivers/mfd/rtsx_pcr.h

index 947e79b..dd435d7 100644
--- a/drivers/mfd/rtsx_pcr.h
+++ b/drivers/mfd/rtsx_pcr.h
@@ -32,7 +32,6 @@ void rts5229_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);

-void rtl8411b_init_params(struct rtsx_pcr *pcr);

static inline u8 map_sd_drive(int idx)
{
@@ -63,4 +62,12 @@ static inline u8 map_sd_drive(int idx)
#define rtl8411_reg_to_sd30_drive_sel_3v3(reg) (((reg) >> 5) & 0x07)
#define rtl8411b_reg_to_sd30_drive_sel_3v3(reg) ((reg) & 0x03)

+#define set_pull_ctrl_tables(__device) \
+do { \

+ 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; \

+} while (0)
+
#endif

[micky...@realsil.com.cn]

From: Lee Jones <lee....@linaro.org>

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

Signed-off-by: Lee Jones <lee....@linaro.org>
Signed-off-by: Micky Ching <micky...@realsil.com.cn>
---

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 8e4b546..948b5c7 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);

[Lee Jones]

> From: Lee Jones <lee....@linaro.org>
>
> in order to remove duplicated code in rtl8411, we make 8411 as the base
> init params, and other like-8411 chips will just change the different
> value with 8411, this can save some source code.
>
> Signed-off-by: Lee Jones <lee....@linaro.org>
> Signed-off-by: Micky Ching <micky...@realsil.com.cn>

It's not good etiquette to send patches 'From:' and 'Signed-off-by:' a
person when they are neither from or signed-off by that person.

It's much better practice to reply to the original patches with
comments placed directly under the code you wish to reference.

<snip>

> -void rtl8411b_init_params(struct rtsx_pcr *pcr)
> +void rtl8411_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;
> -
> - 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);

So what happened to these?

> - pcr->ic_version = rtl8411_get_ic_version(pcr);
> + /* rtl8411 params */
> + rtl8411_init_base_params(pcr);
> + set_pull_ctrl_tables(rtl8411);
> +
> + /* different with rtl8411 */
> + switch (PCI_PID(pcr)) {
> + case 0x5287:
> + rtl8411_pcr_ops.fetch_vendor_settings =
> + rtl8411b_fetch_vendor_settings;
> + rtl8411_pcr_ops.extra_init_hw = rtl8411b_extra_init_hw;
> +
> + if (rtl8411b_is_qfn48(pcr))
> + set_pull_ctrl_tables(rtl8411b_qfn48);
> + else
> + set_pull_ctrl_tables(rtl8411b_qfn64);

I'm not a big fan of this.

<snip>

> diff --git a/drivers/mfd/rtsx_pcr.c b/drivers/mfd/rtsx_pcr.c
> index 11e20af..ecc6852 100644
> --- a/drivers/mfd/rtsx_pcr.c
> +++ b/drivers/mfd/rtsx_pcr.c
> @@ -1046,10 +1046,6 @@ static int rtsx_pci_init_chip(struct rtsx_pcr *pcr)
> rts5229_init_params(pcr);
> break;
>
> - case 0x5289:
> - rtl8411_init_params(pcr);
> - break;
> -
> case 0x5227:
> rts5227_init_params(pcr);
> break;
> @@ -1059,7 +1055,8 @@ static int rtsx_pci_init_chip(struct rtsx_pcr *pcr)
> break;
>
> case 0x5287:
> - rtl8411b_init_params(pcr);
> + case 0x5289:
> + rtl8411_init_params(pcr);
> break;
> }

I see where you're going with this, but my personal opinion is that it
looks neater and more readable set out as two separate init functions.

> diff --git a/drivers/mfd/rtsx_pcr.h b/drivers/mfd/rtsx_pcr.h
> index 947e79b..dd435d7 100644
> --- a/drivers/mfd/rtsx_pcr.h
> +++ b/drivers/mfd/rtsx_pcr.h
> @@ -32,7 +32,6 @@ void rts5229_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);
> -void rtl8411b_init_params(struct rtsx_pcr *pcr);
>
> static inline u8 map_sd_drive(int idx)
> {
> @@ -63,4 +62,12 @@ static inline u8 map_sd_drive(int idx)
> #define rtl8411_reg_to_sd30_drive_sel_3v3(reg) (((reg) >> 5) & 0x07)
> #define rtl8411b_reg_to_sd30_drive_sel_3v3(reg) ((reg) & 0x03)
>
> +#define set_pull_ctrl_tables(__device) \
> +do { \
> + 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; \
> +} while (0)

Great spot Micky. I'll fix this up and resend.

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

[Lee Jones]

On Fri, 15 Nov 2013, micky...@realsil.com.cn wrote:

> From: Lee Jones <lee....@linaro.org>
>
> drivers/mfd/rtl8411.c: In function -F¡rtl8411_fetch_vendor_settings¢: -A
> drivers/mfd/rtl8411.c:58:7: warning: -F¡reg1¢ is used uninitialized in this function [-Wuninitialized] -A
> drivers/mfd/rtl8411.c: In function -F¡rtl8411b_fetch_vendor_settings¢: -A
> drivers/mfd/rtl8411.c:79:7: warning: -F¡reg¢ is used uninitialized in this function [-Wuninitialized] -A
> drivers/mfd/rtl8411.c: In function -F¡rtl8411_fetch_vendor_settings¢: -A
> drivers/mfd/rtl8411.c:69:26: warning: -F¡reg3¢ may be used uninitialized in this function [-Wuninitialized] -A

Something wrong with your email client here.

Using `git send-mail` is preferable.

Please read:
Documentation/email-clients.txt

> Signed-off-by: Lee Jones <lee....@linaro.org>
> Signed-off-by: Micky Ching <micky...@realsil.com.cn>

All you need to do with this patch is reply to the one I sent with an
Acked-by, Reviewed-by or Tested-by.

Please read:
Documentation/SubmittingPatches

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

[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.

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

[Lee Jones]

On Fri, 15 Nov 2013, micky...@realsil.com.cn wrote:

> From: Micky Ching <micky...@realsil.com.cn>
>
> Add card reader rtl8042, rtl8402 is much like rtl8411, so just add it to
> rtl8411.c
>
> Signed-off-by: Micky Ching <micky...@realsil.com.cn>
> ---
> drivers/mfd/rtl8411.c | 29 +++++++++++++++++++++++++++++
> drivers/mfd/rtsx_pcr.c | 2 ++
> 2 files changed, 31 insertions(+)

Looks like you left me off of the addressee list for this and the
cover letter.

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