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]

Date:	Sun, 28 Sep 2014 03:15:04 +0000
From:	"Chen, Alvin" <alvin.chen@...el.com>
To:	Mark Brown <broonie@...nel.org>
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-arm-kernel@...ts.infradead.org>,
	"linux-spi@...r.kernel.org" <linux-spi@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"Westerberg, Mika" <mika.westerberg@...el.com>,
	"Kweh, Hock Leong" <hock.leong.kweh@...el.com>,
	"Ong, Boon Leong" <boon.leong.ong@...el.com>,
	"Tan, Raymond" <raymond.tan@...el.com>,
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Subject: RE: [PATCH] SPI: spi-pxa2xx: SPI support for Intel Quark X1000


> 
> > +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.
> 
OK. Let me use switch. BTW, for two patches, actually, using helpers is due to we support quark.
Do you mean the first patch just introduce helpers, maybe it only support one type, then another patch to add
quark supporting. Am I right?

> > +/*  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.
OK. I will document it.

> 
> > @@ -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.
> 
I will remove it.

> > @@ -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.
OK. I will use switch.
--
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/