Hi Wei,
On Wed, Jul 17, 2013 at 04:09:51PM +0800, wei_wang@xxxxxxxxxxxxxx wrote:
> diff --git a/drivers/mfd/rtl8411.c b/drivers/mfd/rtl8411.c
> index c436bf2..1fbb978 100644
> --- a/drivers/mfd/rtl8411.c
> +++ b/drivers/mfd/rtl8411.c
> @@ -1,6 +1,6 @@
> /* Driver for Realtek PCI-Express card reader
> *
> - * Copyright(c) 2009 Realtek Semiconductor Corp. All rights reserved.
> + * Copyright(c) 2009-2013 Realtek Semiconductor Corp. All rights reserved.
> *
> * This program is free software; you can redistribute it and/or modify it
> * under the terms of the GNU General Public License as published by the
> @@ -17,7 +17,7 @@
> *
> * Author:
> * Wei WANG <wei_wang@xxxxxxxxxxxxxx>
> - * No. 450, Shenhu Road, Suzhou Industry Park, Suzhou, China
> + * Roger Tseng <rogerable@xxxxxxxxxxx>
> */
Please do not mix actual code with copyright changes. Remove those
chunks from this patch, and submit a separate patch for them.
>
> #include <linux/module.h>
> @@ -47,19 +47,70 @@ static int rtl8411b_is_qfn48(struct rtsx_pcr *pcr)
> return 0;
> }
>
> +static void rtl8411_init_settings(struct rtsx_pcr *pcr)
> +{
> + u32 reg1;
> + u8 reg3;
> +
> + rtsx_pci_read_config_dword(pcr, PCR_SETTING_REG1, ®1);
> + dev_dbg(&(pcr->pci->dev), "Cfg 0x%x: 0x%x\n", PCR_SETTING_REG1, reg1);
> +
> + if (rtsx_vendor_setting_valid(reg1)) {
A more readable construct would be:
if (!rtsx_vendor_setting_valid(reg1))
return;
pcr->aspm_en = rtsx_reg_to_aspm(reg1);
[...]
> @@ -385,6 +438,12 @@ void rtl8411_init_params(struct rtsx_pcr *pcr)
> pcr->num_slots = 2;
> pcr->ops = &rtl8411_pcr_ops;
>
> + pcr->flags = 0;
> + pcr->card_drive_sel = 0x55;
> + pcr->sd30_drive_sel_1v8 = 0x03;
> + pcr->sd30_drive_sel_3v3 = 0x01;
> + pcr->aspm_en = 0x02;
Those values seem to be repeated for many SKUs, could we please have a
definition instead of hardcoded values ?
> @@ -747,6 +753,7 @@ struct pcr_ops {
> u8 voltage);
> unsigned int (*cd_deglitch)(struct rtsx_pcr *pcr);
> int (*conv_clk_and_div_n)(int clk, int dir);
> + void (*init_settings)(struct rtsx_pcr *pcr);
init_settings is a bit too generic I believe. What you're doing is
fetching vendor settings, so what about fetch_vendor_settings instead ?
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
