On Tue, Feb 06, 2024 at 11:30:17PM +1030, Subhajit Ghosh wrote:
> Driver support for Avago (Broadcom) APDS9306 Ambient Light Sensor.
> It has two channels - ALS and CLEAR. The ALS (Ambient Light Sensor)
> channel approximates the response of the human-eye providing direct
> read out where the output count is proportional to ambient light levels.
> It is internally temperature compensated and rejects 50Hz and 60Hz flicker
> caused by artificial light sources. Hardware interrupt configuration is
> optional. It is a low power device with 20 bit resolution and has
> configurable adaptive interrupt mode and interrupt persistence mode.
> The device also features inbuilt hardware gain, multiple integration time
> selection options and sampling frequency selection options.
>
> This driver also uses the IIO GTS (Gain Time Scale) Helpers Namespace for
> Scales, Gains and Integration time implementation.
..
> +#define APDS9306_ALS_READ_DATA_DELAY_US 20000
(20 * USEC_PER_MSEC) ?
..
> +/*
Make it real kernel doc?
> + * struct part_id_gts_multiplier - Part no. and corresponding gts multiplier
> + *
> + * GTS (Gain Time Scale) are helper functions for Light sensors which along
> + * with hardware gains also has gains associated with Integration times.
> + *
> + * There are two variants of the device with slightly different characteristics,
> + * they have same ADC count for different Lux levels as mentioned in the
> + * datasheet. This multiplier array is used to store the derived Lux per count
> + * value for the two variants to be used by the GTS helper functions.
> + *
> + * part_id: Part ID of the device
> + * max_scale_int: Multiplier for iio_init_iio_gts()
> + * max_scale_nano: Multiplier for iio_init_iio_gts()
(It will be wrong format for referring fields, but easy to fix.)
> + */
..
> +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.
..
> + struct mutex mutex;
checkpatch probably wants this to have a comment.
..
> + 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?
Btw, it's used only twice, open coding saves the LoCs!
Try making the next submission so the driver LoCs is < 1400.
..
> + ret = regmap_read_poll_timeout(data->regmap, APDS9306_MAIN_STATUS_REG,
> + status, (status & (APDS9306_ALS_DATA_STAT_MASK |
> + APDS9306_ALS_INT_STAT_MASK)) ||
Oh, this is interesting indentation...
> + data->read_data_available,
> + APDS9306_ALS_READ_DATA_DELAY_US, delay * 2);
Can we write it as
ret = regmap_read_poll_timeout(data->regmap, APDS9306_MAIN_STATUS_REG,
status, data->read_data_available ||
(status & (APDS9306_ALS_DATA_STAT_MASK |
APDS9306_ALS_INT_STAT_MASK)),
APDS9306_ALS_READ_DATA_DELAY_US, delay * 2);
?
> + if (ret)
> + return ret;
..
> + switch (type) {
> + case IIO_EV_TYPE_THRESH:
> + if (dir == IIO_EV_DIR_EITHER && info == IIO_EV_INFO_PERIOD)
> + return apds9306_event_period_set(data, val);
> + else
Redundant 'else'.
> + return apds9306_event_thresh_set(data, dir, val);
> + case IIO_EV_TYPE_THRESH_ADAPTIVE:
> + return apds9306_event_thresh_adaptive_set(data, val);
> + default:
> + return -EINVAL;
> + }
..
> + if (chan->type == IIO_LIGHT)
> + return int_en & event_ch_is_light;
> + else if (chan->type == IIO_INTENSITY)
Ditto.
> + return int_en & !event_ch_is_light;
..
> + case IIO_EV_TYPE_THRESH_ADAPTIVE:
> + return regmap_field_write(data->regfield_int_thresh_var_en,
> + state);
One line?
..
> +static int apds9306_init_iio_gts(struct apds9306_data *data)
> +{
> + int i, ret, part_id;
> +
> + ret = regmap_read(data->regmap, APDS9306_PART_ID_REG, &part_id);
> + if (ret)
> + return ret;
> +
> + for (i = 0; i < ARRAY_SIZE(apds9306_gts_mul); i++) {
> + if (part_id == apds9306_gts_mul[i].part_id)
break;
}
if (i == ARRAY_SIZE(...))
return -...;
return devm_iio_init_iio_gts(data->dev,
apds9306_gts_mul[i].max_scale_int,
apds9306_gts_mul[i].max_scale_nano,
apds9306_gains, ARRAY_SIZE(apds9306_gains),
apds9306_itimes, ARRAY_SIZE(apds9306_itimes),
&data->gts);
?
> + return devm_iio_init_iio_gts(data->dev,
> + apds9306_gts_mul[i].max_scale_int,
> + apds9306_gts_mul[i].max_scale_nano,
> + apds9306_gains, ARRAY_SIZE(apds9306_gains),
> + apds9306_itimes, ARRAY_SIZE(apds9306_itimes),
> + &data->gts);
> + }
> +
> + return -ENXIO;
> +}
..
> + return dev_err_probe(dev, ret,
> + "regfield initialization failed\n");
One line.
..
> + indio_dev->num_channels =
> + ARRAY_SIZE(apds9306_channels_with_events);
Ditto.
..
> + ret = devm_request_threaded_irq(dev, client->irq, NULL,
> + apds9306_irq_handler, IRQF_ONESHOT,
> + "apds9306_event", indio_dev);
Fix indentation.
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "failed to assign interrupt.\n");
One line.
..
> + return dev_err_probe(dev, ret,
> + "failed to add action or reset\n");
Ditto.
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "failed iio device registration\n");
Ditto.
--
With Best Regards,
Andy Shevchenko
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/[email protected]/
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
On Wed, Feb 07, 2024 at 09:37:37PM +1030, Subhajit Ghosh wrote:
..
> > > +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.
It might be used, but not must be, use your common sense!
In this case it's easier to have all defined properly from the beginning.
..
> > > +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/[email protected]/
I see, what I meant in previous review is to split to separate helpers.
Now, when we see how it looks like and how many actual users, we may
go further and drop them.
> Do you still want me to use the pm functions directly from the calling functions?
It seems a good move forward.
> > Btw, it's used only twice, open coding saves the LoCs!
> Yes, it makes sense.
--
With Best Regards,
Andy Shevchenko