Understood. It will be fixed.
+/*
+ * 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>
+ */
Understood. It will be fixed.+#include <linux/iio/events.h>
+#include <linux/regulator/consumer.h>
Sorted?
+ Blank line.
+#include <asm/unaligned.h>
Namespace? Capital letters?Understood. It will be fixed.
+};
+
+enum apds9306_int_channels {
+ clear,
+ als,
Ditto.
+};
Ok. I'll do that.
+/* 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.
...
Yes. Sure.
Why not converting all comments to a kernel-doc for the entire structure?
For event interface - interrupt enable, adaptive interrupt enable,
...
+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?
Sure. I'll use that.
+ else if (data->int_ch == 0)
+ ret = sprintf(buff, "clear\n");
Must be sysfs_emit().
Understood. It will be done.
+ else
+ ret = -EINVAL;
+
+ return ret;
Make the string literal array for these and...
...
+ channel = 1;
+ else if (!strncmp(buff, "clear", 5))
+ channel = 0;
+ else
+ return -EINVAL;
...use sysfs_match_string().
Sorry. Mistake. It will be fixed.+ NULL,
No comma for a terminator entry.
+};
Sorry, I did not get you. Can you please elaborate?
+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.
I copied the implementation. It will be re-implemented.+ struct device *dev = &data->client->dev;
Why data contains I²C client pointer, what for?
Yes. It will be fixed.
...
+ int ret = 0;
Unneeded assignment...
Ok.
+ 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.
According to the regmap_read_poll_timeout() documentation, the maximum time
...
+ 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().
Ok. It will be done.
...
+ *val = get_unaligned_le24(&buff[0]);
buff is enough, but if you want to be too explicit...
Oh yes.
...
+ ret = apds9306_runtime_power(data, 0);
+ if (ret)
+ return ret;
+
+ return ret;
return apds...(...);
Thank you. It will be fixed.
...
+ 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.
Understood.
...
+ if (thad > 7)
+ return -EINVAL;
This 7 should be defined with a meaningful name.
...
+ if (val < 0 || val > 7)
+ return -EINVAL;
Ditto.
...Got it.
+ 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.
Thank you for this. I've been scratching my head for some time on the
...
+ /* 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.
*/
...Ok, it will be done
+ 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.
Yes, seems to be. It will be fixed.
+ default:
+ return -EINVAL;
+ }
+ return 0;
Is it dead code?
Yes, seems to be. It will be fixed.+ default:
+ return -EINVAL;
+ }
+
+ return 0;
Ditto.
Once, it is clear whether regmap's internal locking should be used or not,
...
+ 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?
Missed it. I'll do that.
...
+ 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?
Yes.
...
+ 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.
Thanks. I'll use that function.
+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.
Yes.+
+static int apds9306_runtime_resume(struct device *dev)
+{
+ struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
Ditto.
AlternativelyThank you. Second option looks nice. I'll stick to that.
static DEFINE_RUNTIME_DEV_PM_OPS(apds9306_pm_ops,
apds9306_runtime_suspend,
apds9306_runtime_resume,
NULL);
It will be fixed.
Redundant blank line.