Re: [RFC PATCH 2/2] iio: light: Add support for APDS9306 Light Sensor

From: Andy Shevchenko
Date: Tue Apr 11 2023 - 08:44:04 EST

Next message: Andrew Lunn: "Re: [PATCH net] net: phy: nxp-c45-tja11xx: add remove callback"
Previous message: Mike Rapoport: "Re: [PATCH 22/29] selftests/mm: Move uffd pagemap test to unit test"
In reply to: Subhajit Ghosh: "[RFC PATCH 2/2] iio: light: Add support for APDS9306 Light Sensor"
Next in thread: Subhajit Ghosh: "Re: [RFC PATCH 2/2] iio: light: Add support for APDS9306 Light Sensor"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Tue, Apr 11, 2023 at 09:12:03AM +0800, Subhajit Ghosh wrote:
> Driver support for Avago (Broadcom) APDS9306
> Ambient Light Sensor with als and clear channels.
> This driver exposes raw values for both the channels.

...

> Datasheet at https://docs.broadcom.com/doc/AV02-4755EN
>
> Signed-off-by: Subhajit Ghosh <subhajit.ghosh@xxxxxxxxxxxxxx>

Drop a blank line and make Datasheet: to be a tag.
Datasheet: https://docs.broadcom.com/doc/AV02-4755EN

...

> +/*
> + * APDS-9306/APDS-9306-065 Ambient Light Sensor
> + *
> + * Datasheet: https://docs.broadcom.com/doc/AV02-4755EN
> + *
> + * I2C Address: 0x52

I guess this can be reordered and condensed a bit:

* APDS-9306/APDS-9306-065 Ambient Light Sensor
* I2C Address: 0x52
* Datasheet: https://docs.broadcom.com/doc/AV02-4755EN

> + * Copyright (C) 2023 Subhajit Ghosh <subhajit.ghosh@xxxxxxxxxxxxxx>
> + */

...

> +#include <linux/pm.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/err.h>
> +#include <linux/mutex.h>
> +#include <linux/delay.h>
> +#include <linux/regmap.h>
> +#include <linux/interrupt.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/events.h>
> +#include <linux/regulator/consumer.h>

Sorted?

+ Blank line.

> +#include <asm/unaligned.h>

...

> +enum apds9306_power_states {
> + standby,
> + active,

Namespace? Capital letters?

> +};
> +
> +enum apds9306_int_channels {
> + clear,
> + als,

Ditto.

> +};

...

> +/* Sampling Frequency in uSec */
> +static const int apds9306_repeat_rate_period[] = {
> + 25000, 50000, 100000, 200000, 500000, 1000000,
> + 2000000

Can be on a single line.

> +};

...

Perhaps you want to add a few static_asserts() to be sure that the ARRAY_SIZE()
of the tables are all greater or equal to each others.

...

> +struct apds9306_data {
> + struct i2c_client *client;
> + struct iio_dev *indio_dev;
> + /*
> + * Protect single als reads and device
> + * power states.
> + */
> + struct mutex mutex;
> +
> + struct regmap *regmap;
> + /* Reg: MAIN_CTRL, Field: SW_Reset */
> + struct regmap_field *regfield_sw_reset;
> + /* Reg: MAIN_CTRL, Field: ALS_EN */
> + struct regmap_field *regfield_en;
> + /* Reg: ALS_MEAS_RATE, Field: ALS Resolution/Bit Width */

Why not converting all comments to a kernel-doc for the entire structure?

> + struct regmap_field *regfield_intg_time;
> + /* Reg: ALS_MEAS_RATE, Field: ALS Measurement Rate */
> + struct regmap_field *regfield_repeat_rate;
> + /* Reg: ALS_GAIN, Field: ALS Gain Range */
> + struct regmap_field *regfield_scale;
> + /* Reg: INT_CFG, Field: ALS Interrupt Source */
> + struct regmap_field *regfield_int_src;
> + /* Reg: INT_CFG, Field: ALS Variation Interrupt Mode */
> + struct regmap_field *regfield_int_thresh_var_en;
> + /* Reg: INT_CFG, Field: ALS Interrupt Enable */
> + struct regmap_field *regfield_int_en;
> + /* Reg: INT_PERSISTENCE, Field: ALS_PERSIST */
> + struct regmap_field *regfield_int_persist_val;
> + /* Reg: ALS_THRESH_VAR, Field: ALS_THRES_VAR */
> + struct regmap_field *regfield_int_thresh_var_val;
> +
> + u8 intg_time_idx;
> + u8 repeat_rate_idx;
> + u8 gain_idx;
> + u8 int_ch;
> +};

...

> +static const struct regmap_config apds9306_regmap = {
> + .name = "apds9306_regmap",
> + .reg_bits = 8,
> + .val_bits = 8,
> + .rd_table = &apds9306_readable_table,
> + .wr_table = &apds9306_writable_table,
> + .volatile_table = &apds9306_volatile_table,
> + .precious_table = &apds9306_precious_table,
> + .max_register = APDS9306_ALS_THRES_VAR,
> + .cache_type = REGCACHE_RBTREE,

Do you need an internal regmap lock? If so, why?

> +};

