On Wed, Sep 09, 2026 at 11:38:13PM -0700, Chang Yu wrote:
This patch adds a driver for the AMS AS7343 14-channel multi-spectral
sensor with I2C interface.
The driver exposes 12 spectral channels (11 visible + 1 near-infrared)
via the IIO sysfs interface. Each channel's raw data is provided as a
16-bit little-endian unsigned integer.
Basic power management (suspend/resume) is supported. Note that power
management is designed in such a way that we only stop spectrual
measurements when susepended and do not power off.
More complex features such as auto-suspend, interrupts, and
configurable gain/integration time will be added in future patches.
It's v3 already. Can you browse the linux-iio@ mailing list archive and see
what are the common comments on the new contributions? I think you may ask
AI to help with the summary. This patch has tons of what has been repeated
over and over...
...
+#include <linux/array_size.h>
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/cleanup.h>
+#include <linux/dev_printk.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/pm.h>
This is implied by pm_runtime.h IIRC.
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/sysfs.h>
+#include <linux/types.h>
...
+/* AS7343 registers */
+#define AS7343_ID 0x5a
+
+#define AS7343_ENABLE 0x80
Make sure the indentation of the definition of the same kind are the same.
...
+/*
+ * Integration time is calculated as (ATIME + 1) * ((ASTEP + 1) * 2.78us).
+ * Setting a 30 * 1.67ms = 50.1ms integration test as the default for now.
+ */
+#define AS7343_ATIME 0x81
I believe this one is related to the register offsets? (see above why)
...
+static int as7343_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan, int *val,
+ int *val2, long mask)
Please, split logically.
struct iio_chan_spec const *chan,
int *val, int *val2, long mask)
+{
+ struct as7343_data *data = iio_priv(indio_dev);
+ struct device *dev = regmap_get_device(data->regmap);
+ int ret;
+ unsigned int unused;
+ __le16 result;
Preserve reversed xmas tree order.
+ ret = pm_runtime_resume_and_get(dev);
+ if (ret)
+ return ret;
Use PM_RUNTIME_ACQUIRE*().
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW: {
+ /* Wait until integration time passes for all 3 cycles. */
+ msleep(160);
+
+ /*
+ * Reading ASTATUS latches all data registers to this read.
+ * We don't care about the returned saturation/gain status for
+ * now.
+ */
+ guard(mutex)(&data->mutex);
+ ret = regmap_read(data->regmap, AS7343_ASTATUS, &unused);
+ if (ret)
+ break;
+
+ ret = regmap_bulk_read(data->regmap, chan->address, &result, 2);
sizeof()
+ if (ret)
+ break;
+
+ *val = le16_to_cpu(result);
+ ret = IIO_VAL_INT;
+ break;
+ }
+
+ default:
+ ret = -EINVAL;
+ break;
+ }
+
+ pm_runtime_put(dev);
+ return ret;
+}
(All comments for the above function is what has been repeated in many
contributions for sure.)
...
+static const char *as7343_channel_label(struct iio_chan_spec const *chan)
+{
+ switch (chan->channel) {
+ case AS7343_CHAN_IDX_FZ:
+ return "FZ";
+ case AS7343_CHAN_IDX_FY:
+ return "FY";
+ case AS7343_CHAN_IDX_FXL:
+ return "FXL";
+ case AS7343_CHAN_IDX_NIR:
+ return "NIR";
+ case AS7343_CHAN_IDX_F2:
+ return "F2";
+ case AS7343_CHAN_IDX_F3:
+ return "F3";
+ case AS7343_CHAN_IDX_F4:
+ return "F4";
+ case AS7343_CHAN_IDX_F6:
+ return "F6";
+ case AS7343_CHAN_IDX_F1:
+ return "F1";
+ case AS7343_CHAN_IDX_F7:
+ return "F7";
+ case AS7343_CHAN_IDX_F8:
+ return "F8";
+ case AS7343_CHAN_IDX_F5:
+ return "F5";
+ default:
+ return NULL;
+ }
Why not keeping this in a static array?
+}
+
+static int as7343_read_label(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan, char *label)
+{
+ const char *name;
+
+ name = as7343_channel_label(chan);
+ if (!name)
+ return -EINVAL;
Why? Can't it be taken from DT?
+ return sysfs_emit(label, "%s\n", name);
+}
...
+static const struct regmap_config as7343_regmap_config = {
+ .name = "as7343",
+ .reg_bits = 8,
+ .val_bits = 8,
+ .max_register = AS7343_MAX,
+ .reg_format_endian = REGMAP_ENDIAN_LITTLE,
+ .val_format_endian = REGMAP_ENDIAN_LITTLE,
+ .cache_type = REGCACHE_NONE,
Why?! This needs a very good justification.
+};
...
+static int as7343_setup_device(struct device *dev, struct as7343_data *data)
+{
struct regmap *map = data->regmap;
will help to reduce verbosity of the below, and might even save some LoC...
+ unsigned int val;
+ __le16 step;
+ int ret;
+
+ /* Power on */
+ ret = regmap_set_bits(data->regmap, AS7343_ENABLE, AS7343_ENABLE_PON);
+ if (ret)
+ return ret;
+
+ /* Need to set REG_BANK to 1 before we can access ID */
+ ret = regmap_set_bits(data->regmap, AS7343_CFG0, AS7343_CFG0_REG_BANK);
+ if (ret)
+ return ret;
+
+ ret = regmap_read(data->regmap, AS7343_ID, &val);
+ if (ret)
+ return ret;
+
+ if (val != 0x81)
+ dev_info(dev, "Unknown device ID: %x\n", val);
+
+ ret = regmap_clear_bits(data->regmap, AS7343_CFG0,
+ AS7343_CFG0_REG_BANK);
...for example, here:
ret = regmap_clear_bits(map, AS7343_CFG0, AS7343_CFG0_REG_BANK);
+ if (ret)
+ return ret;
+
+ /* Configure the SMUX to readout all channels */
+ ret = regmap_update_bits(
Huh?! Please, check the formatting and indentation style.
+ data->regmap, AS7343_CFG20, AS7343_CFG20_AUTO_SMUX,
+ FIELD_PREP(AS7343_CFG20_AUTO_SMUX,
+ AS7343_CFG20_AUTO_SMUX_READOUT_ALL));
+ if (ret)
+ return ret;
+
+ /* Set 50.1ms integration time and x256 gain for now */
+ step = cpu_to_le16(AS7343_ASTEP_VAL);
+ ret = regmap_bulk_write(data->regmap, AS7343_ASTEP, &step, 2);
+ if (ret)
+ return ret;
+
+ ret = regmap_write(data->regmap, AS7343_ATIME, AS7343_ATIME_VAL);
+ if (ret)
+ return ret;
+
+ return regmap_update_bits(data->regmap, AS7343_CFG1, AS7343_CFG1_AGAIN,
+ FIELD_PREP(AS7343_CFG1_AGAIN,
+ AS7343_CFG1_AGAIN_X256));
+}
...
+static int as7343_suspend(struct device *dev)
+{
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct as7343_data *data = iio_priv(indio_dev);
+
+ return regmap_clear_bits(data->regmap, AS7343_ENABLE,
+ AS7343_ENABLE_SP_EN);
+}
+
+static int as7343_resume(struct device *dev)
+{
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct as7343_data *data = iio_priv(indio_dev);
+
+ return regmap_set_bits(data->regmap, AS7343_ENABLE,
+ AS7343_ENABLE_SP_EN);
+}
Same, use temporary for struct regmap.
...
+static int as7343_probe(struct i2c_client *client)
+{
+ struct device *dev = &client->dev;
+ struct as7343_data *data;
+ struct iio_dev *indio_dev;
+ struct regmap *regmap;
+ int ret;
+
+ indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ regmap = devm_regmap_init_i2c(client, &as7343_regmap_config);
+ if (IS_ERR(regmap))
+ return PTR_ERR(regmap);
+
+ data = iio_priv(indio_dev);
+ i2c_set_clientdata(client, indio_dev);
+ data->regmap = regmap;
+ mutex_init(&data->mutex);
devm_mutex_init().
+
+ indio_dev->name = "as7343";
+ indio_dev->info = &as7343_info;
+ indio_dev->channels = as7343_channels;
+ indio_dev->num_channels = ARRAY_SIZE(as7343_channels);
+ indio_dev->modes = INDIO_DIRECT_MODE;
+
+ ret = devm_regulator_get_enable(dev, "vdd");
+ if (ret)
+ return ret;
+
+ ret = as7343_setup_device(dev, data);
+ if (ret)
+ return ret;
+
+ ret = devm_add_action_or_reset(dev, as7343_suspend_action, dev);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "Failed to add suspend action\n");
+
+ ret = pm_runtime_set_active(dev);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "Failed to activate PM runtime\n");
+
+ ret = devm_pm_runtime_enable(dev);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to enable PM runtime\n");
+
+ /* Start measurements */
+ ret = regmap_set_bits(data->regmap, AS7343_ENABLE, AS7343_ENABLE_SP_EN);
+ if (ret)
+ return ret;
+
+ return devm_iio_device_register(dev, indio_dev);
+}
...
+static DEFINE_RUNTIME_DEV_PM_OPS(as7343_pm_ops, as7343_suspend, as7343_resume,
+ NULL);
Again, split logically. Options are:
static DEFINE_RUNTIME_DEV_PM_OPS(as7343_pm_ops, as7343_suspend, as7343_resume, NULL);
static DEFINE_RUNTIME_DEV_PM_OPS(as7343_pm_ops,
as7343_suspend, as7343_resume, NULL);
static DEFINE_RUNTIME_DEV_PM_OPS(as7343_pm_ops,
as7343_suspend,
as7343_resume,
NULL);
(I personally prefer compromise as depicted in the second example).
...
+static const struct of_device_id as7343_of_match[] = {
+ { .compatible = "ams,as7343" },
+ { },
No comma in the terminator entry.
+};
+MODULE_DEVICE_TABLE(of, as7343_of_match);
+
+static const struct i2c_device_id as7343_id[] = {
+ { .name = "as7343" },
+ { },
Ditto.
+};
+MODULE_DEVICE_TABLE(i2c, as7343_id);
...
+static struct i2c_driver as7343_driver = {
+ .driver = {
+ .name = "as7343",
+ .of_match_table = as7343_of_match,
+ .pm = pm_ptr(&as7343_pm_ops),
+ },
+ .probe = as7343_probe,
+ .id_table = as7343_id,
Indentation of the assignees with tabs makes it harder to maintain (in case
more lines got added it might require to reindent all of them).
+};
+module_i2c_driver(as7343_driver);
--
With Best Regards,
Andy Shevchenko