From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S966446AbcKLQR2 (ORCPT ); Sat, 12 Nov 2016 11:17:28 -0500 Received: from saturn.retrosnub.co.uk ([178.18.118.26]:35761 "EHLO saturn.retrosnub.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S966401AbcKLQR0 (ORCPT ); Sat, 12 Nov 2016 11:17:26 -0500 Subject: Re: [PATCH v3 01/28] staging: iio: tsl2583: split out functionality of taos_chip_on() To: Brian Masney , linux-iio@vger.kernel.org References: <1478769964-7065-1-git-send-email-masneyb@onstation.org> <1478769964-7065-2-git-send-email-masneyb@onstation.org> Cc: devel@driverdev.osuosl.org, gregkh@linuxfoundation.org, lars@metafoo.de, pmeerw@pmeerw.net, knaack.h@gmx.de, linux-kernel@vger.kernel.org, Jon.Brenner@ams.com From: Jonathan Cameron Message-ID: <60ebfe0a-2355-408f-a604-f6130b5647d8@kernel.org> Date: Sat, 12 Nov 2016 16:17:22 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <1478769964-7065-2-git-send-email-masneyb@onstation.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 10/11/16 09:25, Brian Masney wrote: > taos_chip_on() reads an eight member array called taos_config > that contains the desired state of the chip's registers. Only four > of the registers actually need to be written to. The four that do > not need to be written to are for the {low,high} byte of the lower > interrupt threshold and the {low,high} byte of the upper interrupt > threshold. Interrupts are currently not supported by this driver > so there is no need to write to these registers. > > This patch removes the taos_config array and separates out the > i2c calls that write to the CONTROL, TIMING, INTERRUPT and ANALOG > registers. This is part of a larger refactor that was split up to > make the code review easier. > > Signed-off-by: Brian Masney Looks good. Applied to the togreg branch of iio.git. Thanks Jonathan > --- > drivers/staging/iio/light/tsl2583.c | 122 +++++++++++++++++------------------- > 1 file changed, 58 insertions(+), 64 deletions(-) > > diff --git a/drivers/staging/iio/light/tsl2583.c b/drivers/staging/iio/light/tsl2583.c > index d74e33b..c196a42 100644 > --- a/drivers/staging/iio/light/tsl2583.c > +++ b/drivers/staging/iio/light/tsl2583.c > @@ -31,9 +31,6 @@ > #include > #include > > -/* Triton register offsets */ > -#define TSL258X_REG_MAX 8 > - > /* Device Registers and Masks */ > #define TSL258X_CNTRL 0x00 > #define TSL258X_ALS_TIME 0X01 > @@ -55,6 +52,7 @@ > > /* tsl2583 cntrl reg masks */ > #define TSL258X_CNTL_ADC_ENBL 0x02 > +#define TSL258X_CNTL_PWR_OFF 0x00 > #define TSL258X_CNTL_PWR_ON 0x01 > > /* tsl2583 status reg masks */ > @@ -64,6 +62,8 @@ > /* Lux calculation constants */ > #define TSL258X_LUX_CALC_OVER_FLOW 65535 > > +#define TSL2583_INTERRUPT_DISABLED 0x00 > + > #define TSL2583_CHIP_ID 0x90 > #define TSL2583_CHIP_ID_MASK 0xf0 > > @@ -95,18 +95,8 @@ struct tsl2583_chip { > int als_time_scale; > int als_saturation; > int taos_chip_status; > - u8 taos_config[8]; > }; > > -/* > - * Initial values for device - this values can/will be changed by driver. > - * and applications as needed. > - * These values are dynamic. > - */ > -static const u8 taos_config[8] = { > - 0x00, 0xee, 0x00, 0x03, 0x00, 0xFF, 0xFF, 0x00 > -}; /* cntrl atime intC Athl0 Athl1 Athh0 Athh1 gain */ > - > struct taos_lux { > unsigned int ratio; > unsigned int ch0; > @@ -362,16 +352,26 @@ static int taos_als_calibrate(struct iio_dev *indio_dev) > return (int)gain_trim_val; > } > > +static int tsl2583_set_power_state(struct tsl2583_chip *chip, u8 state) > +{ > + int ret; > + > + ret = i2c_smbus_write_byte_data(chip->client, > + TSL258X_CMD_REG | TSL258X_CNTRL, state); > + if (ret < 0) > + dev_err(&chip->client->dev, "%s failed to set the power state to %d\n", > + __func__, state); > + > + return ret; > +} > + > /* > * Turn the device on. > * Configuration must be set before calling this function. > */ > -static int taos_chip_on(struct iio_dev *indio_dev) > +static int tsl2583_chip_init_and_power_on(struct iio_dev *indio_dev) > { > - int i; > - int ret; > - u8 *uP; > - u8 utmp; > + int ret, val; > int als_count; > int als_time; > struct tsl2583_chip *chip = iio_priv(indio_dev); > @@ -383,6 +383,20 @@ static int taos_chip_on(struct iio_dev *indio_dev) > return -EINVAL; > } > > + /* Power on the device; ADC off. */ > + ret = tsl2583_set_power_state(chip, TSL258X_CNTL_PWR_ON); > + if (ret < 0) > + return ret; > + > + ret = i2c_smbus_write_byte_data(chip->client, > + TSL258X_CMD_REG | TSL258X_INTERRUPT, > + TSL2583_INTERRUPT_DISABLED); > + if (ret < 0) { > + dev_err(&chip->client->dev, "%s failed to disable interrupts\n", > + __func__); > + return ret; > + } > + > /* determine als integration register */ > als_count = (chip->taos_settings.als_time * 100 + 135) / 270; > if (!als_count) > @@ -390,62 +404,43 @@ static int taos_chip_on(struct iio_dev *indio_dev) > > /* convert back to time (encompasses overrides) */ > als_time = (als_count * 27 + 5) / 10; > - chip->taos_config[TSL258X_ALS_TIME] = 256 - als_count; > + > + val = 256 - als_count; > + ret = i2c_smbus_write_byte_data(chip->client, > + TSL258X_CMD_REG | TSL258X_ALS_TIME, > + val); > + if (ret < 0) { > + dev_err(&chip->client->dev, "%s failed to set the als time to %d\n", > + __func__, val); > + return ret; > + } > > /* Set the gain based on taos_settings struct */ > - chip->taos_config[TSL258X_GAIN] = chip->taos_settings.als_gain; > + ret = i2c_smbus_write_byte_data(chip->client, > + TSL258X_CMD_REG | TSL258X_GAIN, > + chip->taos_settings.als_gain); > + if (ret < 0) { > + dev_err(&chip->client->dev, "%s failed to set the gain to %d\n", > + __func__, chip->taos_settings.als_gain); > + return ret; > + } > > /* set chip struct re scaling and saturation */ > chip->als_saturation = als_count * 922; /* 90% of full scale */ > chip->als_time_scale = (als_time + 25) / 50; > > - /* Power on the device; ADC off. */ > - chip->taos_config[TSL258X_CNTRL] = TSL258X_CNTL_PWR_ON; > - > - /* > - * Use the following shadow copy for our delay before enabling ADC. > - * Write all the registers. > - */ > - for (i = 0, uP = chip->taos_config; i < TSL258X_REG_MAX; i++) { > - ret = i2c_smbus_write_byte_data(chip->client, > - TSL258X_CMD_REG + i, > - *uP++); > - if (ret < 0) { > - dev_err(&chip->client->dev, > - "taos_chip_on failed on reg %d.\n", i); > - return ret; > - } > - } > - > usleep_range(3000, 3500); > - /* > - * NOW enable the ADC > - * initialize the desired mode of operation > - */ > - utmp = TSL258X_CNTL_PWR_ON | TSL258X_CNTL_ADC_ENBL; > - ret = i2c_smbus_write_byte_data(chip->client, > - TSL258X_CMD_REG | TSL258X_CNTRL, > - utmp); > - if (ret < 0) { > - dev_err(&chip->client->dev, "taos_chip_on failed on 2nd CTRL reg.\n"); > + > + ret = tsl2583_set_power_state(chip, TSL258X_CNTL_PWR_ON | > + TSL258X_CNTL_ADC_ENBL); > + if (ret < 0) > return ret; > - } > + > chip->taos_chip_status = TSL258X_CHIP_WORKING; > > return ret; > } > > -static int taos_chip_off(struct iio_dev *indio_dev) > -{ > - struct tsl2583_chip *chip = iio_priv(indio_dev); > - > - /* turn device off */ > - chip->taos_chip_status = TSL258X_CHIP_SUSPENDED; > - return i2c_smbus_write_byte_data(chip->client, > - TSL258X_CMD_REG | TSL258X_CNTRL, > - 0x00); > -} > - > /* Sysfs Interface Functions */ > > static ssize_t in_illuminance_input_target_show(struct device *dev, > @@ -768,7 +763,6 @@ static int taos_probe(struct i2c_client *clientp, > > mutex_init(&chip->als_mutex); > chip->taos_chip_status = TSL258X_CHIP_UNKNOWN; > - memcpy(chip->taos_config, taos_config, sizeof(chip->taos_config)); > > ret = i2c_smbus_read_byte_data(clientp, > TSL258X_CMD_REG | TSL258X_CHIPID); > @@ -808,7 +802,7 @@ static int taos_probe(struct i2c_client *clientp, > taos_defaults(chip); > > /* Make sure the chip is on */ > - ret = taos_chip_on(indio_dev); > + ret = tsl2583_chip_init_and_power_on(indio_dev); > if (ret < 0) > return ret; > > @@ -825,7 +819,7 @@ static int __maybe_unused taos_suspend(struct device *dev) > mutex_lock(&chip->als_mutex); > > if (chip->taos_chip_status == TSL258X_CHIP_WORKING) { > - ret = taos_chip_off(indio_dev); > + ret = tsl2583_set_power_state(chip, TSL258X_CNTL_PWR_OFF); > chip->taos_chip_status = TSL258X_CHIP_SUSPENDED; > } > > @@ -842,7 +836,7 @@ static int __maybe_unused taos_resume(struct device *dev) > mutex_lock(&chip->als_mutex); > > if (chip->taos_chip_status == TSL258X_CHIP_SUSPENDED) > - ret = taos_chip_on(indio_dev); > + ret = tsl2583_chip_init_and_power_on(indio_dev); > > mutex_unlock(&chip->als_mutex); > return ret; >