Re: [PATCH v2 2/2] spi: add driver for Rockchip RK3xxx SoCs integrated SPI

From: Mark Brown <broonie@kernel.org>
To: addy ke <addy.ke@rock-chips.com>
Cc: heiko@sntech.de, grant.likely@linaro.org, robh+dt@kernel.org,
	linux-kernel@vger.kernel.org, linux-spi@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org,
	olof@lixom.net, hj@rock-chips.com, kever.yang@rock-chips.com,
	xjq@rock-chips.com, huangtao@rock-chips.com, zyw@rock-chips.com,
	yzq@rock-chips.com, zhenfu.fang@rock-chips.com,
	cf@rock-chips.com, zhangqing@rock-chips.com,
	wei.luo@rock-chips.com
Subject: Re: [PATCH v2 2/2] spi: add driver for Rockchip RK3xxx SoCs integrated SPI
Date: Mon, 7 Jul 2014 08:16:45 +0100	[thread overview]
Message-ID: <20140707071645.GA30458@sirena.org.uk> (raw)
In-Reply-To: <53B9FB1C.7080008@rock-chips.com>

[-- Attachment #1: Type: text/plain, Size: 1645 bytes --]

On Mon, Jul 07, 2014 at 09:42:52AM +0800, addy ke wrote:

> > The driver shouldn't be doing this, if it needs a delay it needs to
> > implement it itself.  delay_usecs can be set by devices if they need a
> > delay between transfers, it should be in addition to the time taken for
> > the transfer to complete.

> > Please send a followup patch fixing this.

> Are the following modifications reasonable?

Yes, that looks sensible.

> >> +static const struct of_device_id rockchip_spi_dt_match[] = {
> >> +	{ .compatible = "rockchip,rk3066-spi", },
> >> +	{ },
> >> +};
> >> +MODULE_DEVICE_TABLE(of, rockchip_spi_dt_match);

> > Your DT binding defined some additional compatible strings, please add
> > those to the driver.

> So which is better to describe DT binding for rockchip spi driver as follow:

> 2. Add "rockchip,rk3066-spi", "rockchip,rk3066-spi", "rockchip,rk3066-spi" for each soc:
> 
> In Documentation/devicetree/bindings/spi/spi-rockchip.txt
> - compatible: should be one of the following.
>     "rockchip,rk3066-spi" for rk3066.
>     "rockchip,rk3188-spi", "rockchip,rk3066-spi" for rk3188.
>     "rockchip,rk3288-spi", "rockchip,rk3066-spi" for rk3288.
> 
> In drivers/spi/spi-rockchip.c
> static const struct of_device_id rockchip_spi_dt_match[] = {
>         { .compatible = "rockchip,rk3066-spi", },
>         { .compatible = "rockchip,rk3188-spi", },
>         { .compatible = "rockchip,rk3288-spi", },
>         { },
> };

This is better - that way if we need to care about the differences then
we already have device trees which have the specific compatible strings
in them.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]