| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 11 Apr 2023 15:43:51 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Subhajit Ghosh <subhajit.ghosh@...aklogic.com>
Cc: Jonathan Cameron <jic23@...nel.org>,
Lars-Peter Clausen <lars@...afoo.de>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org,
Shreeya Patel <shreeya.patel@...labora.com>,
Paul Gazzillo <paul@...zz.com>,
Zhigang Shi <Zhigang.Shi@...eon.com>,
Dmitry Osipenko <dmitry.osipenko@...labora.com>
Subject: Re: [RFC PATCH 2/2] iio: light: Add support for APDS9306 Light Sensor
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@...aklogic.com>
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@...aklogic.com>
> + */
...
> +#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
Powered by blists - more mailing lists