linux-kernel - Re: [PATCH 1/8] mmc: sdhci-st: Intial support for ST SDHCI controller

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]

Message-ID: <20140604110518.GA1722@griffinp-ThinkPad-X1-Carbon-2nd>
Date:	Wed, 4 Jun 2014 12:05:18 +0100
From:	Peter Griffin <peter.griffin@...aro.org>
To:	Maxime Coquelin <maxime.coquelin@...com>
Cc:	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	patrice.chotard@...com, srinivas.kandagatla@...il.com,
	chris@...ntf.net, ulf.hansson@...aro.org, kernel@...inux.com,
	lee.jones@...aro.org, linux-mmc@...r.kernel.org,
	devicetree@...r.kernel.org,
	Giuseppe Cavallaro <peppe.cavallaro@...com>
Subject: Re: [PATCH 1/8] mmc: sdhci-st: Intial support for ST SDHCI controller

Hi Maxime,

Thanks fo reviewing, see my comments below: -

> >+struct st_mmc_platform_data {
> >+	struct  reset_control	*rstc;
> >+};
> Since it uses the generic reset framework, could we imagine moving
> the reset to the sdhci_pltfm_host struct?
> Doing this, we could get rid of st_mmc_platform_data.

Yes I agree, I will send some RFC patches which moves the reset controller
stuff into the generic code.

For v2 of this series I've removed the reset code, as stih416 doesn't have
seperate reset line each controller, so it will mainly be 
useful for stih407 SoC, which needs additional patches on top of this 
set.

> >+	switch (width) {
> >+	case MMC_BUS_WIDTH_8:
> >+		ctrl |= SDHCI_CTRL_8BITBUS;
> >+		ctrl &= ~SDHCI_CTRL_4BITBUS;
> >+		break;
> >+	case MMC_BUS_WIDTH_4:
> >+		ctrl |= SDHCI_CTRL_4BITBUS;
> >+		ctrl &= ~SDHCI_CTRL_8BITBUS;
> >+		break;
> >+	default:
> >+		ctrl &= ~(SDHCI_CTRL_8BITBUS | SDHCI_CTRL_4BITBUS);
> >+		break;
> You can remove the break here.

Ok, removed in v2

> >+	switch (reg) {
> >+	case SDHCI_CAPABILITIES:
> >+		ret = readl(host->ioaddr + reg);
> >+		/* Support 3.3V and 1.8V */
> >+		ret &= ~SDHCI_CAN_VDD_300;
> >+		break;
> >+	default:
> >+		ret = readl(host->ioaddr + reg);
> 
> Could we use readl_relaxed?

Yes, I've updated to use readl_relaxed in v2

> >+	dev_dbg(&pdev->dev, "SDHCI ST platform driver\n");
> You can remove this I think.

Removed in v2

> >+		host_version, ((host_version & SDHCI_VENDOR_VER_MASK) >>
> >+			       SDHCI_VENDOR_VER_SHIFT));
> Maybe you could change to dev_info here. It might be interresting to
> always have IP version.

Changed to dev_info in v2

Regards,

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