linux-kernel - Re: [PATCH 2/3] spi: mediatek: Add spi bus for Mediatek MT8173

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]

Date:	Tue, 12 May 2015 20:39:16 +0800
From:	leilk liu <leilk.liu@...iatek.com>
To:	Mark Brown <broonie@...nel.org>
CC:	Mark Rutland <mark.rutland@....com>,
	Matthias Brugger <matthias.bgg@...il.com>,
	Rob Herring <robh+dt@...nel.org>,
	Pawel Moll <pawel.moll@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	"Kumar Gala" <galak@...eaurora.org>,
	Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will.deacon@....com>,
	Eddie Huang <eddie.huang@...iatek.com>,
	Hongzhou Yang <hongzhou.yang@...iatek.com>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	<devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<linux-arm-kernel@...ts.infradead.org>,
	<linux-spi@...r.kernel.org>, <linux-mediatek@...ts.infradead.org>,
	<srv_heupstream@...iatek.com>, Sascha Hauer <kernel@...gutronix.de>
Subject: Re: [PATCH 2/3] spi: mediatek: Add spi bus for Mediatek MT8173

Thanks for your review.

On Fri, 2015-05-08 at 18:53 +0100, Mark Brown wrote:
> On Fri, May 08, 2015 at 04:55:42PM +0800, leilk.liu@...iatek.com wrote:
> > From: Leilk Liu <leilk.liu@...iatek.com>
> > 
> > This patch adds basic spi bus for MT8173.
> 
> Please try to only CC relevant people on patches, you've got a very
> broad CC list here and I'm not sure I understand why everyone is on it.
> Sending people irrelevant patches adds to the volume of mail people have
> to handle which takes time away from other things.
> 

I use scripts/get_maintainer.pl on the patches and keep all maintainers.
I'll further refine the list in the future.

> > +config SPI_MT65XX
> > +	tristate "MediaTek SPI controller"
> > +	depends on ARCH_MEDIATEK || COMPILE_TEST
> > +	select SPI_BITBANG
> 
> Unless your controller is geniunely bitbanging you shouldn't be using
> the bitbang framework, modern drivers should implement set_cs() and
> transfer_one() instead.  You should also be using the core helpers for
> DMA mapping.
> 

I will implement set_cs() and transfer_one() instead in next patch.

Could you tell me more details about "You should also be using the core
helpers for DMA mapping"?

> The driver looks basically good other than this, it shouldn't be too
> much work (and ought to save you some code) to refactor to the modern
> interfaces.
> 
> > +#define IDLE 0
> > +#define INPROGRESS 1
> > +#define PAUSED 2
> > +
> > +#define PACKET_SIZE 1024
> 
> You should namespace the constants you're using to avoid clashes with
> headers.
> 
> > +static const struct of_device_id mtk_spi_of_match[] = {
> > +	{ .compatible = "mediatek,mt6589-spi", .data = (void *)COMPAT_MT6589},
> > +	{ .compatible = "mediatek,mt8173-spi", .data = (void *)COMPAT_MT8173},
> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(of, mtk_spi_of_match);
> 
> There were three compatible strings listed in the DT binding but only
> two here.
> 

MT6589 and MT8135 is compatible; 
For MT8135 IC, it can use the follow way in dts to probe:
  compatible = "mediatek,mt8135-spi", 
	       "mediatek,mt6589-spi";
And I test it's ok on MT8135 platform. So I add struct of_device_id
mtk_spi_of_match like this in spi driver code.

> > +	/*set the software reset bit in SPI_CMD_REG.*/
> 
> /* Spaces please */
> 
> > +static unsigned long mtk_get_device_prop(struct platform_device *pdev)
> > +{
> > +	const struct of_device_id *match;
> > +
> > +	match = of_match_node(mtk_spi_of_match, pdev->dev.of_node);
> > +	return (unsigned long)match->data;
> > +}
> 
> This wrapper doesn't seem to be doing a lot and will crash if we somehow
> manage to get the driver loaded without a match (eg, if someone tries to
> instantiate as a regular platform device).
> 
> > +	ret = clk_prepare_enable(mdata->clk);
> > +	if (ret < 0) {
> > +		dev_err(&pdev->dev, "failed to enable clock (%d)\n", ret);
> > +		goto err;
> > +	}
> 
> I'd expect to see runtime PM callbacks to enable and disable this clock
> in order to minimise power consumption.

In the case of other review comments, I will also fix them in next/OK.

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