public inbox for kernel-janitors@vger.kernel.org
 help / color / mirror / Atom feed
* [leds:for-next 18/18] drivers/leds/leds-lm3530.c:432 lm3530_probe() info: why not propagate 'err' fr
@ 2012-09-12  4:20 Fengguang Wu
  2012-09-12  4:32 ` [leds:for-next 18/18] drivers/leds/leds-lm3530.c:432 lm3530_probe() info: why not propagate 'err Fengguang Wu
                   ` (7 more replies)
  0 siblings, 8 replies; 9+ messages in thread
From: Fengguang Wu @ 2012-09-12  4:20 UTC (permalink / raw)
  To: kernel-janitors

[-- Attachment #1: Type: text/plain, Size: 1542 bytes --]

Hi Sachin,

FYI, there are new smatch warnings show up in

tree:   git://git.kernel.org/pub/scm/linux/kernel/git/cooloney/linux-leds.git for-next
head:   2829515a5fb5ceccb4021d819c3d7d0ecaab86eb
commit: 2829515a5fb5ceccb4021d819c3d7d0ecaab86eb [18/18] leds-lm3530: Use devm_regulator_get function

All smatch warnings:

  drivers/leds/leds-lm3530.c:361 lm3530_mode_set() info: why not propagate 'mode' from lm3530_get_mode_from_str() instead of -22?
+ drivers/leds/leds-lm3530.c:432 lm3530_probe() info: why not propagate 'err' from lm3530_init_registers() instead of -19?
+ drivers/leds/leds-lm3530.c:438 lm3530_probe() info: why not propagate 'err' from led_classdev_register() instead of -19?

vim +432 drivers/leds/leds-lm3530.c
   422			err = PTR_ERR(drvdata->regulator);
   423			drvdata->regulator = NULL;
   424			return err;
   425		}
   426	
   427		if (drvdata->pdata->brt_val) {
   428			err = lm3530_init_registers(drvdata);
   429			if (err < 0) {
   430				dev_err(&client->dev,
   431					"Register Init failed: %d\n", err);
 > 432				return -ENODEV;
   433			}
   434		}
   435		err = led_classdev_register(&client->dev, &drvdata->led_dev);
   436		if (err < 0) {
   437			dev_err(&client->dev, "Register led class failed: %d\n", err);
   438			return -ENODEV;
   439		}
   440	
   441		err = device_create_file(drvdata->led_dev.dev, &dev_attr_mode);
   442		if (err < 0) {

---
0-DAY kernel build testing backend         Open Source Technology Centre
Fengguang Wu <wfg@linux.intel.com>                     Intel Corporation

[-- Attachment #2: leds-lm3530.c --]
[-- Type: text/x-csrc, Size: 12952 bytes --]

/*
 * Copyright (C) 2011 ST-Ericsson SA.
 * Copyright (C) 2009 Motorola, Inc.
 *
 * License Terms: GNU General Public License v2
 *
 * Simple driver for National Semiconductor LM3530 Backlight driver chip
 *
 * Author: Shreshtha Kumar SAHU <shreshthakumar.sahu@stericsson.com>
 * based on leds-lm3530.c by Dan Murphy <D.Murphy@motorola.com>
 */

#include <linux/i2c.h>
#include <linux/leds.h>
#include <linux/slab.h>
#include <linux/platform_device.h>
#include <linux/input.h>
#include <linux/led-lm3530.h>
#include <linux/types.h>
#include <linux/regulator/consumer.h>
#include <linux/module.h>

#define LM3530_LED_DEV "lcd-backlight"
#define LM3530_NAME "lm3530-led"

#define LM3530_GEN_CONFIG		0x10
#define LM3530_ALS_CONFIG		0x20
#define LM3530_BRT_RAMP_RATE		0x30
#define LM3530_ALS_IMP_SELECT		0x41
#define LM3530_BRT_CTRL_REG		0xA0
#define LM3530_ALS_ZB0_REG		0x60
#define LM3530_ALS_ZB1_REG		0x61
#define LM3530_ALS_ZB2_REG		0x62
#define LM3530_ALS_ZB3_REG		0x63
#define LM3530_ALS_Z0T_REG		0x70
#define LM3530_ALS_Z1T_REG		0x71
#define LM3530_ALS_Z2T_REG		0x72
#define LM3530_ALS_Z3T_REG		0x73
#define LM3530_ALS_Z4T_REG		0x74
#define LM3530_REG_MAX			14

/* General Control Register */
#define LM3530_EN_I2C_SHIFT		(0)
#define LM3530_RAMP_LAW_SHIFT		(1)
#define LM3530_MAX_CURR_SHIFT		(2)
#define LM3530_EN_PWM_SHIFT		(5)
#define LM3530_PWM_POL_SHIFT		(6)
#define LM3530_EN_PWM_SIMPLE_SHIFT	(7)

#define LM3530_ENABLE_I2C		(1 << LM3530_EN_I2C_SHIFT)
#define LM3530_ENABLE_PWM		(1 << LM3530_EN_PWM_SHIFT)
#define LM3530_POL_LOW			(1 << LM3530_PWM_POL_SHIFT)
#define LM3530_ENABLE_PWM_SIMPLE	(1 << LM3530_EN_PWM_SIMPLE_SHIFT)

/* ALS Config Register Options */
#define LM3530_ALS_AVG_TIME_SHIFT	(0)
#define LM3530_EN_ALS_SHIFT		(3)
#define LM3530_ALS_SEL_SHIFT		(5)

#define LM3530_ENABLE_ALS		(3 << LM3530_EN_ALS_SHIFT)

/* Brightness Ramp Rate Register */
#define LM3530_BRT_RAMP_FALL_SHIFT	(0)
#define LM3530_BRT_RAMP_RISE_SHIFT	(3)

/* ALS Resistor Select */
#define LM3530_ALS1_IMP_SHIFT		(0)
#define LM3530_ALS2_IMP_SHIFT		(4)

/* Zone Boundary Register defaults */
#define LM3530_ALS_ZB_MAX		(4)
#define LM3530_ALS_WINDOW_mV		(1000)
#define LM3530_ALS_OFFSET_mV		(4)

/* Zone Target Register defaults */
#define LM3530_DEF_ZT_0			(0x7F)
#define LM3530_DEF_ZT_1			(0x66)
#define LM3530_DEF_ZT_2			(0x4C)
#define LM3530_DEF_ZT_3			(0x33)
#define LM3530_DEF_ZT_4			(0x19)

/* 7 bits are used for the brightness : LM3530_BRT_CTRL_REG */
#define MAX_BRIGHTNESS			(127)

struct lm3530_mode_map {
	const char *mode;
	enum lm3530_mode mode_val;
};

static struct lm3530_mode_map mode_map[] = {
	{ "man", LM3530_BL_MODE_MANUAL },
	{ "als", LM3530_BL_MODE_ALS },
	{ "pwm", LM3530_BL_MODE_PWM },
};

/**
 * struct lm3530_data
 * @led_dev: led class device
 * @client: i2c client
 * @pdata: LM3530 platform data
 * @mode: mode of operation - manual, ALS, PWM
 * @regulator: regulator
 * @brighness: previous brightness value
 * @enable: regulator is enabled
 */
struct lm3530_data {
	struct led_classdev led_dev;
	struct i2c_client *client;
	struct lm3530_platform_data *pdata;
	enum lm3530_mode mode;
	struct regulator *regulator;
	enum led_brightness brightness;
	bool enable;
};

/*
 * struct lm3530_als_data
 * @config  : value of ALS configuration register
 * @imp_sel : value of ALS resistor select register
 * @zone    : values of ALS ZB(Zone Boundary) registers
 */
struct lm3530_als_data {
	u8 config;
	u8 imp_sel;
	u8 zones[LM3530_ALS_ZB_MAX];
};

static const u8 lm3530_reg[LM3530_REG_MAX] = {
	LM3530_GEN_CONFIG,
	LM3530_ALS_CONFIG,
	LM3530_BRT_RAMP_RATE,
	LM3530_ALS_IMP_SELECT,
	LM3530_BRT_CTRL_REG,
	LM3530_ALS_ZB0_REG,
	LM3530_ALS_ZB1_REG,
	LM3530_ALS_ZB2_REG,
	LM3530_ALS_ZB3_REG,
	LM3530_ALS_Z0T_REG,
	LM3530_ALS_Z1T_REG,
	LM3530_ALS_Z2T_REG,
	LM3530_ALS_Z3T_REG,
	LM3530_ALS_Z4T_REG,
};

static int lm3530_get_mode_from_str(const char *str)
{
	int i;

	for (i = 0; i < ARRAY_SIZE(mode_map); i++)
		if (sysfs_streq(str, mode_map[i].mode))
			return mode_map[i].mode_val;

	return -1;
}

static void lm3530_als_configure(struct lm3530_platform_data *pdata,
				struct lm3530_als_data *als)
{
	int i;
	u32 als_vmin, als_vmax, als_vstep;

	if (pdata->als_vmax == 0) {
		pdata->als_vmin = 0;
		pdata->als_vmax = LM3530_ALS_WINDOW_mV;
	}

	als_vmin = pdata->als_vmin;
	als_vmax = pdata->als_vmax;

	if ((als_vmax - als_vmin) > LM3530_ALS_WINDOW_mV)
		pdata->als_vmax = als_vmax = als_vmin + LM3530_ALS_WINDOW_mV;

	/* n zone boundary makes n+1 zones */
	als_vstep = (als_vmax - als_vmin) / (LM3530_ALS_ZB_MAX + 1);

	for (i = 0; i < LM3530_ALS_ZB_MAX; i++)
		als->zones[i] = (((als_vmin + LM3530_ALS_OFFSET_mV) +
			als_vstep + (i * als_vstep)) * LED_FULL) / 1000;

	als->config =
		(pdata->als_avrg_time << LM3530_ALS_AVG_TIME_SHIFT) |
		(LM3530_ENABLE_ALS) |
		(pdata->als_input_mode << LM3530_ALS_SEL_SHIFT);

	als->imp_sel =
		(pdata->als1_resistor_sel << LM3530_ALS1_IMP_SHIFT) |
		(pdata->als2_resistor_sel << LM3530_ALS2_IMP_SHIFT);
}

static int lm3530_init_registers(struct lm3530_data *drvdata)
{
	int ret = 0;
	int i;
	u8 gen_config;
	u8 brt_ramp;
	u8 brightness;
	u8 reg_val[LM3530_REG_MAX];
	struct lm3530_platform_data *pdata = drvdata->pdata;
	struct i2c_client *client = drvdata->client;
	struct lm3530_pwm_data *pwm = &pdata->pwm_data;
	struct lm3530_als_data als;

	memset(&als, 0, sizeof(struct lm3530_als_data));

	gen_config = (pdata->brt_ramp_law << LM3530_RAMP_LAW_SHIFT) |
			((pdata->max_current & 7) << LM3530_MAX_CURR_SHIFT);

	switch (drvdata->mode) {
	case LM3530_BL_MODE_MANUAL:
		gen_config |= LM3530_ENABLE_I2C;
		break;
	case LM3530_BL_MODE_ALS:
		gen_config |= LM3530_ENABLE_I2C;
		lm3530_als_configure(pdata, &als);
		break;
	case LM3530_BL_MODE_PWM:
		gen_config |= LM3530_ENABLE_PWM | LM3530_ENABLE_PWM_SIMPLE |
			      (pdata->pwm_pol_hi << LM3530_PWM_POL_SHIFT);
		break;
	}

	brt_ramp = (pdata->brt_ramp_fall << LM3530_BRT_RAMP_FALL_SHIFT) |
			(pdata->brt_ramp_rise << LM3530_BRT_RAMP_RISE_SHIFT);

	if (drvdata->brightness)
		brightness = drvdata->brightness;
	else
		brightness = drvdata->brightness = pdata->brt_val;

	if (brightness > drvdata->led_dev.max_brightness)
		brightness = drvdata->led_dev.max_brightness;

	reg_val[0] = gen_config;	/* LM3530_GEN_CONFIG */
	reg_val[1] = als.config;	/* LM3530_ALS_CONFIG */
	reg_val[2] = brt_ramp;		/* LM3530_BRT_RAMP_RATE */
	reg_val[3] = als.imp_sel;	/* LM3530_ALS_IMP_SELECT */
	reg_val[4] = brightness;	/* LM3530_BRT_CTRL_REG */
	reg_val[5] = als.zones[0];	/* LM3530_ALS_ZB0_REG */
	reg_val[6] = als.zones[1];	/* LM3530_ALS_ZB1_REG */
	reg_val[7] = als.zones[2];	/* LM3530_ALS_ZB2_REG */
	reg_val[8] = als.zones[3];	/* LM3530_ALS_ZB3_REG */
	reg_val[9] = LM3530_DEF_ZT_0;	/* LM3530_ALS_Z0T_REG */
	reg_val[10] = LM3530_DEF_ZT_1;	/* LM3530_ALS_Z1T_REG */
	reg_val[11] = LM3530_DEF_ZT_2;	/* LM3530_ALS_Z2T_REG */
	reg_val[12] = LM3530_DEF_ZT_3;	/* LM3530_ALS_Z3T_REG */
	reg_val[13] = LM3530_DEF_ZT_4;	/* LM3530_ALS_Z4T_REG */

	if (!drvdata->enable) {
		ret = regulator_enable(drvdata->regulator);
		if (ret) {
			dev_err(&drvdata->client->dev,
					"Enable regulator failed\n");
			return ret;
		}
		drvdata->enable = true;
	}

	for (i = 0; i < LM3530_REG_MAX; i++) {
		/* do not update brightness register when pwm mode */
		if (lm3530_reg[i] == LM3530_BRT_CTRL_REG &&
		    drvdata->mode == LM3530_BL_MODE_PWM) {
			if (pwm->pwm_set_intensity)
				pwm->pwm_set_intensity(reg_val[i],
					drvdata->led_dev.max_brightness);
			continue;
		}

		ret = i2c_smbus_write_byte_data(client,
				lm3530_reg[i], reg_val[i]);
		if (ret)
			break;
	}

	return ret;
}

static void lm3530_brightness_set(struct led_classdev *led_cdev,
				     enum led_brightness brt_val)
{
	int err;
	struct lm3530_data *drvdata =
	    container_of(led_cdev, struct lm3530_data, led_dev);
	struct lm3530_platform_data *pdata = drvdata->pdata;
	struct lm3530_pwm_data *pwm = &pdata->pwm_data;
	u8 max_brightness = led_cdev->max_brightness;

	switch (drvdata->mode) {
	case LM3530_BL_MODE_MANUAL:

		if (!drvdata->enable) {
			err = lm3530_init_registers(drvdata);
			if (err) {
				dev_err(&drvdata->client->dev,
					"Register Init failed: %d\n", err);
				break;
			}
		}

		/* set the brightness in brightness control register*/
		err = i2c_smbus_write_byte_data(drvdata->client,
				LM3530_BRT_CTRL_REG, brt_val);
		if (err)
			dev_err(&drvdata->client->dev,
				"Unable to set brightness: %d\n", err);
		else
			drvdata->brightness = brt_val;

		if (brt_val == 0) {
			err = regulator_disable(drvdata->regulator);
			if (err)
				dev_err(&drvdata->client->dev,
					"Disable regulator failed\n");
			drvdata->enable = false;
		}
		break;
	case LM3530_BL_MODE_ALS:
		break;
	case LM3530_BL_MODE_PWM:
		if (pwm->pwm_set_intensity)
			pwm->pwm_set_intensity(brt_val, max_brightness);
		break;
	default:
		break;
	}
}

static ssize_t lm3530_mode_get(struct device *dev,
		struct device_attribute *attr, char *buf)
{
	struct led_classdev *led_cdev = dev_get_drvdata(dev);
	struct lm3530_data *drvdata;
	int i, len = 0;

	drvdata = container_of(led_cdev, struct lm3530_data, led_dev);
	for (i = 0; i < ARRAY_SIZE(mode_map); i++)
		if (drvdata->mode == mode_map[i].mode_val)
			len += sprintf(buf + len, "[%s] ", mode_map[i].mode);
		else
			len += sprintf(buf + len, "%s ", mode_map[i].mode);

	len += sprintf(buf + len, "\n");

	return len;
}

static ssize_t lm3530_mode_set(struct device *dev, struct device_attribute
				   *attr, const char *buf, size_t size)
{
	struct led_classdev *led_cdev = dev_get_drvdata(dev);
	struct lm3530_data *drvdata;
	struct lm3530_pwm_data *pwm;
	u8 max_brightness;
	int mode, err;

	drvdata = container_of(led_cdev, struct lm3530_data, led_dev);
	pwm = &drvdata->pdata->pwm_data;
	max_brightness = led_cdev->max_brightness;
	mode = lm3530_get_mode_from_str(buf);
	if (mode < 0) {
		dev_err(dev, "Invalid mode\n");
		return -EINVAL;
	}

	drvdata->mode = mode;

	/* set pwm to low if unnecessary */
	if (mode != LM3530_BL_MODE_PWM && pwm->pwm_set_intensity)
		pwm->pwm_set_intensity(0, max_brightness);

	err = lm3530_init_registers(drvdata);
	if (err) {
		dev_err(dev, "Setting %s Mode failed :%d\n", buf, err);
		return err;
	}

	return sizeof(drvdata->mode);
}
static DEVICE_ATTR(mode, 0644, lm3530_mode_get, lm3530_mode_set);

static int __devinit lm3530_probe(struct i2c_client *client,
			   const struct i2c_device_id *id)
{
	struct lm3530_platform_data *pdata = client->dev.platform_data;
	struct lm3530_data *drvdata;
	int err = 0;

	if (pdata == NULL) {
		dev_err(&client->dev, "platform data required\n");
		return -ENODEV;
	}

	/* BL mode */
	if (pdata->mode > LM3530_BL_MODE_PWM) {
		dev_err(&client->dev, "Illegal Mode request\n");
		return -EINVAL;
	}

	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
		dev_err(&client->dev, "I2C_FUNC_I2C not supported\n");
		return -EIO;
	}

	drvdata = devm_kzalloc(&client->dev, sizeof(struct lm3530_data),
				GFP_KERNEL);
	if (drvdata == NULL)
		return -ENOMEM;

	drvdata->mode = pdata->mode;
	drvdata->client = client;
	drvdata->pdata = pdata;
	drvdata->brightness = LED_OFF;
	drvdata->enable = false;
	drvdata->led_dev.name = LM3530_LED_DEV;
	drvdata->led_dev.brightness_set = lm3530_brightness_set;
	drvdata->led_dev.max_brightness = MAX_BRIGHTNESS;

	i2c_set_clientdata(client, drvdata);

	drvdata->regulator = devm_regulator_get(&client->dev, "vin");
	if (IS_ERR(drvdata->regulator)) {
		dev_err(&client->dev, "regulator get failed\n");
		err = PTR_ERR(drvdata->regulator);
		drvdata->regulator = NULL;
		return err;
	}

	if (drvdata->pdata->brt_val) {
		err = lm3530_init_registers(drvdata);
		if (err < 0) {
			dev_err(&client->dev,
				"Register Init failed: %d\n", err);
			return -ENODEV;
		}
	}
	err = led_classdev_register(&client->dev, &drvdata->led_dev);
	if (err < 0) {
		dev_err(&client->dev, "Register led class failed: %d\n", err);
		return -ENODEV;
	}

	err = device_create_file(drvdata->led_dev.dev, &dev_attr_mode);
	if (err < 0) {
		dev_err(&client->dev, "File device creation failed: %d\n", err);
		err = -ENODEV;
		goto err_create_file;
	}

	return 0;

err_create_file:
	led_classdev_unregister(&drvdata->led_dev);
	return err;
}

static int __devexit lm3530_remove(struct i2c_client *client)
{
	struct lm3530_data *drvdata = i2c_get_clientdata(client);

	device_remove_file(drvdata->led_dev.dev, &dev_attr_mode);

	if (drvdata->enable)
		regulator_disable(drvdata->regulator);
	led_classdev_unregister(&drvdata->led_dev);
	return 0;
}

static const struct i2c_device_id lm3530_id[] = {
	{LM3530_NAME, 0},
	{}
};
MODULE_DEVICE_TABLE(i2c, lm3530_id);

static struct i2c_driver lm3530_i2c_driver = {
	.probe = lm3530_probe,
	.remove = __devexit_p(lm3530_remove),
	.id_table = lm3530_id,
	.driver = {
		.name = LM3530_NAME,
		.owner = THIS_MODULE,
	},
};

module_i2c_driver(lm3530_i2c_driver);

MODULE_DESCRIPTION("Back Light driver for LM3530");
MODULE_LICENSE("GPL v2");
MODULE_AUTHOR("Shreshtha Kumar SAHU <shreshthakumar.sahu@stericsson.com>");

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [leds:for-next 18/18] drivers/leds/leds-lm3530.c:432 lm3530_probe() info: why not propagate 'err
  2012-09-12  4:20 [leds:for-next 18/18] drivers/leds/leds-lm3530.c:432 lm3530_probe() info: why not propagate 'err' fr Fengguang Wu
@ 2012-09-12  4:32 ` Fengguang Wu
  2012-09-12  4:41 ` Sachin Kamat
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Fengguang Wu @ 2012-09-12  4:32 UTC (permalink / raw)
  To: kernel-janitors

Hi Sachin,

On Wed, Sep 12, 2012 at 09:59:38AM +0530, Sachin Kamat wrote:
> Hi Fengguang,
> 
> Thanks for bringing this to my notice. I compile tested this patch
> using the latest (11 Sep 2012) linux-next tree.
> Unfortunately I cannot not understand the meaning of these smatch warnings.
> Could you please simplify them for me?

I think the warnings can be quieted by

-                         return -ENODEV;
+                         return err;

Thanks,
Fengguang

> On 12 September 2012 09:50, Fengguang Wu <fengguang.wu@intel.com> wrote:
> > Hi Sachin,
> >
> > FYI, there are new smatch warnings show up in
> >
> > tree:   git://git.kernel.org/pub/scm/linux/kernel/git/cooloney/linux-leds.git for-next
> > head:   2829515a5fb5ceccb4021d819c3d7d0ecaab86eb
> > commit: 2829515a5fb5ceccb4021d819c3d7d0ecaab86eb [18/18] leds-lm3530: Use devm_regulator_get function
> >
> > All smatch warnings:
> >
> >   drivers/leds/leds-lm3530.c:361 lm3530_mode_set() info: why not propagate 'mode' from lm3530_get_mode_from_str() instead of -22?
> > + drivers/leds/leds-lm3530.c:432 lm3530_probe() info: why not propagate 'err' from lm3530_init_registers() instead of -19?
> > + drivers/leds/leds-lm3530.c:438 lm3530_probe() info: why not propagate 'err' from led_classdev_register() instead of -19?
> >
> > vim +432 drivers/leds/leds-lm3530.c
> >    422                  err = PTR_ERR(drvdata->regulator);
> >    423                  drvdata->regulator = NULL;
> >    424                  return err;
> >    425          }
> >    426
> >    427          if (drvdata->pdata->brt_val) {
> >    428                  err = lm3530_init_registers(drvdata);
> >    429                  if (err < 0) {
> >    430                          dev_err(&client->dev,
> >    431                                  "Register Init failed: %d\n", err);
> >  > 432                          return -ENODEV;
> >    433                  }
> >    434          }
> >    435          err = led_classdev_register(&client->dev, &drvdata->led_dev);
> >    436          if (err < 0) {
> >    437                  dev_err(&client->dev, "Register led class failed: %d\n", err);
> >    438                  return -ENODEV;
> >    439          }
> >    440
> >    441          err = device_create_file(drvdata->led_dev.dev, &dev_attr_mode);
> >    442          if (err < 0) {
> >
> > ---
> > 0-DAY kernel build testing backend         Open Source Technology Centre
> > Fengguang Wu <wfg@linux.intel.com>                     Intel Corporation
> 
> 
> 
> -- 
> With warm regards,
> Sachin

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [leds:for-next 18/18] drivers/leds/leds-lm3530.c:432 lm3530_probe() info: why not propagate 'err
  2012-09-12  4:20 [leds:for-next 18/18] drivers/leds/leds-lm3530.c:432 lm3530_probe() info: why not propagate 'err' fr Fengguang Wu
  2012-09-12  4:32 ` [leds:for-next 18/18] drivers/leds/leds-lm3530.c:432 lm3530_probe() info: why not propagate 'err Fengguang Wu
@ 2012-09-12  4:41 ` Sachin Kamat
  2012-09-12  4:55 ` Fengguang Wu
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Sachin Kamat @ 2012-09-12  4:41 UTC (permalink / raw)
  To: kernel-janitors

Hi Fengguang,

Thanks for bringing this to my notice. I compile tested this patch
using the latest (11 Sep 2012) linux-next tree.
Unfortunately I cannot not understand the meaning of these smatch warnings.
Could you please simplify them for me?

Thanks.

On 12 September 2012 09:50, Fengguang Wu <fengguang.wu@intel.com> wrote:
> Hi Sachin,
>
> FYI, there are new smatch warnings show up in
>
> tree:   git://git.kernel.org/pub/scm/linux/kernel/git/cooloney/linux-leds.git for-next
> head:   2829515a5fb5ceccb4021d819c3d7d0ecaab86eb
> commit: 2829515a5fb5ceccb4021d819c3d7d0ecaab86eb [18/18] leds-lm3530: Use devm_regulator_get function
>
> All smatch warnings:
>
>   drivers/leds/leds-lm3530.c:361 lm3530_mode_set() info: why not propagate 'mode' from lm3530_get_mode_from_str() instead of -22?
> + drivers/leds/leds-lm3530.c:432 lm3530_probe() info: why not propagate 'err' from lm3530_init_registers() instead of -19?
> + drivers/leds/leds-lm3530.c:438 lm3530_probe() info: why not propagate 'err' from led_classdev_register() instead of -19?
>
> vim +432 drivers/leds/leds-lm3530.c
>    422                  err = PTR_ERR(drvdata->regulator);
>    423                  drvdata->regulator = NULL;
>    424                  return err;
>    425          }
>    426
>    427          if (drvdata->pdata->brt_val) {
>    428                  err = lm3530_init_registers(drvdata);
>    429                  if (err < 0) {
>    430                          dev_err(&client->dev,
>    431                                  "Register Init failed: %d\n", err);
>  > 432                          return -ENODEV;
>    433                  }
>    434          }
>    435          err = led_classdev_register(&client->dev, &drvdata->led_dev);
>    436          if (err < 0) {
>    437                  dev_err(&client->dev, "Register led class failed: %d\n", err);
>    438                  return -ENODEV;
>    439          }
>    440
>    441          err = device_create_file(drvdata->led_dev.dev, &dev_attr_mode);
>    442          if (err < 0) {
>
> ---
> 0-DAY kernel build testing backend         Open Source Technology Centre
> Fengguang Wu <wfg@linux.intel.com>                     Intel Corporation



-- 
With warm regards,
Sachin

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [leds:for-next 18/18] drivers/leds/leds-lm3530.c:432 lm3530_probe() info: why not propagate 'err
  2012-09-12  4:20 [leds:for-next 18/18] drivers/leds/leds-lm3530.c:432 lm3530_probe() info: why not propagate 'err' fr Fengguang Wu
  2012-09-12  4:32 ` [leds:for-next 18/18] drivers/leds/leds-lm3530.c:432 lm3530_probe() info: why not propagate 'err Fengguang Wu
  2012-09-12  4:41 ` Sachin Kamat
@ 2012-09-12  4:55 ` Fengguang Wu
  2012-09-12  4:59 ` Sachin Kamat
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Fengguang Wu @ 2012-09-12  4:55 UTC (permalink / raw)
  To: kernel-janitors

On Wed, Sep 12, 2012 at 10:17:55AM +0530, Sachin Kamat wrote:
> Hi Fengguang,
> 
> On 12 September 2012 10:02, Fengguang Wu <fengguang.wu@intel.com> wrote:
> > Hi Sachin,
> >
> > On Wed, Sep 12, 2012 at 09:59:38AM +0530, Sachin Kamat wrote:
> >> Hi Fengguang,
> >>
> >> Thanks for bringing this to my notice. I compile tested this patch
> >> using the latest (11 Sep 2012) linux-next tree.
> >> Unfortunately I cannot not understand the meaning of these smatch warnings.
> >> Could you please simplify them for me?
> >
> > I think the warnings can be quieted by
> >
> > -                         return -ENODEV;
> > +                         return err;
> >
> 
> The original code was something like this:
> 
>                 err = -ENODEV;
>                 return err;
> 
> which i simplified to (as a result of using devm_*)
>                 return -ENODEV;
> 
> Hence technically i don't think I changed anything. Do you want me to
> revert this change back?

I think the intention of the warning is that the err value from
lm3530_init_registers() can be directly returned, like this:

                  err = lm3530_init_registers(drvdata);
                  if (err < 0) {
                          dev_err(&client->dev,
                                  "Register Init failed: %d\n", err);
-                         return -ENODEV;
+                         return err;
                  }

That will remove the assumption that lm3530_init_registers() always
returns -ENODEV on error.

Thanks,
Fengguang

> >> On 12 September 2012 09:50, Fengguang Wu <fengguang.wu@intel.com> wrote:
> >> > Hi Sachin,
> >> >
> >> > FYI, there are new smatch warnings show up in
> >> >
> >> > tree:   git://git.kernel.org/pub/scm/linux/kernel/git/cooloney/linux-leds.git for-next
> >> > head:   2829515a5fb5ceccb4021d819c3d7d0ecaab86eb
> >> > commit: 2829515a5fb5ceccb4021d819c3d7d0ecaab86eb [18/18] leds-lm3530: Use devm_regulator_get function
> >> >
> >> > All smatch warnings:
> >> >
> >> >   drivers/leds/leds-lm3530.c:361 lm3530_mode_set() info: why not propagate 'mode' from lm3530_get_mode_from_str() instead of -22?
> >> > + drivers/leds/leds-lm3530.c:432 lm3530_probe() info: why not propagate 'err' from lm3530_init_registers() instead of -19?
> >> > + drivers/leds/leds-lm3530.c:438 lm3530_probe() info: why not propagate 'err' from led_classdev_register() instead of -19?
> >> >
> >> > vim +432 drivers/leds/leds-lm3530.c
> >> >    422                  err = PTR_ERR(drvdata->regulator);
> >> >    423                  drvdata->regulator = NULL;
> >> >    424                  return err;
> >> >    425          }
> >> >    426
> >> >    427          if (drvdata->pdata->brt_val) {
> >> >    428                  err = lm3530_init_registers(drvdata);
> >> >    429                  if (err < 0) {
> >> >    430                          dev_err(&client->dev,
> >> >    431                                  "Register Init failed: %d\n", err);
> >> >  > 432                          return -ENODEV;
> >> >    433                  }
> >> >    434          }
> >> >    435          err = led_classdev_register(&client->dev, &drvdata->led_dev);
> >> >    436          if (err < 0) {
> >> >    437                  dev_err(&client->dev, "Register led class failed: %d\n", err);
> >> >    438                  return -ENODEV;
> >> >    439          }
> >> >    440
> >> >    441          err = device_create_file(drvdata->led_dev.dev, &dev_attr_mode);
> >> >    442          if (err < 0) {
> >> >
> >> > ---
> >> > 0-DAY kernel build testing backend         Open Source Technology Centre
> >> > Fengguang Wu <wfg@linux.intel.com>                     Intel Corporation
> >>
> >>
> >>
> >> --
> >> With warm regards,
> >> Sachin
> 
> 
> 
> -- 
> With warm regards,
> Sachin

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [leds:for-next 18/18] drivers/leds/leds-lm3530.c:432 lm3530_probe() info: why not propagate 'err
  2012-09-12  4:20 [leds:for-next 18/18] drivers/leds/leds-lm3530.c:432 lm3530_probe() info: why not propagate 'err' fr Fengguang Wu
                   ` (2 preceding siblings ...)
  2012-09-12  4:55 ` Fengguang Wu
@ 2012-09-12  4:59 ` Sachin Kamat
  2012-09-12  5:24 ` Fengguang Wu
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Sachin Kamat @ 2012-09-12  4:59 UTC (permalink / raw)
  To: kernel-janitors

Hi Fengguang,

On 12 September 2012 10:02, Fengguang Wu <fengguang.wu@intel.com> wrote:
> Hi Sachin,
>
> On Wed, Sep 12, 2012 at 09:59:38AM +0530, Sachin Kamat wrote:
>> Hi Fengguang,
>>
>> Thanks for bringing this to my notice. I compile tested this patch
>> using the latest (11 Sep 2012) linux-next tree.
>> Unfortunately I cannot not understand the meaning of these smatch warnings.
>> Could you please simplify them for me?
>
> I think the warnings can be quieted by
>
> -                         return -ENODEV;
> +                         return err;
>

The original code was something like this:

                err = -ENODEV;
                return err;

which i simplified to (as a result of using devm_*)
                return -ENODEV;

Hence technically i don't think I changed anything. Do you want me to
revert this change back?


> Thanks,
> Fengguang
>
>> On 12 September 2012 09:50, Fengguang Wu <fengguang.wu@intel.com> wrote:
>> > Hi Sachin,
>> >
>> > FYI, there are new smatch warnings show up in
>> >
>> > tree:   git://git.kernel.org/pub/scm/linux/kernel/git/cooloney/linux-leds.git for-next
>> > head:   2829515a5fb5ceccb4021d819c3d7d0ecaab86eb
>> > commit: 2829515a5fb5ceccb4021d819c3d7d0ecaab86eb [18/18] leds-lm3530: Use devm_regulator_get function
>> >
>> > All smatch warnings:
>> >
>> >   drivers/leds/leds-lm3530.c:361 lm3530_mode_set() info: why not propagate 'mode' from lm3530_get_mode_from_str() instead of -22?
>> > + drivers/leds/leds-lm3530.c:432 lm3530_probe() info: why not propagate 'err' from lm3530_init_registers() instead of -19?
>> > + drivers/leds/leds-lm3530.c:438 lm3530_probe() info: why not propagate 'err' from led_classdev_register() instead of -19?
>> >
>> > vim +432 drivers/leds/leds-lm3530.c
>> >    422                  err = PTR_ERR(drvdata->regulator);
>> >    423                  drvdata->regulator = NULL;
>> >    424                  return err;
>> >    425          }
>> >    426
>> >    427          if (drvdata->pdata->brt_val) {
>> >    428                  err = lm3530_init_registers(drvdata);
>> >    429                  if (err < 0) {
>> >    430                          dev_err(&client->dev,
>> >    431                                  "Register Init failed: %d\n", err);
>> >  > 432                          return -ENODEV;
>> >    433                  }
>> >    434          }
>> >    435          err = led_classdev_register(&client->dev, &drvdata->led_dev);
>> >    436          if (err < 0) {
>> >    437                  dev_err(&client->dev, "Register led class failed: %d\n", err);
>> >    438                  return -ENODEV;
>> >    439          }
>> >    440
>> >    441          err = device_create_file(drvdata->led_dev.dev, &dev_attr_mode);
>> >    442          if (err < 0) {
>> >
>> > ---
>> > 0-DAY kernel build testing backend         Open Source Technology Centre
>> > Fengguang Wu <wfg@linux.intel.com>                     Intel Corporation
>>
>>
>>
>> --
>> With warm regards,
>> Sachin



-- 
With warm regards,
Sachin

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [leds:for-next 18/18] drivers/leds/leds-lm3530.c:432 lm3530_probe() info: why not propagate 'err
  2012-09-12  4:20 [leds:for-next 18/18] drivers/leds/leds-lm3530.c:432 lm3530_probe() info: why not propagate 'err' fr Fengguang Wu
                   ` (3 preceding siblings ...)
  2012-09-12  4:59 ` Sachin Kamat
@ 2012-09-12  5:24 ` Fengguang Wu
  2012-09-12  5:30 ` Sachin Kamat
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Fengguang Wu @ 2012-09-12  5:24 UTC (permalink / raw)
  To: kernel-janitors

> > I think the intention of the warning is that the err value from
> > lm3530_init_registers() can be directly returned, like this:
> >
> >                   err = lm3530_init_registers(drvdata);
> >                   if (err < 0) {
> >                           dev_err(&client->dev,
> >                                   "Register Init failed: %d\n", err);
> > -                         return -ENODEV;
> > +                         return err;
> >                   }
> >
> > That will remove the assumption that lm3530_init_registers() always
> > returns -ENODEV on error.
> 
> Oh OK. Now I got it :)
> Thanks for the clarification.
> Since this was a prevalent issue with this driver (and maybe with
> other driver too?), I will send a patch to fix this up.

Yes, I think so.. Thank you very much!

Thanks,
Fengguang

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [leds:for-next 18/18] drivers/leds/leds-lm3530.c:432 lm3530_probe() info: why not propagate 'err
  2012-09-12  4:20 [leds:for-next 18/18] drivers/leds/leds-lm3530.c:432 lm3530_probe() info: why not propagate 'err' fr Fengguang Wu
                   ` (4 preceding siblings ...)
  2012-09-12  5:24 ` Fengguang Wu
@ 2012-09-12  5:30 ` Sachin Kamat
  2012-09-12  5:46 ` Bryan Wu
  2012-09-12  5:52 ` Sachin Kamat
  7 siblings, 0 replies; 9+ messages in thread
From: Sachin Kamat @ 2012-09-12  5:30 UTC (permalink / raw)
  To: kernel-janitors

On 12 September 2012 10:25, Fengguang Wu <fengguang.wu@intel.com> wrote:
> On Wed, Sep 12, 2012 at 10:17:55AM +0530, Sachin Kamat wrote:
>> Hi Fengguang,
>>
>> On 12 September 2012 10:02, Fengguang Wu <fengguang.wu@intel.com> wrote:
>> > Hi Sachin,
>> >
>> > On Wed, Sep 12, 2012 at 09:59:38AM +0530, Sachin Kamat wrote:
>> >> Hi Fengguang,
>> >>
>> >> Thanks for bringing this to my notice. I compile tested this patch
>> >> using the latest (11 Sep 2012) linux-next tree.
>> >> Unfortunately I cannot not understand the meaning of these smatch warnings.
>> >> Could you please simplify them for me?
>> >
>> > I think the warnings can be quieted by
>> >
>> > -                         return -ENODEV;
>> > +                         return err;
>> >
>>
>> The original code was something like this:
>>
>>                 err = -ENODEV;
>>                 return err;
>>
>> which i simplified to (as a result of using devm_*)
>>                 return -ENODEV;
>>
>> Hence technically i don't think I changed anything. Do you want me to
>> revert this change back?
>
> I think the intention of the warning is that the err value from
> lm3530_init_registers() can be directly returned, like this:
>
>                   err = lm3530_init_registers(drvdata);
>                   if (err < 0) {
>                           dev_err(&client->dev,
>                                   "Register Init failed: %d\n", err);
> -                         return -ENODEV;
> +                         return err;
>                   }
>
> That will remove the assumption that lm3530_init_registers() always
> returns -ENODEV on error.

Oh OK. Now I got it :)
Thanks for the clarification.
Since this was a prevalent issue with this driver (and maybe with
other driver too?), I will send a patch to fix this up.

