From: Mark Brown <broonie@kernel.org>
To: Bert Vermeulen <bert@biot.com>
Cc: Rob Herring <robh+dt@kernel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
linux-spi@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, John Crispin <john@phrozen.org>,
Benjamin Larsson <benjamin.larsson@iopsys.eu>
Subject: Re: [PATCH v2 2/3] spi: Add support for the Airoha EN7523 SoC SPI controller
Date: Tue, 27 Sep 2022 13:23:12 +0100 [thread overview]
Message-ID: <YzLrMHVU3mTJVVh6@sirena.org.uk> (raw)
In-Reply-To: <20220927113229.1214224-3-bert@biot.com>
[-- Attachment #1: Type: text/plain, Size: 2470 bytes --]
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.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
next prev parent reply other threads:[~2022-09-27 12:23 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-27 11:32 [PATCH v2 0/3] Add support for the Airoha EN7523 SPI controller Bert Vermeulen
2022-09-27 11:32 ` [PATCH v2 1/3] dt-bindings: arm: airoha: Add documentation for Airoha " Bert Vermeulen
2022-09-29 22:21 ` Rob Herring
2022-09-30 10:29 ` Krzysztof Kozlowski
2022-09-27 11:32 ` [PATCH v2 2/3] spi: Add support for the Airoha EN7523 SoC " Bert Vermeulen
2022-09-27 12:23 ` Mark Brown [this message]
2022-09-27 15:04 ` Bert Vermeulen
2022-09-27 11:32 ` [PATCH v2 3/3] ARM: dts: en7523: Add SPI node Bert Vermeulen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YzLrMHVU3mTJVVh6@sirena.org.uk \
--to=broonie@kernel.org \
--cc=benjamin.larsson@iopsys.eu \
--cc=bert@biot.com \
--cc=devicetree@vger.kernel.org \
--cc=john@phrozen.org \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-spi@vger.kernel.org \
--cc=robh+dt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).