Re: [PATCH 16/17] auxdisplay: ht16k33: Add support for segment displays

[Robin van der Gracht]

On 2021-03-29 09:15, Geert Uytterhoeven wrote:
> Hoi Robin,
>
> On Mon, Mar 29, 2021 at 9:09 AM Robin van der Gracht 
> <robin@xxxxxxxxxxx> wrote:
> > On 2021-03-22 15:48, Geert Uytterhoeven wrote:
> > > The Holtek HT16K33 LED controller is not only used for driving
> > > dot-matrix displays, but also for driving segment displays.
> > >
> > > Add support for 4-digit 7-segment and quad 14-segment alphanumeric
> > > displays, like the Adafruit 7-segment and 14-segment display backpack
> > > and FeatherWing expansion boards.  Use the character line display core
> > > support to display a message, which will be scrolled if it doesn't fit.
> > >
> > > Signed-off-by: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> > > ---
> > > The 7-segment support is based on schematics, and untested on actual
> > > hardware.
> > > ---
> > >  drivers/auxdisplay/ht16k33.c | 198 +++++++++++++++++++++++++++++++++--
> > >  1 file changed, 191 insertions(+), 7 deletions(-)
> > >
> > ...
> > >
> > > +static int ht16k33_seg_probe(struct i2c_client *client,
> > > +                          struct ht16k33_priv *priv, uint32_t brightness)
> > > +{
> > > +     struct ht16k33_seg *seg = &priv->seg;
> > > +     struct device *dev = &client->dev;
> > > +     int err;
> > > +
> > > +     err = ht16k33_brightness_set(priv, MAX_BRIGHTNESS);
> > > +     if (err)
> > > +             return err;
> > > +
> > > +     switch (priv->type) {
> > > +     case DISP_MATRIX:
> > > +             /* not handled here */
> > > +             break;
> >
> > This 'case' shouldn't happen. Having said that, the break here will
> > still
> > cause the linedisp_register() function to be called for the 
> > DISP_MATRIX
> > type.
> > If you'd like to handle this case, a return (or setting 'err') should
> > prevent this.
>
> This function is never called if priv->type == DISP_MATRIX, so this
> cannot happen.  However, gcc complains if not all enum values are
> handled in a switch() statement, hence the dummy case.

I see. But if it's there it should work right? :P

> Is there a better way to handle this?

How about adding the default case:

default:
    /* not handled */
    err = -EINVAL;

Groetjes/Kind regards,
Robin van der Gracht

--
Protonic Holland
Factorij 36
1689AL Zwaag
+31 (0)229 212928
https://www.protonic.nl