Re: [RESEND PATCH v3 5/5] can: c_can: Add support for Bosch D_CAN controller

[Marc Kleine-Budde <mkl@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 7987 bytes --]

On 05/23/2012 09:49 PM, Wolfgang Grandegger wrote:
> Hi AnilKumar,
> 
> I have some more comments...
> 
> On 05/23/2012 02:15 PM, AnilKumar Ch wrote:
>> This patch adds the support for D_CAN controller driver to the existing
>> C_CAN driver.
>>
>> Bosch D_CAN controller is a full-CAN implementation which is compliant
>> to CAN protocol version 2.0 part A and B. Bosch D_CAN user manual can be
>> obtained from: http://www.semiconductors.bosch.de/media/en/pdf/
>> ipmodules_1/can/d_can_users_manual_111.pdf
>>
>> A new array is added for accessing the d_can registers, according to d_can
>> controller register space.
>>
>> Current D_CAN implementation has following limitations, this is done
>> to avoid large changes to the C_CAN driver.
>> 1. Message objects are limited to 32, 16 for RX and 16 for TX. C_CAN IP
>>    supports upto 32 message objects but in case of D_CAN we can configure
>>    upto 128 message objects.
>> 2. Using two 16bit reads/writes for accessing the 32bit D_CAN registers.
>> 3. These patches have been tested on little endian machine, there might
>>    be some hidden endian-related issues due to the nature of the accesses
>>    (32-bit registers accessed as 2 16-bit registers). However, I do not
>>    have a big-endian D_CAN implementation to confirm.
>>
>> Signed-off-by: AnilKumar Ch <anilkumar@ti.com>
>> ---
>> Link to previous submission
>> 	http://marc.info/?l=linux-can&m=133690316927301&w=2
>>
>>  drivers/net/can/c_can/c_can.h          |   45 +++++++++++++++++++++++++++
>>  drivers/net/can/c_can/c_can_platform.c |   52 ++++++++++++++++++++++++--------
>>  2 files changed, 84 insertions(+), 13 deletions(-)
> 
> You should also update the Kconfig entry, name and help.
> 
>> diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
>> index d1e141e..01a7049 100644
>> --- a/drivers/net/can/c_can/c_can.h
>> +++ b/drivers/net/can/c_can/c_can.h
>> @@ -102,6 +102,51 @@ static const u16 reg_map_c_can[] = {
>>  	[C_CAN_MSGVAL2_REG]	= 0xB2,
>>  };
>>  
>> +static const u16 reg_map_d_can[] = {
>> +	[C_CAN_CTRL_REG]	= 0x00,
>> +	[C_CAN_STS_REG]		= 0x04,
>> +	[C_CAN_ERR_CNT_REG]	= 0x08,
>> +	[C_CAN_BTR_REG]		= 0x0C,
>> +	[C_CAN_BRPEXT_REG]	= 0x0E,
>> +	[C_CAN_INT_REG]		= 0x10,
>> +	[C_CAN_TEST_REG]	= 0x14,
>> +	[C_CAN_TXRQST1_REG]	= 0x88,
>> +	[C_CAN_TXRQST2_REG]	= 0x8A,
>> +	[C_CAN_NEWDAT1_REG]	= 0x9C,
>> +	[C_CAN_NEWDAT2_REG]	= 0x9E,
>> +	[C_CAN_INTPND1_REG]	= 0xB0,
>> +	[C_CAN_INTPND2_REG]	= 0xB2,
>> +	[C_CAN_MSGVAL1_REG]	= 0xC4,
>> +	[C_CAN_MSGVAL2_REG]	= 0xC6,
>> +	[C_CAN_IF1_COMREQ_REG]	= 0x100,
>> +	[C_CAN_IF1_COMMSK_REG]	= 0x102,
>> +	[C_CAN_IF1_MASK1_REG]	= 0x104,
>> +	[C_CAN_IF1_MASK2_REG]	= 0x106,
>> +	[C_CAN_IF1_ARB1_REG]	= 0x108,
>> +	[C_CAN_IF1_ARB2_REG]	= 0x10A,
>> +	[C_CAN_IF1_MSGCTRL_REG]	= 0x10C,
>> +	[C_CAN_IF1_DATA1_REG]	= 0x110,
>> +	[C_CAN_IF1_DATA2_REG]	= 0x112,
>> +	[C_CAN_IF1_DATA3_REG]	= 0x114,
>> +	[C_CAN_IF1_DATA4_REG]	= 0x116,
>> +	[C_CAN_IF2_COMREQ_REG]	= 0x120,
>> +	[C_CAN_IF2_COMMSK_REG]	= 0x122,
>> +	[C_CAN_IF2_MASK1_REG]	= 0x124,
>> +	[C_CAN_IF2_MASK2_REG]	= 0x126,
>> +	[C_CAN_IF2_ARB1_REG]	= 0x128,
>> +	[C_CAN_IF2_ARB2_REG]	= 0x12A,
>> +	[C_CAN_IF2_MSGCTRL_REG]	= 0x12C,
>> +	[C_CAN_IF2_DATA1_REG]	= 0x130,
>> +	[C_CAN_IF2_DATA2_REG]	= 0x132,
>> +	[C_CAN_IF2_DATA3_REG]	= 0x134,
>> +	[C_CAN_IF2_DATA4_REG]	= 0x136,
>> +};
>> +
>> +enum c_can_dev_id {
>> +	C_CAN_DEVTYPE,
> 
> Should probably be:
> 
> 	C_CAN_DEVTYPE = 0,
>
> for legacy C_CAN support.

I don't understand, what you mean by legacy support.

> 
>> +	D_CAN_DEVTYPE,
>> +};
>> +
>>  /* c_can private data structure */
>>  struct c_can_priv {
>>  	struct can_priv can;	/* must be the first member */
>> diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c
>> index a6e9b93..3b1f6c7 100644
>> --- a/drivers/net/can/c_can/c_can_platform.c
>> +++ b/drivers/net/can/c_can/c_can_platform.c
>> @@ -71,6 +71,7 @@ static int __devinit c_can_plat_probe(struct platform_device *pdev)
>>  	void __iomem *addr;
>>  	struct net_device *dev;
>>  	struct c_can_priv *priv;
>> +	const struct platform_device_id *id;
>>  	struct resource *mem;
>>  	int irq;
>>  #ifdef CONFIG_HAVE_CLK
>> @@ -115,7 +116,32 @@ static int __devinit c_can_plat_probe(struct platform_device *pdev)
>>  	}
>>  
>>  	priv = netdev_priv(dev);
>> -	priv->regs = reg_map_c_can;
>> +	id = platform_get_device_id(pdev);
>> +	switch (id->driver_data) {
>> +	case C_CAN_DEVTYPE:
>> +		priv->regs = reg_map_c_can;
>> +		switch (mem->flags & IORESOURCE_MEM_TYPE_MASK) {
>> +		case IORESOURCE_MEM_32BIT:
>> +			priv->read_reg = c_can_plat_read_reg_aligned_to_32bit;
>> +			priv->write_reg = c_can_plat_write_reg_aligned_to_32bit;
>> +			break;
>> +		case IORESOURCE_MEM_16BIT:
>> +		default:
>> +			priv->read_reg = c_can_plat_read_reg_aligned_to_16bit;
>> +			priv->write_reg = c_can_plat_write_reg_aligned_to_16bit;
>> +			break;
>> +		}
>> +		break;
>> +	case D_CAN_DEVTYPE:
>> +		priv->regs = reg_map_d_can;
>> +		priv->can.ctrlmode_supported |= CAN_CTRLMODE_3_SAMPLES;
>> +		priv->read_reg = c_can_plat_read_reg_aligned_to_16bit;
>> +		priv->write_reg = c_can_plat_write_reg_aligned_to_16bit;
>> +		break;
>> +	default:
>> +		ret = -EINVAL;
>> +		goto exit_free_device;
>> +	}
>>  
>>  	dev->irq = irq;
>>  	priv->base = addr;
>> @@ -124,18 +150,6 @@ static int __devinit c_can_plat_probe(struct platform_device *pdev)
>>  	priv->priv = clk;
>>  #endif
>>  
>> -	switch (mem->flags & IORESOURCE_MEM_TYPE_MASK) {
>> -	case IORESOURCE_MEM_32BIT:
>> -		priv->read_reg = c_can_plat_read_reg_aligned_to_32bit;
>> -		priv->write_reg = c_can_plat_write_reg_aligned_to_32bit;
>> -		break;
>> -	case IORESOURCE_MEM_16BIT:
>> -	default:
>> -		priv->read_reg = c_can_plat_read_reg_aligned_to_16bit;
>> -		priv->write_reg = c_can_plat_write_reg_aligned_to_16bit;
>> -		break;
>> -	}
>> -
>>  	platform_set_drvdata(pdev, dev);
>>  	SET_NETDEV_DEV(dev, &pdev->dev);
>>  
>> @@ -189,6 +203,17 @@ static int __devexit c_can_plat_remove(struct platform_device *pdev)
>>  	return 0;
>>  }
>>  
>> +static const struct platform_device_id c_can_id_table[] = {
>> +	{
>> +		.name = "c_can_platform",
>> +		.driver_data = C_CAN_DEVTYPE,
>> +	}, {
>> +		.name = "d_can_platform",
> 
> s/_platform// !

I thought about removing the "_platform", too:

The driver is traditionally called KBUILD_MODNAME, which is
"c_can_platform" in this case. So we need for compatibility a
"c_can_platform" entry in the id_table.

If we have a "c_can" and a "d_can" entry, the driver would still match a
"c_can_platform" device. The platform bus match function will first
match the id_table, then the driver name [1]. However, only when
matching via the id_table, the id_entry is set. So the driver will oops
when "id" will be dereferenced above.

We have these options:
a) use the patch as it is, with "c_can_platform" and "d_can_platform".
b) have "c_can_platform" and "d_can", which I don't think is intuitive
c) have "c_can_platform", "c_can" and "d_can"

[1] http://lxr.free-electrons.com/source/drivers/base/platform.c#L660

>> +		.driver_data = D_CAN_DEVTYPE,
>> +	}, {
>> +	}
>> +};
>> +
>>  static struct platform_driver c_can_plat_driver = {
>>  	.driver = {
>>  		.name = KBUILD_MODNAME,
>> @@ -196,6 +221,7 @@ static struct platform_driver c_can_plat_driver = {
>>  	},
>>  	.probe = c_can_plat_probe,
>>  	.remove = __devexit_p(c_can_plat_remove),
>> +	.id_table = c_can_id_table,
>>  };
>>  
>>  module_platform_driver(c_can_plat_driver);
> 

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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]