Re: [PATCH v6 5/5] iio: light: Add support for APDS9306 Light Sensor

[Andy Shevchenko]

On Tue, Feb 06, 2024 at 11:30:17PM +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.

..

> +#define APDS9306_ALS_READ_DATA_DELAY_US	20000

(20 * USEC_PER_MSEC) ?

..

> +/*

Make it real kernel doc?

> + * struct part_id_gts_multiplier - Part no. and corresponding gts multiplier
> + *
> + * GTS (Gain Time Scale) are helper functions for Light sensors which along
> + * with hardware gains also has gains associated with Integration times.
> + *
> + * There are two variants of the device with slightly different characteristics,
> + * they have same ADC count for different Lux levels as mentioned in the
> + * datasheet. This multiplier array is used to store the derived Lux per count
> + * value for the two variants to be used by the GTS helper functions.
> + *
> + * part_id: Part ID of the device
> + * max_scale_int: Multiplier for iio_init_iio_gts()
> + * max_scale_nano: Multiplier for iio_init_iio_gts()

(It will be wrong format for referring fields, but easy to fix.)

> + */

..

> +static_assert(ARRAY_SIZE(apds9306_repeat_rate_freq) ==
> +		APDS9306_NUM_REPEAT_RATES);

Just make that define to be inside [] in the respective array and drop this
static assert. The assertion might make sense to have different arrays to be
synchronized and when their maximums are different due to semantics (not your
case AFAICS).

..

> +static_assert(ARRAY_SIZE(apds9306_repeat_rate_period) ==
> +		APDS9306_NUM_REPEAT_RATES);

Ditto.

..

> +	struct mutex mutex;

checkpatch probably wants this to have a comment.

..

> +	struct regmap_field *regfield_sw_reset;
> +	struct regmap_field *regfield_en;
> +	struct regmap_field *regfield_intg_time;
> +	struct regmap_field *regfield_repeat_rate;
> +	struct regmap_field *regfield_gain;
> +	struct regmap_field *regfield_int_src;
> +	struct regmap_field *regfield_int_thresh_var_en;
> +	struct regmap_field *regfield_int_en;
> +	struct regmap_field *regfield_int_persist_val;
> +	struct regmap_field *regfield_int_thresh_var_val;

May we reduce the names by

	struct {
		...
		struct regmap_field *int_persist_val;
		struct regmap_field *int_thresh_var_val;
	} regfield;

In the code

	struct regfield *rf = &priv->regfield;

	rf->int...

..

> +static struct attribute *apds9306_event_attributes[] = {
> +	&iio_const_attr_thresh_either_period_available.dev_attr.attr,
> +	&iio_const_attr_thresh_adaptive_either_values_available.dev_attr.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group apds9306_event_attr_group = {
> +	.attrs = apds9306_event_attributes,
> +};

..

> +static int apds9306_runtime_power_on(struct device *dev)
> +{
> +	int ret;
> +
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret < 0)
> +		dev_err_ratelimited(dev, "runtime resume failed: %d\n", ret);
> +
> +	return ret;
> +}
> +
> +static int apds9306_runtime_power_off(struct device *dev)
> +{
> +	pm_runtime_mark_last_busy(dev);
> +	pm_runtime_put_autosuspend(dev);
> +
> +	return 0;
> +}

Seems to me like useless wrappers. Why do you need that message?
Btw, it's used only twice, open coding saves the LoCs!
Try making the next submission so the driver LoCs is < 1400.

..

> +	ret = regmap_read_poll_timeout(data->regmap, APDS9306_MAIN_STATUS_REG,
> +				status, (status & (APDS9306_ALS_DATA_STAT_MASK |
> +				APDS9306_ALS_INT_STAT_MASK)) ||

Oh, this is interesting indentation...

> +				data->read_data_available,
> +				APDS9306_ALS_READ_DATA_DELAY_US, delay * 2);

Can we write it as

	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);

?

> +	if (ret)
> +		return ret;

..

> +	switch (type) {
> +	case IIO_EV_TYPE_THRESH:
> +		if (dir == IIO_EV_DIR_EITHER && info == IIO_EV_INFO_PERIOD)
> +			return apds9306_event_period_set(data, val);

> +		else

Redundant 'else'.

> +			return apds9306_event_thresh_set(data, dir, val);
> +	case IIO_EV_TYPE_THRESH_ADAPTIVE:
> +		return apds9306_event_thresh_adaptive_set(data, val);
> +	default:
> +		return -EINVAL;
> +	}

..

> +		if (chan->type == IIO_LIGHT)
> +			return int_en & event_ch_is_light;
> +		else if (chan->type == IIO_INTENSITY)

Ditto.

> +			return int_en & !event_ch_is_light;

..

> +	case IIO_EV_TYPE_THRESH_ADAPTIVE:

> +		return regmap_field_write(data->regfield_int_thresh_var_en,
> +					  state);

One line?

..

> +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(...))
		return -...;

	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 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;
> +}

..

> +		return dev_err_probe(dev, ret,
> +				     "regfield initialization failed\n");

One line.

..

> +		indio_dev->num_channels =
> +				ARRAY_SIZE(apds9306_channels_with_events);

Ditto.

..

> +		ret = devm_request_threaded_irq(dev, client->irq, NULL,
> +				apds9306_irq_handler, IRQF_ONESHOT,
> +					"apds9306_event", indio_dev);

Fix indentation.

> +		if (ret)

> +			return dev_err_probe(dev, ret,
> +					     "failed to assign interrupt.\n");

One line.

..

> +		return dev_err_probe(dev, ret,
> +				     "failed to add action or reset\n");

Ditto.


> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "failed iio device registration\n");

Ditto.

-- 
With Best Regards,
Andy Shevchenko