Re: [PATCH net-next v2 4/4] net: dsa: motorcomm: Add LED support

[Andrew Lunn]

> +	duty_req = YT921X_LED_DUTY(on > off ? off : on, cycle_req);
> +	for (unsigned int i = ARRAY_SIZE(dutys) - 1;; i--)
> +		if (i <= 0 || duty_req >= (dutys[i - 1] + dutys[i]) / 2) {
> +			duty = dutys[i];
> +			break;
> +		}

The ;; got the looking at this, since it is unusual.

i is an unsigned int, so it can never be < 0. The test should be just
i == 0.

> +static int
> +yt921x_led_blink_set(struct yt921x_priv *priv, int port, int group,
> +		     unsigned long *onp, unsigned long *offp)
> +{
> +	struct yt921x_port *pp = priv->ports[port];
> +	struct yt921x_led *led;
> +	unsigned short cycle;
> +	unsigned short duty;
> +	bool change_cycle;
> +	bool change_duty;
> +	bool use_cycle;
> +	u32 ctrl;
> +	u32 mask;
> +	u32 val;
> +	int res;
> +
> +	if (!pp)
> +		return -ENODEV;

I really do dislike all the tests.

Are LEDs created if pp is NULL?

Please don't use defensive programming, understand the logic of the
code and decide if this can happen. If it can, then yes, have the
test. If however yt921x_leds_setup() has this test, and will never
create and register LEDs with the LED code if pp is NULL, you don't
need any of these tests. Please look at every instance of such tests
and really consider, can it happen?

	Andrew