>
> Thanks,
> Fengguang
>
>> >> On 12 September 2012 09:50, Fengguang Wu <fengguang.wu@intel.com> wrote:
>> >> > Hi Sachin,
>> >> >
>> >> > FYI, there are new smatch warnings show up in
>> >> >
>> >> > tree:   git://git.kernel.org/pub/scm/linux/kernel/git/cooloney/linux-leds.git for-next
>> >> > head:   2829515a5fb5ceccb4021d819c3d7d0ecaab86eb
>> >> > commit: 2829515a5fb5ceccb4021d819c3d7d0ecaab86eb [18/18] leds-lm3530: Use devm_regulator_get function
>> >> >
>> >> > All smatch warnings:
>> >> >
>> >> >   drivers/leds/leds-lm3530.c:361 lm3530_mode_set() info: why not propagate 'mode' from lm3530_get_mode_from_str() instead of -22?
>> >> > + drivers/leds/leds-lm3530.c:432 lm3530_probe() info: why not propagate 'err' from lm3530_init_registers() instead of -19?
>> >> > + drivers/leds/leds-lm3530.c:438 lm3530_probe() info: why not propagate 'err' from led_classdev_register() instead of -19?
>> >> >
>> >> > vim +432 drivers/leds/leds-lm3530.c
>> >> >    422                  err = PTR_ERR(drvdata->regulator);
>> >> >    423                  drvdata->regulator = NULL;
>> >> >    424                  return err;
>> >> >    425          }
>> >> >    426
>> >> >    427          if (drvdata->pdata->brt_val) {
>> >> >    428                  err = lm3530_init_registers(drvdata);
>> >> >    429                  if (err < 0) {
>> >> >    430                          dev_err(&client->dev,
>> >> >    431                                  "Register Init failed: %d\n", err);
>> >> >  > 432                          return -ENODEV;
>> >> >    433                  }
>> >> >    434          }
>> >> >    435          err = led_classdev_register(&client->dev, &drvdata->led_dev);
>> >> >    436          if (err < 0) {
>> >> >    437                  dev_err(&client->dev, "Register led class failed: %d\n", err);
>> >> >    438                  return -ENODEV;
>> >> >    439          }
>> >> >    440
>> >> >    441          err = device_create_file(drvdata->led_dev.dev, &dev_attr_mode);
>> >> >    442          if (err < 0) {
>> >> >
>> >> > ---
>> >> > 0-DAY kernel build testing backend         Open Source Technology Centre
>> >> > Fengguang Wu <wfg@linux.intel.com>                     Intel Corporation
>> >>
>> >>
>> >>
>> >> --
>> >> With warm regards,
>> >> Sachin
>>
>>
>>
>> --
>> With warm regards,
>> Sachin



-- 
With warm regards,
Sachin

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [leds:for-next 18/18] drivers/leds/leds-lm3530.c:432 lm3530_probe() info: why not propagate 'err
  2012-09-12  4:20 [leds:for-next 18/18] drivers/leds/leds-lm3530.c:432 lm3530_probe() info: why not propagate 'err' fr Fengguang Wu
                   ` (5 preceding siblings ...)
  2012-09-12  5:30 ` Sachin Kamat
@ 2012-09-12  5:46 ` Bryan Wu
  2012-09-12  5:52 ` Sachin Kamat
  7 siblings, 0 replies; 9+ messages in thread
From: Bryan Wu @ 2012-09-12  5:46 UTC (permalink / raw)
  To: kernel-janitors

On Wed, Sep 12, 2012 at 1:18 PM, Sachin Kamat <sachin.kamat@linaro.org> wrote:
> On 12 September 2012 10:25, Fengguang Wu <fengguang.wu@intel.com> wrote:
>> On Wed, Sep 12, 2012 at 10:17:55AM +0530, Sachin Kamat wrote:
>>> Hi Fengguang,
>>>
>>> On 12 September 2012 10:02, Fengguang Wu <fengguang.wu@intel.com> wrote:
>>> > Hi Sachin,
>>> >
>>> > On Wed, Sep 12, 2012 at 09:59:38AM +0530, Sachin Kamat wrote:
>>> >> Hi Fengguang,
>>> >>
>>> >> Thanks for bringing this to my notice. I compile tested this patch
>>> >> using the latest (11 Sep 2012) linux-next tree.
>>> >> Unfortunately I cannot not understand the meaning of these smatch warnings.
>>> >> Could you please simplify them for me?
>>> >
>>> > I think the warnings can be quieted by
>>> >
>>> > -                         return -ENODEV;
>>> > +                         return err;
>>> >
>>>
>>> The original code was something like this:
>>>
>>>                 err = -ENODEV;
>>>                 return err;
>>>
>>> which i simplified to (as a result of using devm_*)
>>>                 return -ENODEV;
>>>
>>> Hence technically i don't think I changed anything. Do you want me to
>>> revert this change back?
>>
>> I think the intention of the warning is that the err value from
>> lm3530_init_registers() can be directly returned, like this:
>>
>>                   err = lm3530_init_registers(drvdata);
>>                   if (err < 0) {
>>                           dev_err(&client->dev,
>>                                   "Register Init failed: %d\n", err);
>> -                         return -ENODEV;
>> +                         return err;
>>                   }
>>
>> That will remove the assumption that lm3530_init_registers() always
>> returns -ENODEV on error.
>
> Oh OK. Now I got it :)
> Thanks for the clarification.
> Since this was a prevalent issue with this driver (and maybe with
> other driver too?), I will send a patch to fix this up.
>

