Message-ID: <20240608181331.46cd4ae7@jic23-huawei>
Date: Sat, 8 Jun 2024 18:13:31 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Yasin Lee <yasin.lee.x@...look.com>
Cc: andy.shevchenko@...il.com, lars@...afoo.de, linux-iio@...r.kernel.org,
 linux-kernel@...r.kernel.org, nuno.a@...log.com, swboyd@...omium.org,
 u.kleine-koenig@...gutronix.de, yasin.lee.x@...il.com
Subject: Re: [PATCH v4 2/2] iio:proximity:hx9023s: Add TYHX HX9023S sensor
 driver

On Fri,  7 Jun 2024 19:41:38 +0800
Yasin Lee <yasin.lee.x@...look.com> wrote:

> From: Yasin Lee <yasin.lee.x@...il.com>
> 
> A SAR sensor from NanjingTianyihexin Electronics Ltd.
> 
> The device has the following entry points:
> 
> Usual frequency:
> - sampling_frequency
> 
> Instant reading of current values for different sensors:
> - in_proximity0_raw
> - in_proximity1_raw
> - in_proximity2_raw
> - in_proximity3_raw
> - in_proximity4_raw
> and associated events in events/
> 
> Signed-off-by: Yasin Lee <yasin.lee.x@...il.com>
As Andy already did a detailed review, I only took a fairly quick look.
A few comments inline

Jonathan

> diff --git a/drivers/iio/proximity/hx9023s.c b/drivers/iio/proximity/hx9023s.c
> new file mode 100644
> index 000000000000..b4a583105e03
> --- /dev/null
> +++ b/drivers/iio/proximity/hx9023s.c
> @@ -0,0 +1,1162 @@

