Re: [MeeGo-Dev][PATCH] Topcliff: Update PCH_CAN driver to 2.6.35

[Marc Kleine-Budde <mkl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>]

[-- Attachment #1.1: Type: text/plain, Size: 2538 bytes --]

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   |


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[-- Attachment #2: Type: text/plain, Size: 188 bytes --]

_______________________________________________
Socketcan-core mailing list
Socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org
https://lists.berlios.de/mailman/listinfo/socketcan-core