On Mon, Dec 16, 2013 at 12:27 AM, Sebastian Reichel <sre@xxxxxxxxxx> wrote:
> Add driver handling GPIO pins of Nokia modems. The
> driver provides reset notifications, so that SSI
> clients can subscribe to them easily.
>
> Signed-off-by: Sebastian Reichel <sre@xxxxxxxxxx>
If the driver provides reset notifications, should it rather be
in drivers/reset?
I'm thinking of a generic GPIO reset driver with a generic
notification mechanism, which seems to be what this is.
I.e. it doesn't look device-specific at all, just like some
generic glue code that could be useful to many such
scenarios.
> +config NOKIA_CMT
> + tristate "Enable CMT support"
> + help
> + If you say Y here, you will enable CMT support.
> +
> + If unsure, say Y, or else you will not be able to use the CMT.
None of this explains what this driver actually does and what
CMT means, so please rewrite this a bit to be more helpful.
The way it is written it could as well say "enable FOO support".
> +++ b/drivers/misc/nokia-cmt.c
> @@ -0,0 +1,298 @@
> +/*
> + * CMT support.
So CMT = ...?
> +/**
> + * struct cmt_device - CMT device data
> + * @cmt_rst_ind_tasklet: Bottom half for CMT reset line events
> + * @cmt_rst_ind_irq: IRQ number of the CMT reset line
> + * @n_head: List of notifiers registered to get CMT events
> + * @node: Link on the list of available CMTs
> + * @device: Reference to the CMT platform device
> + */
> +struct cmt_device {
> + struct tasklet_struct cmt_rst_ind_tasklet;
> + unsigned int cmt_rst_ind_irq;
> + struct atomic_notifier_head n_head;
> + struct list_head node;
> + struct device *device;
> + struct cmt_gpio *gpios;
> + int gpio_amount;
> +};
The kerneldoc and and the struct are not in sync. Look
this over.
> +static LIST_HEAD(cmt_list); /* List of CMT devices */
(...)
> +struct cmt_device *cmt_get(const char *name)
> +{
> + struct cmt_device *p, *cmt = ERR_PTR(-ENODEV);
> +
> + list_for_each_entry(p, &cmt_list, node)
> + if (strcmp(name, dev_name(p->device)) == 0) {
> + cmt = p;
> + break;
> + }
> +
> + return cmt;
> +}
> +EXPORT_SYMBOL_GPL(cmt_get);
Is this driver used on non-DT platforms, or can we skip this
struct device * referencing altogether?
I would feel better if this driver looked more like
drivers/mfd/syscon.c
> +static irqreturn_t cmt_rst_ind_isr(int irq, void *cmtdev)
> +{
> + struct cmt_device *cmt = (struct cmt_device *)cmtdev;
> +
> + tasklet_schedule(&cmt->cmt_rst_ind_tasklet);
> +
> + return IRQ_HANDLED;
> +}
Why are you using a tasklet rather than a work
for this?
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
