> -----Original Message----- > From: Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> > Sent: 2024年1月29日 16:26 > To: William Qiu <william.qiu@xxxxxxxxxxxxxxxx> > Cc: devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; > linux-riscv@xxxxxxxxxxxxxxxxxxx; linux-can@xxxxxxxxxxxxxxx; Emil Renner > Berthing <kernel@xxxxxxxx>; Rob Herring <robh+dt@xxxxxxxxxx>; Wolfgang > Grandegger <wg@xxxxxxxxxxxxxx>; Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>; > Krzysztof Kozlowski <krzysztof.kozlowski+dt@xxxxxxxxxx>; Conor Dooley > <conor+dt@xxxxxxxxxx>; David S . Miller <davem@xxxxxxxxxxxxx>; Eric > Dumazet <edumazet@xxxxxxxxxx>; Jakub Kicinski <kuba@xxxxxxxxxx>; Paolo > Abeni <pabeni@xxxxxxxxxx>; Paul Walmsley <paul.walmsley@xxxxxxxxxx>; > Palmer Dabbelt <palmer@xxxxxxxxxxx>; Albert Ou <aou@xxxxxxxxxxxxxxxxx> > Subject: Re: [PATCH v1 3/4] can: cast: add driver for CAST CAN controller > > 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 > Thank you for your notes. I will modify my driver one by one according to your notes。 Thanks, William > -- > 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 |
