Messages in this thread | | | Date | Mon, 26 Feb 2024 16:12:23 +0200 | From | Andy Shevchenko <> | Subject | Re: [PATCH v7 5/5] iio: light: Add support for APDS9306 Light Sensor |
| |
On Sun, Feb 18, 2024 at 04:18:26PM +1030, Subhajit Ghosh wrote: > Driver support for Avago (Broadcom) APDS9306 Ambient Light Sensor. > It has two channels - ALS and CLEAR. The ALS (Ambient Light Sensor) > channel approximates the response of the human-eye providing direct > read out where the output count is proportional to ambient light levels. > It is internally temperature compensated and rejects 50Hz and 60Hz flicker > caused by artificial light sources. Hardware interrupt configuration is > optional. It is a low power device with 20 bit resolution and has > configurable adaptive interrupt mode and interrupt persistence mode. > The device also features inbuilt hardware gain, multiple integration time > selection options and sampling frequency selection options. > > This driver also uses the IIO GTS (Gain Time Scale) Helpers Namespace for > Scales, Gains and Integration time implementation.
..
> +/* > + * Available scales with gain 1x - 18x, timings 3.125, 25, 50, 100, 200, 400 mS
"mS" --> "ms."
> + * Time impacts to gain: 1x, 8x, 16x, 32x, 64x, 128x > + */
..
> + /* > + * If this function runs parallel with the interrupt handler, either > + * this reads and clears the status registers or the interrupt handler > + * does. The interrupt handler sets a flag for read data available > + * in our private structure which we read here. > + */ > + ret = regmap_read_poll_timeout(data->regmap, APDS9306_MAIN_STATUS_REG, > + status, data->read_data_available || > + (status & (APDS9306_ALS_DATA_STAT_MASK | > + APDS9306_ALS_INT_STAT_MASK)), > + APDS9306_ALS_READ_DATA_DELAY_US, delay * 2);
> +
Redundant blank line
> + if (ret) > + return ret;
..
> +static int apds9306_init_iio_gts(struct apds9306_data *data) > +{ > + int i, ret, part_id; > + > + ret = regmap_read(data->regmap, APDS9306_PART_ID_REG, &part_id); > + if (ret) > + return ret; > + > + for (i = 0; i < ARRAY_SIZE(apds9306_gts_mul); i++) > + if (part_id == apds9306_gts_mul[i].part_id) > + break; > + > + if (i == ARRAY_SIZE(apds9306_gts_mul)) > + return -ENXIO;
Strange choice of the error code, why not (one of) -ENOENT / -ENODATA ?
> + return devm_iio_init_iio_gts(data->dev, > + apds9306_gts_mul[i].max_scale_int, > + apds9306_gts_mul[i].max_scale_nano, > + apds9306_gains, ARRAY_SIZE(apds9306_gains), > + apds9306_itimes, ARRAY_SIZE(apds9306_itimes), > + &data->gts);
> + > + return -ENXIO;
Dead code.
> +}
..
Jonathan, are you going to apply this and addressing comments at the same time? Or should it be another version?
-- With Best Regards, Andy Shevchenko
| |