Messages in this thread | | | Date | Wed, 7 Feb 2024 21:37:37 +1030 | Subject | Re: [PATCH v6 5/5] iio: light: Add support for APDS9306 Light Sensor | From | Subhajit Ghosh <> |
| |
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@smile.fi.intel.com/
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
| |