H Marc!
> On 30.11.2017, at 14:04, Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> wrote:
>
> On 11/24/2017 07:35 PM, kernel@xxxxxxxxxxxxxxxx wrote:
>> From: Martin Sperl <kernel@xxxxxxxxxxxxxxxx>
>>
>> This patch adds support for the Microchip mcp2517fd CAN-FD controller.
>> The mcp2517fd is capable of transmitting and receiving standard data
>> frames, extended data frames, remote frames and Can-FD frames.
>> The mcp2517fd interfaces with the host over SPI.
>
> I've review about the last 1/3 of the driver. See comments inline.
Thanks for all the feedback!
I shall incorporate those into V2.
>> + switch (priv->config.gpio0_mode) {
>> + case gpio_mode_standby:
>> + case gpio_mode_int: /* asserted low on TXIF */
>> + case gpio_mode_out_low:
>> + case gpio_mode_out_high:
>> + case gpio_mode_in:
>
> Please add comments for fallthrough for all your switch-case.
I thought /* fall through */ is only needed for the cases
where there is code involved in each case block and you want
to fall through to the next block - checkpatch only complains
in such conditions (there is one location in the driver -
in mcp2517fd_can_ist_handle_status - that requires /* fall through */).
Also Documentation/process/coding-style.rst shows such an example
in the “indentation” section, so I would assume this is valid code.
>>
>> +int mcp2517fd_of_parse(struct mcp2517fd_priv *priv)
>> +{
>> +#ifdef CONFIG_OF_DYNAMIC
>
> Why does this code depend on OF_DYNAMIC?
>
Well - this is only in case of Device Tree - no need to include
all the DT-parsing in case that there is no DT support in the
first place…
I may arrange it as a #ifdef outside of the function
(like the other case you have mentioned).
>>
>> + if (!IS_ERR(clk)) {
>> + ret = clk_prepare_enable(clk);
>> + if (ret)
>> + goto out_free;
>> + }
>
> Why do you keep the clock running after the device has been probed?
> Usually we enable the clock during open().
I took the mcp251x as a blue-print and I believe it follows the
same pattern…
But I may have simplified things during early driver development,
so I will recheck it.
>
>> +
>> + /* check in device tree for overrrides */
>> + ret = mcp2517fd_of_parse(priv);
>> + if (ret)
>> + return ret;
>
> You're keeping the clock running.
See above - I shall look into it...
>> + dev_err(&spi->dev,
>> + "PLL clock frequency %i would exceed limit\n",
>> + priv->can.clock.freq
>> + );
>> + return -EINVAL;
>
> same here
Thanks, Martin--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
