Messages in this thread Patch in this message | ![/](/images/icornerl.gif) | | Date | Mon, 6 Mar 2023 10:23:32 +0100 | From | Uwe Kleine-König <> | Subject | Re: [PATCH v7 2/2] trigger: ledtrig-tty: add additional modes |
| |
On Mon, Mar 06, 2023 at 09:04:56AM +0000, Lee Jones wrote: > On Mon, 06 Mar 2023, Jiri Slaby wrote: > > > On 03. 03. 23, 15:11, Lee Jones wrote: > > > On Wed, 22 Feb 2023, Florian Eckert wrote: > > > > @@ -113,21 +207,38 @@ static void ledtrig_tty_work(struct work_struct *work) > > > > trigger_data->tty = tty; > > > > } > > > > - ret = tty_get_icount(trigger_data->tty, &icount); > > > > - if (ret) { > > > > - dev_info(trigger_data->tty->dev, "Failed to get icount, stopped polling\n"); > > > > - mutex_unlock(&trigger_data->mutex); > > > > - return; > > > > - } > > > > - > > > > - if (icount.rx != trigger_data->rx || > > > > - icount.tx != trigger_data->tx) { > > > > - led_set_brightness_sync(trigger_data->led_cdev, LED_ON); > > > > - > > > > - trigger_data->rx = icount.rx; > > > > - trigger_data->tx = icount.tx; > > > > - } else { > > > > - led_set_brightness_sync(trigger_data->led_cdev, LED_OFF); > > > > + switch (trigger_data->mode) { > > > > + case TTY_LED_CTS: > > > > + ledtrig_tty_flags(trigger_data, TIOCM_CTS); > > > > + break; > > > > + case TTY_LED_DSR: > > > > + ledtrig_tty_flags(trigger_data, TIOCM_DSR); > > > > + break; > > > > + case TTY_LED_CAR: > > > > + ledtrig_tty_flags(trigger_data, TIOCM_CAR); > > > > + break; > > > > + case TTY_LED_RNG: > > > > + ledtrig_tty_flags(trigger_data, TIOCM_RNG); > > > > + break; > > > > + case TTY_LED_CNT: > > > > > > I believe this requires a 'fall-through' statement. > > > > I don't think this is the case. Isn't fallthrough required only in cases > > when there is at least one statement, i.e. a block? > > There's no mention of this caveat in the document. > > To my untrained eyes, the rule looks fairly explicit, starting with "All". > > " > All switch/case blocks must end in one of: > > * break; > * fallthrough; > * continue; > * goto <label>; > * return [expression]; > " > > If you're aware of something I'm not, please consider updating the doc.
Just to add my 0.02€: I think this case (i.e.
case TTY_LED_CNT: default: ...
) doesn't need a fall-through for two reasons:
a) The compilers don't warn about this construct even with the fall-through warning enabled; and b) I wouldn't call the TTY_LED_CNT line a "block", so the quoted document[1] doesn't apply. (Though I agree that there is some potential for improvement by mentioning this case. (haha))
Best regards Uwe
[1] Documentation/process/deprecated.rst
-- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | [unhandled content-type:application/pgp-signature] | ![\](/images/icornerr.gif) |