On Mon, Jul 04, 2022 at 06:47:33PM +0000, Miod Vallat wrote:
> When the fdt does not provide a list of brightness states, pwmbl(4)
> builds a 256 state ramp (i.e. state[i] = i with 0 <= i < 256).
>
> The following diff keeps that behaviour, but gets rid of the malloc
> call for that ramp, since the values are trivially known.
>
> Compiles but not tested due to the lack of such hardware.
>
> Index: sys/dev/fdt/pwmbl.c
> ===================================================================
> RCS file: /OpenBSD/src/sys/dev/fdt/pwmbl.c,v
> retrieving revision 1.6
> diff -u -p -r1.6 pwmbl.c
> --- sys/dev/fdt/pwmbl.c 24 Oct 2021 17:52:26 -0000 1.6
> +++ sys/dev/fdt/pwmbl.c 4 Jul 2022 18:45:16 -0000
> @@ -35,7 +35,7 @@ struct pwmbl_softc {
> struct device sc_dev;
> uint32_t *sc_pwm;
> int sc_pwm_len;
> - uint32_t *sc_levels;
> + uint32_t *sc_levels; /* NULL if simple ramp */
> int sc_nlevels;
> uint32_t sc_max_level;
> uint32_t sc_def_level;
> @@ -73,7 +73,7 @@ pwmbl_attach(struct device *parent, stru
> struct pwmbl_softc *sc = (struct pwmbl_softc *)self;
> struct fdt_attach_args *faa = aux;
> uint32_t *gpios;
> - int i, len;
> + int len;
>
> len = OF_getproplen(faa->fa_node, "pwms");
> if (len < 0) {
> @@ -95,7 +95,7 @@ pwmbl_attach(struct device *parent, stru
> }
>
> len = OF_getproplen(faa->fa_node, "brightness-levels");
> - if (len > 0) {
> + if (len >= sizeof(uint32_t)) {
This actually breaks my machine. malloc() is saying allocation too
large. OF_getproplen will return -1 on that. Is it possible that
len is treated as uint64_t as it is an int and sizeof is effectively
uint64_t?
Moving len to ssize_t doesn't fix it, but doing
if (len >= (int)sizeof(uint32_t)) {
works. So I wonder if
if (len > 0 && len >= sizeof(uint32_t)) {
would work as well. Or maybe let's just keep it as it is?
> sc->sc_levels = malloc(len, M_DEVBUF, M_WAITOK);
> OF_getpropintarray(faa->fa_node, "brightness-levels",
> sc->sc_levels, len);
> @@ -107,13 +107,9 @@ pwmbl_attach(struct device *parent, stru
> sc->sc_def_level = sc->sc_nlevels - 1;
> sc->sc_def_level = sc->sc_levels[sc->sc_def_level];
> } else {
> + /* No levels, assume a simple 0..255 ramp. */
> sc->sc_nlevels = 256;
> - sc->sc_levels = mallocarray(sc->sc_nlevels,
> - sizeof(uint32_t), M_DEVBUF, M_WAITOK);
> - for (i = 0; i < sc->sc_nlevels; i++)
> - sc->sc_levels[i] = i;
> - sc->sc_max_level = sc->sc_levels[sc->sc_nlevels - 1];
> - sc->sc_def_level = sc->sc_levels[sc->sc_nlevels - 1];
> + sc->sc_max_level = sc->sc_def_level = sc->sc_nlevels - 1;
> }
>
> printf("\n");
> @@ -144,17 +140,22 @@ pwmbl_find_brightness(struct pwmbl_softc
> uint32_t mid;
> int i;
Might be easier to have a check like:
if (sc->sc_channels == NULL)
return level < sc->sc_nlevels ? level : sc->sc_nlevels - 1;
Then you don't need to indent the whole block. Makes the diff smaller
and a bit easier to understand?
Cheers,
Patrick
>
> - for (i = 0; i < sc->sc_nlevels - 1; i++) {
> - mid = (sc->sc_levels[i] + sc->sc_levels[i + 1]) / 2;
> - if (sc->sc_levels[i] <= level && level <= mid)
> + if (sc->sc_levels) {
> + for (i = 0; i < sc->sc_nlevels - 1; i++) {
> + mid = (sc->sc_levels[i] + sc->sc_levels[i + 1]) / 2;
> + if (sc->sc_levels[i] <= level && level <= mid)
> + return sc->sc_levels[i];
> + if (mid < level && level <= sc->sc_levels[i + 1])
> + return sc->sc_levels[i + 1];
> + }
> + if (level < sc->sc_levels[0])
> + return sc->sc_levels[0];
> + else
> return sc->sc_levels[i];
> - if (mid < level && level <= sc->sc_levels[i + 1])
> - return sc->sc_levels[i + 1];
> +
> + } else {
> + return level < sc->sc_nlevels ? level : sc->sc_nlevels - 1;
> }
> - if (level < sc->sc_levels[0])
> - return sc->sc_levels[0];
> - else
> - return sc->sc_levels[i];
> }
>
> int
>
