On Fri, Mar 01, 2024 at 02:42:00PM +1300, Chris Packham wrote: > Add a driver for a 7-segment LED display. At the moment only one > character is supported but it should be possible to expand this to > support more characters and/or 14-segment displays in the future. ... > + * Driver for a 7 segment LED display 7-segment ... > + * The GPIOs are wired to the 7 segments in a clockwise fashion starting from > + * the top. Not exactly. They can wire them as they wish, we just need to agree on the sequence of the segments in DT to be mapped to the 7-segment diagram. ... > + * -a- > + * | | > + * f b > + * | | > + * -g- > + * | | > + * e c > + * | | > + * -d- I would drop this as it's available in UAPI header... ... > +#include <linux/bitmap.h> > +#include <linux/container_of.h> > +#include <linux/errno.h> > +#include <linux/gpio/consumer.h> > +#include <linux/mod_devicetable.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/types.h> > +#include <linux/workqueue.h> ...which you forgot to include here. ... > +static void seg_led_update(struct work_struct *work) > +{ > + struct seg_led_priv *priv = container_of(work, struct seg_led_priv, work.work); > + struct linedisp *linedisp = &priv->linedisp; > + struct linedisp_map *map = linedisp->map; > + DECLARE_BITMAP(values, 8); > + bitmap_zero(values, 8); Why do you need this zeroing? > + bitmap_set_value8(values, map_to_seg7(&map->map.seg7, linedisp->buf[0]), 0); > + > + gpiod_set_array_value_cansleep(priv->segment_gpios->ndescs, priv->segment_gpios->desc, > + priv->segment_gpios->info, values); > +} -- With Best Regards, Andy Shevchenko