Message-ID: <4539487.31r3eYUQgx@g550jk>
Date:   Sun, 08 Mar 2020 12:32:41 +0100
From:   Luca Weiss <luca@...tu.xyz>
To:     linux-leds@...r.kernel.org,
        Jacek Anaszewski <jacek.anaszewski@...il.com>
Cc:     ~postmarketos/upstreaming@...ts.sr.ht, Pavel Machek <pavel@....cz>,
        Dan Murphy <dmurphy@...com>, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH] leds: add sgm3140 driver

Hi Jacek,

Thanks for your review! Replies are inline below.

I'm wondering if I should implement support for the flash-max-timeout-us dt 
property ("Maximum timeout in microseconds after which the flash LED is turned 
off.") to configure the timeout to turn the flash off as I've currently hardcoded 
250ms but this might not be ideal for all uses of the sgm3140. The datasheet 
states:

> Flash mode is usually used with a pulse of about 200 to 300 milliseconds to 
> generate a high intensity Flash.

so it might be useful to have this configurable in the devicetree. The value of 
250ms works fine for my use case.

Theoretically also the .timeout_set op could be implemented but I'm not sure 
if this fits nicely into the existing "timeout" API and if it even makes sense 
to implement that.

Regards,
Luca

On Donnerstag, 5. März 2020 22:09:16 CET Jacek Anaszewski wrote:
> Hi Luca,
> 
> Thank you for the patch.
> 
> On 2/27/20 7:50 PM, Luca Weiss wrote:
> > Add a driver for the SGMICRO SGM3140 Buck/Boost Charge Pump LED driver.
> > 
> > This device is controller by two GPIO lines, one for enabling the LED
> > and the second one for switching between torch and flash mode.
> > 
> > The device will automatically switch to torch mode after being in flash
> > mode for about 250-300ms, so after that time the driver will turn the
> > LED off again automatically.
> > 
> > Signed-off-by: Luca Weiss <luca@...tu.xyz>
> > ---
> > Hi, this driver is controllable via sysfs and v4l2 APIs (as documented
> > in Documentation/leds/leds-class-flash.rst).
> > 
> > The following is possible:
> > 
> > # Torch on
> > echo 1 > /sys/class/leds/white\:flash/brightness
> > # Torch off
> > echo 0 > /sys/class/leds/white\:flash/brightness
> > # Activate flash
> > echo 1 > /sys/class/leds/white\:flash/flash_strobe
> > 
> > # Torch on
> > v4l2-ctl -d /dev/video1 -c led_mode=2
> > # Torch off
> > v4l2-ctl -d /dev/video1 -c led_mode=0
> > # Activate flash
> > v4l2-ctl -d /dev/video1 -c strobe=1
> 
> What is /dev/video1 ? Did you register vl42 flash subdev
> in some v4l2 media controller device?

On the Allwinner A64 SoC /dev/video0 is the node for cedrus (video encoder/
decoder), so the sun6i-csi driver gets to be /dev/video1

# v4l2-ctl --list-devices
cedrus (platform:cedrus):
        /dev/video0
        /dev/media0

sun6i-csi (platform:csi):
        /dev/video1

Allwinner Video Capture Device (platform:sun6i-csi):
        /dev/media1


Here's the relevant part from my dts:

sgm3140 {
    compatible = "sgmicro,sgm3140";
    flash-gpios = <&pio 3 24 GPIO_ACTIVE_HIGH>; /* FLASH_TRIGOUT: PD24 */
    enable-gpios = <&pio 2 3 GPIO_ACTIVE_HIGH>; /* FLASH_EN: PC3 */

    sgm3140_flash: led {
        function = LED_FUNCTION_FLASH;
        color = <LED_COLOR_ID_WHITE>;
    };
};

/* as subnode of csi (compatible: allwinner,sun50i-a64-csi) */
ov5640: rear-camera@4c {
    compatible = "ovti,ov5640";
    <snip>
    flash-leds = <&sgm3140_flash>;
};

> 
> > Unfortunately the last command (enabling the 'flash' via v4l2 results in
> > 
> > the following being printed and nothing happening:
> >   VIDIOC_S_EXT_CTRLS: failed: Resource busy
> >   strobe: Resource busy
> > 
> > Unfortunately I couldn't figure out the reason so I'm hoping to get some
> > guidance for this. iirc it worked at some point but then stopped.
> 
> You have to be in flash mode to strobe i.e. led_mode=1.

Of course..! Makes sense, I just never realized the v4l2 device had to be in 
this mode for the strobe button to work. It works nicely with that, thanks!

> <<snip>>

> > +static void sgm3140_powerdown_timer(struct timer_list *t)
> > +{
> > +	struct sgm3140 *priv = from_timer(priv, t, powerdown_timer);
> > +
> > +	gpiod_set_value_cansleep(priv->enable_gpio, 0);
> > +	gpiod_set_value_cansleep(priv->flash_gpio, 0);
> 
> You could also implement strobe_get op and return from it a flag
> indicating if LED is strobing.

Makes sense.
> > +}
> > +
> > +#if IS_ENABLED(CONFIG_V4L2_FLASH_LED_CLASS)
> > +static void sgm3140_init_v4l2_flash_config(struct sgm3140 *priv,
> > +					   struct 
v4l2_flash_config *v4l2_sd_cfg)
> > +{
> > +	struct led_classdev *led_cdev = &priv->fled_cdev.led_cdev;
> > +	struct led_flash_setting *s;
> > +
> > +	strlcpy(v4l2_sd_cfg->dev_name, led_cdev->dev->kobj.name,
> > +		sizeof(v4l2_sd_cfg->dev_name));
> > +
> > +	s = &v4l2_sd_cfg->intensity;
> > +	s->min = 0;
> > +	s->max = 1;
> > +	s->step = 1;
> > +	s->val = 1;
> > +}
> > +
> > +#else
> > +static void sgm3140_init_v4l2_flash_config(struct sgm3140 *priv,
> > +					   struct 
v4l2_flash_config *v4l2_sd_cfg)
> > +{
> > +}
> > +#endif
> > +
> > +static int sgm3140_probe(struct platform_device *pdev)
> > +{
> > +	struct sgm3140 *priv;
> > +	struct led_classdev *led_cdev;
> > +	struct led_classdev_flash *fled_cdev;
> > +	struct led_init_data init_data = {};
> > +	struct device_node *child_node;
> > +	struct v4l2_flash_config v4l2_sd_cfg;
> 
> s/v4l2_sd_cfg;/v4l2_sd_cfg = {};/
> 
> Otherwise it is possible that some controls would be initialized
> to random values.
> 

Ack

> > +	int ret;
> > +
> > +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	priv->flash_gpio = devm_gpiod_get(&pdev->dev, "flash", 
GPIOD_OUT_LOW);
> > +	ret = PTR_ERR_OR_ZERO(priv->flash_gpio);
> > +	if (ret) {
> > +		if (ret != -EPROBE_DEFER)
> > +			dev_err(&pdev->dev, "Failed to request flash 
gpio: %d\n",
> > +				ret);
> > +		return ret;
> > +	}
> > +
> > +	priv->enable_gpio = devm_gpiod_get(&pdev->dev, "enable", 
GPIOD_OUT_LOW);
> > +	ret = PTR_ERR_OR_ZERO(priv->enable_gpio);
> > +	if (ret) {
> > +		if (ret != -EPROBE_DEFER)
> > +			dev_err(&pdev->dev, "Failed to request 
enable gpio: %d\n",
> > +				ret);
> > +		return ret;
> > +	}
> > +
> > +	child_node = of_get_next_available_child(pdev->dev.of_node, NULL);
> > +	if (!child_node) {
> > +		dev_err(&pdev->dev, "No DT child node found for 
connected LED.\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	timer_setup(&priv->powerdown_timer, sgm3140_powerdown_timer, 0);
> > +
> > +	fled_cdev = &priv->fled_cdev;
> > +	led_cdev = &fled_cdev->led_cdev;
> > +
> > +	fled_cdev->ops = &sgm3140_flash_ops;
> > +
> > +	led_cdev->brightness_set_blocking = sgm3140_brightness_set;
> > +	led_cdev->max_brightness = LED_ON;
> > +	led_cdev->flags |= LED_DEV_CAP_FLASH;
> > +
> > +	init_data.fwnode = of_fwnode_handle(child_node);
> > +	init_data.devicename = SGM3140_NAME;
> 
> devicename should be skipped in new drivers.
> 

Ack

> > +
> > +	platform_set_drvdata(pdev, priv);
> > +
> > +	/* Register in the LED subsystem */
> > +	ret = led_classdev_flash_register_ext(&pdev->dev, fled_cdev,
> > &init_data);
> 
> We already have devm_* prefixed version thereof.
> 

Ack, switched to the devm_ variant

> > +	if (ret < 0) {
> > +		dev_err(&pdev->dev, "Failed to register flash device: 
%d\n",
> > +			ret);
> > +		goto err_flash_register;
> > +	}
> > +
> > +	sgm3140_init_v4l2_flash_config(priv, &v4l2_sd_cfg);
> > +
> > +	/* Create V4L2 Flash subdev */
> > +	priv->v4l2_flash = v4l2_flash_init(&pdev->dev,
> > of_fwnode_handle(child_node), +					
   fled_cdev, NULL,
> > +					   &v4l2_sd_cfg);
> > +	if (IS_ERR(priv->v4l2_flash)) {
> > +		ret = PTR_ERR(priv->v4l2_flash);
> > +		goto err_v4l2_flash_init;
> > +	}
> > +
> > +	return 0;
> > +
> > +err_v4l2_flash_init:
> > +	led_classdev_flash_unregister(fled_cdev);
> > +err_flash_register:
> > +	of_node_put(child_node);
> 
> You need to relase of_node also in case of success.
> 

Done.

> > +	return ret;
> > +}
> > +
> > +static int sgm3140_remove(struct platform_device *pdev)
> > +{
> > +	struct sgm3140 *priv = platform_get_drvdata(pdev);
> > +
> > +	del_timer_sync(&priv->powerdown_timer);
> > +
> > +	v4l2_flash_release(priv->v4l2_flash);
> > +	led_classdev_flash_unregister(&priv->fled_cdev);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id sgm3140_dt_match[] = {
> > +	{ .compatible = "sgmicro,sgm3140" },
> > +	{ /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, sgm3140_dt_match);
> > +
> > +static struct platform_driver sgm3140_driver = {
> > +	.probe	= sgm3140_probe,
> > +	.remove	= sgm3140_remove,
> > +	.driver	= {
> > +		.name	= "sgm3140",
> > +		.of_match_table = sgm3140_dt_match,
> > +	},
> > +};
> > +
> > +module_platform_driver(sgm3140_driver);
> > +
> > +MODULE_AUTHOR("Luca Weiss <luca@...tu.xyz>");
> > +MODULE_DESCRIPTION("SG Micro SGM3140 charge pump led driver");
> > +MODULE_LICENSE("GPL v2");

Powered by blists - more mailing lists