lkml.org 
[lkml]   [2022]   [Mar]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 3/3] iio: light: Add support for ltrf216a sensor
From

On 27/03/22 20:00, Jonathan Cameron wrote:

Hi Jonathan,

Thanks for your detailed review. I am working on v2 with the modifications
suggested by you.

Just one comment inline.

> On Fri, 25 Mar 2022 16:00:14 +0530
> Shreeya Patel <shreeya.patel@collabora.com> wrote:
>
> Hi Zhigang, Shreeya,
>
> Comments inline.
>
> Thanks,
>
> Jonathan
>> From: Zhigang Shi <Zhigang.Shi@liteon.com>
>>
>> Add initial support for ltrf216a ambient light sensor.
>>
>> Datasheet :-
>> https://gitlab.steamos.cloud/shreeya/iio/-/blob/main/LTR-F216A-QT.pdf
> We now have a Datasheet tag, so make this part of the tag block so automated
> tools can find it easily:
>>
> Datasheet: https://gitlab.steamos.cloud/shreeya/iio/-/blob/main/LTR-F216A-QT.pdf
>> Co-developed-by: Shreeya Patel <shreeya.patel@collabora.com>
>> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
>> Signed-off-by: Zhigang Shi <Zhigang.Shi@liteon.com>
>> ---
>> drivers/iio/light/Kconfig | 10 ++
>> drivers/iio/light/Makefile | 1 +
>> drivers/iio/light/ltrf216a.c | 334 +++++++++++++++++++++++++++++++++++
>> 3 files changed, 345 insertions(+)
>> create mode 100644 drivers/iio/light/ltrf216a.c
>>
>> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
>> index a62c7b4b8678..08fa383a8ca7 100644
>> --- a/drivers/iio/light/Kconfig
>> +++ b/drivers/iio/light/Kconfig
>> @@ -318,6 +318,16 @@ config SENSORS_LM3533
>> changes. The ALS-control output values can be set per zone for the
>> three current output channels.
>>
>> +config LTRF216A
>> + tristate "Liteon LTRF216A Light Sensor"
>> + depends on I2C
>> + help
>> + If you say Y or M here, you get support for Liteon LTRF216A
>> + Ambient Light Sensor.
>> +
>> + If built as a dynamically linked module, it will be called
>> + ltrf216a.
>> +
>> config LTR501
>> tristate "LTR-501ALS-01 light sensor"
>> depends on I2C
>> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
>> index d10912faf964..8fa91b9fe5b6 100644
>> --- a/drivers/iio/light/Makefile
>> +++ b/drivers/iio/light/Makefile
>> @@ -30,6 +30,7 @@ obj-$(CONFIG_SENSORS_ISL29028) += isl29028.o
>> obj-$(CONFIG_ISL29125) += isl29125.o
>> obj-$(CONFIG_JSA1212) += jsa1212.o
>> obj-$(CONFIG_SENSORS_LM3533) += lm3533-als.o
>> +obj-$(CONFIG_LTRF216A) += ltrf216a.o
>> obj-$(CONFIG_LTR501) += ltr501.o
>> obj-$(CONFIG_LV0104CS) += lv0104cs.o
>> obj-$(CONFIG_MAX44000) += max44000.o
>> diff --git a/drivers/iio/light/ltrf216a.c b/drivers/iio/light/ltrf216a.c
>> new file mode 100644
>> index 000000000000..99295358a7fe
>> --- /dev/null
>> +++ b/drivers/iio/light/ltrf216a.c
>> @@ -0,0 +1,334 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * LTRF216A Ambient Light Sensor
>> + *
>> + * Copyright (C) 2021 Lite-On Technology Corp (Singapore)
>> + * Author: Shi Zhigang <Zhigang.Shi@liteon.com>
>> + *
>> + * IIO driver for LTRF216A (7-bit I2C slave address 0x53).
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/init.h>
>> +#include <linux/i2c.h>
>> +#include <linux/mutex.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/sysfs.h>
> mod_devicetable.h for the id tables
>
>> +#include <linux/pm.h>
>> +#include <linux/delay.h>
>> +
>> +#define LTRF216A_DRV_NAME "ltrf216a"
>> +
>> +#define LTRF216A_MAIN_CTRL 0x00
>> +
>> +#define LTRF216A_ALS_MEAS_RATE 0x04
> MEAS_RES seems more appropriate from datasheet.
> As mentioned below, also add defines for all the fields
> of the registers you will access and where the fields are
> obvious numerical things, add defines for the values they
> can take.
>
>
>> +#define LTRF216A_MAIN_STATUS 0x07
>> +#define LTRF216A_CLEAR_DATA_0 0x0A
>> +
>> +#define LTRF216A_ALS_DATA_0 0x0D
>> +
>> +static const int int_time_mapping[] = { 400000, 200000, 100000 };
>> +
>> +struct ltrf216a_data {
>> + struct i2c_client *client;
>> + u32 int_time;
>> + u8 int_time_fac;
>> + u8 als_gain_fac;
>> + struct mutex mutex;
> All locks need a comment to say exactly what they are protecting.
>
>> +};
>> +
>> +/* open air. need to update based on TP transmission rate. */
>> +#define WIN_FAC 1
>> +
>> +static const struct iio_chan_spec ltrf216a_channels[] = {
>> + {
>> + .type = IIO_LIGHT,
>> + .info_mask_separate =
>> + BIT(IIO_CHAN_INFO_PROCESSED) |
>> + BIT(IIO_CHAN_INFO_INT_TIME),
>> + }
>> +};
>> +
>> +static IIO_CONST_ATTR_INT_TIME_AVAIL("0.1 0.2 0.4");
>> +
>> +static struct attribute *ltrf216a_attributes[] = {
>> + &iio_const_attr_integration_time_available.dev_attr.attr,
> please use the read_avail callback and set the appropriate
> _available bit.
>
> That allows in kernel access to this information + is probably
> shorter in this case as you won't have an attribute group etc
> to deal wtih.
>
>> + NULL
>> +};
>> +
>> +static const struct attribute_group ltrf216a_attribute_group = {
>> + .attrs = ltrf216a_attributes,
>> +};
>> +
>> +static int ltrf216a_init(struct iio_dev *indio_dev)
>> +{
>> + int ret;
>> + struct ltrf216a_data *data = iio_priv(indio_dev);
>> +
>> + ret = i2c_smbus_read_byte_data(data->client, LTRF216A_MAIN_CTRL);
>> + if (ret < 0) {
>> + dev_err(&data->client->dev, "Error reading LTRF216A_MAIN_CTRL\n");
>> + return ret;
>> + }
>> +
>> + /* enable sensor */
>> + ret |= 0x02;
> Needs a #define and preferably use
> ret |= FIELD_PREP()...
>
>> + ret = i2c_smbus_write_byte_data(data->client, LTRF216A_MAIN_CTRL, ret);
>> + if (ret < 0) {
>> + dev_err(&data->client->dev, "Error writing LTRF216A_MAIN_CTRL\n");
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int ltrf216a_disable(struct iio_dev *indio_dev)
>> +{
>> + int ret;
>> + struct ltrf216a_data *data = iio_priv(indio_dev);
>> +
>> + ret = i2c_smbus_write_byte_data(data->client, LTRF216A_MAIN_CTRL, 0);
>> + if (ret < 0)
>> + dev_err(&data->client->dev, "Error writing LTRF216A_MAIN_CTRL\n");
>> +
>> + return ret;
>> +}
>> +
>> +static int ltrf216a_set_it_time(struct ltrf216a_data *data, int itime)
>> +{
>> + int i, ret, index = -1;
>> + u8 reg;
>> +
>> + for (i = 0; i < ARRAY_SIZE(int_time_mapping); i++) {
>> + if (int_time_mapping[i] == itime) {
>> + index = i;
>> + break;
>> + }
>> + }
>> + /* Make sure integration time index is valid */
>> + if (index < 0)
>> + return -EINVAL;
>> +
>> + if (index == 0) {
> Switch statement seems more appropriate than this stack of if else
>
>> + reg = 0x03;
> reg isn't a great name as I assume this is the value, not the address
> which was my first thought... Perhaps reg_val?
>> + data->int_time_fac = 4;
>> + } else if (index == 1) {
>> + reg = 0x13;
>> + data->int_time_fac = 2;
>> + } else {
>> + reg = (index << 4) | 0x02;
> Unless I'm missing something index == 2 if we get here.
> So why the calculation? I'd suggest defining the two fields and using
> FIELD_PREP() to set up each part probably to one of a set of
> #define LTRF216A_ALS_MEAS_RATE_

I think the calculation here is to set the default value when the
integration time = 1. In this case, reg value will be 34 (0x22) which
is the default value of ALS_MEAS_RATE register.

I will still confirm it once from Zhigang before sending a v2.

>> + data->int_time_fac = 1;
>> + }
>> +
>> + ret = i2c_smbus_write_byte_data(data->client, LTRF216A_ALS_MEAS_RATE, reg);
> Called MEAS_RES on the datasheet, why this name for the register?
>
>> + if (ret < 0)
>> + return ret;
>> +
>> + data->int_time = itime;
>> +
>> + return 0;
>> +}
>> +
>> +static int ltrf216a_get_it_time(struct ltrf216a_data *data, int *val, int *val2)
>> +{
>> + *val = 0;
>> + *val2 = data->int_time;
> I'd put this inline as it avoids a question I had at the call site on why
> you passed *val in as it would always be 0.
>
>> +
>> + return IIO_VAL_INT_PLUS_MICRO;
>> +}
>> +
>> +static int ltrf216a_read_data(struct ltrf216a_data *data, u8 addr)
>> +{
>> + int ret;
>> + int tries = 25;
>> + int val_0, val_1, val_2;
>> +
>> + while (tries--) {
>> + ret = i2c_smbus_read_byte_data(data->client, LTRF216A_MAIN_STATUS);
>> + if (ret < 0)
>> + return ret;
>> + if (ret & 0x08)
> That 0x08 is a magic number and also better defined using BIT(3)
> Anyhow, use a define for that.
>
>> + break;
>> + msleep(20);
>> + }
>> +
>> + val_0 = i2c_smbus_read_byte_data(data->client, addr);
> All of these can return errors so you should check.
> Device doesn't support any larger reads?
>
>> + val_1 = i2c_smbus_read_byte_data(data->client, addr + 1);
>> + val_2 = i2c_smbus_read_byte_data(data->client, addr + 2);
>> + ret = (val_2 << 16) + (val_1 << 8) + val_0;
> This is a le24_to_cpu() conversion.
> Preferred choice would be to use something like
> u8 buf[3];
> int i;
>
> for (i = 0; i < 3; i++) {
> ret = i2c_smbus_read_byte_data(data->client, addr);
> if (ret < 0)
> return ret;
> buf[i] = ret;
> }
> return le24_to_cpu(buf);
>
>> +
>> + return ret;
>> +}
>> +
>> +static int ltrf216a_get_lux(struct ltrf216a_data *data)
>> +{
>> + int greendata, cleardata, lux;
>> +
>> + greendata = ltrf216a_read_data(data, LTRF216A_ALS_DATA_0);
>> + cleardata = ltrf216a_read_data(data, LTRF216A_CLEAR_DATA_0);
>> +
>> + if (greendata < 0 || cleardata < 0)
> I'd rather see the error passed all the way up than eaten here.
>
>> + lux = 0;
>> + else
>> + lux = greendata * 8 * WIN_FAC / data->als_gain_fac / data->int_time_fac / 10;
> This feels like it would be better reported as
> IIO_VAL_FRACTIONAL;
> Won't make any difference if the only user is sysfs, but if anyone
> ever wants to use the in kernel access to this device they will get
> access to better precision.
>
> Also possible the core handling for IIO_VAL_FRACTIONAL will do a better
> job on retaining precision.
>
>> +
>> + return lux;
>> +}
>> +
>> +static int ltrf216a_read_raw(struct iio_dev *indio_dev,
>> + struct iio_chan_spec const *chan, int *val,
>> + int *val2, long mask)
>> +{
>> + int ret;
>> + struct ltrf216a_data *data = iio_priv(indio_dev);
>> +
>> + mutex_lock(&data->mutex);
>> +
>> + switch (mask) {
>> + case IIO_CHAN_INFO_PROCESSED:
>> + ret = ltrf216a_get_lux(data);
>> + *val = ret;
>> + ret = IIO_VAL_INT;
>> + break;
>> + case IIO_CHAN_INFO_INT_TIME:
>> + ret = ltrf216a_get_it_time(data, val, val2);
>> + break;
>> + default:
>> + ret = -EINVAL;
>> + }
>> +
>> + mutex_unlock(&data->mutex);
>> +
>> + return ret;
>> +}
>> +
>> +static int ltrf216a_write_raw(struct iio_dev *indio_dev,
>> + struct iio_chan_spec const *chan, int val,
>> + int val2, long mask)
>> +{
>> + struct ltrf216a_data *data = iio_priv(indio_dev);
>> + int ret;
>> +
>> + switch (mask) {
>> + case IIO_CHAN_INFO_INT_TIME:
>> + if (val != 0)
>> + return -EINVAL;
>> + mutex_lock(&data->mutex);
>> + ret = ltrf216a_set_it_time(data, val2);
>> + mutex_unlock(&data->mutex);
>> + return ret;
>> + default:
>> + return -EINVAL;
>> + }
>> +}
>> +
>> +static const struct iio_info ltrf216a_info = {
>> + .read_raw = ltrf216a_read_raw,
>> + .write_raw = ltrf216a_write_raw,
>> + .attrs = &ltrf216a_attribute_group,
>> +};
>> +
>> +static int ltrf216a_probe(struct i2c_client *client,
>> + const struct i2c_device_id *id)
> Whilst it's taking a long time to clean out the old
> approach, probe_new() callback preferred for new i2c drivers.
>
>> +{
>> + struct ltrf216a_data *data;
>> + struct iio_dev *indio_dev;
>> + int ret;
>> +
>> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
>> + if (!indio_dev)
>> + return -ENOMEM;
>> +
>> + data = iio_priv(indio_dev);
>> + i2c_set_clientdata(client, indio_dev);
>> + data->client = client;
>> +
>> + mutex_init(&data->mutex);
>> +
>> + indio_dev->info = &ltrf216a_info;
>> + indio_dev->name = LTRF216A_DRV_NAME;
>> + indio_dev->channels = ltrf216a_channels;
>> + indio_dev->num_channels = ARRAY_SIZE(ltrf216a_channels);
>> + indio_dev->modes = INDIO_DIRECT_MODE;
>> +
>> + ret = ltrf216a_init(indio_dev);
>> + if (ret < 0) {
>> + dev_err(&client->dev, "ltrf216a chip init failed\n");
>> + return ret;
> As below, slightly preference for dev_err_probe().
> It doesn't really matter when deferral isn't a possibility but using
> that function ends up slightly neater.
>
>> + }
> blank line here. Adding separation so we have
> "function call
> error handler
>
> next thing.."
>
> Just makes things a tiny bit easier to read quickly.
>
>
>> + data->int_time = 100000;
>> + data->int_time_fac = 1;
>> + data->als_gain_fac = 3;
>> +
>> + ret = iio_device_register(indio_dev);
>> + if (ret < 0) {
>> + dev_err(&client->dev, "failed to register iio dev\n");
> Once using devm, this can be
> return dev_err_probe(&client->dev, ret,
> "failed to register iio dev\n");
>
> Which is neater but only use this function to print errors for things
> that happen in probe.
>
>> + goto err_init;
>> + }
>> +
>> + return 0;
>> +err_init:
>> + ltrf216a_disable(indio_dev);
>> + return ret;
>> +}
>> +
>> +static int ltrf216a_remove(struct i2c_client *client)
>> +{
>> + struct iio_dev *indio_dev = i2c_get_clientdata(client);
>> +
>> + iio_device_unregister(indio_dev);
>> + ltrf216a_disable(indio_dev);
> As Kryzstof mentioned, use
> devm versions and where they don't exist you can use
> devm_add_action_or_reset() (e.g. for this disable)
>
> That way we don't need error handling or a remove function.
>> +
>> + return 0;
>> +}
>> +
>> +#ifdef CONFIG_PM_SLEEP
>> +static int ltrf216a_suspend(struct device *dev)
>> +{
>> + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>> +
>> + return ltrf216a_disable(indio_dev);
>> +}
>> +
>> +static int ltrf216a_resume(struct device *dev)
>> +{
>> + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>> +
>> + return ltrf216a_init(indio_dev);
>> +}
>> +
>> +static SIMPLE_DEV_PM_OPS(ltrf216a_pm_ops, ltrf216a_suspend, ltrf216a_resume);
>> +#define LTRF216A_PM_OPS (&ltrf216a_pm_ops)
>> +#else
>> +#define LTRF216A_PM_OPS NULL
>> +#endif
> Recently some 'magic' macros were added to pm.h that allow you to just
> specify this stuff without guards
>
> static DEFINE_SIMPLE_PM_OPS(ltrf216a_pm_ops, lt..._)
>
> and use
>
> .pm = pm_sleep_ptr(&ltrf216a_pm_ops),
>
> below
>
> and then the compiler can see enough to remove the unused code
> if CONFIG_PM_SLEEP isn't defined without the need for messy
> __maybe_unused etc.
>
> We are part way through driving this change through IIO and
> less of the way along with the rest of the kernel, but it's
> a nice cleanup so please apply it to new code like this.
>
>> +
>> +static const struct i2c_device_id ltrf216a_id[] = {
>> + { LTRF216A_DRV_NAME, 0},
> Space after 0
>
>> + {}
>> +};
>> +MODULE_DEVICE_TABLE(i2c, ltrf216a_id);
>> +
>> +static const struct of_device_id ltrf216a_of_match[] = {
>> + { .compatible = "ltr,ltrf216a", },
>> + { .compatible = "liteon,ltrf216a", },
>> + {}
>> +};
>> +MODULE_DEVICE_TABLE(of, ltrf216a_of_match);
>> +
>> +static struct i2c_driver ltrf216a_driver = {
>> + .driver = {
>> + .name = LTRF216A_DRV_NAME,
>> + .pm = LTRF216A_PM_OPS,
>> + .of_match_table = ltrf216a_of_match,
>> + },
>> + .probe = ltrf216a_probe,
>> + .remove = ltrf216a_remove,
>> + .id_table = ltrf216a_id,
>> +};
>> +
>> +module_i2c_driver(ltrf216a_driver);
>> +
>> +MODULE_AUTHOR("Shi Zhigang <Zhigang.Shi@liteon.com>");
>> +MODULE_DESCRIPTION("LTRF216A ambient light sensor driver");
>> +MODULE_LICENSE("GPL v2");
>

\
 
 \ /
  Last update: 2022-03-29 22:04    [W:0.113 / U:0.844 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site