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] [thread-next>] [day] [month] [year] [list]

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

Hi Srini,

Thanks for reviewing, see my comments below: -

On Fri, 23 May 2014, Srinivas Kandagatla wrote:

> >+struct st_mmc_platform_data {
> >+	struct  reset_control	*rstc;
> >+};
> 
> st_mmc_platform_data name is bit missleading as this data is not
> part of platform data. Probably you could rename it to struct
> sdhci_st_data.

I've now removed this reset controller code for v2, as based on Maxime's feedback
I think this would be better off going in the generic code, so all 
platforms could benefit if they have a reset controller implementation.

I plan to send some RFC patches which implement this seperately to this series.

> >+	pltfm_host->priv = pdata;
> >+
> >+	platform_set_drvdata(pdev, host);
> 
> Why not platform_set_drvdata(pdev, priv_host);
> As you are not using sdhci_host directly, this will reduced few
> indirections in the driver.

Your right, and I was going to change this, but with the re-think on the reset
controller code above I will now need sdhci_host so would prefer to leave it as
is for now.

> >+err_out:
> >+	clk_disable_unprepare(clk);
> >+	sdhci_pltfm_free(pdev);
> >+
> IP could be left out of reset in this path.

Good spot, I'll remember this when I add the reset code
back in :-)

> 
> replace:
> [...
> >+static SIMPLE_DEV_PM_OPS(sdhci_st_pmops, sdhci_st_suspend, sdhci_st_resume);
> >+#define SDHCI_ST_PMOPS (&sdhci_st_pmops)
> >+#else
> >+#define SDHCI_ST_PMOPS NULL
> >+#endif
> ...]
> 
> with :
> 
> #endif
> 
> static SIMPLE_DEV_PM_OPS(sdhci_st_pmops, sdhci_st_suspend, sdhci_st_resume);

Fixed 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/