On Thu, Aug 21, 2014 at 12:37:15PM -0500, Nishanth Menon wrote:

On 12:32-20140821, Murphy, Dan wrote:

quoted

On 08/21/2014 12:19 PM, Menon, Nishanth wrote:

quoted

On 08/21/2014 11:59 AM, Murphy, Dan wrote:
[...]
Ooops.. missed answering one addition statement:

quoted

quoted

+	of_property_read_u32(np, "ti,palmas-long-press-seconds", &val);

Probably should check the return to make sure the value exists and that is is
within an expected range.

It is an optional parameter and may not exist in dt. when it does
exist, the logic tries to do a best match (this is the for loop in the
logic just below).

The issue is val might be returned as a negative which will then proceed to
set the config->long_press_time_val to the lowest time value which then overrides
your initial setting of config->long_press_time_val = ARRAY_SIZE(lpk_times) - 1;

Does the following as a replacement look OK? if yes, I can incorporate
as part of v3 of this series.
/**
 * palmas_pwron_params_ofinit() - device tree parameter parser
 * @dev:	palmas button device
 * @config:	configuration params that this fills up
 */
static void palmas_pwron_params_ofinit(struct device *dev,
				       struct palmas_pwron_config *config)
{
	struct device_node *np;
	u32 val;
	int i, ret;
	u8 lpk_times[] = { 6, 8, 10, 12 };
	int pwr_on_deb_ms[] = { 15, 100, 500, 1000 };

	/* Handle cases where device node based configuration is not present */
	if (!of_have_populated_dt())
		return;
	np = of_node_get(dev->of_node);
	if (!np)
		return;

	/* Default config parameters - least debounce, max long key press */
	config->pwron_debounce_val = 0;
	config->long_press_time_val = ARRAY_SIZE(lpk_times) - 1;

	ret = of_property_read_u32(np, "ti,palmas-long-press-seconds", &val);
	if (ret)
		goto skip_lpk;

Just without gotos please. They are nice in error handling paths and in some
other rare circumstances, but not here.

	for (i = 0; i < ARRAY_SIZE(lpk_times); i++) {
		if (val <= lpk_times[i]) {
			config->long_press_time_val = i;
			break;
		}
	}

skip_lpk:
	ret = of_property_read_u32(np, "ti,palmas-pwron-debounce-milli-seconds",
				   &val);
	if (ret)
		goto skip_debounce;
	for (i = 0; i < ARRAY_SIZE(pwr_on_deb_ms); i++) {
		if (val <= pwr_on_deb_ms[i]) {
			config->pwron_debounce_val = i;
			break;
		}
	}

skip_debounce:
	dev_info(dev, "h/w controlled shutdown duration=%d seconds\n",
		 lpk_times[config->long_press_time_val]);

	of_node_put(np);

BTW, I do not think you need to use of_node_get/put here, it's not going anywhere.

Thanks.

-- 
Dmitry