| Message ID | 20180626063025.7778-1-matt.ranostay@konsulko.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Matt,
I love your patch! Yet something to improve:
[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on v4.18-rc2 next-20180625]
[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/Matt-Ranostay/media-video-i2c-add-hwmon-support-for-amg88xx/20180626-151137
base: git://linuxtv.org/media_tree.git master
config: x86_64-randconfig-x014-201825 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
>> drivers/media//i2c/video-i2c.c:183:17: error: lvalue required as unary '&' operand
.hwmon_init = &amg88xx_hwmon_init,
^
vim +183 drivers/media//i2c/video-i2c.c
174
175 static const struct video_i2c_chip video_i2c_chip[] = {
176 [AMG88XX] = {
177 .size = &amg88xx_size,
178 .format = &amg88xx_format,
179 .max_fps = 10,
180 .buffer_size = 128,
181 .bpp = 16,
182 .xfer = &amg88xx_xfer,
> 183 .hwmon_init = &amg88xx_hwmon_init,
184 },
185 };
186
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On 06/25/2018 11:30 PM, Matt Ranostay wrote: > AMG88xx has an on-board thermistor which is used for more accurate > processing of its temperature readings from the 8x8 thermopile array > > Cc: linux-hwmon@vger.kernel.org > Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com> > --- > drivers/media/i2c/video-i2c.c | 73 +++++++++++++++++++++++++++++++++++ > 1 file changed, 73 insertions(+) > > diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c > index 0b347cc19aa5..16c3e03af219 100644 > --- a/drivers/media/i2c/video-i2c.c > +++ b/drivers/media/i2c/video-i2c.c > @@ -10,6 +10,8 @@ > > #include <linux/delay.h> > #include <linux/freezer.h> > +#include <linux/hwmon.h> > +#include <linux/hwmon-sysfs.h> The second include is not needed. > #include <linux/kthread.h> > #include <linux/i2c.h> > #include <linux/list.h> > @@ -77,6 +79,9 @@ struct video_i2c_chip { > > /* xfer function */ > int (*xfer)(struct video_i2c_data *data, char *buf); > + > + /* hwmon init function */ > + int (*hwmon_init)(struct video_i2c_data *data); > }; > > static int amg88xx_xfer(struct video_i2c_data *data, char *buf) > @@ -101,6 +106,70 @@ static int amg88xx_xfer(struct video_i2c_data *data, char *buf) > return (ret == 2) ? 0 : -EIO; > } > > +#if defined(CONFIG_HWMON) || (defined(MODULE) && defined(CONFIG_HWMON_MODULE)) > + > +static const u32 amg88xx_temp_config[] = { > + HWMON_T_INPUT, > + 0 > +}; > + > +static const struct hwmon_channel_info amg88xx_temp = { > + .type = hwmon_temp, > + .config = amg88xx_temp_config, > +}; > + > +static const struct hwmon_channel_info *amg88xx_info[] = { > + &amg88xx_temp, > + NULL > +}; > + > +static umode_t amg88xx_is_visible(const void *drvdata, > + enum hwmon_sensor_types type, > + u32 attr, int channel) > +{ > + return 0444; > +} > + > +static int amg88xx_read(struct device *dev, enum hwmon_sensor_types type, > + u32 attr, int channel, long *val) > +{ > + struct video_i2c_data *data = dev_get_drvdata(dev); > + struct i2c_client *client = data->client; > + int tmp = i2c_smbus_read_word_data(client, 0x0e); > + > + if (tmp < 0) > + return -EINVAL; > + Please return the error. This does not reflect an invalid argument. > + /* check for sign bit, and invert temp reading to a negative value */ > + if (0x800 & tmp) Yoda programming dislike I do. All it does to obfuscate the code and confuse the reader. Don't tell me that "if (0 > ret)" is somehow better than "if (ret < 0)". On top of that, it is misguided here. It was introduced to prevent sloppy programmers from writing code such as if (i = 15) which does not apply here. > + tmp = -(tmp & 0x7ff); 0x800 => 0x0 -> -0x0 -> 0x0 seems wrong. Maybe consider using sign_extend32() instead ? > + > + *val = (tmp * 625) / 10; > + > + return 0; > +} > + > +static const struct hwmon_ops amg88xx_hwmon_ops = { > + .is_visible = amg88xx_is_visible, > + .read = amg88xx_read, > +}; > + > +static const struct hwmon_chip_info amg88xx_chip_info = { > + .ops = &amg88xx_hwmon_ops, > + .info = amg88xx_info, > +}; > + > +static int amg88xx_hwmon_init(struct video_i2c_data *data) > +{ > + void *hwmon = devm_hwmon_device_register_with_info(&data->client->dev, > + "amg88xx", data, &amg88xx_chip_info, NULL); > + > + return IS_ERR(hwmon); What is the point of not returning the error (eg with PTR_ERR_OR_ZERO) but a boolean ? > +} > +#else > +#define amg88xx_hwmon_init NULL > +#endif > + > #define AMG88XX 0 > > static const struct video_i2c_chip video_i2c_chip[] = { > @@ -111,6 +180,7 @@ static const struct video_i2c_chip video_i2c_chip[] = { > .buffer_size = 128, > .bpp = 16, > .xfer = &amg88xx_xfer, > + .hwmon_init = &amg88xx_hwmon_init, This won't work if amg88xx_hwmon_init is NULL. Besides, & in front of a function name is unnecessary. > } > }; > > @@ -505,6 +575,9 @@ static int video_i2c_probe(struct i2c_client *client, > video_set_drvdata(&data->vdev, data); > i2c_set_clientdata(client, data); > > + if (data->chip->hwmon_init) > + data->chip->hwmon_init(data); What is the point of having the function return an int (or bool) and if the return value is ignored ? > + > ret = video_register_device(&data->vdev, VFL_TYPE_GRABBER, -1); > if (ret < 0) > goto error_unregister_device; >
On Tue, Jun 26, 2018 at 6:47 AM, Guenter Roeck <linux@roeck-us.net> wrote: > On 06/25/2018 11:30 PM, Matt Ranostay wrote: >> >> AMG88xx has an on-board thermistor which is used for more accurate >> processing of its temperature readings from the 8x8 thermopile array >> >> Cc: linux-hwmon@vger.kernel.org >> Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com> >> --- >> drivers/media/i2c/video-i2c.c | 73 +++++++++++++++++++++++++++++++++++ >> 1 file changed, 73 insertions(+) >> >> diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c >> index 0b347cc19aa5..16c3e03af219 100644 >> --- a/drivers/media/i2c/video-i2c.c >> +++ b/drivers/media/i2c/video-i2c.c >> @@ -10,6 +10,8 @@ >> #include <linux/delay.h> >> #include <linux/freezer.h> >> +#include <linux/hwmon.h> >> +#include <linux/hwmon-sysfs.h> > > > The second include is not needed. > Noted. > >> #include <linux/kthread.h> >> #include <linux/i2c.h> >> #include <linux/list.h> >> @@ -77,6 +79,9 @@ struct video_i2c_chip { >> /* xfer function */ >> int (*xfer)(struct video_i2c_data *data, char *buf); >> + >> + /* hwmon init function */ >> + int (*hwmon_init)(struct video_i2c_data *data); >> }; >> static int amg88xx_xfer(struct video_i2c_data *data, char *buf) >> @@ -101,6 +106,70 @@ static int amg88xx_xfer(struct video_i2c_data *data, >> char *buf) >> return (ret == 2) ? 0 : -EIO; >> } >> +#if defined(CONFIG_HWMON) || (defined(MODULE) && >> defined(CONFIG_HWMON_MODULE)) >> + >> +static const u32 amg88xx_temp_config[] = { >> + HWMON_T_INPUT, >> + 0 >> +}; >> + >> +static const struct hwmon_channel_info amg88xx_temp = { >> + .type = hwmon_temp, >> + .config = amg88xx_temp_config, >> +}; >> + >> +static const struct hwmon_channel_info *amg88xx_info[] = { >> + &amg88xx_temp, >> + NULL >> +}; >> + >> +static umode_t amg88xx_is_visible(const void *drvdata, >> + enum hwmon_sensor_types type, >> + u32 attr, int channel) >> +{ >> + return 0444; >> +} >> + >> +static int amg88xx_read(struct device *dev, enum hwmon_sensor_types type, >> + u32 attr, int channel, long *val) >> +{ >> + struct video_i2c_data *data = dev_get_drvdata(dev); >> + struct i2c_client *client = data->client; >> + int tmp = i2c_smbus_read_word_data(client, 0x0e); >> + >> + if (tmp < 0) >> + return -EINVAL; >> + > > > Please return the error. This does not reflect an invalid argument. Ok noted. > > >> + /* check for sign bit, and invert temp reading to a negative value >> */ >> + if (0x800 & tmp) > > > Yoda programming dislike I do. All it does to obfuscate the code and confuse > the reader. Don't tell me that "if (0 > ret)" is somehow better than > "if (ret < 0)". On top of that, it is misguided here. It was introduced > to prevent sloppy programmers from writing code such as > if (i = 15) > which does not apply here. Understood :) and probably should use the BIT() macro > >> + tmp = -(tmp & 0x7ff); > > > 0x800 => 0x0 -> -0x0 -> 0x0 > > seems wrong. Maybe consider using sign_extend32() instead ? > Heh yes I knew someone was going to see this and scratch their head on this.. So the sign bit just says if the value is positive or negative, and the value isn't two's (or even one's) complement value. If it is negative you have to basically have to invert the absolute value (tested this with a freezer to be sure it wasn't a datasheet mistake). Datasheet: https://cdn-learn.adafruit.com/assets/assets/000/043/261/original/Grid-EYE_SPECIFICATIONS%28Reference%29.pdf >> + >> + *val = (tmp * 625) / 10; >> + >> + return 0; >> +} >> + >> +static const struct hwmon_ops amg88xx_hwmon_ops = { >> + .is_visible = amg88xx_is_visible, >> + .read = amg88xx_read, >> +}; >> + >> +static const struct hwmon_chip_info amg88xx_chip_info = { >> + .ops = &amg88xx_hwmon_ops, >> + .info = amg88xx_info, >> +}; >> + >> +static int amg88xx_hwmon_init(struct video_i2c_data *data) >> +{ >> + void *hwmon = >> devm_hwmon_device_register_with_info(&data->client->dev, >> + "amg88xx", data, &amg88xx_chip_info, >> NULL); >> + >> + return IS_ERR(hwmon); > > > What is the point of not returning the error (eg with PTR_ERR_OR_ZERO) but a > boolean ? > No reason, and you are correct it should return the error code in the return value. >> +} >> +#else >> +#define amg88xx_hwmon_init NULL >> +#endif >> + >> #define AMG88XX 0 >> static const struct video_i2c_chip video_i2c_chip[] = { >> @@ -111,6 +180,7 @@ static const struct video_i2c_chip video_i2c_chip[] = >> { >> .buffer_size = 128, >> .bpp = 16, >> .xfer = &amg88xx_xfer, >> + .hwmon_init = &amg88xx_hwmon_init, > > > This won't work if amg88xx_hwmon_init is NULL. Besides, & in front of > a function name is unnecessary. > D'oh yea explains why kbuild bot just emailed me about this.. >> } >> }; >> @@ -505,6 +575,9 @@ static int video_i2c_probe(struct i2c_client >> *client, >> video_set_drvdata(&data->vdev, data); >> i2c_set_clientdata(client, data); >> + if (data->chip->hwmon_init) >> + data->chip->hwmon_init(data); > > > What is the point of having the function return an int (or bool) and > if the return value is ignored ? > Correct... I'll put a warning here since I don't think the video side should fail to register if the hwmon bits fail. > >> + >> ret = video_register_device(&data->vdev, VFL_TYPE_GRABBER, -1); >> if (ret < 0) >> goto error_unregister_device; >> >
On 06/26/2018 08:20 PM, Matt Ranostay wrote: > On Tue, Jun 26, 2018 at 6:47 AM, Guenter Roeck <linux@roeck-us.net> wrote: >> On 06/25/2018 11:30 PM, Matt Ranostay wrote: >>> >>> AMG88xx has an on-board thermistor which is used for more accurate >>> processing of its temperature readings from the 8x8 thermopile array >>> >>> Cc: linux-hwmon@vger.kernel.org >>> Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com> >>> --- >>> drivers/media/i2c/video-i2c.c | 73 +++++++++++++++++++++++++++++++++++ >>> 1 file changed, 73 insertions(+) >>> ... > >> >>> + tmp = -(tmp & 0x7ff); >> >> >> 0x800 => 0x0 -> -0x0 -> 0x0 >> >> seems wrong. Maybe consider using sign_extend32() instead ? >> > > Heh yes I knew someone was going to see this and scratch their head on this.. > > So the sign bit just says if the value is positive or negative, and > the value isn't two's (or even one's) complement value. > If it is negative you have to basically have to invert the absolute > value (tested this with a freezer to be sure it wasn't a datasheet > mistake). > > Datasheet: https://cdn-learn.adafruit.com/assets/assets/000/043/261/original/Grid-EYE_SPECIFICATIONS%28Reference%29.pdf > Outch, that hurts. Please add a comment into the code explaining this. Also, to be on the safe side, it might make sense to mask tmp with 0x0fff - the upper bits seem to be undefined. Thanks, Guenter
diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c index 0b347cc19aa5..16c3e03af219 100644 --- a/drivers/media/i2c/video-i2c.c +++ b/drivers/media/i2c/video-i2c.c @@ -10,6 +10,8 @@ #include <linux/delay.h> #include <linux/freezer.h> +#include <linux/hwmon.h> +#include <linux/hwmon-sysfs.h> #include <linux/kthread.h> #include <linux/i2c.h> #include <linux/list.h> @@ -77,6 +79,9 @@ struct video_i2c_chip { /* xfer function */ int (*xfer)(struct video_i2c_data *data, char *buf); + + /* hwmon init function */ + int (*hwmon_init)(struct video_i2c_data *data); }; static int amg88xx_xfer(struct video_i2c_data *data, char *buf) @@ -101,6 +106,70 @@ static int amg88xx_xfer(struct video_i2c_data *data, char *buf) return (ret == 2) ? 0 : -EIO; } +#if defined(CONFIG_HWMON) || (defined(MODULE) && defined(CONFIG_HWMON_MODULE)) + +static const u32 amg88xx_temp_config[] = { + HWMON_T_INPUT, + 0 +}; + +static const struct hwmon_channel_info amg88xx_temp = { + .type = hwmon_temp, + .config = amg88xx_temp_config, +}; + +static const struct hwmon_channel_info *amg88xx_info[] = { + &amg88xx_temp, + NULL +}; + +static umode_t amg88xx_is_visible(const void *drvdata, + enum hwmon_sensor_types type, + u32 attr, int channel) +{ + return 0444; +} + +static int amg88xx_read(struct device *dev, enum hwmon_sensor_types type, + u32 attr, int channel, long *val) +{ + struct video_i2c_data *data = dev_get_drvdata(dev); + struct i2c_client *client = data->client; + int tmp = i2c_smbus_read_word_data(client, 0x0e); + + if (tmp < 0) + return -EINVAL; + + /* check for sign bit, and invert temp reading to a negative value */ + if (0x800 & tmp) + tmp = -(tmp & 0x7ff); + + *val = (tmp * 625) / 10; + + return 0; +} + +static const struct hwmon_ops amg88xx_hwmon_ops = { + .is_visible = amg88xx_is_visible, + .read = amg88xx_read, +}; + +static const struct hwmon_chip_info amg88xx_chip_info = { + .ops = &amg88xx_hwmon_ops, + .info = amg88xx_info, +}; + +static int amg88xx_hwmon_init(struct video_i2c_data *data) +{ + void *hwmon = devm_hwmon_device_register_with_info(&data->client->dev, + "amg88xx", data, &amg88xx_chip_info, NULL); + + return IS_ERR(hwmon); +} +#else +#define amg88xx_hwmon_init NULL +#endif + #define AMG88XX 0 static const struct video_i2c_chip video_i2c_chip[] = { @@ -111,6 +180,7 @@ static const struct video_i2c_chip video_i2c_chip[] = { .buffer_size = 128, .bpp = 16, .xfer = &amg88xx_xfer, + .hwmon_init = &amg88xx_hwmon_init, }, }; @@ -505,6 +575,9 @@ static int video_i2c_probe(struct i2c_client *client, video_set_drvdata(&data->vdev, data); i2c_set_clientdata(client, data); + if (data->chip->hwmon_init) + data->chip->hwmon_init(data); + ret = video_register_device(&data->vdev, VFL_TYPE_GRABBER, -1); if (ret < 0) goto error_unregister_device;
AMG88xx has an on-board thermistor which is used for more accurate processing of its temperature readings from the 8x8 thermopile array Cc: linux-hwmon@vger.kernel.org Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com> --- drivers/media/i2c/video-i2c.c | 73 +++++++++++++++++++++++++++++++++++ 1 file changed, 73 insertions(+)