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

[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@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