On Mon, Oct 15, 2012 at 10:58:37AM +0530, Arun Murthy wrote:
I'm going to ignore your .c logic, as there are things in it that I
don't think is correct. But it all comes down to your data structures,
if you fix them, then the .c logic will become correct:
> --- /dev/null
> +++ b/include/linux/modem_shm/modem.h
> @@ -0,0 +1,59 @@
> +/*
> + * Copyright (C) ST-Ericsson SA 2011
> + *
> + * License Terms: GNU General Public License v2
> + * Author: Kumar Sanghvi
> + * Arun Murthy <arun.murthy@xxxxxxxxxxxxxx>
> + *
> + * Heavily adapted from Regulator framework
> + */
> +#ifndef __MODEM_H__
> +#define __MODEM_H__
__MODEM_SHM_MODEM_H__, right?
> +
> +#include <linux/device.h>
> +
> +struct clients {
> + struct device *dev;
Why is this a pointer? It should be embedded in the structure.
> + const char *name;
Why is this needed? It should be the same as the device, right?
> + atomic_t cnt;
Why is this needed at all? And if it's needed, why is it an atomic?
(hint, your use of atomic_t really isn't correct at all in this patch,
it's not doing what you think it is doing...)
> +};
Also, the name of the structure here is _VERY_ generic, that's not
acceptable in the global kernel namespace. Hint, it probably isn't even
needed to be defined in this .h file at all, right?
> +
> +struct modem_desc {
> + int (*request)(struct modem_desc *);
> + void (*release)(struct modem_desc *);
> + int (*is_requested)(struct modem_desc *);
> + struct clients *mclients;
Why do you have a pointer to a device, and yet:
> + struct device *dev;
have a device here?
> + char *name;
Same *dev and name comment as above.
> + u8 no_clients;
> + atomic_t use_cnt;
> + atomic_t cli_cnt;
Same question about these atomic_t variables, why are they here, and
most importantly, why are they an atomic variable?
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
