[PATCH v3 0/2] mfd: rtsx: decrease driver size and add new device

public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed

* [PATCH v3 0/2] mfd: rtsx: decrease driver size and add new device
@ 2013-12-17  2:36 micky_ching
  2013-12-17  2:36 ` [PATCH v3 1/2] mfd: rtsx: reduce code duplication in rtl8411 micky_ching
  2013-12-17  2:36 ` [PATCH v3 2/2] mfd: rtsx: add card reader rtl8402 micky_ching
  0 siblings, 2 replies; 6+ messages in thread
From: micky_ching @ 2013-12-17  2:36 UTC (permalink / raw)
  To: sameo; +Cc: devel, linux-kernel, gregkh, rogerable, wei_wang, Micky Ching

From: Micky Ching <micky_ching@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. And rtl8402 is supported from
this patch.

Micky Ching (2):
  mfd: rtsx: reduce code duplication in rtl8411
  mfd: rtsx: add card reader rtl8402

 drivers/mfd/rtl8411.c  |   89 ++++++++++++++++++------------------------------
 drivers/mfd/rtsx_pcr.c |    5 +++
 drivers/mfd/rtsx_pcr.h |    9 +++++
 3 files changed, 48 insertions(+), 55 deletions(-)

--
1.7.9.5

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v3 1/2] mfd: rtsx: reduce code duplication in rtl8411
  2013-12-17  2:36 [PATCH v3 0/2] mfd: rtsx: decrease driver size and add new device micky_ching
@ 2013-12-17  2:36 ` micky_ching
  2013-12-17  7:28   ` Dan Carpenter
  2013-12-17  8:02   ` Dan Carpenter
  2013-12-17  2:36 ` [PATCH v3 2/2] mfd: rtsx: add card reader rtl8402 micky_ching
  1 sibling, 2 replies; 6+ messages in thread
From: micky_ching @ 2013-12-17  2:36 UTC (permalink / raw)
  To: sameo
  Cc: devel, linux-kernel, gregkh, rogerable, wei_wang, Micky Ching,
	Lee Jones

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

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.jones@linaro.org>
Signed-off-by: Micky Ching <micky_ching@realsil.com.cn>
---
 drivers/mfd/rtl8411.c  |   63 +++++++++---------------------------------------
 drivers/mfd/rtsx_pcr.h |    8 ++++++
 2 files changed, 19 insertions(+), 52 deletions(-)

diff --git a/drivers/mfd/rtl8411.c b/drivers/mfd/rtl8411.c
index 5280135..c9eab9d 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
@@ -456,45 +440,20 @@ 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;
+
+	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;
-
-	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);
+	rtl8411_init_params(pcr);
 
-	pcr->ic_version = rtl8411_get_ic_version(pcr);
+	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)) {
-		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/drivers/mfd/rtsx_pcr.h b/drivers/mfd/rtsx_pcr.h
index 947e79b..26b52ec 100644
--- a/drivers/mfd/rtsx_pcr.h
+++ b/drivers/mfd/rtsx_pcr.h
@@ -63,4 +63,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
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 1/2] mfd: rtsx: reduce code duplication in rtl8411
  2013-12-17  2:36 ` [PATCH v3 1/2] mfd: rtsx: reduce code duplication in rtl8411 micky_ching
@ 2013-12-17  7:28   ` Dan Carpenter
  2013-12-18  1:17     ` micky
  2013-12-17  8:02   ` Dan Carpenter
  1 sibling, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2013-12-17  7:28 UTC (permalink / raw)
  To: micky_ching
  Cc: sameo, gregkh, linux-kernel, wei_wang, rogerable, devel,
	Lee Jones

On Tue, Dec 17, 2013 at 10:36:58AM +0800, micky_ching@realsil.com.cn wrote:
> diff --git a/drivers/mfd/rtsx_pcr.h b/drivers/mfd/rtsx_pcr.h
> index 947e79b..26b52ec 100644
> --- a/drivers/mfd/rtsx_pcr.h
> +++ b/drivers/mfd/rtsx_pcr.h
> @@ -63,4 +63,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

