linux-kernel - Re: [PATCH 1/1] mmc: sdhci-pci-dwc-mshc: synopsys dwc mshc support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 28 May 2018 13:03:21 +0000
From:   Prabu Thangamuthu <Prabu.T@...opsys.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
CC:     "ulf.hansson@...aro.org" <ulf.hansson@...aro.org>,
        Adrian Hunter <adrian.hunter@...el.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
        Manjunath M Bettegowda <Manjunath.MB@...opsys.com>
Subject: Re: [PATCH 1/1] mmc: sdhci-pci-dwc-mshc: synopsys dwc mshc support

Hi Andy,

Thanks for pointing all. We will fix it.

Regards,
Prabu

On 5/28/2018 5:49 PM, Andy Shevchenko wrote:
> On Tue, May 22, 2018 at 9:42 AM, Prabu Thangamuthu <Prabu.T@...opsys.com> wrote:
>> To enable Synopsys DWC MSHC controller on HPAS-DX platform connected using
>> PCIe interface. As Clock generation logic is implemented in MMCM module of
>> HAPS-DX platform, we have separate functions to control the MMCM to
>> generate required clocks with respect to speed mode. Also we have platform
>> specific set_power function to support different VDD of eMMC devices.
>> + * Authors:
>> + *    Prabu Thangamuthu <prabu.t@...opsys.com>
>> + *    Manjunath M B <manjumb@...opsys.com>
>> + *
> Redundant last line in above excerpt.
>
>> +#include <linux/pci.h>
>> +#include <linux/delay.h>
>> +#include <linux/module.h>
>> +#include <linux/moduleparam.h>
> This one is effectively included by module.h.
>
>> +/* Default emmc vdd is set to 3.3V */
>> +static unsigned int emmc_vdd = SDHC_EMMC_VDD_330V;
>> +module_param(emmc_vdd, int, 0444);
> This looks weird. Why do you need a module parameter?
>
>> +    u16 clk = 0;
>> +    u32 reg = 0;
>> +    u32 vendor_ptr = 0;
> Why do above have assignments?
>
>> +MODULE_PARM_DESC(emmc_vdd, "VDD to configure eMMC device supply voltage");
> This better to be near to the parameter declaration. Though, as I said
> above, it feels wrong to have it in the first place.
>
>
>> +/* Synopsys Vendor Specific Registers */
>> +#define SDHC_GPIO_OUT                          0x34
>> +#define SDHC_AT_CTRL_R                   0x40
>> +
>> +#define SDHC_SW_TUNE_EN                   0x00000010
>> +
>> +/* MMCM DRP */
>> +#define SDHC_MMCM_DIV_REG               0x1020
>> +#define DIV_REG_100_MHZ                   0x1145
>> +
>> +#define SDHC_MMCM_CLKFBOUT               0x1024
>> +#define CLKFBOUT_100_MHZ               0x0000
>> +
>> +#define SDHC_CCLK_MMCM_RST               0x00000001
> Do you need these all in the header file? Why?
>
>
>> +#define PCI_DEVICE_ID_SYNOPSYS_DWC_MSHC 0xC202
> Absent blank line.
> Broken style of the value.
>
> Who knows what else wrong with this patch? Please, pay attention to
> the details and check twice before submit.
>