linux-kernel - Re: [PATCH] SPI: spi-pxa2xx: SPI support for Intel Quark X1000

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: <20140926100832.GE27755@sirena.org.uk>
Date:	Fri, 26 Sep 2014 11:08:32 +0100
From:	Mark Brown <broonie@...nel.org>
To:	Weike Chen <alvin.chen@...el.com>
Cc:	Eric Miao <eric.y.miao@...il.com>,
	Russell King <linux@....linux.org.uk>,
	Haojian Zhuang <haojian.zhuang@...il.com>,
	linux-arm-kernel@...ts.infradead.org, linux-spi@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	Mika Westerberg <mika.westerberg@...el.com>,
	Hock Leong Kweh <hock.leong.kweh@...el.com>,
	Boon Leong Ong <boon.leong.ong@...el.com>,
	Raymond Tan <raymond.tan@...el.com>,
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Subject: Re: [PATCH] SPI: spi-pxa2xx: SPI support for Intel Quark X1000

On Fri, Sep 26, 2014 at 10:25:49AM -0700, Weike Chen wrote:

> +static u32 pxa2xx_spi_get_ssrc1_change_mask(const struct driver_data *drv_data)
> +{
> +	if (!is_quark_x1000_ssp(drv_data))
> +		return SSCR1_CHANGE_MASK;
> +
> +	return QUARK_X1000_SSCR1_CHANGE_MASK;
> +}

These functions would be much better written as switch statements -
think how they're going to look when we've got another controller which
needs custom values.  It might also be helpful for review to have two
patches, one splitting things out into the functions and another adding
the Quark support.

> +/*  see Quark SPI data sheet for implementation rationale */
> +static u32 quark_x1000_set_clk_regvals(u32 rate, u32 *dds, u32 *clk_div)
> +{

Please document this in the driver - I don't know if this datasheet is
public but even if it is it may not stay that way.

> @@ -613,6 +759,8 @@ static void pump_transfers(unsigned long data)
>  	u32 cr1;
>  	u32 dma_thresh = drv_data->cur_chip->dma_threshold;
>  	u32 dma_burst = drv_data->cur_chip->dma_burst_size;
> +	u32 change_mask = pxa2xx_spi_get_ssrc1_change_mask(drv_data);
> +
>  

Extra blank line being added here.

> @@ -145,6 +147,9 @@ static inline int pxa25x_ssp_comp(struct driver_data *drv_data)
>  		return 1;
>  	if (drv_data->ssp_type == CE4100_SSP)
>  		return 1;
> +	if (drv_data->ssp_type == QUARK_X1000_SSP)
> +		return 1;
> +
>  	return 0;
>  }

Things like this should also be refactored into switch statements - in
general anything that's deciding what to do based on ssp_type probably
ought to be using switch statements.

Download attachment "signature.asc" of type "application/pgp-signature" (474 bytes)