Re: [PATCH 01/11 v6] coresight: add CoreSight core layer framework

[Greg KH]

Some first impressions in glancing at the code, not a complete review at
all:

On Thu, Sep 11, 2014 at 09:49:08AM -0600, mathieu.poirier@xxxxxxxxxx wrote:
> --- /dev/null
> +++ b/drivers/coresight/coresight.c
> @@ -0,0 +1,663 @@
> +/* Copyright (c) 2012, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#define pr_fmt(fmt) "coresight: " fmt

MODULE_NAME ?

> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/types.h>
> +#include <linux/device.h>
> +#include <linux/io.h>
> +#include <linux/err.h>
> +#include <linux/export.h>
> +#include <linux/slab.h>
> +#include <linux/semaphore.h>
> +#include <linux/clk.h>
> +#include <linux/coresight.h>
> +#include <linux/of_platform.h>
> +#include <linux/debugfs.h>
> +#include <linux/delay.h>
> +
> +#include "coresight-priv.h"
> +
> +struct dentry *cs_debugfs_parent = NULL;
> +
> +static LIST_HEAD(coresight_orph_conns);
> +static LIST_HEAD(coresight_devs);

You are a struct device, you don't need a separate list, why not just
iterate over your bus list of devices?

> +/**
> + * @id:		unique ID of the component.
> + * @conns:	list of connections associated to this component.
> + * @type:	as defined by @coresight_dev_type.
> + * @subtype:	as defined by @coresight_dev_subtype.
> + * @ops:	generic operations for this component, as defined
> +		by @coresight_ops.
> + * @de:		handle on a component's debugfs entry.
> + * @dev:	The device entity associated to this component.
> + * @kref:	keeping count on component references.
> + * @dev_link:	link of current component into list of all components
> +		discovered in the system.
> + * @path_link:	link of current component into the path being enabled.
> + * @owner:	typically "THIS_MODULE".
> + * @enable:	'true' if component is currently part of an active path.
> + * @activated:	'true' only if a _sink_ has been activated.  A sink can be
> +		activated but not yet enabled.  Enabling for a _sink_
> +		happens when a source has been selected for that it.
> + */
> +struct coresight_device {
> +	int id;

Why not use the device name instead?

> +	struct coresight_connection *conns;
> +	int nr_conns;
> +	enum coresight_dev_type type;
> +	struct coresight_dev_subtype subtype;
> +	const struct coresight_ops *ops;
> +	struct dentry *de;

All devices have a dentry?  in debugfs?  isn't that overkill?

> +	struct device dev;
> +	struct kref kref;

You CAN NOT have two reference counts in the same structure, that's a
huge design mistake.  Stick with one reference count, otherwise they are
guaranteed to get out of sync and bad things will happen.

> +	struct list_head dev_link;

As discussed above, I don't think you need this.

> +	struct list_head path_link;

Odds are, you don't need this either.

> +	struct module *owner;

devices aren't owned by modules, they are data, not code.


> +	bool enable;	/* true only if configured as part of a path */
> +	bool activated;	/* true only if a sink is part of a path */
> +};
> +
> +#define to_coresight_device(d) container_of(d, struct coresight_device, dev)
> +
> +#define source_ops(csdev)	csdev->ops->source_ops
> +#define sink_ops(csdev)		csdev->ops->sink_ops
> +#define link_ops(csdev)		csdev->ops->link_ops
> +
> +#define CORESIGHT_DEBUGFS_ENTRY(__name, __entry_name,			\
> +				 __mode, __get, __set, __fmt)		\
> +DEFINE_SIMPLE_ATTRIBUTE(__name ## _ops, __get, __set, __fmt)		\

Use the RW and RO only versions please.  No need to ever set your own
mode value.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/