From: Daniel Baluta <daniel.baluta@intel.com> To: Joe Perches <joe@perches.com> Cc: Daniel Baluta <daniel.baluta@intel.com>, Kevin Tsai <ktsai@capellamicro.com>, Wolfram Sang <wsa@the-dreams.de>, Rob Herring <robh+dt@kernel.org>, Pawel Moll <pawel.moll@arm.com>, Mark Rutland <mark.rutland@arm.com>, Ian Campbell <ijc+devicetree@hellion.org.uk>, Kumar Gala <galak@codeaurora.org>, Jonathan Cameron <jic23@kernel.org>, Hartmut Knaack <knaack.h@gmx.de>, Lars-Peter Clausen <lars@metafoo.de>, Peter Meerwald <pmeerw@pmeerw.net>, Grant Likely <grant.likely@linaro.org>, Andrew Morton <akpm@linux-foundation.org>, "David S. Miller" <davem@davemloft.net>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Mauro Carvalho Chehab <mchehab@osg.samsung.com>, Antti Palosaari <crope@iki.fi>, Archana Patni <archana.patni@linux.intel.com>, linux-i2c@vger.kernel.org, devicetree@vger.kernel.org, "linux-iio@vger.kernel.org" <linux-iio@vger.kernel.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org> Subject: Re: [PATCH V1 1/1] iio: Added Capella cm3232 ambient light sensor driver. Date: Mon, 5 Jan 2015 16:20:02 +0200 [thread overview] Message-ID: <CAEnQRZBmwurFCJU4E4ArO8waU+YcGWrRYr3DRVxq9haduX2t=w@mail.gmail.com> (raw) In-Reply-To: <1420463396.2652.8.camel@perches.com> On Mon, Jan 5, 2015 at 3:09 PM, Joe Perches <joe@perches.com> wrote: > On Mon, 2015-01-05 at 12:51 +0200, Daniel Baluta wrote: >> On Thu, Jan 1, 2015 at 2:10 AM, Kevin Tsai <ktsai@capellamicro.com> wrote: >> > CM3232 is an advanced ambient light sensor with I2C protocol interface. >> > The I2C slave address is internally hardwired as 0x10 (7-bit). Writing >> > to configure register is byte mode, but reading ALS register requests to >> > use word mode for 16-bit resolution. >> >> This looks good to me. Few comments inline. > [] >> > diff --git a/drivers/iio/light/cm3232.c b/drivers/iio/light/cm3232.c > [] >> > +/** >> > + * cm3232_reg_init() - Initialize CM3232 registers >> > + * @chip: pointer of struct cm3232. >> > + * >> > + * Initialize CM3232 ambient light sensor register to default values. >> > + * >> > + Return: 0 for success; otherwise for error code. >> > + */ >> > +static int cm3232_reg_init(struct cm3232_chip *chip) >> > +{ >> > + struct i2c_client *client = chip->client; >> > + struct cm3232_als_info *als_info; >> > + s32 ret; >> > + >> > + /* Identify device */ >> > + ret = i2c_smbus_read_word_data(client, CM3232_REG_ADDR_ID); >> > + if (ret < 0) >> > + return ret; >> > + if ((ret & 0xFF) != 0x32) >> > + return -ENODEV; >> > + >> > + /* Disable and reset device */ >> > + chip->regs_cmd = CM3232_CMD_ALS_DISABLE | CM3232_CMD_ALS_RESET; >> > + ret = i2c_smbus_write_byte_data(client, CM3232_REG_ADDR_CMD, >> > + chip->regs_cmd); >> > + if (ret < 0) >> > + return ret; >> > + >> > + /* Initialization */ >> > + als_info = chip->als_info = &cm3232_als_info_default; >> > + chip->regs_cmd = CM3232_CMD_DEFAULT; >> > + >> > + /* Configure register */ >> > + ret = i2c_smbus_write_byte_data(client, CM3232_REG_ADDR_CMD, >> > + chip->regs_cmd); >> >> You could directly return i2c_smbus_write_byte_data(..). > > Sometimes it's better to return a specific value > for the error instead of depending on correctness > of all the indirect functions in the call chain. > > In this case, all the smbus_xfer functions must > return 0 on success. Do they? Yes. http://lxr.free-electrons.com/source/drivers/i2c/i2c-core.c#L2845 > > Inspecting the codebase for correctness or not > depending on that correctness is sometimes better > to avoid doing. You have a valid point. Anyhow, AFAIK the code from IIO prefers a direct return. Also, I guess kbuild test robot will complain about it, like here: http://marc.info/?l=linux-kernel&m=141963536230419&w=2 thanks, Daniel.
WARNING: multiple messages have this Message-ID (diff)
From: Daniel Baluta <daniel.baluta-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> To: Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org> Cc: Daniel Baluta <daniel.baluta-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>, Kevin Tsai <ktsai-GubuWUlQtMwciDkP5Hr2oA@public.gmane.org>, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>, Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>, Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org>, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>, Ian Campbell <ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org>, Kumar Gala <galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>, Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>, Hartmut Knaack <knaack.h-Mmb7MZpHnFY@public.gmane.org>, Lars-Peter Clausen <lars-Qo5EllUWu/uELgA04lAiVw@public.gmane.org>, Peter Meerwald <pmeerw-jW+XmwGofnusTnJN9+BGXg@public.gmane.org>, Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>, Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>, "David S. Miller" <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>, Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>, Mauro Carvalho Chehab <mchehab-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org>, Antti Palosaari <crope-X3B1VOXEql0@public.gmane.org>, Archana Patni <archana.patni-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>, linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, "linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" <linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, Linux Subject: Re: [PATCH V1 1/1] iio: Added Capella cm3232 ambient light sensor driver. Date: Mon, 5 Jan 2015 16:20:02 +0200 [thread overview] Message-ID: <CAEnQRZBmwurFCJU4E4ArO8waU+YcGWrRYr3DRVxq9haduX2t=w@mail.gmail.com> (raw) In-Reply-To: <1420463396.2652.8.camel-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org> On Mon, Jan 5, 2015 at 3:09 PM, Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org> wrote: > On Mon, 2015-01-05 at 12:51 +0200, Daniel Baluta wrote: >> On Thu, Jan 1, 2015 at 2:10 AM, Kevin Tsai <ktsai-GubuWUlQtMwciDkP5Hr2oA@public.gmane.org> wrote: >> > CM3232 is an advanced ambient light sensor with I2C protocol interface. >> > The I2C slave address is internally hardwired as 0x10 (7-bit). Writing >> > to configure register is byte mode, but reading ALS register requests to >> > use word mode for 16-bit resolution. >> >> This looks good to me. Few comments inline. > [] >> > diff --git a/drivers/iio/light/cm3232.c b/drivers/iio/light/cm3232.c > [] >> > +/** >> > + * cm3232_reg_init() - Initialize CM3232 registers >> > + * @chip: pointer of struct cm3232. >> > + * >> > + * Initialize CM3232 ambient light sensor register to default values. >> > + * >> > + Return: 0 for success; otherwise for error code. >> > + */ >> > +static int cm3232_reg_init(struct cm3232_chip *chip) >> > +{ >> > + struct i2c_client *client = chip->client; >> > + struct cm3232_als_info *als_info; >> > + s32 ret; >> > + >> > + /* Identify device */ >> > + ret = i2c_smbus_read_word_data(client, CM3232_REG_ADDR_ID); >> > + if (ret < 0) >> > + return ret; >> > + if ((ret & 0xFF) != 0x32) >> > + return -ENODEV; >> > + >> > + /* Disable and reset device */ >> > + chip->regs_cmd = CM3232_CMD_ALS_DISABLE | CM3232_CMD_ALS_RESET; >> > + ret = i2c_smbus_write_byte_data(client, CM3232_REG_ADDR_CMD, >> > + chip->regs_cmd); >> > + if (ret < 0) >> > + return ret; >> > + >> > + /* Initialization */ >> > + als_info = chip->als_info = &cm3232_als_info_default; >> > + chip->regs_cmd = CM3232_CMD_DEFAULT; >> > + >> > + /* Configure register */ >> > + ret = i2c_smbus_write_byte_data(client, CM3232_REG_ADDR_CMD, >> > + chip->regs_cmd); >> >> You could directly return i2c_smbus_write_byte_data(..). > > Sometimes it's better to return a specific value > for the error instead of depending on correctness > of all the indirect functions in the call chain. > > In this case, all the smbus_xfer functions must > return 0 on success. Do they? Yes. http://lxr.free-electrons.com/source/drivers/i2c/i2c-core.c#L2845 > > Inspecting the codebase for correctness or not > depending on that correctness is sometimes better > to avoid doing. You have a valid point. Anyhow, AFAIK the code from IIO prefers a direct return. Also, I guess kbuild test robot will complain about it, like here: http://marc.info/?l=linux-kernel&m=141963536230419&w=2 thanks, Daniel.
next prev parent reply other threads:[~2015-01-05 14:20 UTC|newest] Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top 2015-01-01 0:10 [PATCH V1 1/1] iio: Added Capella cm3232 ambient light sensor driver Kevin Tsai 2015-01-01 0:10 ` Kevin Tsai 2015-01-01 3:38 ` Jeremiah Mahler 2015-01-01 11:02 ` Peter Meerwald 2015-01-02 4:23 ` Kevin Tsai 2015-01-05 10:51 ` Daniel Baluta 2015-01-05 10:51 ` Daniel Baluta 2015-01-05 13:09 ` Joe Perches 2015-01-05 13:09 ` Joe Perches 2015-01-05 13:09 ` Joe Perches 2015-01-05 14:20 ` Daniel Baluta [this message] 2015-01-05 14:20 ` Daniel Baluta 2015-01-05 16:42 ` Joe Perches 2015-01-05 16:42 ` Joe Perches 2015-01-05 16:42 ` Joe Perches 2015-01-05 17:50 ` Daniel Baluta 2015-01-05 17:50 ` Daniel Baluta 2015-01-05 17:56 ` Joe Perches 2015-01-05 17:56 ` Joe Perches 2015-01-10 14:06 ` Jonathan Cameron 2015-01-10 14:06 ` Jonathan Cameron
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAEnQRZBmwurFCJU4E4ArO8waU+YcGWrRYr3DRVxq9haduX2t=w@mail.gmail.com' \ --to=daniel.baluta@intel.com \ --cc=akpm@linux-foundation.org \ --cc=archana.patni@linux.intel.com \ --cc=crope@iki.fi \ --cc=davem@davemloft.net \ --cc=devicetree@vger.kernel.org \ --cc=galak@codeaurora.org \ --cc=grant.likely@linaro.org \ --cc=gregkh@linuxfoundation.org \ --cc=ijc+devicetree@hellion.org.uk \ --cc=jic23@kernel.org \ --cc=joe@perches.com \ --cc=knaack.h@gmx.de \ --cc=ktsai@capellamicro.com \ --cc=lars@metafoo.de \ --cc=linux-i2c@vger.kernel.org \ --cc=linux-iio@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mark.rutland@arm.com \ --cc=mchehab@osg.samsung.com \ --cc=pawel.moll@arm.com \ --cc=pmeerw@pmeerw.net \ --cc=robh+dt@kernel.org \ --cc=wsa@the-dreams.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.