| Message ID | 20240301014203.2033844-1-chris.packham@alliedtelesis.co.nz |
|---|---|
| Headers | show |
| Series | auxdisplay: 7 segment LED display | expand |
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); > +}
On Fri, Mar 01, 2024 at 02:42:01PM +1300, Chris Packham wrote: > Add bindings for a generic 7-segment LED display using GPIOs. ... > + segment-gpios: > + description: > + An array of GPIOs one per segment. The first GPIO corresponds to the A > + segment the last GPIO corresponds to the G segment. > + minItems: 7 > + maxItems: 7 As we discussed, put the ASCII art here to be sure everybody on the same page what 'a' - 'g' are and I would put 'dp' as well for the better understanding of the possible configurations (it doesn't mean you have to support it). ... Either way this will require an Ack by DT maintainers.
On Fri, Mar 01, 2024 at 02:41:59PM +1300, Chris Packham wrote: > This series adds a driver for a 7 segment LED display. > > I haven't had a chance to look at the gpio changes that'd be required to > have multiple characters as subnodes. I wanted to get the code that > addressed Andy and Rob's comments out before my weekend. Thank you for the update! Almost fine, one more iteration needed for some minor fixes.
On 2/03/24 07:18, Andy Shevchenko wrote: >> +static void seg_led_update(struct work_struct *work) >> +{ >> + struct seg_led_priv *priv = container_of(work, struct seg_led_priv,http://scanmail.trustwave.com/?c=20988&d=iZzi5b3S-TQCft9iEXDE69U9UtY0-7GANk9t1WkCxg&u=http%3a%2f%2fwork%2ework%29%3b >> + 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); >> + Without the zeroing above GCC complains about use of a potentially uninitialized variable here. I think because bitmap_set_value8() does &= and |=. >> + gpiod_set_array_value_cansleep(priv->segment_gpios->ndescs, priv->segment_gpios->desc, >> + priv->segment_gpios->info, values); >> +}
+Cc: Rasmus, Yury On Sun, Mar 3, 2024 at 9:58 PM Chris Packham <Chris.Packham@alliedtelesis.co.nz> wrote: > On 2/03/24 07:18, Andy Shevchenko wrote: ... > >> + 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); > Without the zeroing above GCC complains about use of a potentially > uninitialized variable here. I think because bitmap_set_value8() does &= > and |=. Hmm... Rasmus, Yury, do we have any ideas how to get rid of this redundancy?
On Sun, Mar 03, 2024 at 10:35:03PM +0200, Andy Shevchenko wrote: > +Cc: Rasmus, Yury > > On Sun, Mar 3, 2024 at 9:58 PM Chris Packham > <Chris.Packham@alliedtelesis.co.nz> wrote: > > On 2/03/24 07:18, Andy Shevchenko wrote: > > ... > > > >> + 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); > > > Without the zeroing above GCC complains about use of a potentially > > uninitialized variable here. I think because bitmap_set_value8() does &= > > and |=. > > Hmm... Rasmus, Yury, do we have any ideas how to get rid of this redundancy? DECLARE_BITMAP(values, 8) = { 0 };
On 2/03/24 07:18, Andy Shevchenko wrote: > 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. > > ... Actually do I need to? I'm not using anything in map_to_7segment.h, that's all taken care of in the line-display code.
On 4/03/24 13:45, Chris Packham wrote: > > On 2/03/24 07:18, Andy Shevchenko wrote: >> 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. >> >> ... > Actually do I need to? I'm not using anything in map_to_7segment.h, > that's all taken care of in the line-display code. Oops no I still need it for map_to_seg7().