Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <ab1d9746-4d23-efcc-0ee1-d2b8c634becd@tweaklogic.com>
Date:   Wed, 12 Apr 2023 12:29:15 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.0
Subject: Re: [RFC PATCH 2/2] iio: light: Add support for APDS9306 Light Sensor
To:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc:     Jonathan Cameron <jic23@kernel.org>,
        Lars-Peter Clausen <lars@metafoo.de>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Shreeya Patel <shreeya.patel@collabora.com>,
        Paul Gazzillo <paul@pgazz.com>,
        Zhigang Shi <Zhigang.Shi@liteon.com>,
        Dmitry Osipenko <dmitry.osipenko@collabora.com>
References: <20230411011203.5013-1-subhajit.ghosh@tweaklogic.com>
 <20230411011203.5013-3-subhajit.ghosh@tweaklogic.com>
 <ZDVWB9xV9Cdbwyqd@smile.fi.intel.com>
Content-Language: en-US
From:   Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>
In-Reply-To: <ZDVWB9xV9Cdbwyqd@smile.fi.intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi Andy,
Thank you so much for the detailed review.
Appreciate you taking time to do this.
It would be helpful if you could answers few questions inline?

> 
>> +/*
>> + * APDS-9306/APDS-9306-065 Ambient Light Sensor
>> + *
>> + * Datasheet: https://docs.broadcom.com/doc/AV02-4755EN
>> + *
>> + * I2C Address: 0x52
> 
> I guess this can be reordered and condensed a bit:
> 
>   * APDS-9306/APDS-9306-065 Ambient Light Sensor
>   * I2C Address: 0x52
>   * Datasheet: https://docs.broadcom.com/doc/AV02-4755EN
> 
>> + * Copyright (C) 2023 Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>
>> + */
> 
Understood. It will be fixed.

>> +#include <linux/iio/events.h>
>> +#include <linux/regulator/consumer.h>
> 
> Sorted?
> 
> + Blank line.
> 
>> +#include <asm/unaligned.h>
> 
Understood. It will be fixed.

> Namespace? Capital letters?
> 
>> +};
>> +
>> +enum apds9306_int_channels {
>> +	clear,
>> +	als,
> 
> Ditto.
> 
>> +};
> 
Understood. It will be fixed.

> 
>> +/* Sampling Frequency in uSec */
>> +static const int apds9306_repeat_rate_period[] = {
>> +	25000, 50000, 100000, 200000, 500000, 1000000,
>> +	2000000
> 
> Can be on a single line.
> 
>> +};
> 
> ...
> 
> Perhaps you want to add a few static_asserts() to be sure that the ARRAY_SIZE()
> of the tables are all greater or equal to each others.
> 
> ...
Ok. I'll do that.

> 
> Why not converting all comments to a kernel-doc for the entire structure?
>
Yes. Sure.
  
> 
> ...
> 
>> +static const struct regmap_config apds9306_regmap = {
>> +	.name = "apds9306_regmap",
>> +	.reg_bits = 8,
>> +	.val_bits = 8,
>> +	.rd_table = &apds9306_readable_table,
>> +	.wr_table = &apds9306_writable_table,
>> +	.volatile_table = &apds9306_volatile_table,
>> +	.precious_table = &apds9306_precious_table,
>> +	.max_register = APDS9306_ALS_THRES_VAR,
>> +	.cache_type = REGCACHE_RBTREE,
> 
> Do you need an internal regmap lock? If so, why?
For event interface - interrupt enable, adaptive interrupt enable,
upper and lower threshold values, selection of clear or als
channels for interrupt, the mutex in the driver's private data structure
is not used.
I thought to use the regmap's internal locking mechanism for
mutual exclusion as the values are directly written to or read from
the device registers form the write_event(), read_event(),
write_event_config() and read_event_config().
What do you think?
> 
>> +	else if (data->int_ch == 0)
>> +		ret = sprintf(buff, "clear\n");
> 
> Must be sysfs_emit().
Sure. I'll use that.
> 
>> +	else
>> +		ret = -EINVAL;
>> +
>> +	return ret;
> 
> Make the string literal array for these and...
> ...
>> +		channel = 1;
>> +	else if (!strncmp(buff, "clear", 5))
>> +		channel = 0;
>> +	else
>> +		return -EINVAL;
> 
> ...use sysfs_match_string().
Understood. It will be done.

>> +	NULL,
> 
> No comma for a terminator entry.
> 
>> +};
> 
Sorry. Mistake. It will be fixed.
> 
>> +static int apds9306_power_state(struct apds9306_data *data,
>> +		enum apds9306_power_states state)
>> +{
>> +	int ret;
>> +
>> +	/* Reset not included as it causes ugly I2C bus error */
>> +	switch (state) {
>> +	case standby:
>> +		return regmap_field_write(data->regfield_en, 0);
>> +	case active:
>> +		ret = regmap_field_write(data->regfield_en, 1);
>> +		if (ret)
>> +			return ret;
>> +		/* 5ms wake up time */
>> +		usleep_range(5000, 10000);
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
> 
>> +	return 0;
> 
> Move that to a single user of this line inside the switch-case.
Sorry, I did not get you. Can you please elaborate?

>> +	struct device *dev = &data->client->dev;
> 
> Why data contains I²C client pointer, what for?
I copied the implementation. It will be re-implemented.
> 
> ...
> 
>> +	int ret = 0;
> 
> Unneeded assignment...
Yes. It will be fixed.
> 
>> +	if (en) {
>> +		ret = pm_runtime_resume_and_get(dev);
>> +		if (ret < 0) {
>> +			dev_err(dev, "runtime resume failed: %d\n", ret);
>> +			return ret;
>> +		}
>> +		ret = 0;
>> +	} else {
>> +		pm_runtime_mark_last_busy(dev);
>> +		pm_runtime_put_autosuspend(dev);
>> +	}
>> +
>> +	return ret;
> 
> ...just return 0 here.
Ok.
> 
> ...
> 
>> +		while (retries--) {
>> +			ret = regmap_read(data->regmap, APDS9306_MAIN_STATUS,
>> +					&status);
>> +			if (ret) {
>> +				dev_err(dev, "read status failed: %d\n", ret);
>> +				return ret;
>> +			}
>> +			if (status & APDS9306_ALS_DATA_STAT_MASK)
>> +				break;
>> +			/*
>> +			 * In case of continuous one-shot read from userspace,
>> +			 * new data is available after sampling period.
>> +			 * Delays are in the range of 25ms to 2secs.
>> +			 */
>> +			fsleep(delay);
>> +		}
> 
> regmap_read_poll_timeout().
According to the regmap_read_poll_timeout() documentation, the maximum time
to sleep between reads should be less than ~20ms as it uses usleep_range().

If userspace is doing continuous reads, then data is available after sampling
period (25ms to 2sec) or integration time (3.125ms to 400ms) whichever is
greater.

The runtime_suspend() function is called after 5 seconds, so the device is
still active and running.

If the ALS data bit is not set in status reg, it is efficient to sleep for
one sampling period rather than continuously checking the status reg
within ~20ms if we use regmap_read_poll_timeout().

Do you have any suggestions?

> 
> ...
> 
>> +	*val = get_unaligned_le24(&buff[0]);
> 
> buff is enough, but if you want to be too explicit...
Ok. It will be done.
> 
> ...
> 
>> +	ret = apds9306_runtime_power(data, 0);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return ret;
> 
> 	return apds...(...);
Oh yes.
> 
> ...
> 
>> +		if (apds9306_intg_time[i][0] ==  val &&
>> +				apds9306_intg_time[i][1] ==  val2) {
> 
> Too many spaces and wrong indentation.
> 
> ...
> 
>> +		if (apds9306_repeat_rate_freq[i][0] ==  val &&
>> +				apds9306_repeat_rate_freq[i][1] ==  val2) {
> 
> Ditto.
> 
> ...
> 
>> +		if (apds9306_gain[i] ==  val) {
> 
> Too many spaces.
Thank you. It will be fixed.
> 
> ...
> 
>> +	if (thad > 7)
>> +		return -EINVAL;
> 
> This 7 should be defined with a meaningful name.
> 
> ...
> 
>> +	if (val < 0 || val > 7)
>> +		return -EINVAL;
> 
> Ditto.
>
Understood.
> ...
> 
>> +	struct apds9306_data *data = iio_priv(indio_dev);
>> +	int ret = -EINVAL;
> 
> This assignment is used only once, so make it explicit in that user below.
>
Got it.

> 
> ...
> 
>> +	/* The interrupt line is released and the interrupt flag is
>> +	 * cleared as a result of reading the status register
>> +	 */
> 
> /*
>   * Style of the multi-line comment
>   * is wrong.
>   */
> 
Thank you for this. I've been scratching my head for some time on the
style of commenting.
> ...
> 
>> +	break;
>> +	case IIO_EV_TYPE_THRESH_ADAPTIVE:
>> +		ret = apds9306_event_thresh_adaptive_get(data, val);
>> +		if (ret)
>> +			return ret;
> 
>> +	break;
> 
> Wrong indentation of this in entire switch-case. Why the style is different
> from piece of code to piece of code?
> 
> ...
> 
>> +		if (val < 0 || val > 0xFFFFF)
>> +			return -EINVAL;
> 
> Definition and use some plain decimal number if it's about the upper limit of
> the respective non-bitwise value.
Ok, it will be done
> 
>> +	default:
>> +		return -EINVAL;
>> +	}
> 
>> +	return 0;
> 
> Is it dead code?
Yes, seems to be. It will be fixed.

>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
> 
> Ditto.
Yes, seems to be. It will be fixed.
> 
> ...
> 
>> +	switch (type) {
>> +	case IIO_EV_TYPE_THRESH:
>> +		ret = regmap_field_read(data->regfield_int_en, &curr_state);
>> +		if (ret)
>> +			return ret;
>> +		if (curr_state == state)
>> +			return 0;
>> +		ret = regmap_field_write(data->regfield_int_en, state);
>> +		if (ret)
>> +			return ret;
>> +		mutex_lock(&data->mutex);
>> +		ret = apds9306_runtime_power(data, state);
>> +		mutex_unlock(&data->mutex);
>> +		if (ret)
>> +			return ret;
>> +		break;
>> +	case IIO_EV_TYPE_THRESH_ADAPTIVE:
>> +		ret = regmap_field_write(data->regfield_int_thresh_var_en, state);
>> +		if (ret)
>> +			return ret;
>> +		break;
>> +	default:
>> +		ret = -EINVAL;
> 
> Again, use the same style, here you have no lock, so you may return directly.
> No need to have this.
> 
>> +	}
>> +
>> +	return ret;
> 
> Why ret?
Once, it is clear whether regmap's internal locking should be used or not,
this function will be re-implemented.
> 
> ...
> 
>> +	regmap_field_write(data->regfield_int_en, 0);
>> +	/* Clear status */
>> +	regmap_read(data->regmap, APDS9306_MAIN_STATUS, &status);
>> +	/* Put the device in standby mode */
>> +	apds9306_power_state(data, standby);
> 
> 
> No error checks at all?
Missed it. I'll do that.
> 
> ...
> 
>> +		ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
>> +				apds9306_irq_handler, IRQF_TRIGGER_FALLING |
>> +				IRQF_ONESHOT, "apds9306_event", indio_dev);
> 
> Re-indent them to have logical split on the lines.
Yes.

> 
>> +static int apds9306_runtime_suspend(struct device *dev)
>> +{
>> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> 
> WHy? Use dev_get_drvdata() directly.
Thanks. I'll use that function.

>> +
>> +static int apds9306_runtime_resume(struct device *dev)
>> +{
>> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> 
> Ditto.
Yes.

> Alternatively
> 
> static DEFINE_RUNTIME_DEV_PM_OPS(apds9306_pm_ops,
> 				 apds9306_runtime_suspend,
> 				 apds9306_runtime_resume,
> 				 NULL);
> 
Thank you. Second option looks nice. I'll stick to that.
> 
> Redundant blank line.
It will be fixed.


Regards,
Subhajit Ghosh