On Wed, Dec 4, 2013 at 2:38 PM, Mark Brown <broonie@xxxxxxxxxx> wrote:
> On Sat, Nov 30, 2013 at 12:42:06PM +0100, Jonas Gorski wrote:
>> Add a driver for the High Speed SPI controller found on newer BCM63XX SoCs.
>>
>> It does feature some new modes like 3-wire or dual spi, but neither of it
>> is currently implemented.
>
> This is basically good so I've applied it but there are a few minor
> things that could use improving, please send incremental patches for
> these.
Cool, thanks, will do. Expect a few days delay since I'm in the middle
of a cold. I Don't want to introduce thinkos ;-)
>
>> + clk = clk_get(dev, "hsspi");
>
> Use devm_clk_get().
Oh, it indeed seems to work now. The last time I tried it broke
compilation because bcm63xx does not implement CLKDEV (or something
like that).
>
>> + clk_prepare_enable(clk);
>
> You should check the return value here.
True, even if it can't fail in the current supported platforms.
>
>> + /* register and we are done */
>> + ret = spi_register_master(master);
>> + if (ret)
>> + goto out_put_master;
>
> devm_spi_register_master().
Sure thing.
>
>> +#ifdef CONFIG_PM
>
> This should be CONFIG_PM_SLEEP.
Ah, dammit, I knew I forgot something. I remember the same issue was
fixed for spi-bcm63xx.c, but somehow forgot to check for it in this
driver.
>> +static int bcm63xx_hsspi_suspend(struct device *dev)
>> +{
>> + struct spi_master *master = dev_get_drvdata(dev);
>> + struct bcm63xx_hsspi *bs = spi_master_get_devdata(master);
>> +
>> + spi_master_suspend(master);
>> + clk_disable(bs->clk);
>
> This ought to be disable_unprepare(). It would also be better to move
> this to runtime PM (you can set auto_runtime_pm to have the core manage
> the enable and disable for you) since that will save a bit of power.
I already set auto_runtime_pm to true. I basically copied what's
currently in spi-bcm63xx.c *coughs*. Is there anything else needed
besides what you mentioned?
>
>> +static int bcm63xx_hsspi_resume(struct device *dev)
>> +{
>> + struct spi_master *master = dev_get_drvdata(dev);
>> + struct bcm63xx_hsspi *bs = spi_master_get_devdata(master);
>> +
>> + clk_enable(bs->clk);
>> + spi_master_resume(master);
>
> Similar comments here, plus you should be checking errors.
Will fix that, too.
Regards
Jonas