This is nasty...  With readable code, you should understand just from
looking at it what the code is doing but this obscures it completely.
It shouldn't reference the "pcr" variable without passing it in as an
argument but even with that cleanup I don't really like it.

To be honest, I don't see the problem with the original code.

regards,
dan carpenter

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 1/2] mfd: rtsx: reduce code duplication in rtl8411
  2013-12-17  7:28   ` Dan Carpenter
@ 2013-12-18  1:17     ` micky
  0 siblings, 0 replies; 6+ messages in thread
From: micky @ 2013-12-18  1:17 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: sameo, gregkh, linux-kernel, wei_wang, rogerable, devel,
	Lee Jones

On 12/17/2013 03:28 PM, Dan Carpenter wrote:
> On Tue, Dec 17, 2013 at 10:36:58AM +0800, micky_ching@realsil.com.cn wrote:
>> diff --git a/drivers/mfd/rtsx_pcr.h b/drivers/mfd/rtsx_pcr.h
>> index 947e79b..26b52ec 100644
>> --- a/drivers/mfd/rtsx_pcr.h
>> +++ b/drivers/mfd/rtsx_pcr.h
>> @@ -63,4 +63,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
> This is nasty...  With readable code, you should understand just from
> looking at it what the code is doing but this obscures it completely.
> It shouldn't reference the "pcr" variable without passing it in as an
> argument but even with that cleanup I don't really like it.
>
> To be honest, I don't see the problem with the original code.
>
>
Using macro can reduce some code, and define as set_pull_ctl_tables(pcr, 
dev)
is much better for understand

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 1/2] mfd: rtsx: reduce code duplication in rtl8411
  2013-12-17  2:36 ` [PATCH v3 1/2] mfd: rtsx: reduce code duplication in rtl8411 micky_ching
  2013-12-17  7:28   ` Dan Carpenter
@ 2013-12-17  8:02   ` Dan Carpenter
  1 sibling, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2013-12-17  8:02 UTC (permalink / raw)
  To: micky_ching
  Cc: sameo, gregkh, linux-kernel, wei_wang, rogerable, devel,
	Lee Jones

On Tue, Dec 17, 2013 at 10:36:58AM +0800, micky_ching@realsil.com.cn wrote:
>  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;
> -
> -	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);
> +	rtl8411_init_params(pcr);
>  
> -	pcr->ic_version = rtl8411_get_ic_version(pcr);
> +	rtl8411_pcr_ops.fetch_vendor_settings =
> +		rtl8411b_fetch_vendor_settings;
> +	rtl8411_pcr_ops.extra_init_hw = rtl8411b_extra_init_hw;
>  

This is a bug here.  If we have both kinds of devices connected at the
same time then the kernel crashes.

Really structures which hold function pointers should be const.  This
code is not as good as the original.

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v3 2/2] mfd: rtsx: add card reader rtl8402
  2013-12-17  2:36 [PATCH v3 0/2] mfd: rtsx: decrease driver size and add new device micky_ching
  2013-12-17  2:36 ` [PATCH v3 1/2] mfd: rtsx: reduce code duplication in rtl8411 micky_ching
