linux-kernel - Re: [PATCH v2 10/12] spi: add driver for J-Core SPI controller

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: <20160520102317.GH8206@sirena.org.uk>
Date:	Fri, 20 May 2016 11:23:17 +0100
From:	Mark Brown <broonie@...nel.org>
To:	Rich Felker <dalias@...c.org>
Cc:	linux-kernel@...r.kernel.org, linux-sh@...r.kernel.org,
	linux-spi@...r.kernel.org
Subject: Re: [PATCH v2 10/12] spi: add driver for J-Core SPI controller

On Fri, May 20, 2016 at 02:53:04AM +0000, Rich Felker wrote:
> Signed-off-by: Rich Felker <dalias@...c.org>
> ---
> My previous post of the patch series accidentally omitted omitted
> Cc'ing of subsystem maintainers for the necessary clocksource,
> irqchip, and spi drivers. Please ack if this looks ok because I want
> to get it merged as part of the arch/sh pull request for 4.7.

This is *extremely* late for a first posting of a driver for v4.7 (you
missed the list as well as the maintainers).

> +static void jcore_spi_chipsel(struct spi_device *spi, bool value)
> +{
> +	struct jcore_spi *hw = spi_master_get_devdata(spi->master);
> +
> +	pr_debug("%s: CS=%d\n", __func__, value);

dev_dbg()

> +
> +	hw->csReg = ( JCORE_SPI_CTRL_ACS | JCORE_SPI_CTRL_CCS | JCORE_SPI_CTRL_DCS )
> +		^ (!value << 2*spi->chip_select);

Why the xor here and not an or?  The coding style is also weird, a mix
of extra spaces around the () and missing ones around *.  I'm finding
the intent of the code confusing here.

> +static int jcore_spi_txrx(struct spi_master *master, struct spi_device *spi, struct spi_transfer *t)

Coding style, please keep lines under 80 columns unless there's a good
reason.

> +#if !USE_MESSAGE_MODE
> +	spi_finalize_current_transfer(master);
> +#endif

I'm not sure what the if is about but it doesn't belong upstream, you
shouldn't be open coding bits of the framework.

> +	/* register our spi controller */
> +	err = spi_register_master(master);

devm_

> +static int jcore_spi_remove(struct platform_device *dev)
> +{
> +	struct jcore_spi *hw = platform_get_drvdata(dev);
> +	struct spi_master *master = hw->master;
> +
> +	platform_set_drvdata(dev, NULL);
> +	spi_master_put(master);
> +	return 0;
> +}

This can be removed entirely.

> +static const struct of_device_id jcore_spi_of_match[] = {
> +	{ .compatible = "jcore,spi2" },
> +	{},
> +};

This is adding a DT binding with no binding document.  All new DT
bindings need to be documented.

> +		.owner = THIS_MODULE,
> +		.pm = NULL,

No need to set either of these.

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