...

> +static ssize_t thresh_channel_select_show(struct device *dev,
> + struct device_attribute *attr, char *buff)
> +{
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct apds9306_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + if (data->int_ch == 1)
> + ret = sprintf(buff, "als\n");

Must be sysfs_emit().

> + else if (data->int_ch == 0)
> + ret = sprintf(buff, "clear\n");

Must be sysfs_emit().

> + else
> + ret = -EINVAL;
> +
> + return ret;

Make the string literal array for these and...

> +}
> +
> +static ssize_t thresh_channel_select_store(struct device *dev,
> + struct device_attribute *attr, const char *buff, size_t len)
> +{
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct apds9306_data *data = iio_priv(indio_dev);
> + int ret, channel;
> +
> + if (len <= 0 || len > 6)
> + return -EINVAL;
> +
> + if (!strncmp(buff, "als", 3))
> + channel = 1;
> + else if (!strncmp(buff, "clear", 5))
> + channel = 0;
> + else
> + return -EINVAL;

...use sysfs_match_string().

> +
> + ret = regmap_field_write(data->regfield_int_src, channel);
> + if (ret)
> + return ret;
> +
> + data->int_ch = channel;
> +
> + return len;
> +}

...

> +static struct attribute *apds9306_event_attributes[] = {
> + &iio_const_attr_thresh_either_period_available.dev_attr.attr,
> + &iio_const_attr_thresh_channels_available.dev_attr.attr,
> + &iio_const_attr_thresh_adaptive_either_values_available.dev_attr.attr,
> + &iio_dev_attr_thresh_channel_select.dev_attr.attr,
> + NULL,

No comma for a terminator entry.

> +};

...

> +static int apds9306_power_state(struct apds9306_data *data,
> + enum apds9306_power_states state)
> +{
> + int ret;
> +
> + /* Reset not included as it causes ugly I2C bus error */
> + switch (state) {
> + case standby:
> + return regmap_field_write(data->regfield_en, 0);
> + case active:
> + ret = regmap_field_write(data->regfield_en, 1);
> + if (ret)
> + return ret;
> + /* 5ms wake up time */
> + usleep_range(5000, 10000);
> + break;
> + default:
> + return -EINVAL;
> + }

> + return 0;

Move that to a single user of this line inside the switch-case.

> +}

...

> + struct device *dev = &data->client->dev;

Why data contains I²C client pointer, what for?

...

> + int ret = 0;

Unneeded assignment...

> + if (en) {
> + ret = pm_runtime_resume_and_get(dev);
> + if (ret < 0) {
> + dev_err(dev, "runtime resume failed: %d\n", ret);
> + return ret;
> + }
> + ret = 0;
> + } else {
> + pm_runtime_mark_last_busy(dev);
> + pm_runtime_put_autosuspend(dev);
> + }
> +
> + return ret;

...just return 0 here.

...

> + while (retries--) {
> + ret = regmap_read(data->regmap, APDS9306_MAIN_STATUS,
> + &status);
> + if (ret) {
> + dev_err(dev, "read status failed: %d\n", ret);
> + return ret;
> + }
> + if (status & APDS9306_ALS_DATA_STAT_MASK)
> + break;
> + /*
> + * In case of continuous one-shot read from userspace,
> + * new data is available after sampling period.
> + * Delays are in the range of 25ms to 2secs.
> + */
> + fsleep(delay);
> + }

regmap_read_poll_timeout().

...

> + *val = get_unaligned_le24(&buff[0]);

buff is enough, but if you want to be too explicit...

...

> + ret = apds9306_runtime_power(data, 0);
> + if (ret)
> + return ret;
> +
> + return ret;

return apds...(...);

...

> + if (apds9306_intg_time[i][0] == val &&
> + apds9306_intg_time[i][1] == val2) {

Too many spaces and wrong indentation.

...

> + if (apds9306_repeat_rate_freq[i][0] == val &&
> + apds9306_repeat_rate_freq[i][1] == val2) {

Ditto.

...

> + if (apds9306_gain[i] == val) {

Too many spaces.

...

> + if (thad > 7)
> + return -EINVAL;

This 7 should be defined with a meaningful name.

...

> + if (val < 0 || val > 7)
> + return -EINVAL;

Ditto.

...

> + struct apds9306_data *data = iio_priv(indio_dev);
> + int ret = -EINVAL;

This assignment is used only once, so make it explicit in that user below.

> + mutex_lock(&data->mutex);
> + switch (mask) {
> + case IIO_CHAN_INFO_INT_TIME:
> + ret = apds9306_intg_time_set(data, val, val2);
> + break;
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + ret = apds9306_sampling_freq_set(data, val, val2);
> + break;
> + case IIO_CHAN_INFO_SCALE:
> + if (val2 != 0)
> + break;
> + ret = apds9306_gain_set(data, val);
> + break;
> + default:
> + ret = -EINVAL;
> + }
> + mutex_unlock(&data->mutex);
> +
> + return ret;
> +}

...

> + /* The interrupt line is released and the interrupt flag is
> + * cleared as a result of reading the status register
> + */

/*
* Style of the multi-line comment
* is wrong.
*/

...

> + break;
> + case IIO_EV_TYPE_THRESH_ADAPTIVE:
> + ret = apds9306_event_thresh_adaptive_get(data, val);
> + if (ret)
> + return ret;

> + break;

Wrong indentation of this in entire switch-case. Why the style is different
from piece of code to piece of code?

...

> + if (val < 0 || val > 0xFFFFF)
> + return -EINVAL;

Definition and use some plain decimal number if it's about the upper limit of
the respective non-bitwise value.

> + put_unaligned_le24(val, buff);
> + return regmap_bulk_write(data->regmap, var, buff, sizeof(buff));
> + case IIO_EV_TYPE_THRESH_ADAPTIVE:
> + return apds9306_event_thresh_adaptive_set(data, val);
> + default:
> + return -EINVAL;
> + }

> + return 0;

Is it dead code?

> +}

