linux-kernel - Re: [PATCH v3 1/2] backlight arcxcnn add support for ArcticSand devices

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] [day] [month] [year] [list]

Date:   Fri, 06 Jan 2017 17:31:17 -0800
From:   Joe Perches <joe@...ches.com>
To:     Olimpiu Dejeu <olimpiu@...ticsand.com>, robh@...nel.org
Cc:     lee.jones@...aro.org, linux-kernel@...r.kernel.org,
        linux-fbdev@...r.kernel.org, devicetree@...r.kernel.org,
        jingoohan1@...il.com, bdodge@...ticsand.com
Subject: Re: [PATCH v3 1/2] backlight arcxcnn add support for ArcticSand
 devices

On Fri, 2017-01-06 at 15:48 -0500, Olimpiu Dejeu wrote:
> backlight: Add support for Arctic Sand LED backlight driver chips
> This driver provides support for the Arctic Sand arc2c0608 chip, 
>     and provides a framework to support future devices.

style trivia:

> diff --git a/drivers/video/backlight/arcxcnn_bl.c b/drivers/video/backlight/arcxcnn_bl.c
[]
> +static int arcxcnn_bl_update_status(struct backlight_device *bl)
> +{
> +	struct arcxcnn *lp = bl_get_data(bl);
> +	u32 brightness = bl->props.brightness;
> +
> +	if (bl->props.state & (BL_CORE_SUSPENDED | BL_CORE_FBBLANK))
> +		brightness = 0;
> +
> +	arcxcnn_set_brightness(lp, brightness);
> +
> +	/* set power-on/off/save modes */
> +	if (bl->props.power == 0)
> +		/* take out of standby */
> +		arcxcnn_update_bit(lp, ARCXCNN_CMD, ARCXCNN_CMD_STDBY, 0);
> +	else
> +		 /* place in low-power standby mode */
> +		arcxcnn_update_bit(lp, ARCXCNN_CMD,
> +				ARCXCNN_CMD_STDBY, ARCXCNN_CMD_STDBY);

This is generally smaller code using a temporary and
a single call instead of two calls like:

	int cmd;

	...

	if (bl->props.power == 0)
		cmd = 0;		/* take out of standby */
	else
		cmd = ARCXCNN_CMD_STDBY;
	arcxcnn_update_bit(lp, ARCXCNN_CMD, ARCXCNN_CMD_STDBY, cmd);

or maybe

	arcxcnn_update_bit(lp, ARCXCNN_CMD, ARCXCNN_CMD_STDBY,
			   bl->props.power == 0 ? 0 : ARCXCNN_CMD_STDBY);

> +static int arcxcnn_backlight_register(struct arcxcnn *lp)
> +{
> +	struct backlight_properties *props;
> +	const char *name = lp->pdata->name ? : "arctic_bl";
> +
> +	props = devm_kzalloc(lp->dev, sizeof(*props), GFP_KERNEL);
> +	if (!props)
> +		return -ENOMEM;
> +
> +	memset(props, 0, sizeof(props));

props has already been zeroed by devm_kzalloc and doesn't
need to be memset to 0 again.

> +	props->type = BACKLIGHT_PLATFORM;
> +	props->max_brightness = MAX_BRIGHTNESS;
> +
> +	if (lp->pdata->initial_brightness > props->max_brightness)
> +		lp->pdata->initial_brightness = props->max_brightness;
> +
> +	props->brightness = lp->pdata->initial_brightness;
> +
> +	lp->bl = devm_backlight_device_register(lp->dev, name, lp->dev, lp,
> +				       &arcxcnn_bl_ops, props);
> +

This blank line above is generally not needed.

> +	if (IS_ERR(lp->bl))
> +		return PTR_ERR(lp->bl);

the typical style is:

	rtn = foo(...)
	if (rtn < 0)
		error_handler...

> +static void arcxcnn_parse_dt(struct arcxcnn *lp)
> +{
> +	struct device *dev = lp->dev;
> +	struct device_node *node = dev->of_node;
> +	u32 prog_val, num_entry, entry, sources[ARCXCNN_LEDEN_BITS];
> +	int ret;
[]
> +	ret = of_property_count_u32_elems(node, "led-sources");
> +	if (ret < 0) {
> +		lp->pdata->leden = ARCXCNN_LEDEN_MASK; /* all on is default */
> +	} else {
> +		num_entry = ret;
> +		if (num_entry > ARCXCNN_LEDEN_BITS)
> +			num_entry = ARCXCNN_LEDEN_BITS;
> +
> +		ret = of_property_read_u32_array(node, "led-sources", sources,
> +					num_entry);
> +		if (ret < 0) {
> +			dev_err(dev, "led-sources node is invalid.\n");
> +		} else {
> +			u8 onbit;
> +
> +			lp->pdata->leden = 0;
> +
> +			/* for each enable in source, set bit in led enable */
> +			for (entry = 0; entry < num_entry; entry++) {
> +				onbit = 1 << sources[entry];
> +				lp->pdata->leden |= onbit;
> +			}
> +		}
> +	}
> +}

The cascading indentation can be avoided by using return;
as necessary

	ret = of_property_count_u32_elems(node, "led-sources");
	if (ret < 0) {
		lp->pdata->leden = ARCXCNN_LEDEN_MASK; /* all on is default */
		return;
	}

	num_entry = min(ret, ARCXCNN_LEDEN_BITS);
	ret = of_property_read_u32_array(node, "led-sources", sources, num_entry);
	if (ret < 0) {
		dev_err(dev, "led-sources node is invalid\n");
		return;
	}

	lp->pdata->leden = 0;

	/* for each enable in source, set bit in led enable */
	for (entry = 0; entry < num_entry; entry++) {
		u8 onbit = 1 << sources[entry];

		lp->pdata->leden |= onbit;
	}