Re: [PATCH 09/10] mfd: rtsx: add support for rts525A

Lee Jones
2015-01-18T11:13:50Z

On Thu, 15 Jan 2015, micky_ching@realsil.com.cn wrote:

> From: Micky Ching <micky_ching@realsil.com.cn>
>
> add support for new chip rts525A.
>
> Signed-off-by: Micky Ching <micky_ching@realsil.com.cn>
> ---
> drivers/mfd/rts5249.c | 93 +++++++++++++++++++++++++++++++++++++++++++++++++-
> drivers/mfd/rtsx_pcr.c | 13 +++++--
> drivers/mfd/rtsx_pcr.h | 1 +
> 3 files changed, 103 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mfd/rts5249.c b/drivers/mfd/rts5249.c
> index 1ce03a6..46b6522 100644
> --- a/drivers/mfd/rts5249.c
> +++ b/drivers/mfd/rts5249.c
> @@ -97,7 +97,7 @@ static void rts5249_force_power_down(struct rtsx_pcr *pcr, u8 pm_state)
> rtsx_pci_write_register(pcr, AUTOLOAD_CFG_BASE + 3, 0x01, 0);
>
> if (pm_state == HOST_ENTER_S3) {
> - if (PCI_PID(pcr) == 0x524A)
> + if (PCI_PID(pcr) == 0x524A || PCI_PID(pcr) == 0x525A)

Shouldn't these be defined somewhere?

I have a particular distaste for magic numbers.

> rtsx_pci_write_register(pcr, RTS524A_PM_CTRL3,
> D3_DELINK_MODE_EN, D3_DELINK_MODE_EN);
> else if (PCI_PID(pcr) == 0x5249)
> @@ -430,3 +430,94 @@ void rts524a_init_params(struct rtsx_pcr *pcr)
> pcr->ops = &rts524a_pcr_ops;
> }
>
> +static int rts525a_card_power_on(struct rtsx_pcr *pcr, int card)
> +{
> + rtsx_pci_write_register(pcr, LDO_VCC_CFG1,
> + LDO_VCC_TUNE_MASK, LDO_VCC_3V3);
> + return rts5249_card_power_on(pcr, card);
> +}

Small formatting nit: I'd prefer if you'd line up the wrap to the '('
and leave a '\n' before the return line.

