Hi Marc,
Marc Kleine-Budde wrote:
> christian pellegrin wrote:
>> Hi,
>>
>> I'm attaching the patch that implements the OSM for the mcp2515.
>> Unfortunately right now I have just a mcp2510 available for testing
>> which doesn't support OSM, so if someone acknowledges it's OK I'll
>> send a proper patch. I should have an mcp2515 on a new hardware on mid
>> January to test it myself, but, you know, never trust the deadlines
>> that the hardware guy has told you ;-).
>>
>> BTW it's a pity that OSM is implemented only on the mcp2515 and not on
>> the mcp2510. Perhaps it could be a best default mode of operation
>> because it gives more control to the software to account for the bus
>> state.
>
> Do you want to make one-shot mode default on the mcp2515? What does the
> CAN spec define?
Why do you think it's the default? CAN_CTRLMODE_ONE_SHOT must be set by
the user/app first.
>> ---8<----- against SVN trunk ------
>> --- kernel/2.6/drivers/net/can/mcp251x.c (revision 1095)
>> +++ kernel/2.6/drivers/net/can/mcp251x.c (working copy)
>> @@ -539,6 +539,7 @@
>>
>> static void mcp251x_set_normal_mode(struct spi_device *spi)
>> {
>> + struct mcp251x_platform_data *pdata = spi->dev.platform_data;
>> struct mcp251x_priv *priv = dev_get_drvdata(&spi->dev);
>> unsigned long timeout;
>>
>> @@ -553,7 +554,10 @@
>> mcp251x_write_reg(spi, CANCTRL, CANCTRL_REQOP_LOOPBACK);
>> } else {
>> /* Put device into normal mode */
>> - mcp251x_write_reg(spi, CANCTRL, CANCTRL_REQOP_NORMAL);
>> + mcp251x_write_reg(spi, CANCTRL, CANCTRL_REQOP_NORMAL |
>> + ((pdata->model == CAN_MCP251X_MCP2515 &&
>> + priv->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT)
>> ?
>> + CANCTRL_OSM : 0));
>>
>> /* Wait for the device to enter normal mode */
>> timeout = jiffies + HZ;
>> ---8<----- against 2.6.33-rc1 ------
>> diff --git a/drivers/net/can/mcp251x.c b/drivers/net/can/mcp251x.c
>> index 9c5a153..68ec171 100644
>> --- a/drivers/net/can/mcp251x.c
>> +++ b/drivers/net/can/mcp251x.c
>> @@ -531,6 +531,7 @@ static int mcp251x_do_set_mode(struct net_device
>> *net, enum can_mode mode)
>>
>> static void mcp251x_set_normal_mode(struct spi_device *spi)
>> {
>> + struct mcp251x_platform_data *pdata = spi->dev.platform_data;
>> struct mcp251x_priv *priv = dev_get_drvdata(&spi->dev);
>> unsigned long timeout;
>>
>> @@ -545,7 +546,10 @@ static void mcp251x_set_normal_mode(struct spi_device
>> *spi)
>> mcp251x_write_reg(spi, CANCTRL, CANCTRL_REQOP_LOOPBACK);
>> } else {
>> /* Put device into normal mode */
>> - mcp251x_write_reg(spi, CANCTRL, CANCTRL_REQOP_NORMAL);
>> + mcp251x_write_reg(spi, CANCTRL, CANCTRL_REQOP_NORMAL |
>> + ((pdata->model == CAN_MCP251X_MCP2515 &&
>> + priv->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT)
>> ?
>> + CANCTRL_OSM : 0));
>>
>> /* Wait for the device to enter normal mode */
>> timeout = jiffies + HZ;
>
> What about returning some error value if the adapter doesn't support the
> settings.
This would indeed make sense here, as the MPC2510 does not support
one-shot. In general, we do not yet check the capabilities of a CAN
controller, like loopback or listen-only. This could be done in a common
way be introducing "priv->can.ctrlmode_supported". The driver can then
set the bits for the supported features to allow common code to do the
checking. Just some quick brainstorming.
Wolfgang.
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
