Received-SPF: pass (google.com: domain of linux-kernel+bounces-42348-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Mon, 29 Jan 2024 09:26:05 +0100
From: Marc Kleine-Budde <mkl@pengutronix.de>
To: William Qiu <william.qiu@starfivetech.com>
Cc: devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, 
	linux-riscv@lists.infradead.org, linux-can@vger.kernel.org, 
	Emil Renner Berthing <kernel@esmil.dk>, Rob Herring <robh+dt@kernel.org>, 
	Wolfgang Grandegger <wg@grandegger.com>, Philipp Zabel <p.zabel@pengutronix.de>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>, 
	"David S . Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, 
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, 
	Paul Walmsley <paul.walmsley@sifive.com>, Palmer Dabbelt <palmer@dabbelt.com>, 
	Albert Ou <aou@eecs.berkeley.edu>
Subject: Re: [PATCH v1 3/4] can: cast: add driver for CAST CAN controller
Message-ID: <20240129-zone-defame-c5580e596f72-mkl@pengutronix.de>
References: <20240129031239.17037-1-william.qiu@starfivetech.com>
 <20240129031239.17037-4-william.qiu@starfivetech.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="zggniggrx7ssk34l"
Content-Disposition: inline
In-Reply-To: <20240129031239.17037-4-william.qiu@starfivetech.com>


--zggniggrx7ssk34l
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Hello William Qiu,

thank you for your contribution. I've some quick notes about your
driver.

On 29.01.2024 11:12:38, William Qiu wrote:
> Add driver for CAST CAN Controller. And add compatibility code which
> based on StarFive JH7110 SoC.

Please add yourself or someone else at starfivetech to the Maintainers
file.

Please use BIT() and/or GEN_MASK() to create the _MASK enums. Please use
FIELD_GET(), FIELD_PREP.

Please replace the ccan_ioread8() by a proper 32 bit read and use
FIELD_GET to access any non 32 bit value. Instead of ccan_iowrite8() use
FIELD_PREP and a proper 32 bit write.

The enum ccan_reg_bitchange looks very strange, why do you have OFF and
SET values?

The ccan_reigister_set_bit() and ccan_reigister_off_bit() functions
looks very strange, too. I suggest to use a 32 bit read, set, clear the
bits followed by a 32 bit write. Having set_bit() clear_bit() functions
may lead to more register accesses than needed, if not handled with care.

If you think the driver absolutely needs bit set/clear functions, please
follow the name and signature of the regmap_update_bits(),
regmap_set_bits() and regmap_clear_bits().

Please use can_put_echo_skb(), can_get_echo_skb().

Please implement proper TX-flow control. Stop the TX queue, if you HW
queue is full, start the TX queue once the HW queue has space again.

Consider using the rx_offload helper

You claim you IRQ handler works with shared interrupts, but you return
an error if there are no interrupts by your IP core.

Please enable the clocks during open() and disabled during close()

Marc

--=20
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung N=C3=BCrnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

--zggniggrx7ssk34l
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAABCgAdFiEEDs2BvajyNKlf9TJQvlAcSiqKBOgFAmW3YRoACgkQvlAcSiqK
BOgKmgf8CP4DHYvozr25yPE/15GRtr4yDQDy5doYFxV8tAO2r1Y1iiU0mIz3aCLp
e3qWwbV9jcftKht75T5CgcurR8Ouc6P4O89HsGkpyRz0c4Pg4X/gswBJ29hTRFQw
JyaxQCaelQ5mbSAq0i2ZFCNiqBEzk6KyegUcmUabqOHmQTlpcXRYiJWRgKlKMoHr
xjkTF4HgTcViIXb/JislwP7q8C4F2/qEEtSLfqgOCzZjaPSRP6cTo8yy+YdITFhx
ntGdaMLEktRyMJVCcMfk5TXzea45E7yCa6cLL/4+VNzm/dsB1YF7Si9CC/nTXP9U
G5qmdcL6AIGzthY2poRxdC/oX7ppgw==
=HX7Y
-----END PGP SIGNATURE-----

--zggniggrx7ssk34l--