Thanks and please post a new patch, I can replace the old one to it in
my for-next branch.

>>
>> Thanks,
>> Fengguang

And thanks Fengguang.

-Bryan

>>
>>> >> On 12 September 2012 09:50, Fengguang Wu <fengguang.wu@intel.com> wrote:
>>> >> > Hi Sachin,
>>> >> >
>>> >> > FYI, there are new smatch warnings show up in
>>> >> >
>>> >> > tree:   git://git.kernel.org/pub/scm/linux/kernel/git/cooloney/linux-leds.git for-next
>>> >> > head:   2829515a5fb5ceccb4021d819c3d7d0ecaab86eb
>>> >> > commit: 2829515a5fb5ceccb4021d819c3d7d0ecaab86eb [18/18] leds-lm3530: Use devm_regulator_get function
>>> >> >
>>> >> > All smatch warnings:
>>> >> >
>>> >> >   drivers/leds/leds-lm3530.c:361 lm3530_mode_set() info: why not propagate 'mode' from lm3530_get_mode_from_str() instead of -22?
>>> >> > + drivers/leds/leds-lm3530.c:432 lm3530_probe() info: why not propagate 'err' from lm3530_init_registers() instead of -19?
>>> >> > + drivers/leds/leds-lm3530.c:438 lm3530_probe() info: why not propagate 'err' from led_classdev_register() instead of -19?
>>> >> >
>>> >> > vim +432 drivers/leds/leds-lm3530.c
>>> >> >    422                  err = PTR_ERR(drvdata->regulator);
>>> >> >    423                  drvdata->regulator = NULL;
>>> >> >    424                  return err;
>>> >> >    425          }
>>> >> >    426
>>> >> >    427          if (drvdata->pdata->brt_val) {
>>> >> >    428                  err = lm3530_init_registers(drvdata);
>>> >> >    429                  if (err < 0) {
>>> >> >    430                          dev_err(&client->dev,
>>> >> >    431                                  "Register Init failed: %d\n", err);
>>> >> >  > 432                          return -ENODEV;
>>> >> >    433                  }
>>> >> >    434          }
>>> >> >    435          err = led_classdev_register(&client->dev, &drvdata->led_dev);
>>> >> >    436          if (err < 0) {
>>> >> >    437                  dev_err(&client->dev, "Register led class failed: %d\n", err);
>>> >> >    438                  return -ENODEV;
>>> >> >    439          }
>>> >> >    440
>>> >> >    441          err = device_create_file(drvdata->led_dev.dev, &dev_attr_mode);
>>> >> >    442          if (err < 0) {
>>> >> >
>>> >> > ---
>>> >> > 0-DAY kernel build testing backend         Open Source Technology Centre
>>> >> > Fengguang Wu <wfg@linux.intel.com>                     Intel Corporation
>>> >>
>>> >>
>>> >>
>>> >> --
>>> >> With warm regards,
>>> >> Sachin
>>>
>>>
>>>
>>> --
>>> With warm regards,
>>> Sachin
>
>
>
> --
> With warm regards,
> Sachin



-- 
Bryan Wu <bryan.wu@canonical.com>
Kernel Developer    +86.186-168-78255 Mobile
Canonical Ltd.      www.canonical.com
Ubuntu - Linux for human beings | www.ubuntu.com

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [leds:for-next 18/18] drivers/leds/leds-lm3530.c:432 lm3530_probe() info: why not propagate 'err
  2012-09-12  4:20 [leds:for-next 18/18] drivers/leds/leds-lm3530.c:432 lm3530_probe() info: why not propagate 'err' fr Fengguang Wu
                   ` (6 preceding siblings ...)
  2012-09-12  5:46 ` Bryan Wu
@ 2012-09-12  5:52 ` Sachin Kamat
  7 siblings, 0 replies; 9+ messages in thread
From: Sachin Kamat @ 2012-09-12  5:52 UTC (permalink / raw)
  To: kernel-janitors

On 12 September 2012 11:16, Bryan Wu <bryan.wu@canonical.com> wrote:
> On Wed, Sep 12, 2012 at 1:18 PM, Sachin Kamat <sachin.kamat@linaro.org> wrote:
>> On 12 September 2012 10:25, Fengguang Wu <fengguang.wu@intel.com> wrote:
>>> On Wed, Sep 12, 2012 at 10:17:55AM +0530, Sachin Kamat wrote:
>>>> Hi Fengguang,
>>>>
>>>> On 12 September 2012 10:02, Fengguang Wu <fengguang.wu@intel.com> wrote:
>>>> > Hi Sachin,
>>>> >
>>>> > On Wed, Sep 12, 2012 at 09:59:38AM +0530, Sachin Kamat wrote:
>>>> >> Hi Fengguang,
>>>> >>
>>>> >> Thanks for bringing this to my notice. I compile tested this patch
>>>> >> using the latest (11 Sep 2012) linux-next tree.
>>>> >> Unfortunately I cannot not understand the meaning of these smatch warnings.
>>>> >> Could you please simplify them for me?
>>>> >
>>>> > I think the warnings can be quieted by
>>>> >
>>>> > -                         return -ENODEV;
>>>> > +                         return err;
>>>> >
>>>>
>>>> The original code was something like this:
>>>>
>>>>                 err = -ENODEV;
>>>>                 return err;
>>>>
>>>> which i simplified to (as a result of using devm_*)
>>>>                 return -ENODEV;
>>>>
>>>> Hence technically i don't think I changed anything. Do you want me to
>>>> revert this change back?
>>>
>>> I think the intention of the warning is that the err value from
>>> lm3530_init_registers() can be directly returned, like this:
>>>
>>>                   err = lm3530_init_registers(drvdata);
>>>                   if (err < 0) {
>>>                           dev_err(&client->dev,
>>>                                   "Register Init failed: %d\n", err);
>>> -                         return -ENODEV;
>>> +                         return err;
>>>                   }
>>>
>>> That will remove the assumption that lm3530_init_registers() always
>>> returns -ENODEV on error.
>>
>> Oh OK. Now I got it :)
>> Thanks for the clarification.
>> Since this was a prevalent issue with this driver (and maybe with
>> other driver too?), I will send a patch to fix this up.
>>
>
> Thanks and please post a new patch, I can replace the old one to it in
> my for-next branch.

I will post an add on patch (on top of the previous one) to fix the
smatch warnings.
Since they fix an issue already present in that driver (not introduced
by my change), I prefer not to mix the 2 changes.

>
>>>
>>> Thanks,
>>> Fengguang
>
> And thanks Fengguang.
>
> -Bryan
>
>>>
>>>> >> On 12 September 2012 09:50, Fengguang Wu <fengguang.wu@intel.com> wrote:
>>>> >> > Hi Sachin,
>>>> >> >
>>>> >> > FYI, there are new smatch warnings show up in
>>>> >> >
>>>> >> > tree:   git://git.kernel.org/pub/scm/linux/kernel/git/cooloney/linux-leds.git for-next
>>>> >> > head:   2829515a5fb5ceccb4021d819c3d7d0ecaab86eb
>>>> >> > commit: 2829515a5fb5ceccb4021d819c3d7d0ecaab86eb [18/18] leds-lm3530: Use devm_regulator_get function
>>>> >> >
>>>> >> > All smatch warnings:
>>>> >> >
>>>> >> >   drivers/leds/leds-lm3530.c:361 lm3530_mode_set() info: why not propagate 'mode' from lm3530_get_mode_from_str() instead of -22?
>>>> >> > + drivers/leds/leds-lm3530.c:432 lm3530_probe() info: why not propagate 'err' from lm3530_init_registers() instead of -19?
>>>> >> > + drivers/leds/leds-lm3530.c:438 lm3530_probe() info: why not propagate 'err' from led_classdev_register() instead of -19?
>>>> >> >
>>>> >> > vim +432 drivers/leds/leds-lm3530.c
>>>> >> >    422                  err = PTR_ERR(drvdata->regulator);
>>>> >> >    423                  drvdata->regulator = NULL;
>>>> >> >    424                  return err;
>>>> >> >    425          }
>>>> >> >    426
>>>> >> >    427          if (drvdata->pdata->brt_val) {
>>>> >> >    428                  err = lm3530_init_registers(drvdata);
>>>> >> >    429                  if (err < 0) {
>>>> >> >    430                          dev_err(&client->dev,
>>>> >> >    431                                  "Register Init failed: %d\n", err);
>>>> >> >  > 432                          return -ENODEV;
>>>> >> >    433                  }
>>>> >> >    434          }
>>>> >> >    435          err = led_classdev_register(&client->dev, &drvdata->led_dev);
>>>> >> >    436          if (err < 0) {
>>>> >> >    437                  dev_err(&client->dev, "Register led class failed: %d\n", err);
>>>> >> >    438                  return -ENODEV;
>>>> >> >    439          }
>>>> >> >    440
>>>> >> >    441          err = device_create_file(drvdata->led_dev.dev, &dev_attr_mode);
>>>> >> >    442          if (err < 0) {
>>>> >> >
>>>> >> > ---
>>>> >> > 0-DAY kernel build testing backend         Open Source Technology Centre
>>>> >> > Fengguang Wu <wfg@linux.intel.com>                     Intel Corporation
>>>> >>
>>>> >>
>>>> >>
>>>> >> --
>>>> >> With warm regards,
>>>> >> Sachin
>>>>
>>>>
>>>>
>>>> --
>>>> With warm regards,
>>>> Sachin
>>
>>
>>
>> --
>> With warm regards,
>> Sachin
>
>
>
> --
> Bryan Wu <bryan.wu@canonical.com>
> Kernel Developer    +86.186-168-78255 Mobile
> Canonical Ltd.      www.canonical.com
> Ubuntu - Linux for human beings | www.ubuntu.com



-- 
With warm regards,
Sachin

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2012-09-12  5:52 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-09-12  4:20 [leds:for-next 18/18] drivers/leds/leds-lm3530.c:432 lm3530_probe() info: why not propagate 'err' fr Fengguang Wu
2012-09-12  4:32 ` [leds:for-next 18/18] drivers/leds/leds-lm3530.c:432 lm3530_probe() info: why not propagate 'err Fengguang Wu
2012-09-12  4:41 ` Sachin Kamat
2012-09-12  4:55 ` Fengguang Wu
2012-09-12  4:59 ` Sachin Kamat
2012-09-12  5:24 ` Fengguang Wu
2012-09-12  5:30 ` Sachin Kamat
2012-09-12  5:46 ` Bryan Wu
2012-09-12  5:52 ` Sachin Kamat

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox