Re: [PATCH 1/3] media: v4l2-core: cleanup coding style at V4L2 async/fwnode
Hi Mauro,
On Fri, Oct 05, 2018 at 07:12:02AM -0300, Mauro Carvalho Chehab wrote:
> Em Fri, 5 Oct 2018 10:55:58 +0300
> Sakari Ailus escreveu:
...
> > > @@ -436,8 +437,7 @@ static int __v4l2_fwnode_endpoint_parse(struct
> > > fwnode_handle *fwnode,
> > > if (mbus_type != V4L2_MBUS_UNKNOWN &&
> > > vep->bus_type != mbus_type) {
> > > pr_debug("expecting bus type %s\n",
> > > - v4l2_fwnode_mbus_type_to_string(
> > > - vep->bus_type));
> > > +
> > > v4l2_fwnode_mbus_type_to_string(vep->bus_type));
> >
> > This one's over 80. I preferred what it was. But I have no strong
> > preference here.
> >
> > > return -ENXIO;
> > > }
> > > } else {
> > > @@ -452,8 +452,8 @@ static int __v4l2_fwnode_endpoint_parse(struct
> > > fwnode_handle *fwnode,
> > > return rval;
> > >
> > > if (vep->bus_type == V4L2_MBUS_UNKNOWN)
> > > - v4l2_fwnode_endpoint_parse_parallel_bus(
> > > - fwnode, vep, V4L2_MBUS_UNKNOWN);
> >
> > This is not uncommon way of aligning function arguments when short of
> > space. It is also not exceeding 80 characters, as the replacement below.
>
> Well, Lindent used to align like that. That's why we see it on lots of
> code inside media: in the past, people tend to use it to get rid of
> some checkpatch warnings. Lindent script has long gone (still people
> sometimes call indent), and now checkpatch evolved, and has a
> --fix-inplace, with is usually enough to pinpoint where to change
> (although it does a crap job for more multi-line function args).
>
> As a reviewer, this hurts my eyes. It took me more time to review
> something like
> v4l2_fwnode_endpoint_parse_parallel_bus(
> fwnode, vep, V4L2_MBUS_UNKNOWN);
>
> than something like:
> v4l2_fwnode_endpoint_parse_parallel_bus(fwnode, vep,
>
> V4L2_MBUS_UNKNOWN);
I think this is somewhat a matter of taste and I prefer it different. :-)
>
> The parenthesis alignment really helps to identify that the second
> line are arguments.
>
> Btw, if you use checkpatch with --strict, you'll see that this is
> not the right Kernel coding style. It will complain for both ending a
> line with an open parenthesis and that the second line is not aligned.
Right; V4L2 has a lot of that pattern (also elsewhere) but you'd get told
to fix that if it were in another tree (non-media). I think we agree on
renaming the very long function names; it'll get rid of probably much of
that pattern. I'll submit patches for that later on, possibly including
other improvements to the API. But that'll be after 4.20.
>
> >
> > > + v4l2_fwnode_endpoint_parse_parallel_bus(fwnode, vep,
> > > +
> > > V4L2_MBUS_UNKNOWN);
> > >
> > > pr_debug("assuming media bus type %s (%u)\n",
> > >v4l2_fwnode_mbus_type_to_string(vep->bus_type),
> > > @@ -511,8 +511,8 @@ void v4l2_fwnode_endpoint_free(struct
> > > v4l2_fwnode_endpoint *vep)
> > > }
> > > EXPORT_SYMBOL_GPL(v4l2_fwnode_endpoint_free);
> > >
> > > -int v4l2_fwnode_endpoint_alloc_parse(
> > > - struct fwnode_handle *fwnode, struct v4l2_fwnode_endpoint *vep)
> > > +int v4l2_fwnode_endpoint_alloc_parse(struct fwnode_handle *fwnode,
> > > + struct v4l2_fwnode_endpoint *vep)
> > > {
> > > int rval;
> > >
> > > @@ -533,9 +533,10 @@ int v4l2_fwnode_endpoint_alloc_parse(
> > >
> > > vep->nr_of_link_frequencies = rval;
> > >
> > > - rval = fwnode_property_read_u64_array(
> > > - fwnode, "link-frequencies", vep->link_frequencies,
> > > - vep->nr_of_link_frequencies);
> > > + rval = fwnode_property_read_u64_array(fwnode,
> > > + "link-frequencies",
> > > + vep->link_frequencies,
> > > +
> > > vep->nr_of_link_frequencies);
> >
> > Over 80 characters.
>
> True, but it is better to violate 80-cols (those days, I guess everybody
> uses a graphical environment), than to not align the arguments.
>
> The 80-cols is there nowadays mostly to warn about code complexity, where
> multiple indentations are present.
I also review the patches using Mutt and my terminal window width is set at
80 characters. That's not uncommon I believe.
>
> For a reviewer, the parenthesis alignment is a way more relevant, as
> it allows to immediately notice that the two following lines are
> arguments of the function, and not a new indentation level.
That's a valid point, yet more important is that it's not at the same level
than the first line of the statement
Re: [PATCH 1/3] media: v4l2-core: cleanup coding style at V4L2 async/fwnode
Em Fri, 5 Oct 2018 10:55:58 +0300
Sakari Ailus escreveu:
> Hi Mauro,
>
> On Thu, Oct 04, 2018 at 06:13:46PM -0400, Mauro Carvalho Chehab wrote:
> > There are several coding style issues at those definitions,
> > and the previous patchset added even more.
> >
> > Address the trivial ones by first calling:
> >
> > ./scripts/checkpatch.pl --strict --fix-inline
> > include/media/v4l2-async.h include/media/v4l2-fwnode.h
> > include/media/v4l2-mediabus.h drivers/media/v4l2-core/v4l2-async.c
> > drivers/media/v4l2-core/v4l2-fwnode.c
> >
> > and then manually adjusting the style where needed.
> >
> > Signed-off-by: Mauro Carvalho Chehab
> > ---
> > drivers/media/v4l2-core/v4l2-async.c | 45 ---
> > drivers/media/v4l2-core/v4l2-fwnode.c | 185 +++---
> > include/media/v4l2-async.h| 12 +-
> > include/media/v4l2-fwnode.h | 46 ---
> > include/media/v4l2-mediabus.h | 32 +++--
> > 5 files changed, 179 insertions(+), 141 deletions(-)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c
> > b/drivers/media/v4l2-core/v4l2-async.c
> > index 70adbd9a01a2..6fdda745a1da 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > @@ -57,6 +57,7 @@ static bool match_i2c(struct v4l2_subdev *sd, struct
> > v4l2_async_subdev *asd)
> > {
> > #if IS_ENABLED(CONFIG_I2C)
> > struct i2c_client *client = i2c_verify_client(sd->dev);
> > +
> > return client &&
> > asd->match.i2c.adapter_id == client->adapter->nr &&
> > asd->match.i2c.address == client->addr;
> > @@ -89,10 +90,11 @@ static LIST_HEAD(subdev_list);
> > static LIST_HEAD(notifier_list);
> > static DEFINE_MUTEX(list_lock);
> >
> > -static struct v4l2_async_subdev *v4l2_async_find_match(
> > - struct v4l2_async_notifier *notifier, struct v4l2_subdev *sd)
> > +static struct
> > +v4l2_async_subdev *v4l2_async_find_match(struct v4l2_async_notifier
> > *notifier,
>
> This looks odd. If you want to rewrap it, then I'd put the stuff before and
> including the first asterisk on the first line.
Yeah, makes sense.
>
> > +struct v4l2_subdev *sd)
> > {
> > - bool (*match)(struct v4l2_subdev *, struct v4l2_async_subdev *);
> > + bool (*match)(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd);
> > struct v4l2_async_subdev *asd;
> >
> > list_for_each_entry(asd, >waiting, list) {
> > @@ -150,8 +152,8 @@ static bool asd_equal(struct v4l2_async_subdev *asd_x,
> > }
> >
> > /* Find the sub-device notifier registered by a sub-device driver. */
> > -static struct v4l2_async_notifier *v4l2_async_find_subdev_notifier(
> > - struct v4l2_subdev *sd)
> > +static struct v4l2_async_notifier *
> > +v4l2_async_find_subdev_notifier(struct v4l2_subdev *sd)
>
> It's better here. Below, too.
>
> > {
> > struct v4l2_async_notifier *n;
> >
> > @@ -163,8 +165,8 @@ static struct v4l2_async_notifier
> > *v4l2_async_find_subdev_notifier(
> > }
> >
> > /* Get v4l2_device related to the notifier if one can be found. */
> > -static struct v4l2_device *v4l2_async_notifier_find_v4l2_dev(
> > - struct v4l2_async_notifier *notifier)
> > +static struct v4l2_device *
> > +v4l2_async_notifier_find_v4l2_dev(struct v4l2_async_notifier *notifier)
> > {
> > while (notifier->parent)
> > notifier = notifier->parent;
> > @@ -175,8 +177,8 @@ static struct v4l2_device
> > *v4l2_async_notifier_find_v4l2_dev(
> > /*
> > * Return true if all child sub-device notifiers are complete, false
> > otherwise.
> > */
> > -static bool v4l2_async_notifier_can_complete(
> > - struct v4l2_async_notifier *notifier)
> > +static bool
> > +v4l2_async_notifier_can_complete(struct v4l2_async_notifier *notifier)
> > {
> > struct v4l2_subdev *sd;
> >
> > @@ -199,8 +201,8 @@ static bool v4l2_async_notifier_can_complete(
> > * Complete the master notifier if possible. This is done when all async
> > * sub-devices have been bound; v4l2_device is also available then.
> > */
> > -static int v4l2_async_notifier_try_complete(
> > - struct v4l2_async_notifier *notifier)
> > +static int
> > +v4l2_async_notifier_try_complete(struct v4l2_async_notifier *notifier)
> > {
> > /* Quick check whether there are still more sub-devices here. */
> > if (!list_empty(>waiting))
> > @@ -221,8 +223,8 @@ static int v4l2_async_notifier_try_complete(
> > return v4l2_async_notifier_call_complete(notifier);
> > }
> >
> > -static int v4l2_async_notifier_try_all_subdevs(
> > - struct v4l2_async_notifier *notifier);
> > +static int
> > +v4l2_async_notifier_try_all_subdevs(struct v4l2_async_notifier *notifier);
> >
> > static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,
> >struct v4l2_device *v4l2_dev,
> > @@ -268,8 +270,8 @@ static int v4l2_async_match_notify(struct
> > v4l2_async_notifier *notifier,
> > }
> >
> >
Re: [PATCH 1/3] media: v4l2-core: cleanup coding style at V4L2 async/fwnode
Hi Mauro,
On Thu, Oct 04, 2018 at 06:13:46PM -0400, Mauro Carvalho Chehab wrote:
> There are several coding style issues at those definitions,
> and the previous patchset added even more.
>
> Address the trivial ones by first calling:
>
> ./scripts/checkpatch.pl --strict --fix-inline
> include/media/v4l2-async.h include/media/v4l2-fwnode.h
> include/media/v4l2-mediabus.h drivers/media/v4l2-core/v4l2-async.c
> drivers/media/v4l2-core/v4l2-fwnode.c
>
> and then manually adjusting the style where needed.
>
> Signed-off-by: Mauro Carvalho Chehab
> ---
> drivers/media/v4l2-core/v4l2-async.c | 45 ---
> drivers/media/v4l2-core/v4l2-fwnode.c | 185 +++---
> include/media/v4l2-async.h| 12 +-
> include/media/v4l2-fwnode.h | 46 ---
> include/media/v4l2-mediabus.h | 32 +++--
> 5 files changed, 179 insertions(+), 141 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-async.c
> b/drivers/media/v4l2-core/v4l2-async.c
> index 70adbd9a01a2..6fdda745a1da 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -57,6 +57,7 @@ static bool match_i2c(struct v4l2_subdev *sd, struct
> v4l2_async_subdev *asd)
> {
> #if IS_ENABLED(CONFIG_I2C)
> struct i2c_client *client = i2c_verify_client(sd->dev);
> +
> return client &&
> asd->match.i2c.adapter_id == client->adapter->nr &&
> asd->match.i2c.address == client->addr;
> @@ -89,10 +90,11 @@ static LIST_HEAD(subdev_list);
> static LIST_HEAD(notifier_list);
> static DEFINE_MUTEX(list_lock);
>
> -static struct v4l2_async_subdev *v4l2_async_find_match(
> - struct v4l2_async_notifier *notifier, struct v4l2_subdev *sd)
> +static struct
> +v4l2_async_subdev *v4l2_async_find_match(struct v4l2_async_notifier
> *notifier,
This looks odd. If you want to rewrap it, then I'd put the stuff before and
including the first asterisk on the first line.
> + struct v4l2_subdev *sd)
> {
> - bool (*match)(struct v4l2_subdev *, struct v4l2_async_subdev *);
> + bool (*match)(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd);
> struct v4l2_async_subdev *asd;
>
> list_for_each_entry(asd, >waiting, list) {
> @@ -150,8 +152,8 @@ static bool asd_equal(struct v4l2_async_subdev *asd_x,
> }
>
> /* Find the sub-device notifier registered by a sub-device driver. */
> -static struct v4l2_async_notifier *v4l2_async_find_subdev_notifier(
> - struct v4l2_subdev *sd)
> +static struct v4l2_async_notifier *
> +v4l2_async_find_subdev_notifier(struct v4l2_subdev *sd)
It's better here. Below, too.
> {
> struct v4l2_async_notifier *n;
>
> @@ -163,8 +165,8 @@ static struct v4l2_async_notifier
> *v4l2_async_find_subdev_notifier(
> }
>
> /* Get v4l2_device related to the notifier if one can be found. */
> -static struct v4l2_device *v4l2_async_notifier_find_v4l2_dev(
> - struct v4l2_async_notifier *notifier)
> +static struct v4l2_device *
> +v4l2_async_notifier_find_v4l2_dev(struct v4l2_async_notifier *notifier)
> {
> while (notifier->parent)
> notifier = notifier->parent;
> @@ -175,8 +177,8 @@ static struct v4l2_device
> *v4l2_async_notifier_find_v4l2_dev(
> /*
> * Return true if all child sub-device notifiers are complete, false
> otherwise.
> */
> -static bool v4l2_async_notifier_can_complete(
> - struct v4l2_async_notifier *notifier)
> +static bool
> +v4l2_async_notifier_can_complete(struct v4l2_async_notifier *notifier)
> {
> struct v4l2_subdev *sd;
>
> @@ -199,8 +201,8 @@ static bool v4l2_async_notifier_can_complete(
> * Complete the master notifier if possible. This is done when all async
> * sub-devices have been bound; v4l2_device is also available then.
> */
> -static int v4l2_async_notifier_try_complete(
> - struct v4l2_async_notifier *notifier)
> +static int
> +v4l2_async_notifier_try_complete(struct v4l2_async_notifier *notifier)
> {
> /* Quick check whether there are still more sub-devices here. */
> if (!list_empty(>waiting))
> @@ -221,8 +223,8 @@ static int v4l2_async_notifier_try_complete(
> return v4l2_async_notifier_call_complete(notifier);
> }
>
> -static int v4l2_async_notifier_try_all_subdevs(
> - struct v4l2_async_notifier *notifier);
> +static int
> +v4l2_async_notifier_try_all_subdevs(struct v4l2_async_notifier *notifier);
>
> static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,
> struct v4l2_device *v4l2_dev,
> @@ -268,8 +270,8 @@ static int v4l2_async_match_notify(struct
> v4l2_async_notifier *notifier,
> }
>
> /* Test all async sub-devices in a notifier for a match. */
> -static int v4l2_async_notifier_try_all_subdevs(
> - struct v4l2_async_notifier *notifier)
> +static int
> +v4l2_async_notifier_try_all_subdevs(struct v4l2_async_notifier *notifier)
> {
> struct v4l2_device
[PATCH 1/3] media: v4l2-core: cleanup coding style at V4L2 async/fwnode
There are several coding style issues at those definitions,
and the previous patchset added even more.
Address the trivial ones by first calling:
./scripts/checkpatch.pl --strict --fix-inline
include/media/v4l2-async.h include/media/v4l2-fwnode.h
include/media/v4l2-mediabus.h drivers/media/v4l2-core/v4l2-async.c
drivers/media/v4l2-core/v4l2-fwnode.c
and then manually adjusting the style where needed.
Signed-off-by: Mauro Carvalho Chehab
---
drivers/media/v4l2-core/v4l2-async.c | 45 ---
drivers/media/v4l2-core/v4l2-fwnode.c | 185 +++---
include/media/v4l2-async.h| 12 +-
include/media/v4l2-fwnode.h | 46 ---
include/media/v4l2-mediabus.h | 32 +++--
5 files changed, 179 insertions(+), 141 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-async.c
b/drivers/media/v4l2-core/v4l2-async.c
index 70adbd9a01a2..6fdda745a1da 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -57,6 +57,7 @@ static bool match_i2c(struct v4l2_subdev *sd, struct
v4l2_async_subdev *asd)
{
#if IS_ENABLED(CONFIG_I2C)
struct i2c_client *client = i2c_verify_client(sd->dev);
+
return client &&
asd->match.i2c.adapter_id == client->adapter->nr &&
asd->match.i2c.address == client->addr;
@@ -89,10 +90,11 @@ static LIST_HEAD(subdev_list);
static LIST_HEAD(notifier_list);
static DEFINE_MUTEX(list_lock);
-static struct v4l2_async_subdev *v4l2_async_find_match(
- struct v4l2_async_notifier *notifier, struct v4l2_subdev *sd)
+static struct
+v4l2_async_subdev *v4l2_async_find_match(struct v4l2_async_notifier *notifier,
+struct v4l2_subdev *sd)
{
- bool (*match)(struct v4l2_subdev *, struct v4l2_async_subdev *);
+ bool (*match)(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd);
struct v4l2_async_subdev *asd;
list_for_each_entry(asd, >waiting, list) {
@@ -150,8 +152,8 @@ static bool asd_equal(struct v4l2_async_subdev *asd_x,
}
/* Find the sub-device notifier registered by a sub-device driver. */
-static struct v4l2_async_notifier *v4l2_async_find_subdev_notifier(
- struct v4l2_subdev *sd)
+static struct v4l2_async_notifier *
+v4l2_async_find_subdev_notifier(struct v4l2_subdev *sd)
{
struct v4l2_async_notifier *n;
@@ -163,8 +165,8 @@ static struct v4l2_async_notifier
*v4l2_async_find_subdev_notifier(
}
/* Get v4l2_device related to the notifier if one can be found. */
-static struct v4l2_device *v4l2_async_notifier_find_v4l2_dev(
- struct v4l2_async_notifier *notifier)
+static struct v4l2_device *
+v4l2_async_notifier_find_v4l2_dev(struct v4l2_async_notifier *notifier)
{
while (notifier->parent)
notifier = notifier->parent;
@@ -175,8 +177,8 @@ static struct v4l2_device
*v4l2_async_notifier_find_v4l2_dev(
/*
* Return true if all child sub-device notifiers are complete, false otherwise.
*/
-static bool v4l2_async_notifier_can_complete(
- struct v4l2_async_notifier *notifier)
+static bool
+v4l2_async_notifier_can_complete(struct v4l2_async_notifier *notifier)
{
struct v4l2_subdev *sd;
@@ -199,8 +201,8 @@ static bool v4l2_async_notifier_can_complete(
* Complete the master notifier if possible. This is done when all async
* sub-devices have been bound; v4l2_device is also available then.
*/
-static int v4l2_async_notifier_try_complete(
- struct v4l2_async_notifier *notifier)
+static int
+v4l2_async_notifier_try_complete(struct v4l2_async_notifier *notifier)
{
/* Quick check whether there are still more sub-devices here. */
if (!list_empty(>waiting))
@@ -221,8 +223,8 @@ static int v4l2_async_notifier_try_complete(
return v4l2_async_notifier_call_complete(notifier);
}
-static int v4l2_async_notifier_try_all_subdevs(
- struct v4l2_async_notifier *notifier);
+static int
+v4l2_async_notifier_try_all_subdevs(struct v4l2_async_notifier *notifier);
static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,
struct v4l2_device *v4l2_dev,
@@ -268,8 +270,8 @@ static int v4l2_async_match_notify(struct
v4l2_async_notifier *notifier,
}
/* Test all async sub-devices in a notifier for a match. */
-static int v4l2_async_notifier_try_all_subdevs(
- struct v4l2_async_notifier *notifier)
+static int
+v4l2_async_notifier_try_all_subdevs(struct v4l2_async_notifier *notifier)
{
struct v4l2_device *v4l2_dev =
v4l2_async_notifier_find_v4l2_dev(notifier);
@@ -306,14 +308,17 @@ static int v4l2_async_notifier_try_all_subdevs(
static void v4l2_async_cleanup(struct v4l2_subdev *sd)
{
v4l2_device_unregister_subdev(sd);
- /* Subdevice driver will reprobe and put the subdev back onto the list
*/
+ /*
+* Subdevice driver will reprobe and put the subdev back
+* onto the list
+
