Messages in this thread | | | Subject | Re: [PATCH v3 2/2] leds: add sgm3140 driver | From | Jacek Anaszewski <> | Date | Mon, 27 Apr 2020 23:10:26 +0200 |
| |
Hi Luca,
On 4/27/20 9:49 PM, Luca Weiss wrote: > On Montag, 27. April 2020 11:51:02 CEST Pavel Machek wrote: >> Hi! >> >>> 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. >>> >>> Signed-off-by: Luca Weiss <luca@z3ntu.xyz> >> >> Thanks, applied, but... I may remove it again. >> >>> +++ b/drivers/leds/leds-sgm3140.c >>> @@ -0,0 +1,320 @@ >>> +// SPDX-License-Identifier: GPL-2.0 >> >> Would you consider GPL-2+? > > I don't really have a preference either way but GPL-2.0-or-later is fine for > me. > >> >>> +#if IS_ENABLED(CONFIG_V4L2_FLASH_LED_CLASS) >>> +static void sgm3140_init_v4l2_flash_config(struct sgm3140 *priv, >> >> Ok. >> >>> +static int sgm3140_probe(struct platform_device *pdev) >>> +{ >> >> ... >> >>> + led_cdev->brightness_set_blocking = sgm3140_brightness_set; >>> + led_cdev->max_brightness = LED_ON; >> >> Don't do this, unless you really have 255 levels of brightness. > > LED_ON is 1, so the brightness available is 0 - 1. > >> >>> + /* Create V4L2 Flash subdev */ >>> + priv->v4l2_flash = v4l2_flash_init(&pdev->dev, >>> + child_node, >>> + fled_cdev, NULL, >>> + &v4l2_sd_cfg); >>> + if (IS_ERR(priv->v4l2_flash)) { >> >> Does this need some #ifdef guards? > > v4l2_flash_init has a NULL-returning version when CONFIG_V4L2_FLASH_LED_CLASS > is not defined (see https://elixir.bootlin.com/linux/latest/source/include/ > media/v4l2-flash-led-class.h#L166 )
Correct.
>> >>> + ret = PTR_ERR(priv->v4l2_flash); >>> + goto err; >>> + } >>> + >>> + return ret; >> >> Should this return 0? > > ret should be 0 here, so it shouldn't matter much. > >> >>> +err: >>> + fwnode_handle_put(child_node); >>> + return ret; >>> +} >> >> Does non-error path needs handle_put, too? > > I don't think so, I'm passing child_node to v4l2_flash_init which then saves > the pointer to v4l2_subdev->fwnode. > > The devm_led_classdev_flash_register_ext function also seems to store the > pointer (led_cdev->dev->fwnode = init_data->fwnode; in > led_classdev_register_ext)
The node needs to be held only during parsing, so you have to call fwnode_handle_put() for both paths.
-- Best regards, Jacek Anaszewski
| |