Re: [PATCH v1 3/4] can: cast: add driver for CAST CAN controller

[Marc Kleine-Budde]

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

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

Attachment: signature.asc
Description: PGP signature