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

[Andy Shevchenko]

On Mon, Mar 30, 2020 at 10:49 PM Luca Weiss <luca@xxxxxxxxx> 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