From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1755787AbcI2TcQ (ORCPT <rfc822;w@1wt.eu>);
        Thu, 29 Sep 2016 15:32:16 -0400
Received: from foss.arm.com ([217.140.101.70]:59376 "EHLO foss.arm.com"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S1755308AbcI2TcH (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
        Thu, 29 Sep 2016 15:32:07 -0400
Date: Thu, 29 Sep 2016 20:32:05 +0100
From: Mark Rutland <mark.rutland@arm.com>
To: Olimpiu Dejeu <olimpiu@arcticsand.com>
Cc: robh@kernel.org, lee.jones@linaro.org, linux-kernel@vger.kernel.org,
        linux-fbdev@vger.kernel.org, devicetree@vger.kernel.org,
        jg1.han@samsung.com
Subject: Re: [PATCH 1/2] backlight: arcxcnn: add support for ArticSand devices
Message-ID: <20160929193204.GB3405@remoulade>
References: <1475166879-6255-1-git-send-email-olimpiu@arcticsand.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1475166879-6255-1-git-send-email-olimpiu@arcticsand.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Thu, Sep 29, 2016 at 12:34:39PM -0400, Olimpiu Dejeu wrote:
> Resubmition of arcxcnn backliught driver adding definitions of the
>  internal registers of the chip. 

s/backliught/backlight/

[...]

> +#ifdef CONFIG_OF
> +static int arcxcnn_parse_dt(struct arcxcnn *lp)
> +{
> +	struct device *dev = lp->dev;
> +	struct device_node *node = dev->of_node;
> +	u32 prog_length, prog_val, num_entry, sources[6];
> +	int ret;
> +
> +	if (!node) {
> +		dev_err(dev, "no platform data.\n");
> +		return -EINVAL;
> +	}
> +	of_property_read_string(node, "label", &lp->pdata->name);
> +	of_property_read_u32(node, "default-brightness", &prog_val);
> +	lp->pdata->initial_brightness = prog_val;

You must check the return value of the of_ accessors. If there's no
default-brightness property in the DT, prog_val will be uninitialised here.

> +	of_property_read_u32(node, "pwm-period", &lp->pdata->period_ns);

This was not in the binding document.

> +	/* Fill program from platform data if defined
> +	*/
> +	lp->pdata->prog_entries = 0;
> +	prog_length = of_get_child_count(node);
> +	if (prog_length > 0) {
> +		struct device_node *child;
> +		u32 i = 0;
> +
> +		for_each_child_of_node(node, child) {
> +			if (i >= ARCXCNN_MAX_PROGENTRIES)
> +				break;
> +			of_property_read_u32(child, "reg", &prog_val);
> +			lp->pdata->prog_addr[i] = (u8)prog_val;
> +			of_property_read_u32(child, "val", &prog_val);
> +			lp->pdata->prog_data[i] = (u8)prog_val;
> +			i++;
> +		}
> +		lp->pdata->prog_entries = i;
> +	}

This was also not documented.

What is this? What is it used for? Why is it in the DT?

> +	/* playback program now that its all installed */
> +	for (i = 0; i < lp->pdata->prog_entries; i++) {
> +		arcxcnn_write_byte(lp, lp->pdata->prog_addr[i],
> +				lp->pdata->prog_data[i]);
> +		/*
> +		printk(KERN_ALERT "%02X, %02X\n",
> +			lp->pdata->prog_addr[i], lp->pdata->prog_data[i]);
> +		*/
> +	}

Why is it necessary to have an arbitrary register programming sequence? What
exactly varies between parts?

I don't think this is the right way to solve your problem.

Thanks,
Mark.