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
