Date: Fri, 8 May 2015 18:53:06 +0100
From: Mark Brown <broonie@kernel.org>
To: leilk.liu@mediatek.com
Cc: Mark Rutland <mark.rutland@arm.com>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        Rob Herring <robh+dt@kernel.org>, Pawel Moll <pawel.moll@arm.com>,
        Ian Campbell <ijc+devicetree@hellion.org.uk>,
        Kumar Gala <galak@codeaurora.org>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will.deacon@arm.com>,
        Eddie Huang <eddie.huang@mediatek.com>,
        Hongzhou Yang <hongzhou.yang@mediatek.com>,
        Sascha Hauer <s.hauer@pengutronix.de>, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-spi@vger.kernel.org, linux-mediatek@lists.infradead.org,
        srv_heupstream@mediatek.com, Sascha Hauer <kernel@pengutronix.de>
Message-ID: <20150508175306.GG2761@sirena.org.uk>
References: <1431075343-7887-1-git-send-email-leilk.liu@mediatek.com>
 <1431075343-7887-3-git-send-email-leilk.liu@mediatek.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
	protocol="application/pgp-signature"; boundary="+1TulI7fc0PCHNy3"
Content-Disposition: inline
In-Reply-To: <1431075343-7887-3-git-send-email-leilk.liu@mediatek.com>
User-Agent: Mutt/1.5.23 (2014-03-12)
Subject: Re: [PATCH 2/3] spi: mediatek: Add spi bus for Mediatek MT8173
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3227
Lines: 98


--+1TulI7fc0PCHNy3
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Fri, May 08, 2015 at 04:55:42PM +0800, leilk.liu@mediatek.com wrote:
> From: Leilk Liu <leilk.liu@mediatek.com>
>=20
> 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.

> +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.

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[] =3D {
> +	{ .compatible =3D "mediatek,mt6589-spi", .data =3D (void *)COMPAT_MT658=
9},
> +	{ .compatible =3D "mediatek,mt8173-spi", .data =3D (void *)COMPAT_MT817=
3},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, mtk_spi_of_match);

There were three compatible strings listed in the DT binding but only
two here.

> +	/*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 =3D 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 =3D 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.

--+1TulI7fc0PCHNy3
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: Digital signature

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQEcBAEBCAAGBQJVTPgBAAoJECTWi3JdVIfQ1w8H/3hvZfa5cZV1niiOOz8VUzKE
bajMDrcI6pptir27kXkEqbvPnzdM2esDaD80IJZblj3y5+fEJ2ogzlLsO2uyOoHZ
lU1ebAU+YOueKBRhpodDbV/i3mlL8NAf1BlM3YImOdwalRRDxxXGpyGowkzp1AcU
AwguJZuBUSGeYEpuWZLOqgEEo/MxlCb46YMQ7Lcrvvd3X0yCDN4X5F2XTrpn5EE7
odrGz0qhsZsgDvvLBNDon1Rfr6qVdNTnXqtFluHge29bqyjjDrMOk9xTXLQ+0/my
exAfdhZ+C751HY0TfH/+9vwO5nLqlJBOyceHT7vo1oJleDBUOT1dfsS37MV0AjE=
=4+lo
-----END PGP SIGNATURE-----

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