Re: [PATCH 2/2 v7] leds: rt8515: Add Richtek RT8515 LED driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi!

> We do not have a proper datasheet for the RT8515 but
> it turns out that RT9387A has a public datasheet and
> is essentially the same chip. We designed the driver
> in accordance with this datasheet. The day someone
> needs to drive a RT9387A this driver can probably
> easily be augmented to handle that chip too.

Please move this to the comment in the sources... perhaps with url for
the documenation.

> +/* This is setting the torch light level */
> +static int rt8515_led_brightness_set(struct led_classdev *led,
> +				     enum led_brightness brightness)
> +{
> +	struct led_classdev_flash *fled = lcdev_to_flcdev(led);
> +	struct rt8515 *rt = to_rt8515(fled);
> +
> +	mutex_lock(&rt->lock);
> +
> +	if (brightness == LED_OFF) {
> +		/* Off */
> +		gpiod_set_value(rt->enable_flash, 0);
> +		gpiod_set_value(rt->enable_torch, 0);
> +	} else if (brightness < RT8515_TORCH_MAX) {
> +		/* Step it up to movie mode brightness using the flash pin */
> +		rt8515_gpio_brightness_commit(rt->enable_torch, brightness);
> +	} else {
> +		/* Max torch brightness requested */
> +		gpiod_set_value(rt->enable_torch, 1);
> +	}
> +
> +	mutex_unlock(&rt->lock);

Do you need to somehow reset the LED to lowest brightness before
rt8515_gpio_brightness_commit()?

> +	ret1 = fwnode_property_read_u32(rt->dev->fwnode, resistance, &res);
> +	ret2 = fwnode_property_read_u32(led, max_ua_prop, &ua);
> +
> +	/* No info in DT, OK go with hardware maxima */
> +	if (ret1 && ret2) {
> +		max_ma = RT8515_MAX_IOUT_MA;
> +		max_intensity = hw_max;
> +		goto out_assign_max;
> +	}
> +
> +	if (ret1 || ret2) {
> +		dev_err(rt->dev,
> +			"either %s or %s missing from DT, using HW max\n",
> +			resistance, max_ua_prop);
> +		max_ma = RT8515_MAX_IOUT_MA;
> +		max_intensity = hw_max;
> +		goto out_assign_max;
> +	}

I'd go with some minimum values if we don't have complete information
from devicetree.

> +	/* Create a V4L2 Flash device if V4L2 flash is enabled */
> +	rt->v4l2_flash = v4l2_flash_init(dev, child, fled, NULL, &v4l2_sd_cfg);
> +	if (IS_ERR(rt->v4l2_flash)) {
> +		ret = PTR_ERR(rt->v4l2_flash);
> +		dev_err(dev, "failed to register V4L2 flash device (%d)\n",
> +			ret);
> +		/*
> +		 * Continue without the V4L2 flash
> +		 * (we still have the classdev)
> +		 */
> +	}
> +
> +	return 0;
> +}
> +
> +static int rt8515_remove(struct platform_device *pdev)
> +{
> +	struct rt8515 *rt = platform_get_drvdata(pdev);
> +
> +	v4l2_flash_release(rt->v4l2_flash);

Is it cool to call v4l2_flash_release() with error pointer?

> +MODULE_LICENSE("GPL v2");

v2+, iirc?

Driver looks good, thanks!
								Pavel

-- 
http://www.livejournal.com/~pavelmachek

Attachment: signature.asc
Description: PGP signature


[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux