Re: [PATCH 1/4] Consolidate and unify state change handling

[Andri Yngvason <andri.yngvason@marel.com>]

On fös 19.sep 2014 20:33, Wolfgang Grandegger wrote:
> On 09/18/2014 06:25 PM, Andri Yngvason wrote:
>
> Please add a brief patch description. E.g. what is it good for and what
> does it improve.
I'll do that. Thanks for pointing this out.
>> ...
>> +
>> +void can_change_state(struct net_device *dev, struct can_frame *cf,
>> +              enum can_state state, enum can_err_dir err_dir)
> Thinking about it... passing txerr and rxerr directly here would make
> the err_dir obsolete and the code more simpler. CAN controllers
> not supporting tx/rx error counter should pass "0, 0".
There are controllers that do not expose the error counters but do expose
the individual states of the error counters (e.g. mscan); or maybe rather
which state changed? What I proposed in my reply to [PATCH 2/4] would
solve this.

>
> Also s/state/new_state/ would improve readability.
Yes.
>
>> +{
>> +    struct can_priv *priv = netdev_priv(dev);
>> +
>> +    if (unlikely(state == priv->state)) {
>> +        netdev_warn(dev, "%s: oops, state did not change", __func__);
>> +        return;
>> +    }
>> +
>> +    if (state != CAN_STATE_BUS_OFF)
>> +        cf->can_id |= CAN_ERR_CRTL;
>> +
>> +    switch (state) {
>> +    case CAN_STATE_ERROR_WARNING:
>> +        if (state > priv->state)
>> +            priv->can_stats.error_warning++;
>> +        cf->data[1] = can_make_state_warning(err_dir);
>> +        break;
>> +
>> +    case CAN_STATE_ERROR_PASSIVE:
>> +        if (state > priv->state)
>> +            priv->can_stats.error_passive++;
>> +        cf->data[1] = can_make_state_passive(err_dir);
>> +        break;
>> +
>> +    case CAN_STATE_ERROR_ACTIVE:
>> +        cf->data[1] = CAN_ERR_CRTL_ACTIVE;
>> +        break;
>> +
>> +    case CAN_STATE_BUS_OFF:
>> +        priv->can_stats.bus_off++;
>> +        cf->can_id |= CAN_ERR_BUSOFF;
>> +        break;
>> +
>> +    default:
> A netdev_warn would be nice here.
Yes, that would probably we wise in case someone accidentally passes 
something
that isn't really a state.

Andri