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
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
<[email protected]> 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?
--
With Best Regards,
Andy Shevchenko
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
> <[email protected]> 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().