Here is another patch series that inches the driver closer to moving out of staging. The most interesting changes are the last two in the series. One gets the proximity sensor working and the other one removes the unnecessary in_proximity0_calibscale_available sysfs attribute from two of the supported devices. The rest of the patches are not that interesting and either reduce duplicate code, add error handling, or other minor cosmetic changes. Brian Masney (12): staging: iio: tsl2x7x: remove power functions from tsl2X7X_platform_data staging: iio: tsl2x7x: add common function for clearing interrupts staging: iio: tsl2x7x: add common function for reading chip status staging: iio: tsl2x7x: add common function for writing to the control register staging: iio: tsl2x7x: convert mutex_trylock() to mutex_lock() staging: iio: tsl2x7x: correctly return errors in tsl2x7x_get_prox() staging: iio: tsl2x7x: correct 'Avoid CamelCase' warning from checkpatch staging: iio: tsl2x7x: add error handling to tsl2x7x_prox_cal() staging: iio: tsl2x7x: add missing error checks staging: iio: tsl2x7x: make logging consistent and correct newlines staging: iio: tsl2x7x: remove unnecessary sysfs attribute staging: iio: tsl2x7x: make proximity sensor function correctly drivers/staging/iio/light/tsl2x7x.c | 346 ++++++++++++++++++------------------ drivers/staging/iio/light/tsl2x7x.h | 6 +- 2 files changed, 179 insertions(+), 173 deletions(-) -- 2.14.3
The tsl2X7X_platform_data structure contains the platform_power, power_on, and power_off function pointers. These power management functions should not be in the platform data. These functions were likely used before the regulator framework was put in place. There are no users of these functions in the mainline kernel. Signed-off-by: Brian Masney <masneyb@onstation.org> --- drivers/staging/iio/light/tsl2x7x.c | 18 ------------------ drivers/staging/iio/light/tsl2x7x.h | 4 ---- 2 files changed, 22 deletions(-) diff --git a/drivers/staging/iio/light/tsl2x7x.c b/drivers/staging/iio/light/tsl2x7x.c index 126e11530ce0..b7e3f966c3a6 100644 --- a/drivers/staging/iio/light/tsl2x7x.c +++ b/drivers/staging/iio/light/tsl2x7x.c @@ -588,9 +588,6 @@ static int tsl2x7x_chip_on(struct iio_dev *indio_dev) struct tsl2X7X_chip *chip = iio_priv(indio_dev); u8 reg_val = 0; - if (chip->pdata && chip->pdata->power_on) - chip->pdata->power_on(indio_dev); - /* Non calculated parameters */ chip->tsl2x7x_config[TSL2X7X_PRX_TIME] = chip->settings.prx_time; chip->tsl2x7x_config[TSL2X7X_WAIT_TIME] = chip->settings.wait_time; @@ -736,9 +733,6 @@ static int tsl2x7x_chip_off(struct iio_dev *indio_dev) ret = i2c_smbus_write_byte_data(chip->client, TSL2X7X_CMD_REG | TSL2X7X_CNTRL, 0x00); - if (chip->pdata && chip->pdata->power_off) - chip->pdata->power_off(chip->client); - return ret; } @@ -1792,12 +1786,6 @@ static int tsl2x7x_suspend(struct device *dev) chip->tsl2x7x_chip_status = TSL2X7X_CHIP_SUSPENDED; } - if (chip->pdata && chip->pdata->platform_power) { - pm_message_t pmm = {PM_EVENT_SUSPEND}; - - chip->pdata->platform_power(dev, pmm); - } - return ret; } @@ -1807,12 +1795,6 @@ static int tsl2x7x_resume(struct device *dev) struct tsl2X7X_chip *chip = iio_priv(indio_dev); int ret = 0; - if (chip->pdata && chip->pdata->platform_power) { - pm_message_t pmm = {PM_EVENT_RESUME}; - - chip->pdata->platform_power(dev, pmm); - } - if (chip->tsl2x7x_chip_status == TSL2X7X_CHIP_SUSPENDED) ret = tsl2x7x_chip_on(indio_dev); diff --git a/drivers/staging/iio/light/tsl2x7x.h b/drivers/staging/iio/light/tsl2x7x.h index df00f2ec1719..6624cbca7a83 100644 --- a/drivers/staging/iio/light/tsl2x7x.h +++ b/drivers/staging/iio/light/tsl2x7x.h @@ -21,7 +21,6 @@ #ifndef __TSL2X7X_H #define __TSL2X7X_H -#include <linux/pm.h> struct tsl2x7x_lux { unsigned int ratio; @@ -91,9 +90,6 @@ struct tsl2x7x_settings { * */ struct tsl2X7X_platform_data { - int (*platform_power)(struct device *dev, pm_message_t); - int (*power_on)(struct iio_dev *indio_dev); - int (*power_off)(struct i2c_client *dev); struct tsl2x7x_lux platform_lux_table[TSL2X7X_MAX_LUX_TABLE_SIZE]; struct tsl2x7x_settings *platform_default_settings; }; -- 2.14.3
There were three places where the same chunk of code was used to clear interrupts. This patch creates a common function tsl2x7x_clear_interrupts() to reduce duplicate code. Signed-off-by: Brian Masney <masneyb@onstation.org> --- drivers/staging/iio/light/tsl2x7x.c | 52 +++++++++++++++++-------------------- 1 file changed, 24 insertions(+), 28 deletions(-) diff --git a/drivers/staging/iio/light/tsl2x7x.c b/drivers/staging/iio/light/tsl2x7x.c index b7e3f966c3a6..c02db03ef369 100644 --- a/drivers/staging/iio/light/tsl2x7x.c +++ b/drivers/staging/iio/light/tsl2x7x.c @@ -279,6 +279,20 @@ static const u8 device_channel_config[] = { ALSPRX2 }; +static int tsl2x7x_clear_interrupts(struct tsl2X7X_chip *chip, int reg) +{ + int ret; + + ret = i2c_smbus_write_byte(chip->client, + TSL2X7X_CMD_REG | TSL2X7X_CMD_SPL_FN | reg); + if (ret < 0) + dev_err(&chip->client->dev, + "%s: failed to clear interrupt status %x: %d\n", + __func__, reg, ret); + + return ret; +} + /** * tsl2x7x_get_lux() - Reads and calculates current lux value. * @indio_dev: pointer to IIO device @@ -346,16 +360,9 @@ static int tsl2x7x_get_lux(struct iio_dev *indio_dev) buf[i] = ret; } - /* clear any existing interrupt status */ - ret = i2c_smbus_write_byte(chip->client, - TSL2X7X_CMD_REG | - TSL2X7X_CMD_SPL_FN | - TSL2X7X_CMD_ALS_INT_CLR); - if (ret < 0) { - dev_err(&chip->client->dev, - "i2c_write_command failed - err = %d\n", ret); - goto out_unlock; /* have no data, so return failure */ - } + ret = tsl2x7x_clear_interrupts(chip, TSL2X7X_CMD_ALS_INT_CLR); + if (ret < 0) + goto out_unlock; /* extract ALS/lux data */ ch0 = le16_to_cpup((const __le16 *)&buf[0]); @@ -706,17 +713,10 @@ static int tsl2x7x_chip_on(struct iio_dev *indio_dev) "%s: failed in tsl2x7x_IOCTL_INT_SET.\n", __func__); - /* Clear out any initial interrupts */ - ret = i2c_smbus_write_byte(chip->client, - TSL2X7X_CMD_REG | - TSL2X7X_CMD_SPL_FN | - TSL2X7X_CMD_PROXALS_INT_CLR); - if (ret < 0) { - dev_err(&chip->client->dev, - "%s: Failed to clear Int status\n", - __func__); - return ret; - } + ret = tsl2x7x_clear_interrupts(chip, + TSL2X7X_CMD_PROXALS_INT_CLR); + if (ret < 0) + return ret; } return ret; @@ -1421,14 +1421,10 @@ static irqreturn_t tsl2x7x_event_handler(int irq, void *private) IIO_EV_DIR_EITHER), timestamp); } - /* Clear interrupt now that we have handled it. */ - ret = i2c_smbus_write_byte(chip->client, - TSL2X7X_CMD_REG | TSL2X7X_CMD_SPL_FN | - TSL2X7X_CMD_PROXALS_INT_CLR); + + ret = tsl2x7x_clear_interrupts(chip, TSL2X7X_CMD_PROXALS_INT_CLR); if (ret < 0) - dev_err(&chip->client->dev, - "Failed to clear irq from event handler. err = %d\n", - ret); + return ret; return IRQ_HANDLED; } -- 2.14.3
There were three places where the same chunk of code was used to read the chip status. This patch creates a common function tsl2x7x_read_status() to reduce duplicate code. This patch also corrects tsl2x7x_event_handler() to properly check for an error after reading the chip status. Signed-off-by: Brian Masney <masneyb@onstation.org> --- drivers/staging/iio/light/tsl2x7x.c | 40 ++++++++++++++++++++++--------------- 1 file changed, 24 insertions(+), 16 deletions(-) diff --git a/drivers/staging/iio/light/tsl2x7x.c b/drivers/staging/iio/light/tsl2x7x.c index c02db03ef369..6bb622816660 100644 --- a/drivers/staging/iio/light/tsl2x7x.c +++ b/drivers/staging/iio/light/tsl2x7x.c @@ -293,6 +293,20 @@ static int tsl2x7x_clear_interrupts(struct tsl2X7X_chip *chip, int reg) return ret; } +static int tsl2x7x_read_status(struct tsl2X7X_chip *chip) +{ + int ret; + + ret = i2c_smbus_read_byte_data(chip->client, + TSL2X7X_CMD_REG | TSL2X7X_STATUS); + if (ret < 0) + dev_err(&chip->client->dev, + "%s: failed to read STATUS register: %d\n", __func__, + ret); + + return ret; +} + /** * tsl2x7x_get_lux() - Reads and calculates current lux value. * @indio_dev: pointer to IIO device @@ -332,13 +346,10 @@ static int tsl2x7x_get_lux(struct iio_dev *indio_dev) goto out_unlock; } - ret = i2c_smbus_read_byte_data(chip->client, - TSL2X7X_CMD_REG | TSL2X7X_STATUS); - if (ret < 0) { - dev_err(&chip->client->dev, - "%s: Failed to read STATUS Reg\n", __func__); + ret = tsl2x7x_read_status(chip); + if (ret < 0) goto out_unlock; - } + /* is data new & valid */ if (!(ret & TSL2X7X_STA_ADC_VALID)) { dev_err(&chip->client->dev, @@ -458,12 +469,9 @@ static int tsl2x7x_get_prox(struct iio_dev *indio_dev) return -EBUSY; } - ret = i2c_smbus_read_byte_data(chip->client, - TSL2X7X_CMD_REG | TSL2X7X_STATUS); - if (ret < 0) { - dev_err(&chip->client->dev, "i2c err=%d\n", ret); + ret = tsl2x7x_read_status(chip); + if (ret < 0) goto prox_poll_err; - } switch (chip->id) { case tsl2571: @@ -1396,13 +1404,13 @@ static irqreturn_t tsl2x7x_event_handler(int irq, void *private) struct tsl2X7X_chip *chip = iio_priv(indio_dev); s64 timestamp = iio_get_time_ns(indio_dev); int ret; - u8 value; - value = i2c_smbus_read_byte_data(chip->client, - TSL2X7X_CMD_REG | TSL2X7X_STATUS); + ret = tsl2x7x_read_status(chip); + if (ret < 0) + return ret; /* What type of interrupt do we need to process */ - if (value & TSL2X7X_STA_PRX_INTR) { + if (ret & TSL2X7X_STA_PRX_INTR) { tsl2x7x_get_prox(indio_dev); /* freshen data for ABI */ iio_push_event(indio_dev, IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, @@ -1412,7 +1420,7 @@ static irqreturn_t tsl2x7x_event_handler(int irq, void *private) timestamp); } - if (value & TSL2X7X_STA_ALS_INTR) { + if (ret & TSL2X7X_STA_ALS_INTR) { tsl2x7x_get_lux(indio_dev); /* freshen data for ABI */ iio_push_event(indio_dev, IIO_UNMOD_EVENT_CODE(IIO_LIGHT, -- 2.14.3
There were four places where the same chunk of code was used to write to the control register. This patch creates a common function tsl2x7x_write_control_reg() to reduce duplicate code. The function tsl2x7x_chip_off() did not correctly return an error code so this patch also corrects that issue. Signed-off-by: Brian Masney <masneyb@onstation.org> --- drivers/staging/iio/light/tsl2x7x.c | 54 +++++++++++++++++-------------------- 1 file changed, 25 insertions(+), 29 deletions(-) diff --git a/drivers/staging/iio/light/tsl2x7x.c b/drivers/staging/iio/light/tsl2x7x.c index 6bb622816660..cf16dd206c0b 100644 --- a/drivers/staging/iio/light/tsl2x7x.c +++ b/drivers/staging/iio/light/tsl2x7x.c @@ -307,6 +307,21 @@ static int tsl2x7x_read_status(struct tsl2X7X_chip *chip) return ret; } +static int tsl2x7x_write_control_reg(struct tsl2X7X_chip *chip, u8 data) +{ + int ret; + + ret = i2c_smbus_write_byte_data(chip->client, + TSL2X7X_CMD_REG | TSL2X7X_CNTRL, data); + if (ret < 0) { + dev_err(&chip->client->dev, + "%s: failed to write to control register %x: %d\n", + __func__, data, ret); + } + + return ret; +} + /** * tsl2x7x_get_lux() - Reads and calculates current lux value. * @indio_dev: pointer to IIO device @@ -597,7 +612,6 @@ static int tsl2x7x_chip_on(struct iio_dev *indio_dev) int i; int ret = 0; u8 *dev_reg; - u8 utmp; int als_count; int als_time; struct tsl2X7X_chip *chip = iio_priv(indio_dev); @@ -659,14 +673,9 @@ static int tsl2x7x_chip_on(struct iio_dev *indio_dev) * TSL2X7X Specific power-on / adc enable sequence * Power on the device 1st. */ - utmp = TSL2X7X_CNTL_PWR_ON; - ret = i2c_smbus_write_byte_data(chip->client, - TSL2X7X_CMD_REG | TSL2X7X_CNTRL, utmp); - if (ret < 0) { - dev_err(&chip->client->dev, - "%s: failed on CNTRL reg.\n", __func__); + ret = tsl2x7x_write_control_reg(chip, TSL2X7X_CNTL_PWR_ON); + if (ret < 0) return ret; - } /* * Use the following shadow copy for our delay before enabling ADC. @@ -691,16 +700,12 @@ static int tsl2x7x_chip_on(struct iio_dev *indio_dev) * NOW enable the ADC * initialize the desired mode of operation */ - utmp = TSL2X7X_CNTL_PWR_ON | - TSL2X7X_CNTL_ADC_ENBL | - TSL2X7X_CNTL_PROX_DET_ENBL; - ret = i2c_smbus_write_byte_data(chip->client, - TSL2X7X_CMD_REG | TSL2X7X_CNTRL, utmp); - if (ret < 0) { - dev_err(&chip->client->dev, - "%s: failed on 2nd CTRL reg.\n", __func__); + ret = tsl2x7x_write_control_reg(chip, + TSL2X7X_CNTL_PWR_ON | + TSL2X7X_CNTL_ADC_ENBL | + TSL2X7X_CNTL_PROX_DET_ENBL); + if (ret < 0) return ret; - } chip->tsl2x7x_chip_status = TSL2X7X_CHIP_WORKING; @@ -713,13 +718,9 @@ static int tsl2x7x_chip_on(struct iio_dev *indio_dev) reg_val |= TSL2X7X_CNTL_PROX_DET_ENBL; reg_val |= chip->settings.interrupts_en; - ret = i2c_smbus_write_byte_data(chip->client, - TSL2X7X_CMD_REG | TSL2X7X_CNTRL, - reg_val); + ret = tsl2x7x_write_control_reg(chip, reg_val); if (ret < 0) - dev_err(&chip->client->dev, - "%s: failed in tsl2x7x_IOCTL_INT_SET.\n", - __func__); + return ret; ret = tsl2x7x_clear_interrupts(chip, TSL2X7X_CMD_PROXALS_INT_CLR); @@ -732,16 +733,11 @@ static int tsl2x7x_chip_on(struct iio_dev *indio_dev) static int tsl2x7x_chip_off(struct iio_dev *indio_dev) { - int ret; struct tsl2X7X_chip *chip = iio_priv(indio_dev); /* turn device off */ chip->tsl2x7x_chip_status = TSL2X7X_CHIP_SUSPENDED; - - ret = i2c_smbus_write_byte_data(chip->client, - TSL2X7X_CMD_REG | TSL2X7X_CNTRL, 0x00); - - return ret; + return tsl2x7x_write_control_reg(chip, 0x00); } /** -- 2.14.3
The driver uses mutex_lock() and mutex_trylock() in several places. Convert the mutex_trylock() to mutex_lock() for consistency with other IIO light drivers in mainline. Signed-off-by: Brian Masney <masneyb@onstation.org> --- drivers/staging/iio/light/tsl2x7x.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/drivers/staging/iio/light/tsl2x7x.c b/drivers/staging/iio/light/tsl2x7x.c index cf16dd206c0b..5c611250127f 100644 --- a/drivers/staging/iio/light/tsl2x7x.c +++ b/drivers/staging/iio/light/tsl2x7x.c @@ -350,8 +350,7 @@ static int tsl2x7x_get_lux(struct iio_dev *indio_dev) u32 ch0lux = 0; u32 ch1lux = 0; - if (mutex_trylock(&chip->als_mutex) == 0) - return chip->als_cur_info.lux; /* busy, so return LAST VALUE */ + mutex_lock(&chip->als_mutex); if (chip->tsl2x7x_chip_status != TSL2X7X_CHIP_WORKING) { /* device is not enabled */ @@ -478,11 +477,7 @@ static int tsl2x7x_get_prox(struct iio_dev *indio_dev) u8 chdata[2]; struct tsl2X7X_chip *chip = iio_priv(indio_dev); - if (mutex_trylock(&chip->prox_mutex) == 0) { - dev_err(&chip->client->dev, - "%s: Can't get prox mutex\n", __func__); - return -EBUSY; - } + mutex_lock(&chip->prox_mutex); ret = tsl2x7x_read_status(chip); if (ret < 0) -- 2.14.3
Not all errors that occurred in tsl2x7x_get_prox() were correctly reported in the return value. This patch changes the error handling so that errors are now returned properly. Note that the ret variable is from the call to tsl2x7x_read_status(), and it already has the correct error check. The -EINVAL error code is for an unexpected value in the register. This patch also corrects an unnecessary word wrap in the call to le16_to_cpup() while changes are being made to this code. Signed-off-by: Brian Masney <masneyb@onstation.org> --- drivers/staging/iio/light/tsl2x7x.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/drivers/staging/iio/light/tsl2x7x.c b/drivers/staging/iio/light/tsl2x7x.c index 5c611250127f..cc209b64ed5a 100644 --- a/drivers/staging/iio/light/tsl2x7x.c +++ b/drivers/staging/iio/light/tsl2x7x.c @@ -489,16 +489,20 @@ static int tsl2x7x_get_prox(struct iio_dev *indio_dev) case tmd2671: case tsl2771: case tmd2771: - if (!(ret & TSL2X7X_STA_ADC_VALID)) + if (!(ret & TSL2X7X_STA_ADC_VALID)) { + ret = -EINVAL; goto prox_poll_err; + } break; case tsl2572: case tsl2672: case tmd2672: case tsl2772: case tmd2772: - if (!(ret & TSL2X7X_STA_PRX_VALID)) + if (!(ret & TSL2X7X_STA_PRX_VALID)) { + ret = -EINVAL; goto prox_poll_err; + } break; } @@ -512,14 +516,13 @@ static int tsl2x7x_get_prox(struct iio_dev *indio_dev) chdata[i] = ret; } - chip->prox_data = - le16_to_cpup((const __le16 *)&chdata[0]); + chip->prox_data = le16_to_cpup((const __le16 *)&chdata[0]); + ret = chip->prox_data; prox_poll_err: - mutex_unlock(&chip->prox_mutex); - return chip->prox_data; + return ret; } /** -- 2.14.3
The statP and calP variables triggered an 'Avoid CamelCase' warning from checkpatch.pl. This patch renames these variables to stat and cal to fix this warning. Signed-off-by: Brian Masney <masneyb@onstation.org> --- drivers/staging/iio/light/tsl2x7x.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/drivers/staging/iio/light/tsl2x7x.c b/drivers/staging/iio/light/tsl2x7x.c index cc209b64ed5a..380a6c96a69a 100644 --- a/drivers/staging/iio/light/tsl2x7x.c +++ b/drivers/staging/iio/light/tsl2x7x.c @@ -773,7 +773,7 @@ static int tsl2x7x_invoke_change(struct iio_dev *indio_dev) } static void tsl2x7x_prox_calculate(int *data, int length, - struct tsl2x7x_prox_stat *statP) + struct tsl2x7x_prox_stat *stat) { int i; int sample_sum; @@ -783,21 +783,21 @@ static void tsl2x7x_prox_calculate(int *data, int length, length = 1; sample_sum = 0; - statP->min = INT_MAX; - statP->max = INT_MIN; + stat->min = INT_MAX; + stat->max = INT_MIN; for (i = 0; i < length; i++) { sample_sum += data[i]; - statP->min = min(statP->min, data[i]); - statP->max = max(statP->max, data[i]); + stat->min = min(stat->min, data[i]); + stat->max = max(stat->max, data[i]); } - statP->mean = sample_sum / length; + stat->mean = sample_sum / length; sample_sum = 0; for (i = 0; i < length; i++) { - tmp = data[i] - statP->mean; + tmp = data[i] - stat->mean; sample_sum += tmp * tmp; } - statP->stddev = int_sqrt((long)sample_sum / length); + stat->stddev = int_sqrt((long)sample_sum / length); } /** @@ -812,7 +812,7 @@ static void tsl2x7x_prox_cal(struct iio_dev *indio_dev) int prox_history[MAX_SAMPLES_CAL + 1]; int i; struct tsl2x7x_prox_stat prox_stat_data[2]; - struct tsl2x7x_prox_stat *calP; + struct tsl2x7x_prox_stat *cal; struct tsl2X7X_chip *chip = iio_priv(indio_dev); u8 tmp_irq_settings; u8 current_state = chip->tsl2x7x_chip_status; @@ -844,13 +844,13 @@ static void tsl2x7x_prox_cal(struct iio_dev *indio_dev) } tsl2x7x_chip_off(indio_dev); - calP = &prox_stat_data[PROX_STAT_CAL]; + cal = &prox_stat_data[PROX_STAT_CAL]; tsl2x7x_prox_calculate(prox_history, - chip->settings.prox_max_samples_cal, calP); - chip->settings.prox_thres_high = (calP->max << 1) - calP->mean; + chip->settings.prox_max_samples_cal, cal); + chip->settings.prox_thres_high = (cal->max << 1) - cal->mean; dev_info(&chip->client->dev, " cal min=%d mean=%d max=%d\n", - calP->min, calP->mean, calP->max); + cal->min, cal->mean, cal->max); dev_info(&chip->client->dev, "%s proximity threshold set to %d\n", chip->client->name, chip->settings.prox_thres_high); -- 2.14.3
tsl2x7x_prox_cal() did not have any error checks. This patch adds the missing error handling and ensures that any errors are reported to user space via in_proximity0_calibrate_store(). Signed-off-by: Brian Masney <masneyb@onstation.org> --- drivers/staging/iio/light/tsl2x7x.c | 36 ++++++++++++++++++++++++++---------- 1 file changed, 26 insertions(+), 10 deletions(-) diff --git a/drivers/staging/iio/light/tsl2x7x.c b/drivers/staging/iio/light/tsl2x7x.c index 380a6c96a69a..8adc6db44790 100644 --- a/drivers/staging/iio/light/tsl2x7x.c +++ b/drivers/staging/iio/light/tsl2x7x.c @@ -807,10 +807,10 @@ static void tsl2x7x_prox_calculate(int *data, int length, * Calculates a standard deviation based on the samples, * and sets the threshold accordingly. */ -static void tsl2x7x_prox_cal(struct iio_dev *indio_dev) +static int tsl2x7x_prox_cal(struct iio_dev *indio_dev) { int prox_history[MAX_SAMPLES_CAL + 1]; - int i; + int i, ret; struct tsl2x7x_prox_stat prox_stat_data[2]; struct tsl2x7x_prox_stat *cal; struct tsl2X7X_chip *chip = iio_priv(indio_dev); @@ -825,25 +825,33 @@ static void tsl2x7x_prox_cal(struct iio_dev *indio_dev) } /* have to stop to change settings */ - tsl2x7x_chip_off(indio_dev); + ret = tsl2x7x_chip_off(indio_dev); + if (ret < 0) + return ret; /* Enable proximity detection save just in case prox not wanted yet*/ tmp_irq_settings = chip->settings.interrupts_en; chip->settings.interrupts_en |= TSL2X7X_CNTL_PROX_INT_ENBL; /*turn on device if not already on*/ - tsl2x7x_chip_on(indio_dev); + ret = tsl2x7x_chip_on(indio_dev); + if (ret < 0) + return ret; /*gather the samples*/ for (i = 0; i < chip->settings.prox_max_samples_cal; i++) { usleep_range(15000, 17500); - tsl2x7x_get_prox(indio_dev); + ret = tsl2x7x_get_prox(indio_dev); + if (ret < 0) + return ret; prox_history[i] = chip->prox_data; dev_info(&chip->client->dev, "2 i=%d prox data= %d\n", i, chip->prox_data); } - tsl2x7x_chip_off(indio_dev); + ret = tsl2x7x_chip_off(indio_dev); + if (ret < 0) + return ret; cal = &prox_stat_data[PROX_STAT_CAL]; tsl2x7x_prox_calculate(prox_history, chip->settings.prox_max_samples_cal, cal); @@ -857,8 +865,13 @@ static void tsl2x7x_prox_cal(struct iio_dev *indio_dev) /* back to the way they were */ chip->settings.interrupts_en = tmp_irq_settings; - if (current_state == TSL2X7X_CHIP_WORKING) - tsl2x7x_chip_on(indio_dev); + if (current_state == TSL2X7X_CHIP_WORKING) { + ret = tsl2x7x_chip_on(indio_dev); + if (ret < 0) + return ret; + } + + return 0; } static ssize_t @@ -1018,8 +1031,11 @@ static ssize_t in_proximity0_calibrate_store(struct device *dev, if (strtobool(buf, &value)) return -EINVAL; - if (value) - tsl2x7x_prox_cal(indio_dev); + if (value) { + ret = tsl2x7x_prox_cal(indio_dev); + if (ret < 0) + return ret; + } ret = tsl2x7x_invoke_change(indio_dev); if (ret < 0) -- 2.14.3
The functions in_illuminance0_calibrate_store() and in_illuminance0_lux_table_store() did not have complete error handling in place. This patch adds the missing error handling. Signed-off-by: Brian Masney <masneyb@onstation.org> --- drivers/staging/iio/light/tsl2x7x.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/drivers/staging/iio/light/tsl2x7x.c b/drivers/staging/iio/light/tsl2x7x.c index 8adc6db44790..da7a4e025083 100644 --- a/drivers/staging/iio/light/tsl2x7x.c +++ b/drivers/staging/iio/light/tsl2x7x.c @@ -940,8 +940,11 @@ static ssize_t in_illuminance0_calibrate_store(struct device *dev, if (strtobool(buf, &value)) return -EINVAL; - if (value) - tsl2x7x_als_calibrate(indio_dev); + if (value) { + ret = tsl2x7x_als_calibrate(indio_dev); + if (ret < 0) + return ret; + } ret = tsl2x7x_invoke_change(indio_dev); if (ret < 0) @@ -1006,8 +1009,11 @@ static ssize_t in_illuminance0_lux_table_store(struct device *dev, return -EINVAL; } - if (chip->tsl2x7x_chip_status == TSL2X7X_CHIP_WORKING) - tsl2x7x_chip_off(indio_dev); + if (chip->tsl2x7x_chip_status == TSL2X7X_CHIP_WORKING) { + ret = tsl2x7x_chip_off(indio_dev); + if (ret < 0) + return ret; + } /* Zero out the table */ memset(chip->tsl2x7x_device_lux, 0, sizeof(chip->tsl2x7x_device_lux)); -- 2.14.3
This patch updates all of the logging commands so that they are consistent with the other messages, includes __func__ in the message, and all of the messages include newlines. This patch also removes some debug log messages from tsl2x7x_prox_cal(). Signed-off-by: Brian Masney <masneyb@onstation.org> --- drivers/staging/iio/light/tsl2x7x.c | 58 ++++++++++++++++++++----------------- 1 file changed, 31 insertions(+), 27 deletions(-) diff --git a/drivers/staging/iio/light/tsl2x7x.c b/drivers/staging/iio/light/tsl2x7x.c index da7a4e025083..fb91c46c8747 100644 --- a/drivers/staging/iio/light/tsl2x7x.c +++ b/drivers/staging/iio/light/tsl2x7x.c @@ -378,7 +378,8 @@ static int tsl2x7x_get_lux(struct iio_dev *indio_dev) ret = i2c_smbus_read_byte_data(chip->client, reg); if (ret < 0) { dev_err(&chip->client->dev, - "failed to read. err=%x\n", ret); + "%s: failed to read from register %x: %d\n", + __func__, reg, ret); goto out_unlock; } @@ -424,7 +425,9 @@ static int tsl2x7x_get_lux(struct iio_dev *indio_dev) /* note: lux is 31 bit max at this point */ if (ch1lux > ch0lux) { - dev_dbg(&chip->client->dev, "ch1lux > ch0lux-return last value\n"); + dev_dbg(&chip->client->dev, + "%s: ch1lux > ch0lux; returning last value\n", + __func__); ret = chip->als_cur_info.lux; goto out_unlock; } @@ -600,7 +603,8 @@ static int tsl2x7x_als_calibrate(struct iio_dev *indio_dev) chip->settings.als_gain_trim = ret; dev_info(&chip->client->dev, - "%s als_calibrate completed\n", chip->client->name); + "%s: %s ALS calibration successfully completed\n", + __func__, chip->client->name); return ret; } @@ -644,7 +648,8 @@ static int tsl2x7x_chip_on(struct iio_dev *indio_dev) /* and make sure we're not already on */ if (chip->tsl2x7x_chip_status == TSL2X7X_CHIP_WORKING) { /* if forcing a register update - turn off, then on */ - dev_info(&chip->client->dev, "device is already enabled\n"); + dev_info(&chip->client->dev, "%s: device is already enabled\n", + __func__); return -EINVAL; } @@ -681,12 +686,14 @@ static int tsl2x7x_chip_on(struct iio_dev *indio_dev) */ for (i = 0, dev_reg = chip->tsl2x7x_config; i < TSL2X7X_MAX_CONFIG_REG; i++) { - ret = i2c_smbus_write_byte_data(chip->client, - TSL2X7X_CMD_REG + i, + int reg = TSL2X7X_CMD_REG + i; + + ret = i2c_smbus_write_byte_data(chip->client, reg, *dev_reg++); if (ret < 0) { dev_err(&chip->client->dev, - "failed on write to reg %d.\n", i); + "%s: failed to write to register %x: %d\n", + __func__, reg, ret); return ret; } } @@ -708,7 +715,8 @@ static int tsl2x7x_chip_on(struct iio_dev *indio_dev) chip->tsl2x7x_chip_status = TSL2X7X_CHIP_WORKING; if (chip->settings.interrupts_en != 0) { - dev_info(&chip->client->dev, "Setting Up Interrupt(s)\n"); + dev_info(&chip->client->dev, "%s: Setting up interrupt(s)\n", + __func__); reg_val = TSL2X7X_CNTL_PWR_ON | TSL2X7X_CNTL_ADC_ENBL; if (chip->settings.interrupts_en == 0x20 || @@ -819,8 +827,8 @@ static int tsl2x7x_prox_cal(struct iio_dev *indio_dev) if (chip->settings.prox_max_samples_cal > MAX_SAMPLES_CAL) { dev_err(&chip->client->dev, - "max prox samples cal is too big: %d\n", - chip->settings.prox_max_samples_cal); + "%s: prox_max_samples_cal %d is too big\n", + __func__, chip->settings.prox_max_samples_cal); chip->settings.prox_max_samples_cal = MAX_SAMPLES_CAL; } @@ -845,8 +853,6 @@ static int tsl2x7x_prox_cal(struct iio_dev *indio_dev) if (ret < 0) return ret; prox_history[i] = chip->prox_data; - dev_info(&chip->client->dev, "2 i=%d prox data= %d\n", - i, chip->prox_data); } ret = tsl2x7x_chip_off(indio_dev); @@ -857,12 +863,6 @@ static int tsl2x7x_prox_cal(struct iio_dev *indio_dev) chip->settings.prox_max_samples_cal, cal); chip->settings.prox_thres_high = (cal->max << 1) - cal->mean; - dev_info(&chip->client->dev, " cal min=%d mean=%d max=%d\n", - cal->min, cal->mean, cal->max); - dev_info(&chip->client->dev, - "%s proximity threshold set to %d\n", - chip->client->name, chip->settings.prox_thres_high); - /* back to the way they were */ chip->settings.interrupts_en = tmp_irq_settings; if (current_state == TSL2X7X_CHIP_WORKING) { @@ -1000,12 +1000,14 @@ static ssize_t in_illuminance0_lux_table_store(struct device *dev, n = value[0]; if ((n % 3) || n < 6 || n > ((ARRAY_SIZE(chip->tsl2x7x_device_lux) - 1) * 3)) { - dev_info(dev, "LUX TABLE INPUT ERROR 1 Value[0]=%d\n", n); + dev_info(dev, "%s: lux table input error 1. n=%d\n", + __func__, n); return -EINVAL; } if ((value[(n - 2)] | value[(n - 1)] | value[n]) != 0) { - dev_info(dev, "LUX TABLE INPUT ERROR 2 Value[0]=%d\n", n); + dev_info(dev, "%s: lux table input error 2. n=%d\n", + __func__, n); return -EINVAL; } @@ -1150,14 +1152,14 @@ static int tsl2x7x_write_event_value(struct iio_dev *indio_dev, chip->settings.persistence &= 0xF0; chip->settings.persistence |= (filter_delay & 0x0F); - dev_info(&chip->client->dev, "%s: ALS persistence = %d", + dev_info(&chip->client->dev, "%s: ALS persistence = %d\n", __func__, filter_delay); } else { chip->settings.persistence &= 0x0F; chip->settings.persistence |= ((filter_delay << 4) & 0xF0); dev_info(&chip->client->dev, - "%s: Proximity persistence = %d", + "%s: Proximity persistence = %d\n", __func__, filter_delay); } ret = 0; @@ -1372,7 +1374,7 @@ static int tsl2x7x_write_raw(struct iio_dev *indio_dev, chip->settings.als_time = TSL2X7X_MAX_TIMER_CNT - (val2 / TSL2X7X_MIN_ITIME); - dev_info(&chip->client->dev, "%s: als time = %d", + dev_info(&chip->client->dev, "%s: als time = %d\n", __func__, chip->settings.als_time); break; default: @@ -1738,8 +1740,9 @@ static int tsl2x7x_probe(struct i2c_client *clientp, ret = i2c_smbus_write_byte(clientp, TSL2X7X_CMD_REG | TSL2X7X_CNTRL); if (ret < 0) { - dev_err(&clientp->dev, "write to cmd reg failed. err = %d\n", - ret); + dev_err(&clientp->dev, + "%s: Failed to write to CMD register: %d\n", + __func__, ret); return ret; } @@ -1773,7 +1776,7 @@ static int tsl2x7x_probe(struct i2c_client *clientp, indio_dev); if (ret) { dev_err(&clientp->dev, - "%s: irq request failed", __func__); + "%s: IRQ request failed\n", __func__); return ret; } } @@ -1790,7 +1793,8 @@ static int tsl2x7x_probe(struct i2c_client *clientp, return ret; } - dev_info(&clientp->dev, "%s Light sensor found.\n", id->name); + dev_info(&clientp->dev, "%s: %s Light sensor found.\n", __func__, + id->name); return 0; } -- 2.14.3
The tsl2771 and tmd2771 devices create the in_proximity0_calibscale_available sysfs attribute. These two particular devices do not support changing the proximity gain value on the chip so this patch removes that sysfs attribute. As expected, these two devices already did not create the IIO_CHAN_INFO_CALIBSCALE channel and proximity0_calibrate sysfs attribute. Page 38 of the tsl2772 data sheet shows that the proximity gain can be adjusted with bits 2-3 on the control register: https://ams.com/eng/content/download/291503/1066377/file/TSL2772_DS000181_2-00.pdf Page 35 of the tsl2771 and tmd2771 data sheets shows that bits 2-3 on the control register are reserved and changing the proximity gain is not supported: https://ams.com/eng/content/download/250264/976045/file/TSL2771_DS000105_3-00.pdf https://ams.com/eng/content/download/250283/976077/file/TMD2771_DS000177_2-00.pdf Signed-off-by: Brian Masney <masneyb@onstation.org> --- drivers/staging/iio/light/tsl2x7x.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/staging/iio/light/tsl2x7x.c b/drivers/staging/iio/light/tsl2x7x.c index fb91c46c8747..8c29a52153c1 100644 --- a/drivers/staging/iio/light/tsl2x7x.c +++ b/drivers/staging/iio/light/tsl2x7x.c @@ -1477,7 +1477,6 @@ static struct attribute *tsl2x7x_ALSPRX_device_attrs[] = { &dev_attr_in_illuminance0_target_input.attr, &dev_attr_in_illuminance0_calibrate.attr, &dev_attr_in_illuminance0_lux_table.attr, - &iio_const_attr_in_proximity0_calibscale_available.dev_attr.attr, NULL }; -- 2.14.3
The bits for setting up the proximity diode were not setup correctly and this was causing the proximity sensor to not function correctly. This patch sets up the correct bit mask in tsl2x7x_chip_on() based on what the data sheet expects. From page 35 of the TSL2771 data sheet: https://ams.com/eng/content/download/250264/976045/file/TSL2771_DS000105_3-00.pdf - Bits 0-1 is the ALS gain control - Bits 2-3 is reserved (The proximity gain control on other tsl2x7x chips) - Bits 4-5 is the proximity diode select - Bits 6-7 is the LED drive strength tsl2x7x_chip_on() had the power and diode hardcoded, so these are extracted out into the settings so that these fields can be configured in the platform data. The default prox_gain is changed from 1 (2X gain) to 0 (1X gain) since the proximity gain control on the TSL2771, TMD2771, and other chips have these fields listed as reserved, and to write 0 into those bits. Verified that the proximity sensor now works correctly on a TSL2771 hooked up to a Raspberry Pi 2. Signed-off-by: Brian Masney <masneyb@onstation.org> --- drivers/staging/iio/light/tsl2x7x.c | 25 ++++++++++++++----------- drivers/staging/iio/light/tsl2x7x.h | 2 ++ 2 files changed, 16 insertions(+), 11 deletions(-) diff --git a/drivers/staging/iio/light/tsl2x7x.c b/drivers/staging/iio/light/tsl2x7x.c index 8c29a52153c1..ab518cdec43e 100644 --- a/drivers/staging/iio/light/tsl2x7x.c +++ b/drivers/staging/iio/light/tsl2x7x.c @@ -109,15 +109,15 @@ #define TSL2X7X_CNTL_INTPROXPON_ENBL 0x2F /*Prox diode to use */ -#define TSL2X7X_DIODE0 0x10 -#define TSL2X7X_DIODE1 0x20 -#define TSL2X7X_DIODE_BOTH 0x30 +#define TSL2X7X_DIODE0 0x01 +#define TSL2X7X_DIODE1 0x02 +#define TSL2X7X_DIODE_BOTH 0x03 /* LED Power */ #define TSL2X7X_100_mA 0x00 -#define TSL2X7X_50_mA 0x40 -#define TSL2X7X_25_mA 0x80 -#define TSL2X7X_13_mA 0xD0 +#define TSL2X7X_50_mA 0x01 +#define TSL2X7X_25_mA 0x02 +#define TSL2X7X_13_mA 0x03 #define TSL2X7X_MAX_TIMER_CNT 0xFF #define TSL2X7X_MIN_ITIME 3 @@ -228,7 +228,7 @@ static const struct tsl2x7x_settings tsl2x7x_default_settings = { .als_time = 219, /* 101 ms */ .als_gain = 0, .prx_time = 254, /* 5.4 ms */ - .prox_gain = 1, + .prox_gain = 0, .wait_time = 245, .prox_config = 0, .als_gain_trim = 1000, @@ -240,7 +240,9 @@ static const struct tsl2x7x_settings tsl2x7x_default_settings = { .prox_thres_low = 0, .prox_thres_high = 512, .prox_max_samples_cal = 30, - .prox_pulse_count = 8 + .prox_pulse_count = 8, + .prox_diode = TSL2X7X_DIODE1, + .prox_power = TSL2X7X_100_mA }; static const s16 tsl2x7x_als_gain[] = { @@ -664,9 +666,10 @@ static int tsl2x7x_chip_on(struct iio_dev *indio_dev) /* Set the gain based on tsl2x7x_settings struct */ chip->tsl2x7x_config[TSL2X7X_GAIN] = - chip->settings.als_gain | - (TSL2X7X_100_mA | TSL2X7X_DIODE1) | - (chip->settings.prox_gain << 2); + (chip->settings.als_gain & 0xFF) | + ((chip->settings.prox_gain & 0xFF) << 2) | + (chip->settings.prox_diode << 4) | + (chip->settings.prox_power << 6); /* set chip struct re scaling and saturation */ chip->als_saturation = als_count * 922; /* 90% of full scale */ diff --git a/drivers/staging/iio/light/tsl2x7x.h b/drivers/staging/iio/light/tsl2x7x.h index 6624cbca7a83..28b0e7fdc9b8 100644 --- a/drivers/staging/iio/light/tsl2x7x.h +++ b/drivers/staging/iio/light/tsl2x7x.h @@ -78,6 +78,8 @@ struct tsl2x7x_settings { int prox_thres_high; int prox_pulse_count; int prox_max_samples_cal; + int prox_diode; + int prox_power; }; /** -- 2.14.3
On Sat, 3 Mar 2018 20:49:31 -0500 Brian Masney <masneyb@onstation.org> wrote: > The tsl2X7X_platform_data structure contains the platform_power, > power_on, and power_off function pointers. These power management > functions should not be in the platform data. These functions were > likely used before the regulator framework was put in place. There are > no users of these functions in the mainline kernel. > > Signed-off-by: Brian Masney <masneyb@onstation.org> Applied to the togreg branch of iio.git and pushed out as testing for the autobuilders to play with it. Thanks, Jonathan > --- > drivers/staging/iio/light/tsl2x7x.c | 18 ------------------ > drivers/staging/iio/light/tsl2x7x.h | 4 ---- > 2 files changed, 22 deletions(-) > > diff --git a/drivers/staging/iio/light/tsl2x7x.c b/drivers/staging/iio/light/tsl2x7x.c > index 126e11530ce0..b7e3f966c3a6 100644 > --- a/drivers/staging/iio/light/tsl2x7x.c > +++ b/drivers/staging/iio/light/tsl2x7x.c > @@ -588,9 +588,6 @@ static int tsl2x7x_chip_on(struct iio_dev *indio_dev) > struct tsl2X7X_chip *chip = iio_priv(indio_dev); > u8 reg_val = 0; > > - if (chip->pdata && chip->pdata->power_on) > - chip->pdata->power_on(indio_dev); > - > /* Non calculated parameters */ > chip->tsl2x7x_config[TSL2X7X_PRX_TIME] = chip->settings.prx_time; > chip->tsl2x7x_config[TSL2X7X_WAIT_TIME] = chip->settings.wait_time; > @@ -736,9 +733,6 @@ static int tsl2x7x_chip_off(struct iio_dev *indio_dev) > ret = i2c_smbus_write_byte_data(chip->client, > TSL2X7X_CMD_REG | TSL2X7X_CNTRL, 0x00); > > - if (chip->pdata && chip->pdata->power_off) > - chip->pdata->power_off(chip->client); > - > return ret; > } > > @@ -1792,12 +1786,6 @@ static int tsl2x7x_suspend(struct device *dev) > chip->tsl2x7x_chip_status = TSL2X7X_CHIP_SUSPENDED; > } > > - if (chip->pdata && chip->pdata->platform_power) { > - pm_message_t pmm = {PM_EVENT_SUSPEND}; > - > - chip->pdata->platform_power(dev, pmm); > - } > - > return ret; > } > > @@ -1807,12 +1795,6 @@ static int tsl2x7x_resume(struct device *dev) > struct tsl2X7X_chip *chip = iio_priv(indio_dev); > int ret = 0; > > - if (chip->pdata && chip->pdata->platform_power) { > - pm_message_t pmm = {PM_EVENT_RESUME}; > - > - chip->pdata->platform_power(dev, pmm); > - } > - > if (chip->tsl2x7x_chip_status == TSL2X7X_CHIP_SUSPENDED) > ret = tsl2x7x_chip_on(indio_dev); > > diff --git a/drivers/staging/iio/light/tsl2x7x.h b/drivers/staging/iio/light/tsl2x7x.h > index df00f2ec1719..6624cbca7a83 100644 > --- a/drivers/staging/iio/light/tsl2x7x.h > +++ b/drivers/staging/iio/light/tsl2x7x.h > @@ -21,7 +21,6 @@ > > #ifndef __TSL2X7X_H > #define __TSL2X7X_H > -#include <linux/pm.h> > > struct tsl2x7x_lux { > unsigned int ratio; > @@ -91,9 +90,6 @@ struct tsl2x7x_settings { > * > */ > struct tsl2X7X_platform_data { > - int (*platform_power)(struct device *dev, pm_message_t); > - int (*power_on)(struct iio_dev *indio_dev); > - int (*power_off)(struct i2c_client *dev); > struct tsl2x7x_lux platform_lux_table[TSL2X7X_MAX_LUX_TABLE_SIZE]; > struct tsl2x7x_settings *platform_default_settings; > }; _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
On Sat, 3 Mar 2018 20:49:31 -0500 Brian Masney <masneyb@onstation.org> wrote: > The tsl2X7X_platform_data structure contains the platform_power, > power_on, and power_off function pointers. These power management > functions should not be in the platform data. These functions were > likely used before the regulator framework was put in place. There are > no users of these functions in the mainline kernel. > > Signed-off-by: Brian Masney <masneyb@onstation.org> Applied to the togreg branch of iio.git and pushed out as testing for the autobuilders to play with it. Thanks, Jonathan > --- > drivers/staging/iio/light/tsl2x7x.c | 18 ------------------ > drivers/staging/iio/light/tsl2x7x.h | 4 ---- > 2 files changed, 22 deletions(-) > > diff --git a/drivers/staging/iio/light/tsl2x7x.c b/drivers/staging/iio/light/tsl2x7x.c > index 126e11530ce0..b7e3f966c3a6 100644 > --- a/drivers/staging/iio/light/tsl2x7x.c > +++ b/drivers/staging/iio/light/tsl2x7x.c > @@ -588,9 +588,6 @@ static int tsl2x7x_chip_on(struct iio_dev *indio_dev) > struct tsl2X7X_chip *chip = iio_priv(indio_dev); > u8 reg_val = 0; > > - if (chip->pdata && chip->pdata->power_on) > - chip->pdata->power_on(indio_dev); > - > /* Non calculated parameters */ > chip->tsl2x7x_config[TSL2X7X_PRX_TIME] = chip->settings.prx_time; > chip->tsl2x7x_config[TSL2X7X_WAIT_TIME] = chip->settings.wait_time; > @@ -736,9 +733,6 @@ static int tsl2x7x_chip_off(struct iio_dev *indio_dev) > ret = i2c_smbus_write_byte_data(chip->client, > TSL2X7X_CMD_REG | TSL2X7X_CNTRL, 0x00); > > - if (chip->pdata && chip->pdata->power_off) > - chip->pdata->power_off(chip->client); > - > return ret; > } > > @@ -1792,12 +1786,6 @@ static int tsl2x7x_suspend(struct device *dev) > chip->tsl2x7x_chip_status = TSL2X7X_CHIP_SUSPENDED; > } > > - if (chip->pdata && chip->pdata->platform_power) { > - pm_message_t pmm = {PM_EVENT_SUSPEND}; > - > - chip->pdata->platform_power(dev, pmm); > - } > - > return ret; > } > > @@ -1807,12 +1795,6 @@ static int tsl2x7x_resume(struct device *dev) > struct tsl2X7X_chip *chip = iio_priv(indio_dev); > int ret = 0; > > - if (chip->pdata && chip->pdata->platform_power) { > - pm_message_t pmm = {PM_EVENT_RESUME}; > - > - chip->pdata->platform_power(dev, pmm); > - } > - > if (chip->tsl2x7x_chip_status == TSL2X7X_CHIP_SUSPENDED) > ret = tsl2x7x_chip_on(indio_dev); > > diff --git a/drivers/staging/iio/light/tsl2x7x.h b/drivers/staging/iio/light/tsl2x7x.h > index df00f2ec1719..6624cbca7a83 100644 > --- a/drivers/staging/iio/light/tsl2x7x.h > +++ b/drivers/staging/iio/light/tsl2x7x.h > @@ -21,7 +21,6 @@ > > #ifndef __TSL2X7X_H > #define __TSL2X7X_H > -#include <linux/pm.h> > > struct tsl2x7x_lux { > unsigned int ratio; > @@ -91,9 +90,6 @@ struct tsl2x7x_settings { > * > */ > struct tsl2X7X_platform_data { > - int (*platform_power)(struct device *dev, pm_message_t); > - int (*power_on)(struct iio_dev *indio_dev); > - int (*power_off)(struct i2c_client *dev); > struct tsl2x7x_lux platform_lux_table[TSL2X7X_MAX_LUX_TABLE_SIZE]; > struct tsl2x7x_settings *platform_default_settings; > };
On Sat, 3 Mar 2018 20:49:32 -0500 Brian Masney <masneyb@onstation.org> wrote: > There were three places where the same chunk of code was used to clear > interrupts. This patch creates a common function > tsl2x7x_clear_interrupts() to reduce duplicate code. > > Signed-off-by: Brian Masney <masneyb@onstation.org> Hmm. This one wasn't immediately clear to me but having looked at the datasheet I see the logic in combining these into one function so fair enough. The odd addresses being set as bits within a register had me confused. Applied Thanks, Jonathan > --- > drivers/staging/iio/light/tsl2x7x.c | 52 +++++++++++++++++-------------------- > 1 file changed, 24 insertions(+), 28 deletions(-) > > diff --git a/drivers/staging/iio/light/tsl2x7x.c b/drivers/staging/iio/light/tsl2x7x.c > index b7e3f966c3a6..c02db03ef369 100644 > --- a/drivers/staging/iio/light/tsl2x7x.c > +++ b/drivers/staging/iio/light/tsl2x7x.c > @@ -279,6 +279,20 @@ static const u8 device_channel_config[] = { > ALSPRX2 > }; > > +static int tsl2x7x_clear_interrupts(struct tsl2X7X_chip *chip, int reg) > +{ > + int ret; > + > + ret = i2c_smbus_write_byte(chip->client, > + TSL2X7X_CMD_REG | TSL2X7X_CMD_SPL_FN | reg); > + if (ret < 0) > + dev_err(&chip->client->dev, > + "%s: failed to clear interrupt status %x: %d\n", > + __func__, reg, ret); > + > + return ret; > +} > + > /** > * tsl2x7x_get_lux() - Reads and calculates current lux value. > * @indio_dev: pointer to IIO device > @@ -346,16 +360,9 @@ static int tsl2x7x_get_lux(struct iio_dev *indio_dev) > buf[i] = ret; > } > > - /* clear any existing interrupt status */ > - ret = i2c_smbus_write_byte(chip->client, > - TSL2X7X_CMD_REG | > - TSL2X7X_CMD_SPL_FN | > - TSL2X7X_CMD_ALS_INT_CLR); > - if (ret < 0) { > - dev_err(&chip->client->dev, > - "i2c_write_command failed - err = %d\n", ret); > - goto out_unlock; /* have no data, so return failure */ > - } > + ret = tsl2x7x_clear_interrupts(chip, TSL2X7X_CMD_ALS_INT_CLR); > + if (ret < 0) > + goto out_unlock; > > /* extract ALS/lux data */ > ch0 = le16_to_cpup((const __le16 *)&buf[0]); > @@ -706,17 +713,10 @@ static int tsl2x7x_chip_on(struct iio_dev *indio_dev) > "%s: failed in tsl2x7x_IOCTL_INT_SET.\n", > __func__); > > - /* Clear out any initial interrupts */ > - ret = i2c_smbus_write_byte(chip->client, > - TSL2X7X_CMD_REG | > - TSL2X7X_CMD_SPL_FN | > - TSL2X7X_CMD_PROXALS_INT_CLR); > - if (ret < 0) { > - dev_err(&chip->client->dev, > - "%s: Failed to clear Int status\n", > - __func__); > - return ret; > - } > + ret = tsl2x7x_clear_interrupts(chip, > + TSL2X7X_CMD_PROXALS_INT_CLR); > + if (ret < 0) > + return ret; > } > > return ret; > @@ -1421,14 +1421,10 @@ static irqreturn_t tsl2x7x_event_handler(int irq, void *private) > IIO_EV_DIR_EITHER), > timestamp); > } > - /* Clear interrupt now that we have handled it. */ > - ret = i2c_smbus_write_byte(chip->client, > - TSL2X7X_CMD_REG | TSL2X7X_CMD_SPL_FN | > - TSL2X7X_CMD_PROXALS_INT_CLR); > + > + ret = tsl2x7x_clear_interrupts(chip, TSL2X7X_CMD_PROXALS_INT_CLR); > if (ret < 0) > - dev_err(&chip->client->dev, > - "Failed to clear irq from event handler. err = %d\n", > - ret); > + return ret; > > return IRQ_HANDLED; > } _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
On Sat, 3 Mar 2018 20:49:32 -0500 Brian Masney <masneyb@onstation.org> wrote: > There were three places where the same chunk of code was used to clear > interrupts. This patch creates a common function > tsl2x7x_clear_interrupts() to reduce duplicate code. > > Signed-off-by: Brian Masney <masneyb@onstation.org> Hmm. This one wasn't immediately clear to me but having looked at the datasheet I see the logic in combining these into one function so fair enough. The odd addresses being set as bits within a register had me confused. Applied Thanks, Jonathan > --- > drivers/staging/iio/light/tsl2x7x.c | 52 +++++++++++++++++-------------------- > 1 file changed, 24 insertions(+), 28 deletions(-) > > diff --git a/drivers/staging/iio/light/tsl2x7x.c b/drivers/staging/iio/light/tsl2x7x.c > index b7e3f966c3a6..c02db03ef369 100644 > --- a/drivers/staging/iio/light/tsl2x7x.c > +++ b/drivers/staging/iio/light/tsl2x7x.c > @@ -279,6 +279,20 @@ static const u8 device_channel_config[] = { > ALSPRX2 > }; > > +static int tsl2x7x_clear_interrupts(struct tsl2X7X_chip *chip, int reg) > +{ > + int ret; > + > + ret = i2c_smbus_write_byte(chip->client, > + TSL2X7X_CMD_REG | TSL2X7X_CMD_SPL_FN | reg); > + if (ret < 0) > + dev_err(&chip->client->dev, > + "%s: failed to clear interrupt status %x: %d\n", > + __func__, reg, ret); > + > + return ret; > +} > + > /** > * tsl2x7x_get_lux() - Reads and calculates current lux value. > * @indio_dev: pointer to IIO device > @@ -346,16 +360,9 @@ static int tsl2x7x_get_lux(struct iio_dev *indio_dev) > buf[i] = ret; > } > > - /* clear any existing interrupt status */ > - ret = i2c_smbus_write_byte(chip->client, > - TSL2X7X_CMD_REG | > - TSL2X7X_CMD_SPL_FN | > - TSL2X7X_CMD_ALS_INT_CLR); > - if (ret < 0) { > - dev_err(&chip->client->dev, > - "i2c_write_command failed - err = %d\n", ret); > - goto out_unlock; /* have no data, so return failure */ > - } > + ret = tsl2x7x_clear_interrupts(chip, TSL2X7X_CMD_ALS_INT_CLR); > + if (ret < 0) > + goto out_unlock; > > /* extract ALS/lux data */ > ch0 = le16_to_cpup((const __le16 *)&buf[0]); > @@ -706,17 +713,10 @@ static int tsl2x7x_chip_on(struct iio_dev *indio_dev) > "%s: failed in tsl2x7x_IOCTL_INT_SET.\n", > __func__); > > - /* Clear out any initial interrupts */ > - ret = i2c_smbus_write_byte(chip->client, > - TSL2X7X_CMD_REG | > - TSL2X7X_CMD_SPL_FN | > - TSL2X7X_CMD_PROXALS_INT_CLR); > - if (ret < 0) { > - dev_err(&chip->client->dev, > - "%s: Failed to clear Int status\n", > - __func__); > - return ret; > - } > + ret = tsl2x7x_clear_interrupts(chip, > + TSL2X7X_CMD_PROXALS_INT_CLR); > + if (ret < 0) > + return ret; > } > > return ret; > @@ -1421,14 +1421,10 @@ static irqreturn_t tsl2x7x_event_handler(int irq, void *private) > IIO_EV_DIR_EITHER), > timestamp); > } > - /* Clear interrupt now that we have handled it. */ > - ret = i2c_smbus_write_byte(chip->client, > - TSL2X7X_CMD_REG | TSL2X7X_CMD_SPL_FN | > - TSL2X7X_CMD_PROXALS_INT_CLR); > + > + ret = tsl2x7x_clear_interrupts(chip, TSL2X7X_CMD_PROXALS_INT_CLR); > if (ret < 0) > - dev_err(&chip->client->dev, > - "Failed to clear irq from event handler. err = %d\n", > - ret); > + return ret; > > return IRQ_HANDLED; > }
On Sat, 3 Mar 2018 20:49:33 -0500 Brian Masney <masneyb@onstation.org> wrote: > There were three places where the same chunk of code was used to read > the chip status. This patch creates a common function > tsl2x7x_read_status() to reduce duplicate code. This patch also corrects > tsl2x7x_event_handler() to properly check for an error after reading the > chip status. > > Signed-off-by: Brian Masney <masneyb@onstation.org> Applied. > --- > drivers/staging/iio/light/tsl2x7x.c | 40 ++++++++++++++++++++++--------------- > 1 file changed, 24 insertions(+), 16 deletions(-) > > diff --git a/drivers/staging/iio/light/tsl2x7x.c b/drivers/staging/iio/light/tsl2x7x.c > index c02db03ef369..6bb622816660 100644 > --- a/drivers/staging/iio/light/tsl2x7x.c > +++ b/drivers/staging/iio/light/tsl2x7x.c > @@ -293,6 +293,20 @@ static int tsl2x7x_clear_interrupts(struct tsl2X7X_chip *chip, int reg) > return ret; > } > > +static int tsl2x7x_read_status(struct tsl2X7X_chip *chip) > +{ > + int ret; > + > + ret = i2c_smbus_read_byte_data(chip->client, > + TSL2X7X_CMD_REG | TSL2X7X_STATUS); > + if (ret < 0) > + dev_err(&chip->client->dev, > + "%s: failed to read STATUS register: %d\n", __func__, > + ret); > + > + return ret; > +} > + > /** > * tsl2x7x_get_lux() - Reads and calculates current lux value. > * @indio_dev: pointer to IIO device > @@ -332,13 +346,10 @@ static int tsl2x7x_get_lux(struct iio_dev *indio_dev) > goto out_unlock; > } > > - ret = i2c_smbus_read_byte_data(chip->client, > - TSL2X7X_CMD_REG | TSL2X7X_STATUS); > - if (ret < 0) { > - dev_err(&chip->client->dev, > - "%s: Failed to read STATUS Reg\n", __func__); > + ret = tsl2x7x_read_status(chip); > + if (ret < 0) > goto out_unlock; > - } > + > /* is data new & valid */ > if (!(ret & TSL2X7X_STA_ADC_VALID)) { > dev_err(&chip->client->dev, > @@ -458,12 +469,9 @@ static int tsl2x7x_get_prox(struct iio_dev *indio_dev) > return -EBUSY; > } > > - ret = i2c_smbus_read_byte_data(chip->client, > - TSL2X7X_CMD_REG | TSL2X7X_STATUS); > - if (ret < 0) { > - dev_err(&chip->client->dev, "i2c err=%d\n", ret); > + ret = tsl2x7x_read_status(chip); > + if (ret < 0) > goto prox_poll_err; > - } > > switch (chip->id) { > case tsl2571: > @@ -1396,13 +1404,13 @@ static irqreturn_t tsl2x7x_event_handler(int irq, void *private) > struct tsl2X7X_chip *chip = iio_priv(indio_dev); > s64 timestamp = iio_get_time_ns(indio_dev); > int ret; > - u8 value; > > - value = i2c_smbus_read_byte_data(chip->client, > - TSL2X7X_CMD_REG | TSL2X7X_STATUS); > + ret = tsl2x7x_read_status(chip); > + if (ret < 0) > + return ret; > > /* What type of interrupt do we need to process */ > - if (value & TSL2X7X_STA_PRX_INTR) { > + if (ret & TSL2X7X_STA_PRX_INTR) { > tsl2x7x_get_prox(indio_dev); /* freshen data for ABI */ > iio_push_event(indio_dev, > IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, > @@ -1412,7 +1420,7 @@ static irqreturn_t tsl2x7x_event_handler(int irq, void *private) > timestamp); > } > > - if (value & TSL2X7X_STA_ALS_INTR) { > + if (ret & TSL2X7X_STA_ALS_INTR) { > tsl2x7x_get_lux(indio_dev); /* freshen data for ABI */ > iio_push_event(indio_dev, > IIO_UNMOD_EVENT_CODE(IIO_LIGHT, _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
On Sat, 3 Mar 2018 20:49:33 -0500 Brian Masney <masneyb@onstation.org> wrote: > There were three places where the same chunk of code was used to read > the chip status. This patch creates a common function > tsl2x7x_read_status() to reduce duplicate code. This patch also corrects > tsl2x7x_event_handler() to properly check for an error after reading the > chip status. > > Signed-off-by: Brian Masney <masneyb@onstation.org> Applied. > --- > drivers/staging/iio/light/tsl2x7x.c | 40 ++++++++++++++++++++++--------------- > 1 file changed, 24 insertions(+), 16 deletions(-) > > diff --git a/drivers/staging/iio/light/tsl2x7x.c b/drivers/staging/iio/light/tsl2x7x.c > index c02db03ef369..6bb622816660 100644 > --- a/drivers/staging/iio/light/tsl2x7x.c > +++ b/drivers/staging/iio/light/tsl2x7x.c > @@ -293,6 +293,20 @@ static int tsl2x7x_clear_interrupts(struct tsl2X7X_chip *chip, int reg) > return ret; > } > > +static int tsl2x7x_read_status(struct tsl2X7X_chip *chip) > +{ > + int ret; > + > + ret = i2c_smbus_read_byte_data(chip->client, > + TSL2X7X_CMD_REG | TSL2X7X_STATUS); > + if (ret < 0) > + dev_err(&chip->client->dev, > + "%s: failed to read STATUS register: %d\n", __func__, > + ret); > + > + return ret; > +} > + > /** > * tsl2x7x_get_lux() - Reads and calculates current lux value. > * @indio_dev: pointer to IIO device > @@ -332,13 +346,10 @@ static int tsl2x7x_get_lux(struct iio_dev *indio_dev) > goto out_unlock; > } > > - ret = i2c_smbus_read_byte_data(chip->client, > - TSL2X7X_CMD_REG | TSL2X7X_STATUS); > - if (ret < 0) { > - dev_err(&chip->client->dev, > - "%s: Failed to read STATUS Reg\n", __func__); > + ret = tsl2x7x_read_status(chip); > + if (ret < 0) > goto out_unlock; > - } > + > /* is data new & valid */ > if (!(ret & TSL2X7X_STA_ADC_VALID)) { > dev_err(&chip->client->dev, > @@ -458,12 +469,9 @@ static int tsl2x7x_get_prox(struct iio_dev *indio_dev) > return -EBUSY; > } > > - ret = i2c_smbus_read_byte_data(chip->client, > - TSL2X7X_CMD_REG | TSL2X7X_STATUS); > - if (ret < 0) { > - dev_err(&chip->client->dev, "i2c err=%d\n", ret); > + ret = tsl2x7x_read_status(chip); > + if (ret < 0) > goto prox_poll_err; > - } > > switch (chip->id) { > case tsl2571: > @@ -1396,13 +1404,13 @@ static irqreturn_t tsl2x7x_event_handler(int irq, void *private) > struct tsl2X7X_chip *chip = iio_priv(indio_dev); > s64 timestamp = iio_get_time_ns(indio_dev); > int ret; > - u8 value; > > - value = i2c_smbus_read_byte_data(chip->client, > - TSL2X7X_CMD_REG | TSL2X7X_STATUS); > + ret = tsl2x7x_read_status(chip); > + if (ret < 0) > + return ret; > > /* What type of interrupt do we need to process */ > - if (value & TSL2X7X_STA_PRX_INTR) { > + if (ret & TSL2X7X_STA_PRX_INTR) { > tsl2x7x_get_prox(indio_dev); /* freshen data for ABI */ > iio_push_event(indio_dev, > IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, > @@ -1412,7 +1420,7 @@ static irqreturn_t tsl2x7x_event_handler(int irq, void *private) > timestamp); > } > > - if (value & TSL2X7X_STA_ALS_INTR) { > + if (ret & TSL2X7X_STA_ALS_INTR) { > tsl2x7x_get_lux(indio_dev); /* freshen data for ABI */ > iio_push_event(indio_dev, > IIO_UNMOD_EVENT_CODE(IIO_LIGHT,
On Sat, 3 Mar 2018 20:49:34 -0500 Brian Masney <masneyb@onstation.org> wrote: > There were four places where the same chunk of code was used to write > to the control register. This patch creates a common function > tsl2x7x_write_control_reg() to reduce duplicate code. > > The function tsl2x7x_chip_off() did not correctly return an error > code so this patch also corrects that issue. > > Signed-off-by: Brian Masney <masneyb@onstation.org> Hmm. These are getting a bit marginal in benefit but this one just makes it over the barrier. The reduction in complexity is really very small... Applied. Jonathan > --- > drivers/staging/iio/light/tsl2x7x.c | 54 +++++++++++++++++-------------------- > 1 file changed, 25 insertions(+), 29 deletions(-) > > diff --git a/drivers/staging/iio/light/tsl2x7x.c b/drivers/staging/iio/light/tsl2x7x.c > index 6bb622816660..cf16dd206c0b 100644 > --- a/drivers/staging/iio/light/tsl2x7x.c > +++ b/drivers/staging/iio/light/tsl2x7x.c > @@ -307,6 +307,21 @@ static int tsl2x7x_read_status(struct tsl2X7X_chip *chip) > return ret; > } > > +static int tsl2x7x_write_control_reg(struct tsl2X7X_chip *chip, u8 data) > +{ > + int ret; > + > + ret = i2c_smbus_write_byte_data(chip->client, > + TSL2X7X_CMD_REG | TSL2X7X_CNTRL, data); > + if (ret < 0) { > + dev_err(&chip->client->dev, > + "%s: failed to write to control register %x: %d\n", > + __func__, data, ret); > + } > + > + return ret; > +} > + > /** > * tsl2x7x_get_lux() - Reads and calculates current lux value. > * @indio_dev: pointer to IIO device > @@ -597,7 +612,6 @@ static int tsl2x7x_chip_on(struct iio_dev *indio_dev) > int i; > int ret = 0; > u8 *dev_reg; > - u8 utmp; > int als_count; > int als_time; > struct tsl2X7X_chip *chip = iio_priv(indio_dev); > @@ -659,14 +673,9 @@ static int tsl2x7x_chip_on(struct iio_dev *indio_dev) > * TSL2X7X Specific power-on / adc enable sequence > * Power on the device 1st. > */ > - utmp = TSL2X7X_CNTL_PWR_ON; > - ret = i2c_smbus_write_byte_data(chip->client, > - TSL2X7X_CMD_REG | TSL2X7X_CNTRL, utmp); > - if (ret < 0) { > - dev_err(&chip->client->dev, > - "%s: failed on CNTRL reg.\n", __func__); > + ret = tsl2x7x_write_control_reg(chip, TSL2X7X_CNTL_PWR_ON); > + if (ret < 0) > return ret; > - } > > /* > * Use the following shadow copy for our delay before enabling ADC. > @@ -691,16 +700,12 @@ static int tsl2x7x_chip_on(struct iio_dev *indio_dev) > * NOW enable the ADC > * initialize the desired mode of operation > */ > - utmp = TSL2X7X_CNTL_PWR_ON | > - TSL2X7X_CNTL_ADC_ENBL | > - TSL2X7X_CNTL_PROX_DET_ENBL; > - ret = i2c_smbus_write_byte_data(chip->client, > - TSL2X7X_CMD_REG | TSL2X7X_CNTRL, utmp); > - if (ret < 0) { > - dev_err(&chip->client->dev, > - "%s: failed on 2nd CTRL reg.\n", __func__); > + ret = tsl2x7x_write_control_reg(chip, > + TSL2X7X_CNTL_PWR_ON | > + TSL2X7X_CNTL_ADC_ENBL | > + TSL2X7X_CNTL_PROX_DET_ENBL); > + if (ret < 0) > return ret; > - } > > chip->tsl2x7x_chip_status = TSL2X7X_CHIP_WORKING; > > @@ -713,13 +718,9 @@ static int tsl2x7x_chip_on(struct iio_dev *indio_dev) > reg_val |= TSL2X7X_CNTL_PROX_DET_ENBL; > > reg_val |= chip->settings.interrupts_en; > - ret = i2c_smbus_write_byte_data(chip->client, > - TSL2X7X_CMD_REG | TSL2X7X_CNTRL, > - reg_val); > + ret = tsl2x7x_write_control_reg(chip, reg_val); > if (ret < 0) > - dev_err(&chip->client->dev, > - "%s: failed in tsl2x7x_IOCTL_INT_SET.\n", > - __func__); > + return ret; > > ret = tsl2x7x_clear_interrupts(chip, > TSL2X7X_CMD_PROXALS_INT_CLR); > @@ -732,16 +733,11 @@ static int tsl2x7x_chip_on(struct iio_dev *indio_dev) > > static int tsl2x7x_chip_off(struct iio_dev *indio_dev) > { > - int ret; > struct tsl2X7X_chip *chip = iio_priv(indio_dev); > > /* turn device off */ > chip->tsl2x7x_chip_status = TSL2X7X_CHIP_SUSPENDED; > - > - ret = i2c_smbus_write_byte_data(chip->client, > - TSL2X7X_CMD_REG | TSL2X7X_CNTRL, 0x00); > - > - return ret; > + return tsl2x7x_write_control_reg(chip, 0x00); > } > > /** _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
On Sat, 3 Mar 2018 20:49:34 -0500 Brian Masney <masneyb@onstation.org> wrote: > There were four places where the same chunk of code was used to write > to the control register. This patch creates a common function > tsl2x7x_write_control_reg() to reduce duplicate code. > > The function tsl2x7x_chip_off() did not correctly return an error > code so this patch also corrects that issue. > > Signed-off-by: Brian Masney <masneyb@onstation.org> Hmm. These are getting a bit marginal in benefit but this one just makes it over the barrier. The reduction in complexity is really very small... Applied. Jonathan > --- > drivers/staging/iio/light/tsl2x7x.c | 54 +++++++++++++++++-------------------- > 1 file changed, 25 insertions(+), 29 deletions(-) > > diff --git a/drivers/staging/iio/light/tsl2x7x.c b/drivers/staging/iio/light/tsl2x7x.c > index 6bb622816660..cf16dd206c0b 100644 > --- a/drivers/staging/iio/light/tsl2x7x.c > +++ b/drivers/staging/iio/light/tsl2x7x.c > @@ -307,6 +307,21 @@ static int tsl2x7x_read_status(struct tsl2X7X_chip *chip) > return ret; > } > > +static int tsl2x7x_write_control_reg(struct tsl2X7X_chip *chip, u8 data) > +{ > + int ret; > + > + ret = i2c_smbus_write_byte_data(chip->client, > + TSL2X7X_CMD_REG | TSL2X7X_CNTRL, data); > + if (ret < 0) { > + dev_err(&chip->client->dev, > + "%s: failed to write to control register %x: %d\n", > + __func__, data, ret); > + } > + > + return ret; > +} > + > /** > * tsl2x7x_get_lux() - Reads and calculates current lux value. > * @indio_dev: pointer to IIO device > @@ -597,7 +612,6 @@ static int tsl2x7x_chip_on(struct iio_dev *indio_dev) > int i; > int ret = 0; > u8 *dev_reg; > - u8 utmp; > int als_count; > int als_time; > struct tsl2X7X_chip *chip = iio_priv(indio_dev); > @@ -659,14 +673,9 @@ static int tsl2x7x_chip_on(struct iio_dev *indio_dev) > * TSL2X7X Specific power-on / adc enable sequence > * Power on the device 1st. > */ > - utmp = TSL2X7X_CNTL_PWR_ON; > - ret = i2c_smbus_write_byte_data(chip->client, > - TSL2X7X_CMD_REG | TSL2X7X_CNTRL, utmp); > - if (ret < 0) { > - dev_err(&chip->client->dev, > - "%s: failed on CNTRL reg.\n", __func__); > + ret = tsl2x7x_write_control_reg(chip, TSL2X7X_CNTL_PWR_ON); > + if (ret < 0) > return ret; > - } > > /* > * Use the following shadow copy for our delay before enabling ADC. > @@ -691,16 +700,12 @@ static int tsl2x7x_chip_on(struct iio_dev *indio_dev) > * NOW enable the ADC > * initialize the desired mode of operation > */ > - utmp = TSL2X7X_CNTL_PWR_ON | > - TSL2X7X_CNTL_ADC_ENBL | > - TSL2X7X_CNTL_PROX_DET_ENBL; > - ret = i2c_smbus_write_byte_data(chip->client, > - TSL2X7X_CMD_REG | TSL2X7X_CNTRL, utmp); > - if (ret < 0) { > - dev_err(&chip->client->dev, > - "%s: failed on 2nd CTRL reg.\n", __func__); > + ret = tsl2x7x_write_control_reg(chip, > + TSL2X7X_CNTL_PWR_ON | > + TSL2X7X_CNTL_ADC_ENBL | > + TSL2X7X_CNTL_PROX_DET_ENBL); > + if (ret < 0) > return ret; > - } > > chip->tsl2x7x_chip_status = TSL2X7X_CHIP_WORKING; > > @@ -713,13 +718,9 @@ static int tsl2x7x_chip_on(struct iio_dev *indio_dev) > reg_val |= TSL2X7X_CNTL_PROX_DET_ENBL; > > reg_val |= chip->settings.interrupts_en; > - ret = i2c_smbus_write_byte_data(chip->client, > - TSL2X7X_CMD_REG | TSL2X7X_CNTRL, > - reg_val); > + ret = tsl2x7x_write_control_reg(chip, reg_val); > if (ret < 0) > - dev_err(&chip->client->dev, > - "%s: failed in tsl2x7x_IOCTL_INT_SET.\n", > - __func__); > + return ret; > > ret = tsl2x7x_clear_interrupts(chip, > TSL2X7X_CMD_PROXALS_INT_CLR); > @@ -732,16 +733,11 @@ static int tsl2x7x_chip_on(struct iio_dev *indio_dev) > > static int tsl2x7x_chip_off(struct iio_dev *indio_dev) > { > - int ret; > struct tsl2X7X_chip *chip = iio_priv(indio_dev); > > /* turn device off */ > chip->tsl2x7x_chip_status = TSL2X7X_CHIP_SUSPENDED; > - > - ret = i2c_smbus_write_byte_data(chip->client, > - TSL2X7X_CMD_REG | TSL2X7X_CNTRL, 0x00); > - > - return ret; > + return tsl2x7x_write_control_reg(chip, 0x00); > } > > /**
On Sat, 3 Mar 2018 20:49:35 -0500 Brian Masney <masneyb@onstation.org> wrote: > The driver uses mutex_lock() and mutex_trylock() in several places. > Convert the mutex_trylock() to mutex_lock() for consistency with other > IIO light drivers in mainline. > > Signed-off-by: Brian Masney <masneyb@onstation.org> This was a little odd given we don't lock the device for long and all this will do is slow down userspace access if it tries several at the same time (a fairy uncommon usecase). Applied Thanks, Jonathan > --- > drivers/staging/iio/light/tsl2x7x.c | 9 ++------- > 1 file changed, 2 insertions(+), 7 deletions(-) > > diff --git a/drivers/staging/iio/light/tsl2x7x.c b/drivers/staging/iio/light/tsl2x7x.c > index cf16dd206c0b..5c611250127f 100644 > --- a/drivers/staging/iio/light/tsl2x7x.c > +++ b/drivers/staging/iio/light/tsl2x7x.c > @@ -350,8 +350,7 @@ static int tsl2x7x_get_lux(struct iio_dev *indio_dev) > u32 ch0lux = 0; > u32 ch1lux = 0; > > - if (mutex_trylock(&chip->als_mutex) == 0) > - return chip->als_cur_info.lux; /* busy, so return LAST VALUE */ > + mutex_lock(&chip->als_mutex); > > if (chip->tsl2x7x_chip_status != TSL2X7X_CHIP_WORKING) { > /* device is not enabled */ > @@ -478,11 +477,7 @@ static int tsl2x7x_get_prox(struct iio_dev *indio_dev) > u8 chdata[2]; > struct tsl2X7X_chip *chip = iio_priv(indio_dev); > > - if (mutex_trylock(&chip->prox_mutex) == 0) { > - dev_err(&chip->client->dev, > - "%s: Can't get prox mutex\n", __func__); > - return -EBUSY; > - } > + mutex_lock(&chip->prox_mutex); > > ret = tsl2x7x_read_status(chip); > if (ret < 0) _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
On Sat, 3 Mar 2018 20:49:35 -0500 Brian Masney <masneyb@onstation.org> wrote: > The driver uses mutex_lock() and mutex_trylock() in several places. > Convert the mutex_trylock() to mutex_lock() for consistency with other > IIO light drivers in mainline. > > Signed-off-by: Brian Masney <masneyb@onstation.org> This was a little odd given we don't lock the device for long and all this will do is slow down userspace access if it tries several at the same time (a fairy uncommon usecase). Applied Thanks, Jonathan > --- > drivers/staging/iio/light/tsl2x7x.c | 9 ++------- > 1 file changed, 2 insertions(+), 7 deletions(-) > > diff --git a/drivers/staging/iio/light/tsl2x7x.c b/drivers/staging/iio/light/tsl2x7x.c > index cf16dd206c0b..5c611250127f 100644 > --- a/drivers/staging/iio/light/tsl2x7x.c > +++ b/drivers/staging/iio/light/tsl2x7x.c > @@ -350,8 +350,7 @@ static int tsl2x7x_get_lux(struct iio_dev *indio_dev) > u32 ch0lux = 0; > u32 ch1lux = 0; > > - if (mutex_trylock(&chip->als_mutex) == 0) > - return chip->als_cur_info.lux; /* busy, so return LAST VALUE */ > + mutex_lock(&chip->als_mutex); > > if (chip->tsl2x7x_chip_status != TSL2X7X_CHIP_WORKING) { > /* device is not enabled */ > @@ -478,11 +477,7 @@ static int tsl2x7x_get_prox(struct iio_dev *indio_dev) > u8 chdata[2]; > struct tsl2X7X_chip *chip = iio_priv(indio_dev); > > - if (mutex_trylock(&chip->prox_mutex) == 0) { > - dev_err(&chip->client->dev, > - "%s: Can't get prox mutex\n", __func__); > - return -EBUSY; > - } > + mutex_lock(&chip->prox_mutex); > > ret = tsl2x7x_read_status(chip); > if (ret < 0)
On Sat, 3 Mar 2018 20:49:36 -0500 Brian Masney <masneyb@onstation.org> wrote: > Not all errors that occurred in tsl2x7x_get_prox() were correctly > reported in the return value. This patch changes the error handling > so that errors are now returned properly. > > Note that the ret variable is from the call to tsl2x7x_read_status(), > and it already has the correct error check. The -EINVAL error code is > for an unexpected value in the register. > > This patch also corrects an unnecessary word wrap in the call to > le16_to_cpup() while changes are being made to this code. > > Signed-off-by: Brian Masney <masneyb@onstation.org> Good cleanup. Applied to the togreg branch of iio.git and pushed out as testing for the autobuilders to work their magic. Thanks, Jonathan > --- > drivers/staging/iio/light/tsl2x7x.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/drivers/staging/iio/light/tsl2x7x.c b/drivers/staging/iio/light/tsl2x7x.c > index 5c611250127f..cc209b64ed5a 100644 > --- a/drivers/staging/iio/light/tsl2x7x.c > +++ b/drivers/staging/iio/light/tsl2x7x.c > @@ -489,16 +489,20 @@ static int tsl2x7x_get_prox(struct iio_dev *indio_dev) > case tmd2671: > case tsl2771: > case tmd2771: > - if (!(ret & TSL2X7X_STA_ADC_VALID)) > + if (!(ret & TSL2X7X_STA_ADC_VALID)) { > + ret = -EINVAL; > goto prox_poll_err; > + } > break; > case tsl2572: > case tsl2672: > case tmd2672: > case tsl2772: > case tmd2772: > - if (!(ret & TSL2X7X_STA_PRX_VALID)) > + if (!(ret & TSL2X7X_STA_PRX_VALID)) { > + ret = -EINVAL; > goto prox_poll_err; > + } > break; > } > > @@ -512,14 +516,13 @@ static int tsl2x7x_get_prox(struct iio_dev *indio_dev) > chdata[i] = ret; > } > > - chip->prox_data = > - le16_to_cpup((const __le16 *)&chdata[0]); > + chip->prox_data = le16_to_cpup((const __le16 *)&chdata[0]); > + ret = chip->prox_data; > > prox_poll_err: > - > mutex_unlock(&chip->prox_mutex); > > - return chip->prox_data; > + return ret; > } > > /** _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
On Sat, 3 Mar 2018 20:49:36 -0500 Brian Masney <masneyb@onstation.org> wrote: > Not all errors that occurred in tsl2x7x_get_prox() were correctly > reported in the return value. This patch changes the error handling > so that errors are now returned properly. > > Note that the ret variable is from the call to tsl2x7x_read_status(), > and it already has the correct error check. The -EINVAL error code is > for an unexpected value in the register. > > This patch also corrects an unnecessary word wrap in the call to > le16_to_cpup() while changes are being made to this code. > > Signed-off-by: Brian Masney <masneyb@onstation.org> Good cleanup. Applied to the togreg branch of iio.git and pushed out as testing for the autobuilders to work their magic. Thanks, Jonathan > --- > drivers/staging/iio/light/tsl2x7x.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/drivers/staging/iio/light/tsl2x7x.c b/drivers/staging/iio/light/tsl2x7x.c > index 5c611250127f..cc209b64ed5a 100644 > --- a/drivers/staging/iio/light/tsl2x7x.c > +++ b/drivers/staging/iio/light/tsl2x7x.c > @@ -489,16 +489,20 @@ static int tsl2x7x_get_prox(struct iio_dev *indio_dev) > case tmd2671: > case tsl2771: > case tmd2771: > - if (!(ret & TSL2X7X_STA_ADC_VALID)) > + if (!(ret & TSL2X7X_STA_ADC_VALID)) { > + ret = -EINVAL; > goto prox_poll_err; > + } > break; > case tsl2572: > case tsl2672: > case tmd2672: > case tsl2772: > case tmd2772: > - if (!(ret & TSL2X7X_STA_PRX_VALID)) > + if (!(ret & TSL2X7X_STA_PRX_VALID)) { > + ret = -EINVAL; > goto prox_poll_err; > + } > break; > } > > @@ -512,14 +516,13 @@ static int tsl2x7x_get_prox(struct iio_dev *indio_dev) > chdata[i] = ret; > } > > - chip->prox_data = > - le16_to_cpup((const __le16 *)&chdata[0]); > + chip->prox_data = le16_to_cpup((const __le16 *)&chdata[0]); > + ret = chip->prox_data; > > prox_poll_err: > - > mutex_unlock(&chip->prox_mutex); > > - return chip->prox_data; > + return ret; > } > > /**
On Sat, 3 Mar 2018 20:49:37 -0500 Brian Masney <masneyb@onstation.org> wrote: > The statP and calP variables triggered an 'Avoid CamelCase' warning > from checkpatch.pl. This patch renames these variables to stat and cal > to fix this warning. > > Signed-off-by: Brian Masney <masneyb@onstation.org> Applied. > --- > drivers/staging/iio/light/tsl2x7x.c | 26 +++++++++++++------------- > 1 file changed, 13 insertions(+), 13 deletions(-) > > diff --git a/drivers/staging/iio/light/tsl2x7x.c b/drivers/staging/iio/light/tsl2x7x.c > index cc209b64ed5a..380a6c96a69a 100644 > --- a/drivers/staging/iio/light/tsl2x7x.c > +++ b/drivers/staging/iio/light/tsl2x7x.c > @@ -773,7 +773,7 @@ static int tsl2x7x_invoke_change(struct iio_dev *indio_dev) > } > > static void tsl2x7x_prox_calculate(int *data, int length, > - struct tsl2x7x_prox_stat *statP) > + struct tsl2x7x_prox_stat *stat) > { > int i; > int sample_sum; > @@ -783,21 +783,21 @@ static void tsl2x7x_prox_calculate(int *data, int length, > length = 1; > > sample_sum = 0; > - statP->min = INT_MAX; > - statP->max = INT_MIN; > + stat->min = INT_MAX; > + stat->max = INT_MIN; > for (i = 0; i < length; i++) { > sample_sum += data[i]; > - statP->min = min(statP->min, data[i]); > - statP->max = max(statP->max, data[i]); > + stat->min = min(stat->min, data[i]); > + stat->max = max(stat->max, data[i]); > } > > - statP->mean = sample_sum / length; > + stat->mean = sample_sum / length; > sample_sum = 0; > for (i = 0; i < length; i++) { > - tmp = data[i] - statP->mean; > + tmp = data[i] - stat->mean; > sample_sum += tmp * tmp; > } > - statP->stddev = int_sqrt((long)sample_sum / length); > + stat->stddev = int_sqrt((long)sample_sum / length); > } > > /** > @@ -812,7 +812,7 @@ static void tsl2x7x_prox_cal(struct iio_dev *indio_dev) > int prox_history[MAX_SAMPLES_CAL + 1]; > int i; > struct tsl2x7x_prox_stat prox_stat_data[2]; > - struct tsl2x7x_prox_stat *calP; > + struct tsl2x7x_prox_stat *cal; > struct tsl2X7X_chip *chip = iio_priv(indio_dev); > u8 tmp_irq_settings; > u8 current_state = chip->tsl2x7x_chip_status; > @@ -844,13 +844,13 @@ static void tsl2x7x_prox_cal(struct iio_dev *indio_dev) > } > > tsl2x7x_chip_off(indio_dev); > - calP = &prox_stat_data[PROX_STAT_CAL]; > + cal = &prox_stat_data[PROX_STAT_CAL]; > tsl2x7x_prox_calculate(prox_history, > - chip->settings.prox_max_samples_cal, calP); > - chip->settings.prox_thres_high = (calP->max << 1) - calP->mean; > + chip->settings.prox_max_samples_cal, cal); > + chip->settings.prox_thres_high = (cal->max << 1) - cal->mean; > > dev_info(&chip->client->dev, " cal min=%d mean=%d max=%d\n", > - calP->min, calP->mean, calP->max); > + cal->min, cal->mean, cal->max); > dev_info(&chip->client->dev, > "%s proximity threshold set to %d\n", > chip->client->name, chip->settings.prox_thres_high); _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
On Sat, 3 Mar 2018 20:49:37 -0500 Brian Masney <masneyb@onstation.org> wrote: > The statP and calP variables triggered an 'Avoid CamelCase' warning > from checkpatch.pl. This patch renames these variables to stat and cal > to fix this warning. > > Signed-off-by: Brian Masney <masneyb@onstation.org> Applied. > --- > drivers/staging/iio/light/tsl2x7x.c | 26 +++++++++++++------------- > 1 file changed, 13 insertions(+), 13 deletions(-) > > diff --git a/drivers/staging/iio/light/tsl2x7x.c b/drivers/staging/iio/light/tsl2x7x.c > index cc209b64ed5a..380a6c96a69a 100644 > --- a/drivers/staging/iio/light/tsl2x7x.c > +++ b/drivers/staging/iio/light/tsl2x7x.c > @@ -773,7 +773,7 @@ static int tsl2x7x_invoke_change(struct iio_dev *indio_dev) > } > > static void tsl2x7x_prox_calculate(int *data, int length, > - struct tsl2x7x_prox_stat *statP) > + struct tsl2x7x_prox_stat *stat) > { > int i; > int sample_sum; > @@ -783,21 +783,21 @@ static void tsl2x7x_prox_calculate(int *data, int length, > length = 1; > > sample_sum = 0; > - statP->min = INT_MAX; > - statP->max = INT_MIN; > + stat->min = INT_MAX; > + stat->max = INT_MIN; > for (i = 0; i < length; i++) { > sample_sum += data[i]; > - statP->min = min(statP->min, data[i]); > - statP->max = max(statP->max, data[i]); > + stat->min = min(stat->min, data[i]); > + stat->max = max(stat->max, data[i]); > } > > - statP->mean = sample_sum / length; > + stat->mean = sample_sum / length; > sample_sum = 0; > for (i = 0; i < length; i++) { > - tmp = data[i] - statP->mean; > + tmp = data[i] - stat->mean; > sample_sum += tmp * tmp; > } > - statP->stddev = int_sqrt((long)sample_sum / length); > + stat->stddev = int_sqrt((long)sample_sum / length); > } > > /** > @@ -812,7 +812,7 @@ static void tsl2x7x_prox_cal(struct iio_dev *indio_dev) > int prox_history[MAX_SAMPLES_CAL + 1]; > int i; > struct tsl2x7x_prox_stat prox_stat_data[2]; > - struct tsl2x7x_prox_stat *calP; > + struct tsl2x7x_prox_stat *cal; > struct tsl2X7X_chip *chip = iio_priv(indio_dev); > u8 tmp_irq_settings; > u8 current_state = chip->tsl2x7x_chip_status; > @@ -844,13 +844,13 @@ static void tsl2x7x_prox_cal(struct iio_dev *indio_dev) > } > > tsl2x7x_chip_off(indio_dev); > - calP = &prox_stat_data[PROX_STAT_CAL]; > + cal = &prox_stat_data[PROX_STAT_CAL]; > tsl2x7x_prox_calculate(prox_history, > - chip->settings.prox_max_samples_cal, calP); > - chip->settings.prox_thres_high = (calP->max << 1) - calP->mean; > + chip->settings.prox_max_samples_cal, cal); > + chip->settings.prox_thres_high = (cal->max << 1) - cal->mean; > > dev_info(&chip->client->dev, " cal min=%d mean=%d max=%d\n", > - calP->min, calP->mean, calP->max); > + cal->min, cal->mean, cal->max); > dev_info(&chip->client->dev, > "%s proximity threshold set to %d\n", > chip->client->name, chip->settings.prox_thres_high);
On Sat, 3 Mar 2018 20:49:38 -0500 Brian Masney <masneyb@onstation.org> wrote: > tsl2x7x_prox_cal() did not have any error checks. This patch adds > the missing error handling and ensures that any errors are reported to > user space via in_proximity0_calibrate_store(). > > Signed-off-by: Brian Masney <masneyb@onstation.org> applied. Thanks, Jonathan > --- > drivers/staging/iio/light/tsl2x7x.c | 36 ++++++++++++++++++++++++++---------- > 1 file changed, 26 insertions(+), 10 deletions(-) > > diff --git a/drivers/staging/iio/light/tsl2x7x.c b/drivers/staging/iio/light/tsl2x7x.c > index 380a6c96a69a..8adc6db44790 100644 > --- a/drivers/staging/iio/light/tsl2x7x.c > +++ b/drivers/staging/iio/light/tsl2x7x.c > @@ -807,10 +807,10 @@ static void tsl2x7x_prox_calculate(int *data, int length, > * Calculates a standard deviation based on the samples, > * and sets the threshold accordingly. > */ > -static void tsl2x7x_prox_cal(struct iio_dev *indio_dev) > +static int tsl2x7x_prox_cal(struct iio_dev *indio_dev) > { > int prox_history[MAX_SAMPLES_CAL + 1]; > - int i; > + int i, ret; > struct tsl2x7x_prox_stat prox_stat_data[2]; > struct tsl2x7x_prox_stat *cal; > struct tsl2X7X_chip *chip = iio_priv(indio_dev); > @@ -825,25 +825,33 @@ static void tsl2x7x_prox_cal(struct iio_dev *indio_dev) > } > > /* have to stop to change settings */ > - tsl2x7x_chip_off(indio_dev); > + ret = tsl2x7x_chip_off(indio_dev); > + if (ret < 0) > + return ret; > > /* Enable proximity detection save just in case prox not wanted yet*/ > tmp_irq_settings = chip->settings.interrupts_en; > chip->settings.interrupts_en |= TSL2X7X_CNTL_PROX_INT_ENBL; > > /*turn on device if not already on*/ > - tsl2x7x_chip_on(indio_dev); > + ret = tsl2x7x_chip_on(indio_dev); > + if (ret < 0) > + return ret; > > /*gather the samples*/ > for (i = 0; i < chip->settings.prox_max_samples_cal; i++) { > usleep_range(15000, 17500); > - tsl2x7x_get_prox(indio_dev); > + ret = tsl2x7x_get_prox(indio_dev); > + if (ret < 0) > + return ret; > prox_history[i] = chip->prox_data; > dev_info(&chip->client->dev, "2 i=%d prox data= %d\n", > i, chip->prox_data); > } > > - tsl2x7x_chip_off(indio_dev); > + ret = tsl2x7x_chip_off(indio_dev); > + if (ret < 0) > + return ret; > cal = &prox_stat_data[PROX_STAT_CAL]; > tsl2x7x_prox_calculate(prox_history, > chip->settings.prox_max_samples_cal, cal); > @@ -857,8 +865,13 @@ static void tsl2x7x_prox_cal(struct iio_dev *indio_dev) > > /* back to the way they were */ > chip->settings.interrupts_en = tmp_irq_settings; > - if (current_state == TSL2X7X_CHIP_WORKING) > - tsl2x7x_chip_on(indio_dev); > + if (current_state == TSL2X7X_CHIP_WORKING) { > + ret = tsl2x7x_chip_on(indio_dev); > + if (ret < 0) > + return ret; > + } > + > + return 0; > } > > static ssize_t > @@ -1018,8 +1031,11 @@ static ssize_t in_proximity0_calibrate_store(struct device *dev, > if (strtobool(buf, &value)) > return -EINVAL; > > - if (value) > - tsl2x7x_prox_cal(indio_dev); > + if (value) { > + ret = tsl2x7x_prox_cal(indio_dev); > + if (ret < 0) > + return ret; > + } > > ret = tsl2x7x_invoke_change(indio_dev); > if (ret < 0) _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
On Sat, 3 Mar 2018 20:49:38 -0500 Brian Masney <masneyb@onstation.org> wrote: > tsl2x7x_prox_cal() did not have any error checks. This patch adds > the missing error handling and ensures that any errors are reported to > user space via in_proximity0_calibrate_store(). > > Signed-off-by: Brian Masney <masneyb@onstation.org> applied. Thanks, Jonathan > --- > drivers/staging/iio/light/tsl2x7x.c | 36 ++++++++++++++++++++++++++---------- > 1 file changed, 26 insertions(+), 10 deletions(-) > > diff --git a/drivers/staging/iio/light/tsl2x7x.c b/drivers/staging/iio/light/tsl2x7x.c > index 380a6c96a69a..8adc6db44790 100644 > --- a/drivers/staging/iio/light/tsl2x7x.c > +++ b/drivers/staging/iio/light/tsl2x7x.c > @@ -807,10 +807,10 @@ static void tsl2x7x_prox_calculate(int *data, int length, > * Calculates a standard deviation based on the samples, > * and sets the threshold accordingly. > */ > -static void tsl2x7x_prox_cal(struct iio_dev *indio_dev) > +static int tsl2x7x_prox_cal(struct iio_dev *indio_dev) > { > int prox_history[MAX_SAMPLES_CAL + 1]; > - int i; > + int i, ret; > struct tsl2x7x_prox_stat prox_stat_data[2]; > struct tsl2x7x_prox_stat *cal; > struct tsl2X7X_chip *chip = iio_priv(indio_dev); > @@ -825,25 +825,33 @@ static void tsl2x7x_prox_cal(struct iio_dev *indio_dev) > } > > /* have to stop to change settings */ > - tsl2x7x_chip_off(indio_dev); > + ret = tsl2x7x_chip_off(indio_dev); > + if (ret < 0) > + return ret; > > /* Enable proximity detection save just in case prox not wanted yet*/ > tmp_irq_settings = chip->settings.interrupts_en; > chip->settings.interrupts_en |= TSL2X7X_CNTL_PROX_INT_ENBL; > > /*turn on device if not already on*/ > - tsl2x7x_chip_on(indio_dev); > + ret = tsl2x7x_chip_on(indio_dev); > + if (ret < 0) > + return ret; > > /*gather the samples*/ > for (i = 0; i < chip->settings.prox_max_samples_cal; i++) { > usleep_range(15000, 17500); > - tsl2x7x_get_prox(indio_dev); > + ret = tsl2x7x_get_prox(indio_dev); > + if (ret < 0) > + return ret; > prox_history[i] = chip->prox_data; > dev_info(&chip->client->dev, "2 i=%d prox data= %d\n", > i, chip->prox_data); > } > > - tsl2x7x_chip_off(indio_dev); > + ret = tsl2x7x_chip_off(indio_dev); > + if (ret < 0) > + return ret; > cal = &prox_stat_data[PROX_STAT_CAL]; > tsl2x7x_prox_calculate(prox_history, > chip->settings.prox_max_samples_cal, cal); > @@ -857,8 +865,13 @@ static void tsl2x7x_prox_cal(struct iio_dev *indio_dev) > > /* back to the way they were */ > chip->settings.interrupts_en = tmp_irq_settings; > - if (current_state == TSL2X7X_CHIP_WORKING) > - tsl2x7x_chip_on(indio_dev); > + if (current_state == TSL2X7X_CHIP_WORKING) { > + ret = tsl2x7x_chip_on(indio_dev); > + if (ret < 0) > + return ret; > + } > + > + return 0; > } > > static ssize_t > @@ -1018,8 +1031,11 @@ static ssize_t in_proximity0_calibrate_store(struct device *dev, > if (strtobool(buf, &value)) > return -EINVAL; > > - if (value) > - tsl2x7x_prox_cal(indio_dev); > + if (value) { > + ret = tsl2x7x_prox_cal(indio_dev); > + if (ret < 0) > + return ret; > + } > > ret = tsl2x7x_invoke_change(indio_dev); > if (ret < 0)
On Sat, 3 Mar 2018 20:49:39 -0500 Brian Masney <masneyb@onstation.org> wrote: > The functions in_illuminance0_calibrate_store() and > in_illuminance0_lux_table_store() did not have complete error handling > in place. This patch adds the missing error handling. > > Signed-off-by: Brian Masney <masneyb@onstation.org> Applied. Thanks, > --- > drivers/staging/iio/light/tsl2x7x.c | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/drivers/staging/iio/light/tsl2x7x.c b/drivers/staging/iio/light/tsl2x7x.c > index 8adc6db44790..da7a4e025083 100644 > --- a/drivers/staging/iio/light/tsl2x7x.c > +++ b/drivers/staging/iio/light/tsl2x7x.c > @@ -940,8 +940,11 @@ static ssize_t in_illuminance0_calibrate_store(struct device *dev, > if (strtobool(buf, &value)) > return -EINVAL; > > - if (value) > - tsl2x7x_als_calibrate(indio_dev); > + if (value) { > + ret = tsl2x7x_als_calibrate(indio_dev); > + if (ret < 0) > + return ret; > + } > > ret = tsl2x7x_invoke_change(indio_dev); > if (ret < 0) > @@ -1006,8 +1009,11 @@ static ssize_t in_illuminance0_lux_table_store(struct device *dev, > return -EINVAL; > } > > - if (chip->tsl2x7x_chip_status == TSL2X7X_CHIP_WORKING) > - tsl2x7x_chip_off(indio_dev); > + if (chip->tsl2x7x_chip_status == TSL2X7X_CHIP_WORKING) { > + ret = tsl2x7x_chip_off(indio_dev); > + if (ret < 0) > + return ret; > + } > > /* Zero out the table */ > memset(chip->tsl2x7x_device_lux, 0, sizeof(chip->tsl2x7x_device_lux)); _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
On Sat, 3 Mar 2018 20:49:39 -0500 Brian Masney <masneyb@onstation.org> wrote: > The functions in_illuminance0_calibrate_store() and > in_illuminance0_lux_table_store() did not have complete error handling > in place. This patch adds the missing error handling. > > Signed-off-by: Brian Masney <masneyb@onstation.org> Applied. Thanks, > --- > drivers/staging/iio/light/tsl2x7x.c | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/drivers/staging/iio/light/tsl2x7x.c b/drivers/staging/iio/light/tsl2x7x.c > index 8adc6db44790..da7a4e025083 100644 > --- a/drivers/staging/iio/light/tsl2x7x.c > +++ b/drivers/staging/iio/light/tsl2x7x.c > @@ -940,8 +940,11 @@ static ssize_t in_illuminance0_calibrate_store(struct device *dev, > if (strtobool(buf, &value)) > return -EINVAL; > > - if (value) > - tsl2x7x_als_calibrate(indio_dev); > + if (value) { > + ret = tsl2x7x_als_calibrate(indio_dev); > + if (ret < 0) > + return ret; > + } > > ret = tsl2x7x_invoke_change(indio_dev); > if (ret < 0) > @@ -1006,8 +1009,11 @@ static ssize_t in_illuminance0_lux_table_store(struct device *dev, > return -EINVAL; > } > > - if (chip->tsl2x7x_chip_status == TSL2X7X_CHIP_WORKING) > - tsl2x7x_chip_off(indio_dev); > + if (chip->tsl2x7x_chip_status == TSL2X7X_CHIP_WORKING) { > + ret = tsl2x7x_chip_off(indio_dev); > + if (ret < 0) > + return ret; > + } > > /* Zero out the table */ > memset(chip->tsl2x7x_device_lux, 0, sizeof(chip->tsl2x7x_device_lux));
On Sat, 3 Mar 2018 20:49:40 -0500 Brian Masney <masneyb@onstation.org> wrote: > This patch updates all of the logging commands so that they are > consistent with the other messages, includes __func__ in the message, > and all of the messages include newlines. > > This patch also removes some debug log messages from tsl2x7x_prox_cal(). > > Signed-off-by: Brian Masney <masneyb@onstation.org> Hmm. I nearly fixed this one up but there are a few too many elements I don't agree with. > --- > drivers/staging/iio/light/tsl2x7x.c | 58 ++++++++++++++++++++----------------- > 1 file changed, 31 insertions(+), 27 deletions(-) > > diff --git a/drivers/staging/iio/light/tsl2x7x.c b/drivers/staging/iio/light/tsl2x7x.c > index da7a4e025083..fb91c46c8747 100644 > --- a/drivers/staging/iio/light/tsl2x7x.c > +++ b/drivers/staging/iio/light/tsl2x7x.c > @@ -378,7 +378,8 @@ static int tsl2x7x_get_lux(struct iio_dev *indio_dev) > ret = i2c_smbus_read_byte_data(chip->client, reg); > if (ret < 0) { > dev_err(&chip->client->dev, > - "failed to read. err=%x\n", ret); > + "%s: failed to read from register %x: %d\n", > + __func__, reg, ret); > goto out_unlock; > } > > @@ -424,7 +425,9 @@ static int tsl2x7x_get_lux(struct iio_dev *indio_dev) > > /* note: lux is 31 bit max at this point */ > if (ch1lux > ch0lux) { > - dev_dbg(&chip->client->dev, "ch1lux > ch0lux-return last value\n"); > + dev_dbg(&chip->client->dev, > + "%s: ch1lux > ch0lux; returning last value\n", > + __func__); > ret = chip->als_cur_info.lux; > goto out_unlock; > } > @@ -600,7 +603,8 @@ static int tsl2x7x_als_calibrate(struct iio_dev *indio_dev) > > chip->settings.als_gain_trim = ret; > dev_info(&chip->client->dev, > - "%s als_calibrate completed\n", chip->client->name); > + "%s: %s ALS calibration successfully completed\n", > + __func__, chip->client->name); I'm happy with having function name in errors, but it's just noise in info prints. Mind you this dev_info should go or become dev_dbg - it is superflous detail to put in the kernel logs. > > return ret; > } > @@ -644,7 +648,8 @@ static int tsl2x7x_chip_on(struct iio_dev *indio_dev) > /* and make sure we're not already on */ > if (chip->tsl2x7x_chip_status == TSL2X7X_CHIP_WORKING) { > /* if forcing a register update - turn off, then on */ > - dev_info(&chip->client->dev, "device is already enabled\n"); > + dev_info(&chip->client->dev, "%s: device is already enabled\n", > + __func__); Again noise. dev_dbg or drop entirely as it doesn't need to be logged. > return -EINVAL; > } > > @@ -681,12 +686,14 @@ static int tsl2x7x_chip_on(struct iio_dev *indio_dev) > */ > for (i = 0, dev_reg = chip->tsl2x7x_config; > i < TSL2X7X_MAX_CONFIG_REG; i++) { > - ret = i2c_smbus_write_byte_data(chip->client, > - TSL2X7X_CMD_REG + i, > + int reg = TSL2X7X_CMD_REG + i; > + > + ret = i2c_smbus_write_byte_data(chip->client, reg, > *dev_reg++); > if (ret < 0) { > dev_err(&chip->client->dev, > - "failed on write to reg %d.\n", i); > + "%s: failed to write to register %x: %d\n", > + __func__, reg, ret); > return ret; > } > } > @@ -708,7 +715,8 @@ static int tsl2x7x_chip_on(struct iio_dev *indio_dev) > chip->tsl2x7x_chip_status = TSL2X7X_CHIP_WORKING; > > if (chip->settings.interrupts_en != 0) { > - dev_info(&chip->client->dev, "Setting Up Interrupt(s)\n"); > + dev_info(&chip->client->dev, "%s: Setting up interrupt(s)\n", > + __func__); > > reg_val = TSL2X7X_CNTL_PWR_ON | TSL2X7X_CNTL_ADC_ENBL; > if (chip->settings.interrupts_en == 0x20 || > @@ -819,8 +827,8 @@ static int tsl2x7x_prox_cal(struct iio_dev *indio_dev) > > if (chip->settings.prox_max_samples_cal > MAX_SAMPLES_CAL) { > dev_err(&chip->client->dev, > - "max prox samples cal is too big: %d\n", > - chip->settings.prox_max_samples_cal); > + "%s: prox_max_samples_cal %d is too big\n", > + __func__, chip->settings.prox_max_samples_cal); > chip->settings.prox_max_samples_cal = MAX_SAMPLES_CAL; > } > > @@ -845,8 +853,6 @@ static int tsl2x7x_prox_cal(struct iio_dev *indio_dev) > if (ret < 0) > return ret; > prox_history[i] = chip->prox_data; > - dev_info(&chip->client->dev, "2 i=%d prox data= %d\n", > - i, chip->prox_data); > } > > ret = tsl2x7x_chip_off(indio_dev); > @@ -857,12 +863,6 @@ static int tsl2x7x_prox_cal(struct iio_dev *indio_dev) > chip->settings.prox_max_samples_cal, cal); > chip->settings.prox_thres_high = (cal->max << 1) - cal->mean; > > - dev_info(&chip->client->dev, " cal min=%d mean=%d max=%d\n", > - cal->min, cal->mean, cal->max); > - dev_info(&chip->client->dev, > - "%s proximity threshold set to %d\n", > - chip->client->name, chip->settings.prox_thres_high); > - > /* back to the way they were */ > chip->settings.interrupts_en = tmp_irq_settings; > if (current_state == TSL2X7X_CHIP_WORKING) { > @@ -1000,12 +1000,14 @@ static ssize_t in_illuminance0_lux_table_store(struct device *dev, > n = value[0]; > if ((n % 3) || n < 6 || > n > ((ARRAY_SIZE(chip->tsl2x7x_device_lux) - 1) * 3)) { > - dev_info(dev, "LUX TABLE INPUT ERROR 1 Value[0]=%d\n", n); > + dev_info(dev, "%s: lux table input error 1. n=%d\n", > + __func__, n); > return -EINVAL; > } > > if ((value[(n - 2)] | value[(n - 1)] | value[n]) != 0) { > - dev_info(dev, "LUX TABLE INPUT ERROR 2 Value[0]=%d\n", n); > + dev_info(dev, "%s: lux table input error 2. n=%d\n", > + __func__, n); dev_dbg or drop it. > return -EINVAL; > } > > @@ -1150,14 +1152,14 @@ static int tsl2x7x_write_event_value(struct iio_dev *indio_dev, > chip->settings.persistence &= 0xF0; > chip->settings.persistence |= > (filter_delay & 0x0F); > - dev_info(&chip->client->dev, "%s: ALS persistence = %d", > + dev_info(&chip->client->dev, "%s: ALS persistence = %d\n", > __func__, filter_delay); Again, dev_dbg at the most - or drop it. > } else { > chip->settings.persistence &= 0x0F; > chip->settings.persistence |= > ((filter_delay << 4) & 0xF0); > dev_info(&chip->client->dev, > - "%s: Proximity persistence = %d", > + "%s: Proximity persistence = %d\n", > __func__, filter_delay); > } > ret = 0; > @@ -1372,7 +1374,7 @@ static int tsl2x7x_write_raw(struct iio_dev *indio_dev, > chip->settings.als_time = > TSL2X7X_MAX_TIMER_CNT - (val2 / TSL2X7X_MIN_ITIME); > > - dev_info(&chip->client->dev, "%s: als time = %d", > + dev_info(&chip->client->dev, "%s: als time = %d\n", > __func__, chip->settings.als_time); Drop it. > break; > default: > @@ -1738,8 +1740,9 @@ static int tsl2x7x_probe(struct i2c_client *clientp, > > ret = i2c_smbus_write_byte(clientp, TSL2X7X_CMD_REG | TSL2X7X_CNTRL); > if (ret < 0) { > - dev_err(&clientp->dev, "write to cmd reg failed. err = %d\n", > - ret); > + dev_err(&clientp->dev, > + "%s: Failed to write to CMD register: %d\n", > + __func__, ret); > return ret; > } > > @@ -1773,7 +1776,7 @@ static int tsl2x7x_probe(struct i2c_client *clientp, > indio_dev); > if (ret) { > dev_err(&clientp->dev, > - "%s: irq request failed", __func__); > + "%s: IRQ request failed\n", __func__); > return ret; > } > } > @@ -1790,7 +1793,8 @@ static int tsl2x7x_probe(struct i2c_client *clientp, > return ret; > } > > - dev_info(&clientp->dev, "%s Light sensor found.\n", id->name); > + dev_info(&clientp->dev, "%s: %s Light sensor found.\n", __func__, > + id->name); Again, unnecessary logging. Definitely doesn't also want the function name. People 'might' care about whether the sensor is found, but not which function it is in. Jonathan > > return 0; > } _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
On Sat, 3 Mar 2018 20:49:40 -0500 Brian Masney <masneyb@onstation.org> wrote: > This patch updates all of the logging commands so that they are > consistent with the other messages, includes __func__ in the message, > and all of the messages include newlines. > > This patch also removes some debug log messages from tsl2x7x_prox_cal(). > > Signed-off-by: Brian Masney <masneyb@onstation.org> Hmm. I nearly fixed this one up but there are a few too many elements I don't agree with. > --- > drivers/staging/iio/light/tsl2x7x.c | 58 ++++++++++++++++++++----------------- > 1 file changed, 31 insertions(+), 27 deletions(-) > > diff --git a/drivers/staging/iio/light/tsl2x7x.c b/drivers/staging/iio/light/tsl2x7x.c > index da7a4e025083..fb91c46c8747 100644 > --- a/drivers/staging/iio/light/tsl2x7x.c > +++ b/drivers/staging/iio/light/tsl2x7x.c > @@ -378,7 +378,8 @@ static int tsl2x7x_get_lux(struct iio_dev *indio_dev) > ret = i2c_smbus_read_byte_data(chip->client, reg); > if (ret < 0) { > dev_err(&chip->client->dev, > - "failed to read. err=%x\n", ret); > + "%s: failed to read from register %x: %d\n", > + __func__, reg, ret); > goto out_unlock; > } > > @@ -424,7 +425,9 @@ static int tsl2x7x_get_lux(struct iio_dev *indio_dev) > > /* note: lux is 31 bit max at this point */ > if (ch1lux > ch0lux) { > - dev_dbg(&chip->client->dev, "ch1lux > ch0lux-return last value\n"); > + dev_dbg(&chip->client->dev, > + "%s: ch1lux > ch0lux; returning last value\n", > + __func__); > ret = chip->als_cur_info.lux; > goto out_unlock; > } > @@ -600,7 +603,8 @@ static int tsl2x7x_als_calibrate(struct iio_dev *indio_dev) > > chip->settings.als_gain_trim = ret; > dev_info(&chip->client->dev, > - "%s als_calibrate completed\n", chip->client->name); > + "%s: %s ALS calibration successfully completed\n", > + __func__, chip->client->name); I'm happy with having function name in errors, but it's just noise in info prints. Mind you this dev_info should go or become dev_dbg - it is superflous detail to put in the kernel logs. > > return ret; > } > @@ -644,7 +648,8 @@ static int tsl2x7x_chip_on(struct iio_dev *indio_dev) > /* and make sure we're not already on */ > if (chip->tsl2x7x_chip_status == TSL2X7X_CHIP_WORKING) { > /* if forcing a register update - turn off, then on */ > - dev_info(&chip->client->dev, "device is already enabled\n"); > + dev_info(&chip->client->dev, "%s: device is already enabled\n", > + __func__); Again noise. dev_dbg or drop entirely as it doesn't need to be logged. > return -EINVAL; > } > > @@ -681,12 +686,14 @@ static int tsl2x7x_chip_on(struct iio_dev *indio_dev) > */ > for (i = 0, dev_reg = chip->tsl2x7x_config; > i < TSL2X7X_MAX_CONFIG_REG; i++) { > - ret = i2c_smbus_write_byte_data(chip->client, > - TSL2X7X_CMD_REG + i, > + int reg = TSL2X7X_CMD_REG + i; > + > + ret = i2c_smbus_write_byte_data(chip->client, reg, > *dev_reg++); > if (ret < 0) { > dev_err(&chip->client->dev, > - "failed on write to reg %d.\n", i); > + "%s: failed to write to register %x: %d\n", > + __func__, reg, ret); > return ret; > } > } > @@ -708,7 +715,8 @@ static int tsl2x7x_chip_on(struct iio_dev *indio_dev) > chip->tsl2x7x_chip_status = TSL2X7X_CHIP_WORKING; > > if (chip->settings.interrupts_en != 0) { > - dev_info(&chip->client->dev, "Setting Up Interrupt(s)\n"); > + dev_info(&chip->client->dev, "%s: Setting up interrupt(s)\n", > + __func__); > > reg_val = TSL2X7X_CNTL_PWR_ON | TSL2X7X_CNTL_ADC_ENBL; > if (chip->settings.interrupts_en == 0x20 || > @@ -819,8 +827,8 @@ static int tsl2x7x_prox_cal(struct iio_dev *indio_dev) > > if (chip->settings.prox_max_samples_cal > MAX_SAMPLES_CAL) { > dev_err(&chip->client->dev, > - "max prox samples cal is too big: %d\n", > - chip->settings.prox_max_samples_cal); > + "%s: prox_max_samples_cal %d is too big\n", > + __func__, chip->settings.prox_max_samples_cal); > chip->settings.prox_max_samples_cal = MAX_SAMPLES_CAL; > } > > @@ -845,8 +853,6 @@ static int tsl2x7x_prox_cal(struct iio_dev *indio_dev) > if (ret < 0) > return ret; > prox_history[i] = chip->prox_data; > - dev_info(&chip->client->dev, "2 i=%d prox data= %d\n", > - i, chip->prox_data); > } > > ret = tsl2x7x_chip_off(indio_dev); > @@ -857,12 +863,6 @@ static int tsl2x7x_prox_cal(struct iio_dev *indio_dev) > chip->settings.prox_max_samples_cal, cal); > chip->settings.prox_thres_high = (cal->max << 1) - cal->mean; > > - dev_info(&chip->client->dev, " cal min=%d mean=%d max=%d\n", > - cal->min, cal->mean, cal->max); > - dev_info(&chip->client->dev, > - "%s proximity threshold set to %d\n", > - chip->client->name, chip->settings.prox_thres_high); > - > /* back to the way they were */ > chip->settings.interrupts_en = tmp_irq_settings; > if (current_state == TSL2X7X_CHIP_WORKING) { > @@ -1000,12 +1000,14 @@ static ssize_t in_illuminance0_lux_table_store(struct device *dev, > n = value[0]; > if ((n % 3) || n < 6 || > n > ((ARRAY_SIZE(chip->tsl2x7x_device_lux) - 1) * 3)) { > - dev_info(dev, "LUX TABLE INPUT ERROR 1 Value[0]=%d\n", n); > + dev_info(dev, "%s: lux table input error 1. n=%d\n", > + __func__, n); > return -EINVAL; > } > > if ((value[(n - 2)] | value[(n - 1)] | value[n]) != 0) { > - dev_info(dev, "LUX TABLE INPUT ERROR 2 Value[0]=%d\n", n); > + dev_info(dev, "%s: lux table input error 2. n=%d\n", > + __func__, n); dev_dbg or drop it. > return -EINVAL; > } > > @@ -1150,14 +1152,14 @@ static int tsl2x7x_write_event_value(struct iio_dev *indio_dev, > chip->settings.persistence &= 0xF0; > chip->settings.persistence |= > (filter_delay & 0x0F); > - dev_info(&chip->client->dev, "%s: ALS persistence = %d", > + dev_info(&chip->client->dev, "%s: ALS persistence = %d\n", > __func__, filter_delay); Again, dev_dbg at the most - or drop it. > } else { > chip->settings.persistence &= 0x0F; > chip->settings.persistence |= > ((filter_delay << 4) & 0xF0); > dev_info(&chip->client->dev, > - "%s: Proximity persistence = %d", > + "%s: Proximity persistence = %d\n", > __func__, filter_delay); > } > ret = 0; > @@ -1372,7 +1374,7 @@ static int tsl2x7x_write_raw(struct iio_dev *indio_dev, > chip->settings.als_time = > TSL2X7X_MAX_TIMER_CNT - (val2 / TSL2X7X_MIN_ITIME); > > - dev_info(&chip->client->dev, "%s: als time = %d", > + dev_info(&chip->client->dev, "%s: als time = %d\n", > __func__, chip->settings.als_time); Drop it. > break; > default: > @@ -1738,8 +1740,9 @@ static int tsl2x7x_probe(struct i2c_client *clientp, > > ret = i2c_smbus_write_byte(clientp, TSL2X7X_CMD_REG | TSL2X7X_CNTRL); > if (ret < 0) { > - dev_err(&clientp->dev, "write to cmd reg failed. err = %d\n", > - ret); > + dev_err(&clientp->dev, > + "%s: Failed to write to CMD register: %d\n", > + __func__, ret); > return ret; > } > > @@ -1773,7 +1776,7 @@ static int tsl2x7x_probe(struct i2c_client *clientp, > indio_dev); > if (ret) { > dev_err(&clientp->dev, > - "%s: irq request failed", __func__); > + "%s: IRQ request failed\n", __func__); > return ret; > } > } > @@ -1790,7 +1793,8 @@ static int tsl2x7x_probe(struct i2c_client *clientp, > return ret; > } > > - dev_info(&clientp->dev, "%s Light sensor found.\n", id->name); > + dev_info(&clientp->dev, "%s: %s Light sensor found.\n", __func__, > + id->name); Again, unnecessary logging. Definitely doesn't also want the function name. People 'might' care about whether the sensor is found, but not which function it is in. Jonathan > > return 0; > }
On Sat, 3 Mar 2018 20:49:41 -0500 Brian Masney <masneyb@onstation.org> wrote: > The tsl2771 and tmd2771 devices create the > in_proximity0_calibscale_available sysfs attribute. These two particular > devices do not support changing the proximity gain value on the > chip so this patch removes that sysfs attribute. As expected, these two > devices already did not create the IIO_CHAN_INFO_CALIBSCALE channel and > proximity0_calibrate sysfs attribute. > > Page 38 of the tsl2772 data sheet shows that the proximity gain can be > adjusted with bits 2-3 on the control register: > https://ams.com/eng/content/download/291503/1066377/file/TSL2772_DS000181_2-00.pdf > > Page 35 of the tsl2771 and tmd2771 data sheets shows that bits 2-3 on > the control register are reserved and changing the proximity gain is > not supported: > https://ams.com/eng/content/download/250264/976045/file/TSL2771_DS000105_3-00.pdf > https://ams.com/eng/content/download/250283/976077/file/TMD2771_DS000177_2-00.pdf > > Signed-off-by: Brian Masney <masneyb@onstation.org> This one applied without the previous so I have done so. Applied. Thanks, Jonathan > --- > drivers/staging/iio/light/tsl2x7x.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/staging/iio/light/tsl2x7x.c b/drivers/staging/iio/light/tsl2x7x.c > index fb91c46c8747..8c29a52153c1 100644 > --- a/drivers/staging/iio/light/tsl2x7x.c > +++ b/drivers/staging/iio/light/tsl2x7x.c > @@ -1477,7 +1477,6 @@ static struct attribute *tsl2x7x_ALSPRX_device_attrs[] = { > &dev_attr_in_illuminance0_target_input.attr, > &dev_attr_in_illuminance0_calibrate.attr, > &dev_attr_in_illuminance0_lux_table.attr, > - &iio_const_attr_in_proximity0_calibscale_available.dev_attr.attr, > NULL > }; > _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
On Sat, 3 Mar 2018 20:49:41 -0500 Brian Masney <masneyb@onstation.org> wrote: > The tsl2771 and tmd2771 devices create the > in_proximity0_calibscale_available sysfs attribute. These two particular > devices do not support changing the proximity gain value on the > chip so this patch removes that sysfs attribute. As expected, these two > devices already did not create the IIO_CHAN_INFO_CALIBSCALE channel and > proximity0_calibrate sysfs attribute. > > Page 38 of the tsl2772 data sheet shows that the proximity gain can be > adjusted with bits 2-3 on the control register: > https://ams.com/eng/content/download/291503/1066377/file/TSL2772_DS000181_2-00.pdf > > Page 35 of the tsl2771 and tmd2771 data sheets shows that bits 2-3 on > the control register are reserved and changing the proximity gain is > not supported: > https://ams.com/eng/content/download/250264/976045/file/TSL2771_DS000105_3-00.pdf > https://ams.com/eng/content/download/250283/976077/file/TMD2771_DS000177_2-00.pdf > > Signed-off-by: Brian Masney <masneyb@onstation.org> This one applied without the previous so I have done so. Applied. Thanks, Jonathan > --- > drivers/staging/iio/light/tsl2x7x.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/staging/iio/light/tsl2x7x.c b/drivers/staging/iio/light/tsl2x7x.c > index fb91c46c8747..8c29a52153c1 100644 > --- a/drivers/staging/iio/light/tsl2x7x.c > +++ b/drivers/staging/iio/light/tsl2x7x.c > @@ -1477,7 +1477,6 @@ static struct attribute *tsl2x7x_ALSPRX_device_attrs[] = { > &dev_attr_in_illuminance0_target_input.attr, > &dev_attr_in_illuminance0_calibrate.attr, > &dev_attr_in_illuminance0_lux_table.attr, > - &iio_const_attr_in_proximity0_calibscale_available.dev_attr.attr, > NULL > }; >
On Sat, 3 Mar 2018 20:49:42 -0500 Brian Masney <masneyb@onstation.org> wrote: > The bits for setting up the proximity diode were not setup correctly and > this was causing the proximity sensor to not function correctly. This > patch sets up the correct bit mask in tsl2x7x_chip_on() based on what > the data sheet expects. From page 35 of the TSL2771 data sheet: > https://ams.com/eng/content/download/250264/976045/file/TSL2771_DS000105_3-00.pdf > > - Bits 0-1 is the ALS gain control > - Bits 2-3 is reserved (The proximity gain control on other tsl2x7x chips) > - Bits 4-5 is the proximity diode select > - Bits 6-7 is the LED drive strength > > tsl2x7x_chip_on() had the power and diode hardcoded, so these are > extracted out into the settings so that these fields can be configured > in the platform data. > > The default prox_gain is changed from 1 (2X gain) to 0 (1X gain) since > the proximity gain control on the TSL2771, TMD2771, and other chips have > these fields listed as reserved, and to write 0 into those bits. > > Verified that the proximity sensor now works correctly on a TSL2771 > hooked up to a Raspberry Pi 2. > > Signed-off-by: Brian Masney <masneyb@onstation.org> Applied to the togreg branch of iio.git and pushed out as testing for the autobuilders to play with it. So just patch 10 that needs a v2. Thanks for the set - nearly there ;) Jonathan > --- > drivers/staging/iio/light/tsl2x7x.c | 25 ++++++++++++++----------- > drivers/staging/iio/light/tsl2x7x.h | 2 ++ > 2 files changed, 16 insertions(+), 11 deletions(-) > > diff --git a/drivers/staging/iio/light/tsl2x7x.c b/drivers/staging/iio/light/tsl2x7x.c > index 8c29a52153c1..ab518cdec43e 100644 > --- a/drivers/staging/iio/light/tsl2x7x.c > +++ b/drivers/staging/iio/light/tsl2x7x.c > @@ -109,15 +109,15 @@ > #define TSL2X7X_CNTL_INTPROXPON_ENBL 0x2F > > /*Prox diode to use */ > -#define TSL2X7X_DIODE0 0x10 > -#define TSL2X7X_DIODE1 0x20 > -#define TSL2X7X_DIODE_BOTH 0x30 > +#define TSL2X7X_DIODE0 0x01 > +#define TSL2X7X_DIODE1 0x02 > +#define TSL2X7X_DIODE_BOTH 0x03 > > /* LED Power */ > #define TSL2X7X_100_mA 0x00 > -#define TSL2X7X_50_mA 0x40 > -#define TSL2X7X_25_mA 0x80 > -#define TSL2X7X_13_mA 0xD0 > +#define TSL2X7X_50_mA 0x01 > +#define TSL2X7X_25_mA 0x02 > +#define TSL2X7X_13_mA 0x03 > #define TSL2X7X_MAX_TIMER_CNT 0xFF > > #define TSL2X7X_MIN_ITIME 3 > @@ -228,7 +228,7 @@ static const struct tsl2x7x_settings tsl2x7x_default_settings = { > .als_time = 219, /* 101 ms */ > .als_gain = 0, > .prx_time = 254, /* 5.4 ms */ > - .prox_gain = 1, > + .prox_gain = 0, > .wait_time = 245, > .prox_config = 0, > .als_gain_trim = 1000, > @@ -240,7 +240,9 @@ static const struct tsl2x7x_settings tsl2x7x_default_settings = { > .prox_thres_low = 0, > .prox_thres_high = 512, > .prox_max_samples_cal = 30, > - .prox_pulse_count = 8 > + .prox_pulse_count = 8, > + .prox_diode = TSL2X7X_DIODE1, > + .prox_power = TSL2X7X_100_mA > }; > > static const s16 tsl2x7x_als_gain[] = { > @@ -664,9 +666,10 @@ static int tsl2x7x_chip_on(struct iio_dev *indio_dev) > > /* Set the gain based on tsl2x7x_settings struct */ > chip->tsl2x7x_config[TSL2X7X_GAIN] = > - chip->settings.als_gain | > - (TSL2X7X_100_mA | TSL2X7X_DIODE1) | > - (chip->settings.prox_gain << 2); > + (chip->settings.als_gain & 0xFF) | > + ((chip->settings.prox_gain & 0xFF) << 2) | > + (chip->settings.prox_diode << 4) | > + (chip->settings.prox_power << 6); > > /* set chip struct re scaling and saturation */ > chip->als_saturation = als_count * 922; /* 90% of full scale */ > diff --git a/drivers/staging/iio/light/tsl2x7x.h b/drivers/staging/iio/light/tsl2x7x.h > index 6624cbca7a83..28b0e7fdc9b8 100644 > --- a/drivers/staging/iio/light/tsl2x7x.h > +++ b/drivers/staging/iio/light/tsl2x7x.h > @@ -78,6 +78,8 @@ struct tsl2x7x_settings { > int prox_thres_high; > int prox_pulse_count; > int prox_max_samples_cal; > + int prox_diode; > + int prox_power; > }; > > /**
On Sat, 10 Mar 2018 14:52:40 +0000 Jonathan Cameron <jic23@kernel.org> wrote: > On Sat, 3 Mar 2018 20:49:40 -0500 > Brian Masney <masneyb@onstation.org> wrote: > > > This patch updates all of the logging commands so that they are > > consistent with the other messages, includes __func__ in the message, > > and all of the messages include newlines. > > > > This patch also removes some debug log messages from tsl2x7x_prox_cal(). > > > > Signed-off-by: Brian Masney <masneyb@onstation.org> > Hmm. I nearly fixed this one up but there are a few too many elements I don't > agree with. I knew there was something I was forgetting. There is no point in putting function names in dynamic debug calls, you can turn that on if you want it for all of them anyway. Jonathan > > > --- > > drivers/staging/iio/light/tsl2x7x.c | 58 ++++++++++++++++++++----------------- > > 1 file changed, 31 insertions(+), 27 deletions(-) > > > > diff --git a/drivers/staging/iio/light/tsl2x7x.c b/drivers/staging/iio/light/tsl2x7x.c > > index da7a4e025083..fb91c46c8747 100644 > > --- a/drivers/staging/iio/light/tsl2x7x.c > > +++ b/drivers/staging/iio/light/tsl2x7x.c > > @@ -378,7 +378,8 @@ static int tsl2x7x_get_lux(struct iio_dev *indio_dev) > > ret = i2c_smbus_read_byte_data(chip->client, reg); > > if (ret < 0) { > > dev_err(&chip->client->dev, > > - "failed to read. err=%x\n", ret); > > + "%s: failed to read from register %x: %d\n", > > + __func__, reg, ret); > > goto out_unlock; > > } > > > > @@ -424,7 +425,9 @@ static int tsl2x7x_get_lux(struct iio_dev *indio_dev) > > > > /* note: lux is 31 bit max at this point */ > > if (ch1lux > ch0lux) { > > - dev_dbg(&chip->client->dev, "ch1lux > ch0lux-return last value\n"); > > + dev_dbg(&chip->client->dev, > > + "%s: ch1lux > ch0lux; returning last value\n", > > + __func__); > > ret = chip->als_cur_info.lux; > > goto out_unlock; > > } > > @@ -600,7 +603,8 @@ static int tsl2x7x_als_calibrate(struct iio_dev *indio_dev) > > > > chip->settings.als_gain_trim = ret; > > dev_info(&chip->client->dev, > > - "%s als_calibrate completed\n", chip->client->name); > > + "%s: %s ALS calibration successfully completed\n", > > + __func__, chip->client->name); > I'm happy with having function name in errors, but it's just noise in info > prints. > > Mind you this dev_info should go or become dev_dbg - it is superflous > detail to put in the kernel logs. > > > > > return ret; > > } > > @@ -644,7 +648,8 @@ static int tsl2x7x_chip_on(struct iio_dev *indio_dev) > > /* and make sure we're not already on */ > > if (chip->tsl2x7x_chip_status == TSL2X7X_CHIP_WORKING) { > > /* if forcing a register update - turn off, then on */ > > - dev_info(&chip->client->dev, "device is already enabled\n"); > > + dev_info(&chip->client->dev, "%s: device is already enabled\n", > > + __func__); > Again noise. dev_dbg or drop entirely as it doesn't need to be logged. > > return -EINVAL; > > } > > > > @@ -681,12 +686,14 @@ static int tsl2x7x_chip_on(struct iio_dev *indio_dev) > > */ > > for (i = 0, dev_reg = chip->tsl2x7x_config; > > i < TSL2X7X_MAX_CONFIG_REG; i++) { > > - ret = i2c_smbus_write_byte_data(chip->client, > > - TSL2X7X_CMD_REG + i, > > + int reg = TSL2X7X_CMD_REG + i; > > + > > + ret = i2c_smbus_write_byte_data(chip->client, reg, > > *dev_reg++); > > if (ret < 0) { > > dev_err(&chip->client->dev, > > - "failed on write to reg %d.\n", i); > > + "%s: failed to write to register %x: %d\n", > > + __func__, reg, ret); > > return ret; > > } > > } > > @@ -708,7 +715,8 @@ static int tsl2x7x_chip_on(struct iio_dev *indio_dev) > > chip->tsl2x7x_chip_status = TSL2X7X_CHIP_WORKING; > > > > if (chip->settings.interrupts_en != 0) { > > - dev_info(&chip->client->dev, "Setting Up Interrupt(s)\n"); > > + dev_info(&chip->client->dev, "%s: Setting up interrupt(s)\n", > > + __func__); > > > > reg_val = TSL2X7X_CNTL_PWR_ON | TSL2X7X_CNTL_ADC_ENBL; > > if (chip->settings.interrupts_en == 0x20 || > > @@ -819,8 +827,8 @@ static int tsl2x7x_prox_cal(struct iio_dev *indio_dev) > > > > if (chip->settings.prox_max_samples_cal > MAX_SAMPLES_CAL) { > > dev_err(&chip->client->dev, > > - "max prox samples cal is too big: %d\n", > > - chip->settings.prox_max_samples_cal); > > + "%s: prox_max_samples_cal %d is too big\n", > > + __func__, chip->settings.prox_max_samples_cal); > > chip->settings.prox_max_samples_cal = MAX_SAMPLES_CAL; > > } > > > > @@ -845,8 +853,6 @@ static int tsl2x7x_prox_cal(struct iio_dev *indio_dev) > > if (ret < 0) > > return ret; > > prox_history[i] = chip->prox_data; > > - dev_info(&chip->client->dev, "2 i=%d prox data= %d\n", > > - i, chip->prox_data); > > } > > > > ret = tsl2x7x_chip_off(indio_dev); > > @@ -857,12 +863,6 @@ static int tsl2x7x_prox_cal(struct iio_dev *indio_dev) > > chip->settings.prox_max_samples_cal, cal); > > chip->settings.prox_thres_high = (cal->max << 1) - cal->mean; > > > > - dev_info(&chip->client->dev, " cal min=%d mean=%d max=%d\n", > > - cal->min, cal->mean, cal->max); > > - dev_info(&chip->client->dev, > > - "%s proximity threshold set to %d\n", > > - chip->client->name, chip->settings.prox_thres_high); > > - > > /* back to the way they were */ > > chip->settings.interrupts_en = tmp_irq_settings; > > if (current_state == TSL2X7X_CHIP_WORKING) { > > @@ -1000,12 +1000,14 @@ static ssize_t in_illuminance0_lux_table_store(struct device *dev, > > n = value[0]; > > if ((n % 3) || n < 6 || > > n > ((ARRAY_SIZE(chip->tsl2x7x_device_lux) - 1) * 3)) { > > - dev_info(dev, "LUX TABLE INPUT ERROR 1 Value[0]=%d\n", n); > > + dev_info(dev, "%s: lux table input error 1. n=%d\n", > > + __func__, n); > > return -EINVAL; > > } > > > > if ((value[(n - 2)] | value[(n - 1)] | value[n]) != 0) { > > - dev_info(dev, "LUX TABLE INPUT ERROR 2 Value[0]=%d\n", n); > > + dev_info(dev, "%s: lux table input error 2. n=%d\n", > > + __func__, n); > dev_dbg or drop it. > > > return -EINVAL; > > } > > > > @@ -1150,14 +1152,14 @@ static int tsl2x7x_write_event_value(struct iio_dev *indio_dev, > > chip->settings.persistence &= 0xF0; > > chip->settings.persistence |= > > (filter_delay & 0x0F); > > - dev_info(&chip->client->dev, "%s: ALS persistence = %d", > > + dev_info(&chip->client->dev, "%s: ALS persistence = %d\n", > > __func__, filter_delay); > Again, dev_dbg at the most - or drop it. > > > } else { > > chip->settings.persistence &= 0x0F; > > chip->settings.persistence |= > > ((filter_delay << 4) & 0xF0); > > dev_info(&chip->client->dev, > > - "%s: Proximity persistence = %d", > > + "%s: Proximity persistence = %d\n", > > __func__, filter_delay); > > } > > ret = 0; > > @@ -1372,7 +1374,7 @@ static int tsl2x7x_write_raw(struct iio_dev *indio_dev, > > chip->settings.als_time = > > TSL2X7X_MAX_TIMER_CNT - (val2 / TSL2X7X_MIN_ITIME); > > > > - dev_info(&chip->client->dev, "%s: als time = %d", > > + dev_info(&chip->client->dev, "%s: als time = %d\n", > > __func__, chip->settings.als_time); > Drop it. > > break; > > default: > > @@ -1738,8 +1740,9 @@ static int tsl2x7x_probe(struct i2c_client *clientp, > > > > ret = i2c_smbus_write_byte(clientp, TSL2X7X_CMD_REG | TSL2X7X_CNTRL); > > if (ret < 0) { > > - dev_err(&clientp->dev, "write to cmd reg failed. err = %d\n", > > - ret); > > + dev_err(&clientp->dev, > > + "%s: Failed to write to CMD register: %d\n", > > + __func__, ret); > > return ret; > > } > > > > @@ -1773,7 +1776,7 @@ static int tsl2x7x_probe(struct i2c_client *clientp, > > indio_dev); > > if (ret) { > > dev_err(&clientp->dev, > > - "%s: irq request failed", __func__); > > + "%s: IRQ request failed\n", __func__); > > return ret; > > } > > } > > @@ -1790,7 +1793,8 @@ static int tsl2x7x_probe(struct i2c_client *clientp, > > return ret; > > } > > > > - dev_info(&clientp->dev, "%s Light sensor found.\n", id->name); > > + dev_info(&clientp->dev, "%s: %s Light sensor found.\n", __func__, > > + id->name); > > Again, unnecessary logging. Definitely doesn't also want the function name. > People 'might' care about whether the sensor is found, but not which function > it is in. > > Jonathan > > > > > return 0; > > } > > -- > To unsubscribe from this list: send the line "unsubscribe linux-iio" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html