> +
> +static int hx9023s_data_lock(struct hx9023s_data *data, bool locked)
> +{
> +	int ret;
> +
> +	if (locked)
> +		ret = regmap_update_bits(data->regmap, HX9023S_DSP_CONFIG_CTRL1,
> +					HX9023S_DATA_LOCK_MASK, HX9023S_DATA_LOCK_MASK);
> +	else
> +		ret = regmap_update_bits(data->regmap, HX9023S_DSP_CONFIG_CTRL1, GENMASK(4, 3), 0);
Odd to write one bit and clear 2.  If really the case, add some documentation
comments.

> +
> +	return ret;
> +}
> +
> +static int hx9023s_ch_cfg(struct hx9023s_data *data)
> +{
> +	int ret;
> +	int i;
> +	u16 reg;
> +	u8 reg_list[HX9023S_CH_NUM * 2];
> +	u8 ch_pos[HX9023S_CH_NUM];
> +	u8 ch_neg[HX9023S_CH_NUM];
> +
> +	data->ch_data[0].cs_position = 0;
> +	data->ch_data[1].cs_position = 2;
> +	data->ch_data[2].cs_position = 4;
> +	data->ch_data[3].cs_position = 6;
> +	data->ch_data[4].cs_position = 8;
> +
> +	for (i = 0; i < HX9023S_CH_NUM; i++) {
> +		if (data->ch_data[i].channel_positive == 255)
> +			ch_pos[i] = 16;
> +		else
> +			ch_pos[i] = data->ch_data[data->ch_data[i].channel_positive].cs_position;
> +		if (data->ch_data[i].channel_negative == 255)
> +			ch_neg[i] = 16;
> +		else
> +			ch_neg[i] = data->ch_data[data->ch_data[i].channel_negative].cs_position;
> +	}
> +
> +	for (i = 0; i < HX9023S_CH_NUM; i++) {
> +		reg = (u16)((0x03 << ch_pos[i]) | (0x02 << ch_neg[i]));
> +		reg_list[i * 2] = (u8)(reg);
> +		reg_list[i * 2 + 1] = (u8)(reg >> 8);

Looks like an odd form of endian manipulation. Try to set reg_list directly or use
an appropriate put_unaligned_*


> +	}
> +
> +	ret = regmap_bulk_write(data->regmap, HX9023S_CH0_CFG_7_0, reg_list, HX9023S_CH_NUM * 2);
> +
> +	return ret;
> +}
>
> +
> +static int hx9023s_ch_en(struct hx9023s_data *data, u8 ch_id, bool en)
> +{
> +	int ret;
> +	unsigned int buf;
> +
> +	ret = regmap_read(data->regmap, HX9023S_CH_NUM_CFG, &buf);
> +	if (ret)
> +		return ret;
> +
> +	data->ch_en_stat = buf;
> +
> +	if (en) {
> +		if (data->ch_en_stat == 0)
> +			data->prox_state_reg = 0;
> +		set_bit(ch_id, &data->ch_en_stat);
> +		ret = regmap_bulk_write(data->regmap, HX9023S_CH_NUM_CFG, &data->ch_en_stat, 1);
> +		if (ret)
> +			return ret;
> +		data->ch_data[ch_id].enable = true;
> +	} else {
> +		clear_bit(ch_id, &data->ch_en_stat);
> +		ret = regmap_bulk_write(data->regmap, HX9023S_CH_NUM_CFG, &data->ch_en_stat, 1);

regmap_write() it's length 1 so not bulk!
Make sure to fix all other cases of this.

>
...


> +
> +static int hx9023s_get_proximity(struct hx9023s_data *data,
> +				const struct iio_chan_spec *chan,
> +				int *val)
> +{
> +	hx9023s_sample(data);
handle errors and return them to userspace.

> +	hx9023s_get_prox_state(data);
handle errors etc.

> +	*val = data->ch_data[chan->channel].diff;
> +	return IIO_VAL_INT;
> +}
> +
>
> +
> +static int hx9023s_set_samp_freq(struct hx9023s_data *data, int val, int val2)
> +{
> +	int i;
> +	int ret;
> +	int period_ms;
> +	struct device *dev = regmap_get_device(data->regmap);
> +
> +	period_ms = div_u64(1000000000ULL, (val * 1000000ULL + val2));
> +
> +	for (i = 0; i < ARRAY_SIZE(hx9023s_samp_freq_table); i++) {
> +		if (period_ms == hx9023s_samp_freq_table[i])
> +			break;
> +	}
> +	if (i == ARRAY_SIZE(hx9023s_samp_freq_table)) {
> +		dev_err(dev, "Period:%dms NOT found!\n", period_ms);
> +		return -EINVAL;
> +	}
> +
> +	ret = regmap_bulk_write(data->regmap, HX9023S_PRF_CFG, &i, 1);
> +
> +	return ret;
return regmap_bulk_write()

> +}


> +static int hx9023s_set_trigger_state(struct iio_trigger *trig, bool state)
> +{
> +	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> +	struct hx9023s_data *data = iio_priv(indio_dev);
> +
> +	guard(mutex)(&data->mutex);
> +	if (state)
> +		hx9023s_interrupt_enable(data);
> +	else if (!data->chan_read)
> +		hx9023s_interrupt_disable(data);
> +	data->trigger_enabled = state;

Ah. So you store this but you also need to use it in resume
along with checking if events are enabled or not.

> +
> +	return 0;
> +}
> +
> +static const struct iio_trigger_ops hx9023s_trigger_ops = {
> +	.set_trigger_state = hx9023s_set_trigger_state,
> +};
> +
> +static irqreturn_t hx9023s_trigger_handler(int irq, void *private)
> +{
> +	struct iio_poll_func *pf = private;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct hx9023s_data *data = iio_priv(indio_dev);
> +	int bit;
> +	int i;
> +
> +	guard(mutex)(&data->mutex);
> +	hx9023s_sample(data);
> +	hx9023s_get_prox_state(data);
No error handling?  If these failed we don't want to provide bad data to
userspace.  Normally we just skip on to iio_trigger_notify_done()
with a warning print to indicate something went wrong.

> +
> +	for_each_set_bit(bit, indio_dev->active_scan_mask, indio_dev->masklength)
> +		data->buffer.channels[i++] =
> +			cpu_to_be16(data->ch_data[indio_dev->channels[bit].channel].diff);

In IIO, for the buffered interface, we general prefer to leave data in the endian
ordering we get from the bus and describe that to userspace.  The basic
philosophy is that userspace has better knowledge on what it is doing with the data
so we provide it the information to process it rather than doing the work in kernel.

> +
> +	iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer, pf->timestamp);
> +
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> 
> +
> +static const struct iio_buffer_setup_ops hx9023s_buffer_setup_ops = {
> +	.preenable = hx9023s_buffer_preenable,
> +	.postdisable = hx9023s_buffer_postdisable,
> +};
> +
> +static int hx9023s_probe(struct i2c_client *client)
> +{
> +	int ret;
> +	unsigned int id;
> +	struct device *dev = &client->dev;
> +	struct iio_dev *indio_dev;
> +	struct hx9023s_data *data;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(struct hx9023s_data));
> +	if (!indio_dev)
> +		return dev_err_probe(dev, -ENOMEM, "device alloc failed\n");
> +
> +	data = iio_priv(indio_dev);
> +	mutex_init(&data->mutex);
> +
> +	data->regmap = devm_regmap_init_i2c(client, &hx9023s_regmap_config);
> +	if (IS_ERR(data->regmap))
> +		return dev_err_probe(dev, PTR_ERR(data->regmap), "regmap init failed\n");
> +
> +	ret = hx9023s_property_get(data);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "dts phase failed\n");
> +
> +	ret = devm_regulator_get_enable(dev, "vdd");
> +	if (ret)
> +		return dev_err_probe(dev, ret, "regulator get failed\n");
> +
> +	fsleep(1000);

If possible, add a specification reference for why that time.
If not add a comment saying that it has been found to work by experimentation.
That can be useful information if it turns out to be a bit short for someone else.

> +
> +	ret = regmap_read(data->regmap, HX9023S_DEVICE_ID, &id);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "id check failed\n");