@ 2013-12-17  2:36 ` micky_ching
  1 sibling, 0 replies; 6+ messages in thread
From: micky_ching @ 2013-12-17  2:36 UTC (permalink / raw)
  To: sameo; +Cc: devel, linux-kernel, gregkh, rogerable, wei_wang, Micky Ching

From: Micky Ching <micky_ching@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_ching@realsil.com.cn>
---
 drivers/mfd/rtl8411.c  |   28 ++++++++++++++++++++++++----
 drivers/mfd/rtsx_pcr.c |    5 +++++
 drivers/mfd/rtsx_pcr.h |    1 +
 3 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/drivers/mfd/rtl8411.c b/drivers/mfd/rtl8411.c
index c9eab9d..01eb31b 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 rtl8411_do_switch_output_voltage(struct rtsx_pcr *pcr, u8 voltage,
+		int bpp_tuned18_shift, int bpp_asic_1v8)
 {
 	u8 mask, val;
 	int err;
 
-	mask = (BPP_REG_TUNED18 << BPP_TUNED18_SHIFT_8411) | BPP_PAD_MASK;
+	mask = (BPP_REG_TUNED18 << bpp_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 << bpp_tuned18_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 << bpp_tuned18_shift) | BPP_PAD_1V8;
 	} else {
 		return -EINVAL;
 	}
@@ -216,6 +217,18 @@ static int rtl8411_switch_output_voltage(struct rtsx_pcr *pcr, u8 voltage)
 	return rtsx_pci_write_register(pcr, LDO_CTL, mask, val);
 }
 
+static int rtl8411_switch_output_voltage(struct rtsx_pcr *pcr, u8 voltage)
+{
+	return rtl8411_do_switch_output_voltage(pcr, voltage,
+			BPP_TUNED18_SHIFT_8411, BPP_ASIC_1V8);
+}
+
+static int rtl8402_switch_output_voltage(struct rtsx_pcr *pcr, u8 voltage)
+{
+	return rtl8411_do_switch_output_voltage(pcr, voltage,
+			BPP_TUNED18_SHIFT_8402, BPP_ASIC_2V0);
+}
+
 static unsigned int rtl8411_cd_deglitch(struct rtsx_pcr *pcr)
 {
 	unsigned int card_exist;
@@ -457,3 +470,10 @@ void rtl8411b_init_params(struct rtsx_pcr *pcr)
 	else
 		set_pull_ctrl_tables(rtl8411b_qfn64);
 }
+
+void rtl8402_init_params(struct rtsx_pcr *pcr)
+{
+	rtl8411_init_params(pcr);
+
+	rtl8411_pcr_ops.switch_output_voltage = rtl8402_switch_output_voltage;
+}
diff --git a/drivers/mfd/rtsx_pcr.c b/drivers/mfd/rtsx_pcr.c
index 11e20af..2af55bb 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, }
 };
 
@@ -1061,6 +1062,10 @@ static int rtsx_pci_init_chip(struct rtsx_pcr *pcr)
 	case 0x5287:
 		rtl8411b_init_params(pcr);
 		break;
+
+	case 0x5286:
+		rtl8402_init_params(pcr);
+		break;
 	}
 
 	dev_dbg(&(pcr->pci->dev), "PID: 0x%04x, IC version: 0x%02x\n",
diff --git a/drivers/mfd/rtsx_pcr.h b/drivers/mfd/rtsx_pcr.h
index 26b52ec..c097dbd 100644
--- a/drivers/mfd/rtsx_pcr.h
+++ b/drivers/mfd/rtsx_pcr.h
@@ -30,6 +30,7 @@
 void rts5209_init_params(struct rtsx_pcr *pcr);
 void rts5229_init_params(struct rtsx_pcr *pcr);
 void rtl8411_init_params(struct rtsx_pcr *pcr);
+void rtl8402_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);
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2013-12-18  1:15 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-12-17  2:36 [PATCH v3 0/2] mfd: rtsx: decrease driver size and add new device micky_ching
2013-12-17  2:36 ` [PATCH v3 1/2] mfd: rtsx: reduce code duplication in rtl8411 micky_ching
2013-12-17  7:28   ` Dan Carpenter
2013-12-18  1:17     ` micky
2013-12-17  8:02   ` Dan Carpenter
2013-12-17  2:36 ` [PATCH v3 2/2] mfd: rtsx: add card reader rtl8402 micky_ching

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox