lkml.org 
[lkml]   [2024]   [Feb]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v6 5/5] iio: light: Add support for APDS9306 Light Sensor
From
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






\
 
 \ /
  Last update: 2024-05-27 14:52    [W:4.800 / U:0.020 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site