Having read the ID, normally we'd compare it with expected and print a
warning if it doesn't match, then carry on anyway (to allow for fallback compatibles
being used for future devices that are backwards compatible for this one but
have a different ID).

> +
> +	indio_dev->channels = hx9023s_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(hx9023s_channels);
> +	indio_dev->info = &hx9023s_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->name = "hx9023s";
> +	i2c_set_clientdata(client, indio_dev);
> +
> +	ret = hx9023s_reg_init(data);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "device init failed\n");
> +
> +	ret = hx9023s_ch_cfg(data);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "channel config failed\n");
> +
> +	ret = regcache_sync(data->regmap);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "regcache sync failed\n");
> +
> +	if (client->irq) {
> +		ret = devm_request_threaded_irq(dev, client->irq, hx9023s_irq_handler,
> +						hx9023s_irq_thread_handler, IRQF_ONESHOT,
> +						"hx9023s_event", indio_dev);
> +		if (ret)
> +			return dev_err_probe(dev, ret, "irq request failed\n");
> +
> +		data->trig = devm_iio_trigger_alloc(dev, "%s-dev%d", indio_dev->name,
> +						iio_device_id(indio_dev));
> +		if (!data->trig)
> +			return dev_err_probe(dev, -ENOMEM,
> +					"iio trigger alloc failed\n");
> +
> +		data->trig->ops = &hx9023s_trigger_ops;
> +		iio_trigger_set_drvdata(data->trig, indio_dev);
> +
> +		ret = devm_iio_trigger_register(dev, data->trig);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					"iio trigger register failed\n");
> +	}
> +
> +	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, iio_pollfunc_store_time,
> +					hx9023s_trigger_handler, &hx9023s_buffer_setup_ops);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				"iio triggered buffer setup failed\n");
> +
> +	ret = devm_iio_device_register(dev, indio_dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "iio device register failed\n");
> +
> +	return 0;
> +}
> +
> +static int hx9023s_suspend(struct device *dev)
> +{
> +	struct hx9023s_data *data = iio_priv(dev_get_drvdata(dev));
> +
> +	hx9023s_interrupt_disable(data);

You call these even if the trigger isn't enabled.  The disable is fine, but
you then enable the interrupt on resume even if it wasn't previously enabled.
This needs some state tracking so you restore to previous state.

> +
> +	return 0;
> +}
> +
> +static int hx9023s_resume(struct device *dev)
> +{
> +	struct hx9023s_data *data = iio_priv(dev_get_drvdata(dev));
> +
> +	hx9023s_interrupt_enable(data);
> +
> +	return 0;
> +}
> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(hx9023s_pm_ops, hx9023s_suspend, hx9023s_resume);
> +
> +static const struct acpi_device_id hx9023s_acpi_match[] = {
> +	{ "TYHX9023" },
> +	{}

As Andy mentioned, drop this or add a comment on what device uses it.

> +};
> +MODULE_DEVICE_TABLE(acpi, hx9023s_acpi_match);
> +
> +static const struct of_device_id hx9023s_of_match[] = {
> +	{ .compatible = "tyhx,hx9023s" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, hx9023s_of_match);
> +
> +static const struct i2c_device_id hx9023s_id[] = {
> +	{ "hx9023s" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, hx9023s_id);
> +
> +static struct i2c_driver hx9023s_driver = {
> +	.driver = {
> +		.name = "hx9023s",
> +		.acpi_match_table = hx9023s_acpi_match,
> +		.of_match_table = hx9023s_of_match,
> +		.pm = &hx9023s_pm_ops,
> +
> +		/*
> +		 * The I2C operations in hx9023s_reg_init() and hx9023s_ch_cfg()
> +		 * are time-consuming. prefer async so we don't delay boot
Prefer (capital P as new sentence)

> +		 * if we're builtin to the kernel.
> +		 */
> +		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
> +	},
> +	.probe = hx9023s_probe,
> +	.id_table = hx9023s_id,
> +};
> +module_i2c_driver(hx9023s_driver);
> +
> +MODULE_AUTHOR("Yasin Lee <yasin.lee.x@...il.com>");
> +MODULE_DESCRIPTION("Driver for TYHX HX9023S SAR sensor");
> +MODULE_LICENSE("GPL");

Powered by blists - more mailing lists