On Sat, Feb 27, 2021 at 6:58 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote: > On Mon, 22 Feb 2021 21:23:12 +0000 > Joe Sandom <joe.g.sandom@xxxxxxxxx> wrote: I will give my review on top of Jonathan's ones. ... > > Datasheet Available at: https://ams.com/tsl25911 Can we use Datasheet tag, please? Datasheet: <URL> > > ...and drop this blank line. > > Signed-off-by: Joe Sandom <joe.g.sandom@xxxxxxxxx> ... > > +config TSL2591 > > + tristate "TAOS TSL2591 ambient light sensor" > > + depends on I2C > > + help > > + Select Y here for support of the AMS/TAOS TSL2591 ambient light sensor, > > + featuring channels for combined visible + IR intensity and lux illuminance. > > + Access als data via iio and sysfs. Supports iio_events. "Access data via IIO ad sysfs." > > + To compile this driver as a module, select M: the > > + module will be called tsl2591. ... > > +/* ALS integration time field value to als time*/ Besides missing space the phrase confuses me (mostly due to "ALS ... als..." passage). > > +#define TSL2591_FVAL_TO_ATIME(x) (((x) + 1) * 100) ... > > +/* TSL2591 command register definitions */ > > +#define TSL2591_CMD_NOP (BIT(5) | BIT(7)) > > +#define TSL2591_CMD_SF_INTSET (BIT(2) | GENMASK(7, 5)) > > +#define TSL2591_CMD_SF_CALS_I (BIT(0) | BIT(2) | GENMASK(7, 5)) > > +#define TSL2591_CMD_SF_CALS_NPI (GENMASK(2, 0) | GENMASK(7, 5)) > > +#define TSL2591_CMD_SF_CNP_ALSI (BIT(1) | BIT(3) | GENMASK(7, 5)) If it's a bit combination, describe each bit field separately, but my guts are telling me that BIT() and GENMASK() use here is wrong. ... > > +/* TSL2591 enable register definitions */ > > +#define TSL2591_PWR_ON BIT(0) > > +#define TSL2591_PWR_OFF 0x00 Similar to above, here you rather use (1 << 0) and (0 << 0), or plain numbers. ... > > +#define TSL2591_CTRL_ALS_INTEGRATION_100MS 0x00 > > +#define TSL2591_CTRL_ALS_INTEGRATION_200MS BIT(0) > > +#define TSL2591_CTRL_ALS_INTEGRATION_300MS BIT(1) > > +#define TSL2591_CTRL_ALS_INTEGRATION_400MS GENMASK(1, 0) > > +#define TSL2591_CTRL_ALS_INTEGRATION_500MS BIT(2) > > +#define TSL2591_CTRL_ALS_INTEGRATION_600MS (BIT(0) | BIT(2)) Similar to the above. Drop all BIT() / GENMASK() use here and convert to plain numbers. ... > > +#define TSL2591_CTRL_ALS_LOW_GAIN 0x00 > > +#define TSL2591_CTRL_ALS_MED_GAIN BIT(4) > > +#define TSL2591_CTRL_ALS_HIGH_GAIN BIT(5) Ditto. And so on. Please, revisit all descriptions above and below. ... > > +#define TSL2591_ALS_MAX_VALUE 65535 If it's limited by the amount of bits in use, I prefer to spell it as (BIT(16) - 1), and we will immediately see this implication. ... > > +/* > > + * Period table is ALS persist cycle x integration time setting > > + * Integration times: 100ms, 200ms, 300ms, 400ms, 500ms, 600ms > > + * ALS cycles: 1, 2, 3, 5, 10, 20, 25, 30, 35, 40, 45, 50, 55, 60 > > + */ > > +static const char * const tsl2591_als_period_list[] = { > > + "0.1 0.2 0.3 0.5 1.0 2.0 2.5 3.0 3.5 4.0 4.5 5.0 5.5 6.0", > > + "0.2 0.4 0.6 1.0 2.0 4.0 5.0 6.0 7.0 8.0 9.0 10.0 11.0 12.0", > > + "0.3 0.6 0.9 1.5 3.0 6.0 7.5 9.0 10.5 12.0 13.5 15.0 16.5 18.0", > > + "0.4 0.8 1.2 2.0 4.0 8.0 10.0 12.0 14.0 16.0 18.0 20.0 22.0 24.0", > > + "0.5 1.0 1.5 2.5 5.0 10.0 12.5 15.0 17.5 20.0 22.5 25.0 27.5 30.0", > > + "0.6 1.2 1.8 3.0 6.0 12.0 15.0 18.0 21.0 24.0 27.0 30.0 33.0 36.0", > > +}; Okay... But it can be generated I guess. ... > > +static int tsl2591_als_time_to_fval(const u32 als_integration_time) > > +{ > > + int i; > > + > > + for (i = 0; i < ARRAY_SIZE(tsl2591_int_time_available); ++i) { > > + if (als_integration_time == tsl2591_int_time_available[i]) > > + return ((als_integration_time / 100) - 1); Looks like a reversed function of a macro you have above. Care to introduce a counterpart macro instead and use it here? > > + if (i == (ARRAY_SIZE(tsl2591_int_time_available) - 1)) > > + break; This doesn't make any sense to me. > > + } > > + > > + return -EINVAL; > > +} ... > > +static int tsl2591_wait_adc_complete(struct tsl2591_chip *chip) > > +{ > > + struct i2c_client *client = chip->client; > > + struct tsl2591_als_settings settings = chip->als_settings; > > + int delay = TSL2591_FVAL_TO_ATIME(settings.als_int_time); Move the assignment closer to the conditional below. > > + int ret; > > + int i; > > + > > + if (!delay) > > + return -EINVAL; > > + > > + /* > > + * Sleep for ALS integration time to allow enough time > > + * for an ADC read cycle to complete. Check status after > > + * delay for ALS valid Missed period. > > + */ > > + msleep(delay); > > + > > + /* Check for status ALS valid flag for up to 100ms */ > > + for (i = 0; i < TSL2591_ALS_STS_VALID_COUNT; ++i) { i++ works perfectly. > > + ret = i2c_smbus_read_byte_data(client, > > + TSL2591_CMD_NOP | TSL2591_STATUS); > > + if (ret < 0) { > > + dev_err(&client->dev, "Failed to read register\n"); > > + return -EINVAL; > > + } > > + ret = FIELD_GET(TSL2591_STS_ALS_VALID_MASK, ret); > > + if (ret == TSL2591_STS_VAL_HIGH_MASK) > > + break; > > + > > + if (i == (TSL2591_ALS_STS_VALID_COUNT - 1)) In many cases you added too many parentheses. It's not a LISP language :-) > > + return -ENODATA; > > + > > + usleep_range(9000, 10000); > > + } This can be done using iopoll.h and readx_poll_timeout() helper. > > + return 0; > > +} ... > > +static int tsl2591_read_channel_data(struct iio_dev *indio_dev, > > + struct iio_chan_spec const *chan, > > + int *val, int *val2) > > +{ > > + struct tsl2591_chip *chip = iio_priv(indio_dev); > > + struct tsl2591_als_settings *settings = &chip->als_settings; > > + struct i2c_client *client = chip->client; > > + int ret; > > + u8 als_data[TSL2591_NUM_DATA_REGISTERS]; Try to keep reversed xmas tree order in the definition block(s). > > + This should not be here. > > + int counts_per_lux; > > + int lux; > > + int gain_multi; > > + int int_time_fval; > > + Ditto. > > + u16 als_ch0; > > + u16 als_ch1; > > + > > + ret = tsl2591_wait_adc_complete(chip); > > + if (ret < 0) { > > + dev_err(&client->dev, "No data available. Err: %d\n", ret); > > + return ret; > > + } > > + > > + ret = i2c_smbus_read_i2c_block_data(client, > > + TSL2591_CMD_NOP | TSL2591_C0_DATAL, > > + sizeof(als_data), als_data); > > + if (ret < 0) { > > + dev_err(&client->dev, "Failed to read data bytes"); > > + return ret; > For comment below if (ret), that won't work on these calls because IIRC they > return the number of bytes transferred. However, you can move the check locally > that this is the right length and ensure 0 is returned for the good path. > > + } > > + als_ch0 = le16_to_cpup((const __le16 *)&als_data[0]); > > + als_ch1 = le16_to_cpup((const __le16 *)&als_data[2]); The casting looks wrong. Why do you need it? Perhaps your type of als_data is not okay? Or perhaps you need to use get_unaligned_le16()? I dunno. > > + switch (chan->type) { > > + case IIO_INTENSITY: > > + if (chan->channel2 == IIO_MOD_LIGHT_BOTH) > > + *val = als_ch0; > > + else if (chan->channel2 == IIO_MOD_LIGHT_IR) > > + *val = als_ch1; > > + else > > + return -EINVAL; > > + break; > > + case IIO_LIGHT: > > + gain_multi = tsl2591_gain_to_multiplier(settings->als_gain); > > + if (gain_multi < 0) { > > + dev_err(&client->dev, "Invalid multiplier"); > > + return gain_multi; > > + } > > + > > + int_time_fval = TSL2591_FVAL_TO_ATIME(settings->als_int_time); > > + /* Calculate counts per lux value */ > > + counts_per_lux = > > + (int_time_fval * gain_multi) / TSL2591_LUX_COEFFICIENT; One line. > > + dev_dbg(&client->dev, "Counts Per Lux: %d\n", counts_per_lux); > > + /* Calculate lux value */ > > + lux = ((als_ch0 - als_ch1) * > > + (1000 - ((als_ch1 * 1000) / als_ch0))) / counts_per_lux; > > + dev_dbg(&client->dev, "Raw lux calculation: %d\n", lux); > > + > > + /* Divide by 1000 to get real lux value before scaling */ > > + *val = lux / 1000; Doing this before the following one makes precision drop. Or not? > > + /* Get the decimal part of lux reading */ > > + *val2 = ((lux - (*val * 1000)) * 1000); > > + > > + break; > > + default: > > + return -EINVAL; > > + } > > + > > + return ret; > > See above for why: I'd return 0 here > > > +} ... > > + als_lower_l = (als_settings.als_lower_thresh & 0x00FF); > > Given you are writing these into a byte field, probably better to express those > masks as 0xFF. u8 type may never be bigger than 255. Masks like that are redundant. > > + als_lower_h = ((als_settings.als_lower_thresh >> 8) & 0x00FF); > > + als_upper_l = (als_settings.als_upper_thresh & 0x00FF); > > + als_upper_h = ((als_settings.als_upper_thresh >> 8) & 0x00FF); Ditto. ... > > +static int tsl2591_write_raw(struct iio_dev *indio_dev, > > + struct iio_chan_spec const *chan, > > + int val, int val2, long mask) > > +{ > > + struct tsl2591_chip *chip = iio_priv(indio_dev); > > + > > + u8 gain; > > + u32 int_time; > > + int ret; No need for power management? > > + mutex_lock(&chip->als_mutex); > > + mutex_unlock(&chip->als_mutex); > > + return ret; > > +} ... > > +static int tsl2591_read_event_value(struct iio_dev *indio_dev, > > + const struct iio_chan_spec *chan, > > + enum iio_event_type type, > > + enum iio_event_direction dir, > > + enum iio_event_info info, int *val, > > + int *val2) Ditto. ... > > +static int __maybe_unused tsl2591_resume(struct device *dev) > > +{ > > + struct iio_dev *indio_dev = dev_get_drvdata(dev); > > + struct tsl2591_chip *chip = iio_priv(indio_dev); > > + int ret; > > + int power_state; > > + > > + if (chip->events_enabled) > > + power_state = TSL2591_PWR_ON | > > + TSL2591_ENABLE_ALS_INT | > > + TSL2591_ENABLE_ALS; At least the last two can be on one line. > > + else > > + power_state = TSL2591_PWR_ON | TSL2591_ENABLE_ALS; > > + > > + mutex_lock(&chip->als_mutex); > > + ret = tsl2591_set_power_state(chip, power_state); > > + mutex_unlock(&chip->als_mutex); > > + > > + return ret; > > +} ... > > +static const struct dev_pm_ops tsl2591_pm_ops = { > > + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, > > + pm_runtime_force_resume) One line? > > + SET_RUNTIME_PM_OPS(tsl2591_suspend, tsl2591_resume, NULL) > > +}; ... > > +static irqreturn_t tsl2591_event_handler(int irq, void *private) > > +{ > > + /* Clear ALS irq */ > > + ret = i2c_smbus_write_byte(client, TSL2591_CMD_SF_CALS_NPI); > > + if (ret < 0) > > + dev_err(&client->dev, "Failed to clear als irq\n"); In the IRQ handler? Hmm... It potentially floods the logs. > > + return IRQ_HANDLED; > > +} ... > > +static const struct of_device_id tsl2591_of_match[] = { > > + { .compatible = "amstaos,tsl2591"}, > > + {}, Comma is not needed on the terminator line. > > +}; -- With Best Regards, Andy Shevchenko
