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
