Re: [PATCH 1/2] hwinit support for non-TI devices

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

On 05/05/2014 02:21 PM, Marc Kleine-Budde wrote:
> On 05/05/2014 02:08 PM, Pavel Machek wrote:
>> Non-TI chips (including socfpga) needs different raminit
>> sequence. Implement it.
>>
>> Tested-by: Thor Thayer <tthayer@altera.com>
>> Signed-off-by: Thor Thayer <tthayer@altera.com>
>> Signed-off-by: Pavel Machek <pavel@denx.de>
>>
>> diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c
>> index 1df0b32..476d1e0 100644
>> --- a/drivers/net/can/c_can/c_can_platform.c
>> +++ b/drivers/net/can/c_can/c_can_platform.c
>> @@ -40,6 +40,7 @@
>>  #define CAN_RAMINIT_START_MASK(i)	(0x001 << (i))
>>  #define CAN_RAMINIT_DONE_MASK(i)	(0x100 << (i))
>>  #define CAN_RAMINIT_ALL_MASK(i)		(0x101 << (i))
>> +#define DCAN_RAM_INIT_BIT	(1 << 3)
>>  static DEFINE_SPINLOCK(raminit_lock);
>>  /*
>>   * 16-bit c_can registers can be arranged differently in the memory
>> @@ -80,7 +81,7 @@ static void c_can_hw_raminit_wait(const struct c_can_priv *priv, u32 mask,
>>  		udelay(1);
>>  }
>>  
>> -static void c_can_hw_raminit(const struct c_can_priv *priv, bool enable)
>> +static void c_can_hw_raminit_ti(const struct c_can_priv *priv, bool enable)
>>  {
>>  	u32 mask = CAN_RAMINIT_ALL_MASK(priv->instance);
>>  	u32 ctrl;
>> @@ -88,7 +89,8 @@ static void c_can_hw_raminit(const struct c_can_priv *priv, bool enable)
>>  	spin_lock(&raminit_lock);
>>  
>>  	ctrl = readl(priv->raminit_ctrlreg);
>> -	/* We clear the done and start bit first. The start bit is
>> +	/*
>> +	 * We clear the done and start bit first. The start bit is
> 
> Please don't reformat comments.
> 
>>  	 * looking at the 0 -> transition, but is not self clearing;
>>  	 * And we clear the init done bit as well.
>>  	 */
>> @@ -108,6 +110,54 @@ static void c_can_hw_raminit(const struct c_can_priv *priv, bool enable)
>>  	spin_unlock(&raminit_lock);
>>  }
>>  
>> +static void c_can_hw_raminit(const struct c_can_priv *priv, bool enable)
>> +{
>> +	u32 ctrl;
>> +
>> +	spin_lock(&raminit_lock);
> 
> Why do you need this spinlock?
> 
>> +
>> +	ctrl = readl(priv->raminit_ctrlreg);
>> +	ctrl &= ~DCAN_RAM_INIT_BIT;
>> +	writel(ctrl, priv->raminit_ctrlreg);
> 
> Why don't use use the reg directly? Have you read my previous review?
> 
>> +	c_can_hw_raminit_wait(priv, ctrl, 0);
>> +
>> +	if (enable) {
>> +		/* Set start bit. */
>> +		ctrl |= DCAN_RAM_INIT_BIT;
>> +		writel(ctrl, priv->raminit_ctrlreg);
>> +		c_can_hw_raminit_wait(priv, ctrl, 0);
>> +	}
>> +	spin_unlock(&raminit_lock);
>> +}
>> +
>> +static u32 c_can_plat_read_reg32(struct c_can_priv *priv, enum reg index)
>> +{
>> +	u32 val;
>> +
>> +	val = priv->read_reg(priv, index);
>> +	val |= ((u32) priv->read_reg(priv, index + 1)) << 16;
>> +
>> +	return val;
>> +}
> 
> Why are you adding the read32() support here? Your commit message
> doesn't mention it. Please move this into a different patch.
> 
>> +
>> +static void c_can_plat_write_reg32(struct c_can_priv *priv, enum reg index,
>> +		u32 val)
>> +{
>> +	priv->write_reg(priv, index + 1, val>>16);
>> +	priv->write_reg(priv, index, val);
>> +}
>> +
>> +static u32 d_can_plat_read_reg32(struct c_can_priv *priv, enum reg index)
>> +{
>> +	return readl(priv->base + priv->regs[index]);
>> +}
>> +
>> +static void d_can_plat_write_reg32(struct c_can_priv *priv, enum reg index,
>> +		u32 val)
>> +{
>> +	writel(val, priv->base + priv->regs[index]);
>> +}
>> +
>>  static struct platform_device_id c_can_id_table[] = {
>>  	[BOSCH_C_CAN_PLATFORM] = {
>>  		.name = KBUILD_MODNAME,
>> @@ -201,11 +251,15 @@ static int c_can_plat_probe(struct platform_device *pdev)
>>  		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;
>> +			priv->read_reg32 = c_can_plat_read_reg32;
>> +			priv->write_reg32 = c_can_plat_write_reg32;

This will not compile.

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   |