> +static int rts525a_switch_output_voltage(struct rtsx_pcr *pcr, u8 voltage)
> +{
> + if (voltage == OUTPUT_3V3) {
> + rtsx_pci_write_register(pcr, LDO_CONFIG2,
> + LDO_D3318_MASK, LDO_D3318_33V);
> + rtsx_pci_write_register(pcr, SD_PAD_CTL, SD_IO_USING_1V8, 0);
> + } else if (voltage == OUTPUT_1V8) {
> + rtsx_pci_write_register(pcr, LDO_CONFIG2,
> + LDO_D3318_MASK, LDO_D3318_18V);
> + rtsx_pci_write_register(pcr, SD_PAD_CTL, SD_IO_USING_1V8,
> + SD_IO_USING_1V8);
> + } else {
> + return -EINVAL;
> + }

This might look nicer as a switch().

> + rtsx_pci_init_cmd(pcr);
> + rts5249_fill_driving(pcr, voltage);
> + return rtsx_pci_send_cmd(pcr, 100);
> +}
> +
> +static int rts525a_optimize_phy(struct rtsx_pcr *pcr)
> +{
> + int err;
> +
> + err = rtsx_pci_write_register(pcr, RTS524A_PM_CTRL3,
> + D3_DELINK_MODE_EN, 0x00);
> + if (err < 0)
> + return err;

rtsx_pci_write_register() can't return >0, so just check for (err).

> + rtsx_pci_write_phy_register(pcr, 0x1D, 0x99FF);
> + rtsx_pci_write_phy_register(pcr, 0x03, 0x2748);

More magic numbers that I have absolutely no idea what the mean.

Please define them (includes the one below).

> + if (is_version(pcr, 0x525A, IC_VER_A))
> + rtsx_pci_write_phy_register(pcr, 0x19, 0x3902);
> +
> + return 0;
> +}
> +
> +static int rts525a_extra_init_hw(struct rtsx_pcr *pcr)
> +{
> + rts5249_extra_init_hw(pcr);
> +
> + rtsx_pci_write_register(pcr, PCLK_CTL, PCLK_MODE_SEL, PCLK_MODE_SEL);
> + if (is_version(pcr, 0x525A, IC_VER_A)) {
> + rtsx_pci_write_register(pcr, L1SUB_CONFIG2,
> + L1SUB_AUTO_CFG, L1SUB_AUTO_CFG);
> + rtsx_pci_write_register(pcr, RREF_CFG,
> + RREF_VBGSEL_MASK, RREF_VBGSEL_1V25);
> + rtsx_pci_write_register(pcr, LDO_VIO_CFG,
> + LDO_VIO_TUNE_MASK, LDO_VIO_1V7);
> + rtsx_pci_write_register(pcr, LDO_DV12S_CFG,
> + LDO_D12_TUNE_MASK, LDO_D12_TUNE_DF);
> + rtsx_pci_write_register(pcr, LDO_AV12S_CFG,
> + LDO_AV12S_TUNE_MASK, LDO_AV12S_TUNE_DF);
> + rtsx_pci_write_register(pcr, LDO_VCC_CFG0,
> + LDO_VCC_LMTVTH_MASK, LDO_VCC_LMTVTH_2A);
> + rtsx_pci_write_register(pcr, OOBS_CONFIG,
> + OOBS_AUTOK_DIS | OOBS_VAL_MASK, 0x89);
> + }
> +
> + return 0;
> +}
> +
> +static const struct pcr_ops rts525a_pcr_ops = {
> + .fetch_vendor_settings = rts5249_fetch_vendor_settings,
> + .extra_init_hw = rts525a_extra_init_hw,
> + .optimize_phy = rts525a_optimize_phy,
> + .turn_on_led = rts5249_turn_on_led,
> + .turn_off_led = rts5249_turn_off_led,
> + .enable_auto_blink = rts5249_enable_auto_blink,
> + .disable_auto_blink = rts5249_disable_auto_blink,
> + .card_power_on = rts525a_card_power_on,
> + .card_power_off = rts5249_card_power_off,
> + .switch_output_voltage = rts525a_switch_output_voltage,
> + .force_power_down = rts5249_force_power_down,
> +};
> +
> +void rts525a_init_params(struct rtsx_pcr *pcr)
> +{
> + rts5249_init_params(pcr);
> +
> + pcr->ops = &rts525a_pcr_ops;
> +}
> +
> diff --git a/drivers/mfd/rtsx_pcr.c b/drivers/mfd/rtsx_pcr.c
> index 17334ba..33aa30b 100644
> --- a/drivers/mfd/rtsx_pcr.c
> +++ b/drivers/mfd/rtsx_pcr.c
> @@ -59,6 +59,7 @@ static const struct pci_device_id rtsx_pci_ids[] = {
> { PCI_DEVICE(0x10EC, 0x5287), PCI_CLASS_OTHERS << 16, 0xFF0000 },
> { PCI_DEVICE(0x10EC, 0x5286), PCI_CLASS_OTHERS << 16, 0xFF0000 },
> { PCI_DEVICE(0x10EC, 0x524A), PCI_CLASS_OTHERS << 16, 0xFF0000 },
> + { PCI_DEVICE(0x10EC, 0x525A), PCI_CLASS_OTHERS << 16, 0xFF0000 },
> { 0, }
> };
>
> @@ -1113,6 +1114,10 @@ static int rtsx_pci_init_chip(struct rtsx_pcr *pcr)
> rts524a_init_params(pcr);
> break;
>
> + case 0x525A:
> + rts525a_init_params(pcr);
> + break;
> +
> case 0x5287:
> rtl8411b_init_params(pcr);
> break;
> @@ -1158,7 +1163,7 @@ static int rtsx_pci_probe(struct pci_dev *pcidev,
> struct rtsx_pcr *pcr;
> struct pcr_handle *handle;
> u32 base, len;
> - int ret, i;
> + int ret, i, bar = 0;
>
> dev_dbg(&(pcidev->dev),
> ": Realtek PCI-E Card Reader found at %s [%04x:%04x] (rev %x)\n",
> @@ -1203,8 +1208,10 @@ static int rtsx_pci_probe(struct pci_dev *pcidev,
> pcr->pci = pcidev;
> dev_set_drvdata(&pcidev->dev, handle);
>
> - len = pci_resource_len(pcidev, 0);
> - base = pci_resource_start(pcidev, 0);
> + if (PCI_PID(pcr) == 0x525A)
> + bar = 1;
> + len = pci_resource_len(pcidev, bar);
> + base = pci_resource_start(pcidev, bar);
> pcr->remap_addr = ioremap_nocache(base, len);
> if (!pcr->remap_addr) {
> ret = -ENOMEM;
> diff --git a/drivers/mfd/rtsx_pcr.h b/drivers/mfd/rtsx_pcr.h
> index 0535265..4c00544 100644
> --- a/drivers/mfd/rtsx_pcr.h
> +++ b/drivers/mfd/rtsx_pcr.h
> @@ -37,6 +37,7 @@ 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 rts524a_init_params(struct rtsx_pcr *pcr);
> +void rts525a_init_params(struct rtsx_pcr *pcr);
> void rtl8411b_init_params(struct rtsx_pcr *pcr);
>
> static inline u8 map_sd_drive(int idx)

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