Re: [PATCH v6 5/5] iio: light: Add support for APDS9306 Light Sensor

[Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>]

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