[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID:
<SN7PR12MB8101EC586E5B365ECFEA3159A4CE2@SN7PR12MB8101.namprd12.prod.outlook.com>
Date: Wed, 19 Jun 2024 02:02:33 +0800
From: Yasin Lee <yasin.lee.x@...look.com>
To: Jonathan Cameron <jic23@...nel.org>
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 2024/6/9 01:13, Jonathan Cameron wrote:
> 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
>
Hi Jonathan,
Thanks for your reply, the details inline below.
Yasin Lee
>> 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.
>
This is indeed an error, modified to only set HX9023S_DATA_LOCK_MASK.
>> +
>> + 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_*
>
Modified to put_unaligned_le16.
>> + }
>> +
>> + 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.
>
OK, I will fix all such issues.
> ...
>
>
>> +
>> +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.
>
Fixed
>> + hx9023s_get_prox_state(data);
> handle errors etc.
>
Fixed
>> + *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()
Fixed
>> +}
>
>> +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.
>
Add if (data->trigger_enabled) in resume Function
>> +
>> + 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.
OK, I added the following error handling logic here:
ret = hx9023s_sample(data);
if (ret) {
dev_warn(dev, "sampling failed\n");
goto out;
}
ret = hx9023s_get_prox_state(data);
if (ret) {
dev_warn(dev, "get prox failed\n");
goto out;
}
>> +
>> + 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.
Yes, I described the reason for using |cpu_to_*| here in another response.
>> +
>> + 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.
I confirmed with the IC design team to drop the delay.
>> +
>> + 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).
>
Added the id_check function to implement the above functionality.
>> +
>> + 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;
>> +}
>> +
Modified in resume
static int hx9023s_resume(struct device *dev)
{
struct hx9023s_data *data = iio_priv(dev_get_drvdata(dev));
if (data->trigger_enabled)
hx9023s_interrupt_enable(data);
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.
Drop this
>> +};
>> +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)
Fixed
>
>> + * 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