Re: [PATCH] iio: light: add driver for bh1730fvc chips
Hi Pierre, Thank you for the patch! Yet something to improve: [auto build test ERROR on iio/togreg] [also build test ERROR on v4.16-rc2 next-20180222] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Pierre-Bourdon/iio-light-add-driver-for-bh1730fvc-chips/20180222-064336 base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg config: i386-allyesconfig (attached as .config) compiler: gcc-7 (Debian 7.3.0-1) 7.3.0 reproduce: # save the attached .config to linux build tree make ARCH=i386 All errors (new ones prefixed by >>): drivers/iio/light/bh1730.o: In function `bh1730_read_raw': >> bh1730.c:(.text+0x31d): undefined reference to `__udivdi3' bh1730.c:(.text+0x37d): undefined reference to `__udivdi3' bh1730.c:(.text+0x38a): undefined reference to `__udivdi3' >> bh1730.c:(.text+0x3a4): undefined reference to `__divdi3' >> bh1730.c:(.text+0x3b8): undefined reference to `__moddi3' --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: application/gzip
Re: [PATCH] iio: light: add driver for bh1730fvc chips
On Wed, Feb 21, 2018 at 6:15 PM, Pierre Bourdon (delroth)
wrote:
> Hi Daniel,
>
> On Wed, Feb 21, 2018 at 4:31 PM, Daniel Baluta
> wrote:
>> On Wed, Feb 21, 2018 at 2:55 PM, Pierre Bourdon wrote:
>>> Ambient light sensor that supports visible light and IR measurements and
>>> configurable gain/integration time.
>>>
>>
>> Can you have a quick look to existing ROHM light sensor support.
>> Perhaps your sensor
>> is similar enough with existing code.
>>
>> I'm talking about:
>>
>> drivers/iio/light/{bh1750.c bh1780.c }
>
> bh1750 does things sufficiently different in its protocol that the
> amount of common code would be fairly small. Unlike bh1730/bh1780 it's
> not fully SMBus compatible for example.
>
> bh1780 could probably be implemented as a subset of bh1730. It's
> basically a simplified bh1730 with less knobs. But there is still a
> significant amount of differences that make common code difficult to
> extract. For example, bh1780 reads lux values directly from the chip,
> whereas bh1730 has two raw channels that need to be combined in
> software to get a processed lux value. It could be done, but my gut
> feeling is that writing a "HAL" for this is going to make things more
> complicated, not simpler.
>
I only had a very quick look at existing ROHM sensors drivers. So, sure
if it doesn't makes sense then we can have bh1730 as a standalone driver.
>>
>> Usually the devicetree binding part should be sent as a separate patch
>> with device-tree mailing list and maintainer at CC.
>>
>> Use ./scripts/get_maintainer.pl to find the exact emails.
>>
>> Mind that you should sent a patchseries:
>>
>> 1/2 - driver code
>> 2/2 - documentation
>
> Will do. Should the of_match_table in the driver code be part of 1/2
> (driver) or 2/2 (dt-bindings)?
>
Should be part of 1/2. 2/2 should only containt the patch for Documentation.
> I'll send a v2 with this + the other changes you've suggested.
Great thanks!
Any idea how to make my gmail not to wrap the lines in plain text mode? :D
It drives me crazy when I reread my emails.
thanks,
Daniel.
Re: [PATCH] iio: light: add driver for bh1730fvc chips
Hi Daniel,
On Wed, Feb 21, 2018 at 4:31 PM, Daniel Baluta wrote:
> On Wed, Feb 21, 2018 at 2:55 PM, Pierre Bourdon wrote:
>> Ambient light sensor that supports visible light and IR measurements and
>> configurable gain/integration time.
>>
>
> Can you have a quick look to existing ROHM light sensor support.
> Perhaps your sensor
> is similar enough with existing code.
>
> I'm talking about:
>
> drivers/iio/light/{bh1750.c bh1780.c }
bh1750 does things sufficiently different in its protocol that the
amount of common code would be fairly small. Unlike bh1730/bh1780 it's
not fully SMBus compatible for example.
bh1780 could probably be implemented as a subset of bh1730. It's
basically a simplified bh1730 with less knobs. But there is still a
significant amount of differences that make common code difficult to
extract. For example, bh1780 reads lux values directly from the chip,
whereas bh1730 has two raw channels that need to be combined in
software to get a processed lux value. It could be done, but my gut
feeling is that writing a "HAL" for this is going to make things more
complicated, not simpler.
My experience in this domain is close to zero, so I'm completely open
to giving it a try if you think merging these two makes sense.
>> Signed-off-by: Pierre Bourdon
>> ---
>> .../devicetree/bindings/iio/light/bh1730.txt | 15 +
>> drivers/iio/light/Kconfig | 10 +
>> drivers/iio/light/Makefile| 1 +
>> drivers/iio/light/bh1730.c| 429 ++
>> 4 files changed, 455 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/iio/light/bh1730.txt
>> create mode 100644 drivers/iio/light/bh1730.c
>>
>> diff --git a/Documentation/devicetree/bindings/iio/light/bh1730.txt
>> b/Documentation/devicetree/bindings/iio/light/bh1730.txt
>> new file mode 100644
>> index ..6b38c4010647
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/light/bh1730.txt
>> @@ -0,0 +1,15 @@
>> +* ROHM BH1730FVC ambient light sensor
>> +
>> +http://www.rohm.com/web/global/products/-/product/BH1730FVC
>> +
>> +Required properties:
>> +
>> + - compatible : should be "rohm,bh1730fvc"
>> + - reg : the I2C address of the sensor
>> +
>> +Example:
>> +
>> +bh1730fvc@29 {
>> + compatible = "rohm,bh1730fvc";
>> + reg = <0x29>;
>> +};
>
> Usually the devicetree binding part should be sent as a separate patch
> with device-tree mailing list and maintainer at CC.
>
> Use ./scripts/get_maintainer.pl to find the exact emails.
>
> Mind that you should sent a patchseries:
>
> 1/2 - driver code
> 2/2 - documentation
Will do. Should the of_match_table in the driver code be part of 1/2
(driver) or 2/2 (dt-bindings)?
I'll send a v2 with this + the other changes you've suggested.
Thanks!
>> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
>> index 93fd421b10d7..286a7f78762b 100644
>> --- a/drivers/iio/light/Kconfig
>> +++ b/drivers/iio/light/Kconfig
>> @@ -63,6 +63,16 @@ config APDS9960
>> To compile this driver as a module, choose M here: the
>> module will be called apds9960
>>
>> +config BH1730
>> + tristate "ROHM BH1730 ambient light sensor"
>> + depends on I2C
>> + help
>> +Say Y here to build support for the ROHM BH1730FVC ambient
>> +light sensor.
>> +
>> +To compile this driver as a module, choose M here: the module will
>> +be called bh1730.
>> +
>> config BH1750
>> tristate "ROHM BH1750 ambient light sensor"
>> depends on I2C
>> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
>> index f714067a7816..eb9010a10536 100644
>> --- a/drivers/iio/light/Makefile
>> +++ b/drivers/iio/light/Makefile
>> @@ -9,6 +9,7 @@ obj-$(CONFIG_ADJD_S311) += adjd_s311.o
>> obj-$(CONFIG_AL3320A) += al3320a.o
>> obj-$(CONFIG_APDS9300) += apds9300.o
>> obj-$(CONFIG_APDS9960) += apds9960.o
>> +obj-$(CONFIG_BH1730) += bh1730.o
>> obj-$(CONFIG_BH1750) += bh1750.o
>> obj-$(CONFIG_BH1780) += bh1780.o
>> obj-$(CONFIG_CM32181) += cm32181.o
>> diff --git a/drivers/iio/light/bh1730.c b/drivers/iio/light/bh1730.c
>> new file mode 100644
>> index ..6912aaa3295b
>> --- /dev/null
>> +++ b/drivers/iio/light/bh1730.c
>> @@ -0,0 +1,429 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * ROHM BH1730 ambient light sensor driver
>> + *
>> + * Copyright (c) 2018 Google, Inc.
>> + * Author: Pierre Bourdon
>> + *
>> + * Based on a previous non-iio BH1730FVC driver:
>> + * Copyright (C) 2012 Samsung Electronics. All rights reserved.
>> + * Author: Won Huh
>> + *
>> + * Data sheets:
>> + * http://www.rohm.com/web/global/datasheet/BH1730FVC/bh1730fvc-e
>> + */
>> +
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +
>> +#define BH1730_CMD_BIT BIT(7)
>> +
>> +#define BH1730_REG_CONTROL 0x00
>> +#define BH1730
Re: [PATCH] iio: light: add driver for bh1730fvc chips
Hi Pierre,
Few comments inline:
On Wed, Feb 21, 2018 at 2:55 PM, Pierre Bourdon wrote:
> Ambient light sensor that supports visible light and IR measurements and
> configurable gain/integration time.
>
Can you have a quick look to existing ROHM light sensor support.
Perhaps your sensor
is similar enough with existing code.
I'm talking about:
drivers/iio/light/{bh1750.c bh1780.c }
> Signed-off-by: Pierre Bourdon
> ---
> .../devicetree/bindings/iio/light/bh1730.txt | 15 +
> drivers/iio/light/Kconfig | 10 +
> drivers/iio/light/Makefile| 1 +
> drivers/iio/light/bh1730.c| 429 ++
> 4 files changed, 455 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/light/bh1730.txt
> create mode 100644 drivers/iio/light/bh1730.c
>
> diff --git a/Documentation/devicetree/bindings/iio/light/bh1730.txt
> b/Documentation/devicetree/bindings/iio/light/bh1730.txt
> new file mode 100644
> index ..6b38c4010647
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/light/bh1730.txt
> @@ -0,0 +1,15 @@
> +* ROHM BH1730FVC ambient light sensor
> +
> +http://www.rohm.com/web/global/products/-/product/BH1730FVC
> +
> +Required properties:
> +
> + - compatible : should be "rohm,bh1730fvc"
> + - reg : the I2C address of the sensor
> +
> +Example:
> +
> +bh1730fvc@29 {
> + compatible = "rohm,bh1730fvc";
> + reg = <0x29>;
> +};
Usually the devicetree binding part should be sent as a separate patch
with device-tree mailing list and maintainer at CC.
Use ./scripts/get_maintainer.pl to find the exact emails.
Mind that you should sent a patchseries:
1/2 - driver code
2/2 - documentation
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index 93fd421b10d7..286a7f78762b 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -63,6 +63,16 @@ config APDS9960
> To compile this driver as a module, choose M here: the
> module will be called apds9960
>
> +config BH1730
> + tristate "ROHM BH1730 ambient light sensor"
> + depends on I2C
> + help
> +Say Y here to build support for the ROHM BH1730FVC ambient
> +light sensor.
> +
> +To compile this driver as a module, choose M here: the module will
> +be called bh1730.
> +
> config BH1750
> tristate "ROHM BH1750 ambient light sensor"
> depends on I2C
> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> index f714067a7816..eb9010a10536 100644
> --- a/drivers/iio/light/Makefile
> +++ b/drivers/iio/light/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_ADJD_S311) += adjd_s311.o
> obj-$(CONFIG_AL3320A) += al3320a.o
> obj-$(CONFIG_APDS9300) += apds9300.o
> obj-$(CONFIG_APDS9960) += apds9960.o
> +obj-$(CONFIG_BH1730) += bh1730.o
> obj-$(CONFIG_BH1750) += bh1750.o
> obj-$(CONFIG_BH1780) += bh1780.o
> obj-$(CONFIG_CM32181) += cm32181.o
> diff --git a/drivers/iio/light/bh1730.c b/drivers/iio/light/bh1730.c
> new file mode 100644
> index ..6912aaa3295b
> --- /dev/null
> +++ b/drivers/iio/light/bh1730.c
> @@ -0,0 +1,429 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ROHM BH1730 ambient light sensor driver
> + *
> + * Copyright (c) 2018 Google, Inc.
> + * Author: Pierre Bourdon
> + *
> + * Based on a previous non-iio BH1730FVC driver:
> + * Copyright (C) 2012 Samsung Electronics. All rights reserved.
> + * Author: Won Huh
> + *
> + * Data sheets:
> + * http://www.rohm.com/web/global/datasheet/BH1730FVC/bh1730fvc-e
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define BH1730_CMD_BIT BIT(7)
> +
> +#define BH1730_REG_CONTROL 0x00
> +#define BH1730_REG_TIMING 0x01
> +#define BH1730_REG_INTERRUPT 0x02
> +#define BH1730_REG_THLLOW 0x03
> +#define BH1730_REG_THLHIGH 0x04
> +#define BH1730_REG_THHLOW 0x05
> +#define BH1730_REG_THHHIGH 0x06
> +#define BH1730_REG_GAIN0x07
> +#define BH1730_REG_ID 0x12
> +#define BH1730_REG_DATA0LOW0x14
> +#define BH1730_REG_DATA0HIGH 0x15
> +#define BH1730_REG_DATA1LOW0x16
> +#define BH1730_REG_DATA1HIGH 0x17
> +
> +#define BH1730_CONTROL_POWER_ONBIT(0)
> +#define BH1730_CONTROL_MEASURE BIT(1)
> +
> +#define BH1730_INTERNAL_CLOCK_NS 2800ULL
> +
> +#define BH1730_DEFAULT_INTEG_MS150
> +
> +enum bh1730_gain {
> + BH1730_GAIN_1X = 0,
> + BH1730_GAIN_2X,
> + BH1730_GAIN_64X,
> + BH1730_GAIN_128X,
> +};
> +
> +struct bh1730_data {
> + struct i2c_client *client;
> + enum bh1730_gain gain;
> + int itime;
> +};
> +
> +static int bh1730_read_word(struct bh1730_data *bh1730, u8 reg)
> +{
> + int ret = i2c_smbus_read_word_data(bh1730->client,
> + BH1730_CMD_BIT | reg)
