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.
