On Sun, Feb 25, 2024 at 11:34 PM Chris Packham
<chris.packham@xxxxxxxxxxxxxxxxxxx> 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.
(I try to comment only on the things that will remain after rebasing
on top of auxdisplay:for-next)
...
> +config SEG_LED
> + bool "Generic 7 segment LED display"
Why can't it be a module?
> + select LINEDISP
> + help
> + This driver supports a generic 7 segment LED display made up
> + of GPIO pins connected to the individual segments.
Checkpatch wants 3+ lines of help, I would add the module name (after
changing it to be tristate, etc).
...
> + * The GPIOs are wired to the 7 segments in a clock wise fashion starting from
clockwise
> + * the top.
...
> + * The decimal point LED presnet on some devices is currently not
present
> + * supported.
...
> +#include <linux/gpio/consumer.h>
> +#include <linux/kernel.h>
> +#include <linux/map_to_7segment.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
This is not used. And actually shouldn't be in a new code like this
with rare exceptions.
> +#include <linux/platform_device.h>
This is rather semirandom, please use IWYU (Include What You Use)
principle. We have, among others, container_of.h, types.h, err.h,
bitmap.h, mod_devicetable.h.
...
With
sturct device *dev = &pdev->dev;
the below code will be neater.
> + priv = devm_kzalloc(&pdev->dev, struct_size(priv, curr, 1), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> + priv->num_char = 1;
> +
> + platform_set_drvdata(pdev, priv);
> +
> + priv->segment_gpios = devm_gpiod_get_array(&pdev->dev, "segment", GPIOD_OUT_LOW);
> + if (IS_ERR(priv->segment_gpios))
> + return PTR_ERR(priv->segment_gpios);
...
> +static struct platform_driver seg_led_driver = {
> + .probe = seg_led_probe,
> + .remove = seg_led_remove,
> + .driver = {
> + .name = "seg-led",
> + .of_match_table = seg_led_of_match,
> + },
> +};
> +
Redundant blank line.
> +module_platform_driver(seg_led_driver);
> +
> +MODULE_AUTHOR("Chris Packham <chris.packham@xxxxxxxxxxxxxxxxxxx>");
> +MODULE_DESCRIPTION("7 segment LED driver");
> +MODULE_LICENSE("GPL");
> +
Seems like a redundant blank line at the end of the file.
--
With Best Regards,
Andy Shevchenko
