Message-ID: <4A97D146.5050806@hartkopp.net>
Date:	Fri, 28 Aug 2009 14:44:54 +0200
From:	Oliver Hartkopp <socketcan@...tkopp.net>
To:	Anant Gole <anantgole@...com>
CC:	socketcan-core@...ts.berlios.de, netdev@...r.kernel.org,
	Wolfgang Grandegger <wg@...ndegger.com>
Subject: Re: [PATCH] net-next:can: add TI CAN (HECC) driver

Anant Gole wrote:
> TI HECC (High End CAN Controller) module is found on many TI devices. It has
> 32 harwdare mailboxes with full implementation of CAN protocol version 2.0B
> and bus speeds up to 1Mbps. The module specifications are available at TI web
> <http://www.ti.com>.
> 
> This driver is tested on OMAP3517 EVM. Suspend/Resume not tested as yet.
> 

Hello Anant,

some nitpicking first:

> +#include <linux/can/platform/ti_hecc_platform.h>

Please use

linux/can/platform/ti_hecc.c

following the other drivers.

> +
> +#define DRV_NAME "TI HECC"

DRV_NAME "ti_hecc"

like your module is called in various places in the Kernel.

This could be used later in

> +static struct platform_driver ti_hecc_driver = {
> +	.driver = {
> +		.name    = "ti_hecc",
> +		.owner   = THIS_MODULE,
> +	},

also.

And it's better for this:


> +/* CAN Bittiming constants as per HECC specs */
> +static struct can_bittiming_const ti_hecc_bittiming_const = {
> +	.name = DRV_NAME,
> +	.tseg1_min = 1,
> (..)


> +
> +#ifdef CONFIG_DEBUG_FS
> +
> +static struct ti_hecc_priv *debug_priv;
> +
> +#define PRINTMBOXREG(r) seq_printf(s, "%d\t%08X %08X %08X %08X %08X\n", r,\
> +	hecc_read(debug_priv, HECC_CANMID(r)),\
> +	hecc_read(debug_priv, HECC_CANMCF(r)),\
> +	hecc_read(debug_priv, HECC_CANMDH(r)),\
> +	hecc_read(debug_priv, HECC_CANMDL(r)),\
> +	hecc_read(debug_priv, HECC_CANLAM(r)))
> +
> +/* Print mailbox data */
> +static void hecc_print_mbox_regs(struct seq_file *s)
> +{
> +	int cnt = 0;
> +	static struct ti_hecc_priv *priv;
> +	priv = debug_priv;
> +	seq_printf(s, "\n--- %s %s - mailbox regs ---\n\n",
> +		DRV_NAME, HECC_MODULE_VERSION);
> +	seq_printf(s, "MbxNo\tMID\t MCF\t  MDH\t   MDL\t   LAM\n");
> +	seq_printf(s, "-----------------------------------------------\n");
> +	for (cnt = 0; cnt < HECC_MAX_MAILBOXES; cnt++)
> +		PRINTMBOXREG(cnt);
> +}
> +
> +#define PRINTREG(d, r) seq_printf(s, "%s\t%08x\n", d, hecc_read(debug_priv, r))
> +/* Print HECC registers */
> +static void hecc_print_regs(struct seq_file *s)
> +{

I discovered lot's of debug code (>20%).

Is this really needed?



> +static char *hecc_debug_can_state[] = {
> +	"CAN_STATE_ERROR_ACTIVE",
> +	"CAN_STATE_ERROR_WARNING",
> +	"CAN_STATE_ERROR_PASSIVE",
> +	"CAN_STATE_BUS_OFF",
> +	"CAN_STATE_STOPPED",
> +	"CAN_STATE_SLEEPING",
> +	"CAN_STATE_MAX"
> +};

Hm - defining this in a driver looks like a bad idea.

Maybe we could move this to the CAN driver interface depending on

CONFIG_CAN_DEBUG_DEVICES

?!?


> +
> +/* Print status and statistics */
> +static void hecc_print_status(struct seq_file *s)
> +{
> +	seq_printf(s, "\n--- %s %s - status ---\n\n",
> +		DRV_NAME, HECC_MODULE_VERSION);
> +	seq_printf(s, "\n--- ti_hecc status ---\n\n");
> +	seq_printf(s, "CAN state \t\t= %s\n",
> +		hecc_debug_can_state[debug_priv->can.state]);
> +	seq_printf(s, "CAN restart_ms \t\t= %u\n", debug_priv->can.restart_ms);
> +	seq_printf(s, "CAN input clock \t= %u\n", debug_priv->can.clock.freq);
> +	seq_printf(s, "CAN Bittiming\n");
> +	seq_printf(s, "\tbitrate \t= %u\n",
> +			debug_priv->can.bittiming.bitrate);
> +	seq_printf(s, "\tsample_point \t= %u\n",
> +			debug_priv->can.bittiming.sample_point);
> +	seq_printf(s, "\ttq \t\t= %u\n",
> +			debug_priv->can.bittiming.tq);
> +	seq_printf(s, "\tprop_seg \t= %u\n",
> +			debug_priv->can.bittiming.prop_seg);
> +	seq_printf(s, "\tphase_seg1 \t= %u\n",
> +			debug_priv->can.bittiming.phase_seg1);
> +	seq_printf(s, "\tphase_seg2 \t= %u\n",
> +			debug_priv->can.bittiming.phase_seg2);
> +	seq_printf(s, "\tsjw \t\t= %u\n",
> +			debug_priv->can.bittiming.sjw);
> +	seq_printf(s, "\tbrp \t\t= %u\n",
> +			debug_priv->can.bittiming.brp);
> +	seq_printf(s, "CAN Bittiming Constants\n");
> +	seq_printf(s, "\ttseg1 min-max \t= %u-%u\n",
> +			debug_priv->can.bittiming_const->tseg1_min,
> +			debug_priv->can.bittiming_const->tseg1_max);
> +	seq_printf(s, "\ttseg2 min-max \t= %u-%u\n",
> +			debug_priv->can.bittiming_const->tseg2_min,
> +			debug_priv->can.bittiming_const->tseg2_max);
> +	seq_printf(s, "\tsjw_max \t= %u\n",
> +			debug_priv->can.bittiming_const->sjw_max);
> +	seq_printf(s, "\tbrp min-max \t= %u-%u\n",
> +			debug_priv->can.bittiming_const->brp_min,
> +			debug_priv->can.bittiming_const->brp_max);
> +	seq_printf(s, "\tbrp_inc \t= %u\n",
> +			debug_priv->can.bittiming_const->brp_inc);

(..)

And this could be also a candidate to be in the CAN driver interface.

@Wolfgang: Any preferences to this idea?


> +
> +/** Toggle HECC Self-Test i.e loopback bit
> + * INFO: Reading this debug variable toggles the loopback status on the device.
> + * This is done to simplify the debug function to set looback
> + */
> +static int hecc_debug_loopback(struct seq_file *s)
> +{
> +	static int toggle;
> +
> +	/* Put module in self test mode i.e. loopback */
> +	if (toggle) {
> +		seq_printf(s, "In Self Test Mode (loopback)\n");
> +		hecc_set_bit(debug_priv, HECC_CANMC, HECC_CANMC_STM);
> +		toggle = 0;
> +	} else {
> +		seq_printf(s, "Out of Self Test Mode (NO loopback)\n");
> +		hecc_clear_bit(debug_priv, HECC_CANMC, HECC_CANMC_STM);
> +		toggle = 1;
> +	}
> +
> +	return 0;
> +}
> +

Ugh!

No. This should definitely be done by netlink.

I did not take a closer look into the device access and error handling right
now. So this was just my first impression :-)

Thanks,
Oliver
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists