Re: [PATCH v2 03/15] [media] adv7180: Use inline function instead of macro
On 02/02/2015 02:36 PM, Mauro Carvalho Chehab wrote: Em Fri, 23 Jan 2015 16:52:22 +0100 Lars-Peter Clausen l...@metafoo.de escreveu: Use a inline function instead of a macro for the container_of helper for getting the driver's state struct from a control. A inline function has the advantage that it is more typesafe and nicer in general. I don't see any advantage on this. See: container_of is already a macro, and it is written in a way that, if you use it with inconsistent values, the compilation will break. Yes, container_of is a macro, because it needs to be a macro. Compilation will also not always break if you pass in a incorrect type, it might succeed with even generating a warning. Furthermore if compilation breaks the error message is completely incomprehensible. Using a function instead makes sure that the error message you get is in the style of passing argument of wrong type to function, expected typeX, got typeY. Also, there's the risk that, for whatever reason, gcc to decide to not inline this. If the compiler does not inline this it probably has a good reason to do so. Not inlining this will not break the functionality, so it is not a problem. - Lars -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 03/15] [media] adv7180: Use inline function instead of macro
Em Fri, 23 Jan 2015 16:52:22 +0100
Lars-Peter Clausen l...@metafoo.de escreveu:
Use a inline function instead of a macro for the container_of helper for
getting the driver's state struct from a control. A inline function has the
advantage that it is more typesafe and nicer in general.
I don't see any advantage on this.
See: container_of is already a macro, and it is written in a way that, if
you use it with inconsistent values, the compilation will break.
Also, there's the risk that, for whatever reason, gcc to decide to not
inline this.
So, this doesn't sound a good idea.
Regards,
Mauro
Signed-off-by: Lars-Peter Clausen l...@metafoo.de
Acked-by: Hans Verkuil hans.verk...@cisco.com
---
drivers/media/i2c/adv7180.c | 13 +++--
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index f424a4d..f2508abe 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -130,9 +130,11 @@ struct adv7180_state {
boolpowered;
u8 input;
};
-#define to_adv7180_sd(_ctrl) (container_of(_ctrl-handler, \
- struct adv7180_state, \
- ctrl_hdl)-sd)
+
+static struct adv7180_state *ctrl_to_adv7180(struct v4l2_ctrl *ctrl)
+{
+ return container_of(ctrl-handler, struct adv7180_state, ctrl_hdl);
+}
static v4l2_std_id adv7180_std_to_v4l2(u8 status1)
{
@@ -345,9 +347,8 @@ static int adv7180_s_power(struct v4l2_subdev *sd, int on)
static int adv7180_s_ctrl(struct v4l2_ctrl *ctrl)
{
- struct v4l2_subdev *sd = to_adv7180_sd(ctrl);
- struct adv7180_state *state = to_state(sd);
- struct i2c_client *client = v4l2_get_subdevdata(sd);
+ struct adv7180_state *state = ctrl_to_adv7180(ctrl);
+ struct i2c_client *client = v4l2_get_subdevdata(state-sd);
int ret = mutex_lock_interruptible(state-mutex);
int val;
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 03/15] [media] adv7180: Use inline function instead of macro
On 02/02/2015 02:36 PM, Mauro Carvalho Chehab wrote:
Em Fri, 23 Jan 2015 16:52:22 +0100
Lars-Peter Clausen l...@metafoo.de escreveu:
Use a inline function instead of a macro for the container_of helper for
getting the driver's state struct from a control. A inline function has the
advantage that it is more typesafe and nicer in general.
I don't see any advantage on this.
See: container_of is already a macro, and it is written in a way that, if
you use it with inconsistent values, the compilation will break.
Also, there's the risk that, for whatever reason, gcc to decide to not
inline this.
For the record: I disagree with this. I think a static inline is more readable
than a macro. It is also consistent with other existing i2c subdev drivers since
they all use a static inline as well. And if in rare cases gcc might decide not
to inline this, then so what? You really won't notice any difference. It's an
i2c device, so it's slow as molasses anyway. Readability is much more important
IMHO.
On the other hand, it's not critical enough for me to make a big deal out of
it if this patch is dropped.
Regards,
Hans
So, this doesn't sound a good idea.
Regards,
Mauro
Signed-off-by: Lars-Peter Clausen l...@metafoo.de
Acked-by: Hans Verkuil hans.verk...@cisco.com
---
drivers/media/i2c/adv7180.c | 13 +++--
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index f424a4d..f2508abe 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -130,9 +130,11 @@ struct adv7180_state {
boolpowered;
u8 input;
};
-#define to_adv7180_sd(_ctrl) (container_of(_ctrl-handler, \
-struct adv7180_state, \
-ctrl_hdl)-sd)
+
+static struct adv7180_state *ctrl_to_adv7180(struct v4l2_ctrl *ctrl)
+{
+return container_of(ctrl-handler, struct adv7180_state, ctrl_hdl);
+}
static v4l2_std_id adv7180_std_to_v4l2(u8 status1)
{
@@ -345,9 +347,8 @@ static int adv7180_s_power(struct v4l2_subdev *sd, int
on)
static int adv7180_s_ctrl(struct v4l2_ctrl *ctrl)
{
-struct v4l2_subdev *sd = to_adv7180_sd(ctrl);
-struct adv7180_state *state = to_state(sd);
-struct i2c_client *client = v4l2_get_subdevdata(sd);
+struct adv7180_state *state = ctrl_to_adv7180(ctrl);
+struct i2c_client *client = v4l2_get_subdevdata(state-sd);
int ret = mutex_lock_interruptible(state-mutex);
int val;
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 03/15] [media] adv7180: Use inline function instead of macro
Use a inline function instead of a macro for the container_of helper for
getting the driver's state struct from a control. A inline function has the
advantage that it is more typesafe and nicer in general.
Signed-off-by: Lars-Peter Clausen l...@metafoo.de
Acked-by: Hans Verkuil hans.verk...@cisco.com
---
drivers/media/i2c/adv7180.c | 13 +++--
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index f424a4d..f2508abe 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -130,9 +130,11 @@ struct adv7180_state {
boolpowered;
u8 input;
};
-#define to_adv7180_sd(_ctrl) (container_of(_ctrl-handler,\
- struct adv7180_state, \
- ctrl_hdl)-sd)
+
+static struct adv7180_state *ctrl_to_adv7180(struct v4l2_ctrl *ctrl)
+{
+ return container_of(ctrl-handler, struct adv7180_state, ctrl_hdl);
+}
static v4l2_std_id adv7180_std_to_v4l2(u8 status1)
{
@@ -345,9 +347,8 @@ static int adv7180_s_power(struct v4l2_subdev *sd, int on)
static int adv7180_s_ctrl(struct v4l2_ctrl *ctrl)
{
- struct v4l2_subdev *sd = to_adv7180_sd(ctrl);
- struct adv7180_state *state = to_state(sd);
- struct i2c_client *client = v4l2_get_subdevdata(sd);
+ struct adv7180_state *state = ctrl_to_adv7180(ctrl);
+ struct i2c_client *client = v4l2_get_subdevdata(state-sd);
int ret = mutex_lock_interruptible(state-mutex);
int val;
--
1.8.0
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
