linux-kernel - Re: [PATCH v2 2/2] leds: add sgm3140 driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 4 Apr 2020 12:58:31 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Luca Weiss <luca@...tu.xyz>
Cc:     Linux LED Subsystem <linux-leds@...r.kernel.org>,
        Dan Murphy <dmurphy@...com>, Heiko Stuebner <heiko@...ech.de>,
        Icenowy Zheng <icenowy@...c.io>,
        Jacek Anaszewski <jacek.anaszewski@...il.com>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Mark Rutland <mark.rutland@....com>,
        Maxime Ripard <mripard@...nel.org>,
        Pavel Machek <pavel@....cz>, Rob Herring <robh+dt@...nel.org>,
        Shawn Guo <shawnguo@...nel.org>,
        devicetree <devicetree@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        ~postmarketos/upstreaming@...ts.sr.ht
Subject: Re: [PATCH v2 2/2] leds: add sgm3140 driver

On Mon, Mar 30, 2020 at 10:49 PM Luca Weiss <luca@...tu.xyz> wrote:
>
> Add a driver for the SGMICRO SGM3140 Buck/Boost Charge Pump LED driver.
>
> This device is controlled by two GPIO pins, one for enabling and the
> second one for switching between torch and flash mode.

...

> +config LEDS_SGM3140
> +       tristate "LED support for the SGM3140"
> +       depends on LEDS_CLASS_FLASH
> +       depends on V4L2_FLASH_LED_CLASS || !V4L2_FLASH_LED_CLASS

> +       depends on OF

depends on OF || COMPILE_TEST ?
But hold on...

...

> +#include <linux/of.h>

Perhaps switch this to property.h and replace OF with more generic
device property / fwnode API?

...

> +struct sgm3140 {
> +       bool enabled;
> +       struct gpio_desc *flash_gpio;
> +       struct gpio_desc *enable_gpio;
> +       struct regulator *vin_regulator;
> +
> +       /* current timeout in us */
> +       u32 timeout;
> +       /* maximum timeout in us */
> +       u32 max_timeout;
> +

> +       struct led_classdev_flash fled_cdev;

I guess it might be slightly better to make it first member of the
struct (I didn't check but the rationale is to put more often used
members at the beginning to utilize cachelines).

> +       struct v4l2_flash *v4l2_flash;
> +
> +       struct timer_list powerdown_timer;
> +};

...

> +static struct sgm3140 *flcdev_to_sgm3140(struct led_classdev_flash *flcdev)
> +{
> +       return container_of(flcdev, struct sgm3140, fled_cdev);
> +}

...and this becomes a no-op AFAICS (doesn't mean you need to remove it).

...

> +       struct device_node *child_node;

> +       child_node = of_get_next_available_child(pdev->dev.of_node, NULL);

> +       ret = of_property_read_u32(child_node, "flash-max-timeout-us",
> +                                  &priv->max_timeout);

> +       init_data.fwnode = of_fwnode_handle(child_node);

> +       of_node_put(child_node);

Device property / fwnode API?

--
With Best Regards,
Andy Shevchenko