On Wed, Oct 21, 2009 at 11:03 AM, Wolfgang Grandegger <[email protected]>
wrote:
> Hi Christian,
Hi,
>
> here's my quick review of the MPC251x driver. First some general comments:
>
> - Use proper Linux style for comments.
>
> - Use a consistent style for indention of function declarations and
> multi-line expressions.
>
> - Use macro definitions for constant values.
>
> - Check the proper usage of brackets { } for if, for, etc.
huh, I hoped that checkpatch.pl would alert me about these style
problems. I'll review this carefully.
>
> - Remove extended debugging message.
ack
>
> - Consider replacing ret with err but only if err==success and
> !err=failure (minor issue). Then use "if ([!]err) ...".
ack
>
> - Don't use __func__ but a meaning full error message.
ack
> I see redundant code above. What about implementing mcp251x_write_regs()
> and mcp251x_read_regs() allowing to read and write successive bytes with
> one SPI transfer, or many for the MCP2510. This might be used in other
> places as well.
>
I see. I'll write a more general function do_cmd which will take care
of difference between the two models. So the logic of
unpacking/packing data won't be duplicated. I'll add macros too.
>
> Ditto. Also please use macro definitons to make the code more readable.
>
ack
>> dev_dbg(&spi->dev, "%s: BRP = %d, PropSeg = %d, PS1 = %d,"
>> " PS2 = %d, SJW = %d\n", __func__, bt->brp,
>> bt->prop_seg, bt->phase_seg1, bt->phase_seg2,
>> bt->sjw);
>
> Just for development?
>
ack, I can read calculated parameters via /sys if I have to check them.
>> mcp251x_write_bits(spi, CNF3, CNF3_PHSEG2_MASK,
>> (bt->phase_seg2 - 1));
>
> Is triple sampling supported?
>
yes, according to paragraph 5.5 of the data sheet. It could be
configured via bit 6 of register CNF2. I grepped other drivers and
found priv->can.ctrlmode as a way to activate it.
> Also, please add a dev_info("Setting CNF1=...) call.
ack for all the CNF.
>>
>> if (pdata->transceiver_enable)
>> pdata->transceiver_enable(1);
>
> This driver also need to handle the tranceiver. And this implementation
> is quite efficient.
>
My customer board has an SN65HVD232 which has to be turned off before
suspending. The lowest power consumption is achieved by forcing
mcp2510 in software sleep and by turning off the trnsceiver actually.
>>
>> mcp251x_write_reg(spi, TXBCTRL(0), 0);
>> if (priv->tx_skb || priv->tx_len) {
>> net->stats.tx_errors++;
>
> Do we need to increase tx_dropped as well? Could/should be done in
> mcp251x_clean(). What type of TX error is that. As we don't increase
> statistics, a dev_dbg message would be nice to have.
I'll verify that it's correct to do the increment in _clean. I had a
look at other drivers and it looks to me that dropped is incremented
only if the packet is not sent for software problems (for example
impossibility to alloc a skb). I'll check this.
>> /* create error frame */
>> skb = alloc_can_err_skb(net, &frame);
>
> An error message for !skb would be nice, like for normal messages.
>
ack
>> else
>> new_state = CAN_STATE_ERROR_ACTIVE;
>
> Any effort to make the error code handling more compact would be nice.
> E.g., this if block could be combined with the above one using a temp.
> variable data1. Then: if (skb && data1) {frame->data[1] = data1, ...}
>
ack, I'll try hard to come out with better code
>
> In case of automatice bus-off recovery, a RESTARTED error message should
> be created (frame->can_id |= CAN_ERR_RESTARTED).
>
ack
>
> Please return the error code of request_irq() in case of error. Also,
> consider requesting the irq in the open function (and free it in the
> stop function.
>
ack
thanks for the review
--
Christian Pellegrin, see http://www.evolware.org/chri/
"Real Programmers don't play tennis, or any other sport which requires
you to change clothes. Mountain climbing is OK, and Real Programmers
wear their climbing boots to work in case a mountain should suddenly
spring up in the middle of the computer room."
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
