Hi Andy,
+ */
...
+static_assert(ARRAY_SIZE(apds9306_repeat_rate_freq) ==
+ APDS9306_NUM_REPEAT_RATES);
Just make that define to be inside [] in the respective array and drop this
static assert. The assertion might make sense to have different arrays to be
synchronized and when their maximums are different due to semantics (not your
case AFAICS).
...
+static_assert(ARRAY_SIZE(apds9306_repeat_rate_period) ==
+ APDS9306_NUM_REPEAT_RATES);
Ditto.
...
I apologize for this. You pointed me out in an earlier review, I misunderstood
it and used the macro in two static asserts! It will be fixed.
+ struct mutex mutex;
checkpatch probably wants this to have a comment.
I used the mainline checkpatch, it did not through any explicit warnings or errors
regarding this.
As per previous review pointed below, I removed the the comment from here to
kernel doc:
https://lore.kernel.org/all/20240121152332.6b15666a@jic23-huawei/
Do you still want me to add a comment before struct mutex?
...
+ struct regmap_field *regfield_sw_reset;
+ struct regmap_field *regfield_en;
+ struct regmap_field *regfield_intg_time;
+ struct regmap_field *regfield_repeat_rate;
+ struct regmap_field *regfield_gain;
+ struct regmap_field *regfield_int_src;
+ struct regmap_field *regfield_int_thresh_var_en;
+ struct regmap_field *regfield_int_en;
+ struct regmap_field *regfield_int_persist_val;
+ struct regmap_field *regfield_int_thresh_var_val;
May we reduce the names by
struct {
...
struct regmap_field *int_persist_val;
struct regmap_field *int_thresh_var_val;
} regfield;
In the code
struct regfield *rf = &priv->regfield;
rf->int...
...
+static struct attribute *apds9306_event_attributes[] = {
+ &iio_const_attr_thresh_either_period_available.dev_attr.attr,
+ &iio_const_attr_thresh_adaptive_either_values_available.dev_attr.attr,
+ NULL
+};
+
+static const struct attribute_group apds9306_event_attr_group = {
+ .attrs = apds9306_event_attributes,
+};
...
+static int apds9306_runtime_power_on(struct device *dev)
+{
+ int ret;
+
+ ret = pm_runtime_resume_and_get(dev);
+ if (ret < 0)
+ dev_err_ratelimited(dev, "runtime resume failed: %d\n", ret);
+
+ return ret;
+}
+
+static int apds9306_runtime_power_off(struct device *dev)
+{
+ pm_runtime_mark_last_busy(dev);
+ pm_runtime_put_autosuspend(dev);
+
+ return 0;
+}
Seems to me like useless wrappers. Why do you need that message?
No specific need for that message, however the wrapper was suggested in a previous review:
https://lore.kernel.org/all/ZTuuUl0PBklbVjb9@xxxxxxxxxxxxxxxxxx/
Do you still want me to use the pm functions directly from the calling functions?
Btw, it's used only twice, open coding saves the LoCs!
Yes, it makes sense.
Try making the next submission so the driver LoCs is < 1400.
The current driver file is 1335 lines, next one, I will definitely try to keep in under 1400 lines.
...
Acknowledging all other review comments. Thank you for reviewing.
Regards,
Subhajit Ghosh
