Masayuki Ohtak wrote: > CAN driver of Topcliff PCH > > Topcliff PCH is the platform controller hub that is going to be used in > Intel's upcoming general embedded platform. All IO peripherals in > Topcliff PCH are actually devices sitting on AMBA bus. > Topcliff PCH has CAN I/F. This driver enables CAN function.
Thanks for your contribution. Some remarks in addition to Wolfgang's.
- Try to send patches directly with git send-email
- Rebase your tree to net-next-2.6.
- don't use global variables
- don't use that "int handle", e.g.:
> static int pch_can_msg_tx(int handle, struct pch_can_msg *msg,
> struct net_device *ndev)
> {
> u32 id1 = 0;
> u32 id2 = 0;
> u32 data_a1 = 0;
> u32 data_a2 = 0;
> u32 data_b1 = 0;
> u32 data_b2 = 0;
> u32 tx_disable_counter = 0;
> u32 buffer_status = 0;
> u32 tx_buffer_avail = 0;
> u32 status;
> u32 i;
> u32 counter;
> enum pch_can_run_mode run_mode;
> int retval = 0;
> u32 if1_creq;
>
> if ((handle == 0) || (msg == NULL)) {
> dev_err(&ndev->dev, "%s -> Invalid Parameter.\n", __func__);
> retval = -EPERM;
> }
>
> else {
> /* Obatining the remap address for access. */
> struct can_hw *can = (struct can_hw *) handle;
>
use a proper struct. There are numerous drawbacks, no type safety it's
not 64 safe, bad style,...
- there are several checks for handle against 0 that make no real sense
- clean up you error paths:
if (error) {
/* error handling */
} else {
/* do real work */
}
This leads to a big indention level in the interesting code path, making
it very hard to read. Please rework your code this way:
if (error) {
/* error handling */
return error;
/* or */
goto out_handle_error;
}
/* do real work */
- get rid of the intermediate struct pch_can_msg:
Your data path is:
struct can_frame -> struct pch_can_msg -> registers
write from struct can_frame into registers directly
- what's the purpose of "p_can_os->can_callback", call the function
directly from the interrupt handler
- implement NAPI
- get rid of "1 << BIT_SHIFT_SIX" and friend,
use "1 << 6" or "BIT(6)" if you like defines
- use defines to set bits in struct can_frame can_id
cheers, Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