...

> + case IIO_EV_TYPE_THRESH:
> + ret = regmap_field_read(data->regfield_int_en, &val);
> + if (ret)
> + return ret;
> + return val;
> + case IIO_EV_TYPE_THRESH_ADAPTIVE:
> + ret = regmap_field_read(data->regfield_int_thresh_var_en, &val);
> + if (ret)
> + return ret;
> + return val;
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;

Ditto.

...

> + switch (type) {
> + case IIO_EV_TYPE_THRESH:
> + ret = regmap_field_read(data->regfield_int_en, &curr_state);
> + if (ret)
> + return ret;
> + if (curr_state == state)
> + return 0;
> + ret = regmap_field_write(data->regfield_int_en, state);
> + if (ret)
> + return ret;
> + mutex_lock(&data->mutex);
> + ret = apds9306_runtime_power(data, state);
> + mutex_unlock(&data->mutex);
> + if (ret)
> + return ret;
> + break;
> + case IIO_EV_TYPE_THRESH_ADAPTIVE:
> + ret = regmap_field_write(data->regfield_int_thresh_var_en, state);
> + if (ret)
> + return ret;
> + break;
> + default:
> + ret = -EINVAL;

Again, use the same style, here you have no lock, so you may return directly.
No need to have this.

> + }
> +
> + return ret;

Why ret?

...

> + regmap_field_write(data->regfield_int_en, 0);
> + /* Clear status */
> + regmap_read(data->regmap, APDS9306_MAIN_STATUS, &status);
> + /* Put the device in standby mode */
> + apds9306_power_state(data, standby);

No error checks at all?

...

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

Re-indent them to have logical split on the lines.

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

...

> +static int apds9306_runtime_suspend(struct device *dev)
> +{
> + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));

WHy? Use dev_get_drvdata() directly.

> + struct apds9306_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + ret = apds9306_power_state(data, standby);
> + if (ret)
> + regcache_cache_only(data->regmap, true);
> +
> + return ret;
> +}
> +
> +static int apds9306_runtime_resume(struct device *dev)
> +{
> + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));

Ditto.

> + struct apds9306_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + regcache_cache_only(data->regmap, false);
> + ret = regcache_sync(data->regmap);

> + if (!ret) {

if (ret)
return ret;

> + ret = apds9306_power_state(data, active);
> + if (ret)
> + regcache_cache_only(data->regmap, true);
> + }
> +
> + return ret;
> +}

...

> +static DEFINE_RUNTIME_DEV_PM_OPS(apds9306_pm_ops, apds9306_runtime_suspend,
> + apds9306_runtime_resume, NULL);

Do a logical split between lines.

static DEFINE_RUNTIME_DEV_PM_OPS(apds9306_pm_ops,
apds9306_runtime_suspend, apds9306_runtime_resume, NULL);

Alternatively

static DEFINE_RUNTIME_DEV_PM_OPS(apds9306_pm_ops,
apds9306_runtime_suspend,
apds9306_runtime_resume,
NULL);

...

> +static struct i2c_driver apds9306_driver = {
> + .driver = {
> + .name = "apds9306",
> + .pm = pm_ptr(&apds9306_pm_ops),
> + .of_match_table = apds9306_of_match,
> + },
> + .probe_new = apds9306_probe,
> + .id_table = apds9306_id,
> +};

> +

Redundant blank line.

> +module_i2c_driver(apds9306_driver);

--
With Best Regards,
Andy Shevchenko

Next message: Andrew Lunn: "Re: [PATCH net] net: phy: nxp-c45-tja11xx: add remove callback"
Previous message: Mike Rapoport: "Re: [PATCH 22/29] selftests/mm: Move uffd pagemap test to unit test"
In reply to: Subhajit Ghosh: "[RFC PATCH 2/2] iio: light: Add support for APDS9306 Light Sensor"
Next in thread: Subhajit Ghosh: "Re: [RFC PATCH 2/2] iio: light: Add support for APDS9306 Light Sensor"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]