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

[Joe Perches <joe@perches.com>]

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;
	}