Re: [hackers] [slstatus][PATCH] backlight: implemented openbsd support

From: Tobias Tschinkowitz <he4d_AT_posteo.de>
Date: Wed, 23 May 2018 21:18:59 +0200

On Wed, May 23, 2018 at 03:57:37PM -0300, lucas wrote:
> Hi Tobias,
>
> Two notes below
>
> > diff --git a/components/backlight.c b/components/backlight.c
> > index f9c4096..21e06a1 100644
> > --- a/components/backlight.c
> > +++ b/components/backlight.c
> > _AT_@ -29,4 +29,30 @@
> >
> > return bprintf("%d", cur * 100 / max);
> > }
> > +
> > +#elif defined(__OpenBSD__)
> > + #include <fcntl.h>
> > + #include <sys/ioctl.h>
> > + #include <sys/time.h>
> > + #include <dev/wscons/wsconsio.h>
> > +
> > + const char *
> > + backlight_perc(const char *unused)
> > + {
> > + int fd, err;
> > + struct wsdisplay_param wsd_param = {
> > + .param = WSDISPLAYIO_PARAM_BRIGHTNESS
> > + };
> > +
> > + if ((fd = open("/dev/ttyC0", O_RDONLY)) < 0) {
>
> This only works if you run startx from tty. If not, /dev/ttyC0 would be 0600
> owned by root.
>

See my notes below the commit message of the patch.

> > + warn("could not open /dev/ttyC0");
> > + return NULL;
> > + }
> > + if ((err = ioctl(fd, WSDISPLAYIO_GETPARAM, &wsd_param)) < 0) {
>
> Why storing the return value in err if it isn't used?
>

Bad habit, removed it, see diff below.

> > + warn("ioctl 'WSDISPLAYIO_GETPARAM' failed");
> > + return NULL;
> > + }
> > + return bprintf("%d", wsd_param.curval * 100 / wsd_param.max);
>
> wsd_param also includes a .min field. It should be properly scaled instead
> of just dividing by .max
>

Fixed that, see diff below.

> > + }
> > +
> > #endif
> > diff --git a/config.def.h b/config.def.h
> > index 75debe5..3a0f838 100644
> > --- a/config.def.h
> > +++ b/config.def.h
> > _AT_@ -14,6 +14,7 @@ static const char unknown_str[] = "n/a";
> > *
> > * backlight_perc backlight percentage device name
> > * (intel_backlight)
> > + * NULL on OpenBSD
> > * battery_perc battery percentage battery name (BAT0)
> > * NULL on OpenBSD
> > * battery_state battery charging state battery name (BAT0)
> > --
> > 2.16.2
> >
> >
>
> Cheers.
>

Thanks for the notes lucas!
Updated diff below.

diff --git a/components/backlight.c b/components/backlight.c
index 21e06a1..4234164 100644
--- a/components/backlight.c
+++ b/components/backlight.c
_AT_@ -39,7 +39,7 @@
         const char *
         backlight_perc(const char *unused)
         {
- int fd, err;
+ int fd;
                 struct wsdisplay_param wsd_param = {
                         .param = WSDISPLAYIO_PARAM_BRIGHTNESS
                 };
_AT_@ -48,11 +48,12 @@
                         warn("could not open /dev/ttyC0");
                         return NULL;
                 }
- if ((err = ioctl(fd, WSDISPLAYIO_GETPARAM, &wsd_param)) < 0) {
+ if ((ioctl(fd, WSDISPLAYIO_GETPARAM, &wsd_param)) < 0) {
                         warn("ioctl 'WSDISPLAYIO_GETPARAM' failed");
                         return NULL;
                 }
- return bprintf("%d", wsd_param.curval * 100 / wsd_param.max);
+ return bprintf("%d", wsd_param.curval * 100 /
+ (wsd_param.max - wsd_param.min));
         }
 
 #endif
Received on Wed May 23 2018 - 21:18:59 CEST