linux-kernel - Re: [PATCH v2 2/3] spi: Add support for the Airoha EN7523 SoC 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: <YzLrMHVU3mTJVVh6@sirena.org.uk>
Date:   Tue, 27 Sep 2022 13:23:12 +0100
From:   Mark Brown <broonie@...nel.org>
To:     Bert Vermeulen <bert@...t.com>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        linux-spi@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, John Crispin <john@...ozen.org>,
        Benjamin Larsson <benjamin.larsson@...sys.eu>
Subject: Re: [PATCH v2 2/3] spi: Add support for the Airoha EN7523 SoC SPI
 controller

On Tue, Sep 27, 2022 at 01:32:28PM +0200, Bert Vermeulen wrote:

>  
> +config SPI_AIROHA_EN7523
> +	bool "Airoha EN7523 SPI controller support"

Why not tristate?

> +	depends on ARCH_AIROHA

I don't see a reason we couldn't have an || COMPILE_TEST here to improve
coverage?

> +	default ARCH_AIROHA

It's unusual to default a SPI controller on, they tend not to be ultra
critical like a clock driver or similar can be?

> +static void __iomem *iobase;

This should be driver data rather than a global, your current SoC might
only have one controller but some other model might build two and it's
fairly trivial to do.

> +static void opfifo_write(u32 cmd, u32 len)
> +{
> +	u32 tmp = ((cmd & 0x1f) << 9) | (len & 0x1ff);
> +
> +	writel(tmp, REG(ENSPI_MANUAL_OPFIFO_WDATA));
> +
> +	/* Wait for room in OPFIFO */
> +	while (readl(REG(ENSPI_MANUAL_OPFIFO_FULL)))
> +		cpu_relax();
> +

Some sort of timeout would be good with these loops, if things go wrong
we'll just lock up which isn't good.

> +       ret = clk_prepare_enable(clk);
> +       if (ret)
> +               return ret;

Nothing ever reverses this unless clk_set_rate() fails.

> +	ret = clk_set_rate(clk, 40000000);
> +	if (ret) {
> +		clk_disable_unprepare(clk);
> +		return ret;
> +	}

Could this be pushed into DT via the clock bindings?  The hard coded
number might need to vary by SoC.

> +static int xfer_read(struct spi_transfer *xfer)
> +{
> +	int opcode;
> +	uint8_t *buf = xfer->rx_buf;
> +
> +	switch (xfer->rx_nbits) {
> +	case SPI_NBITS_SINGLE:
> +		opcode = OP_INS;
> +		break;
> +	case SPI_NBITS_DUAL:
> +		opcode = OP_IND;
> +		break;
> +	}

This should have a default case that returns an error.

> +static int transfer_one_message(struct spi_controller *ctrl, struct spi_message *msg)
> +{
> +	struct spi_transfer *xfer;
> +	int next_xfer_is_rx = 0;
> +
> +	manual_begin_cmd();
> +	set_cs(0);

The driver should not be setting chip select itself, it should just
provide the set_cs() operation to the core and let the core worry about
when to call it.

> +	ctrl->transfer_one_message = transfer_one_message;
> +	err = devm_spi_register_controller(&pdev->dev, ctrl);
> +	if (err) {
> +		dev_err(&pdev->dev, "Could not register SPI controller\n");
> +		return -ENODEV;
> +	}

Don't discard the error code that registeration returned, include it in
the log message and pass it back to the caller.  

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