linux-kernel - Re: [RFCv4 06/11] misc: Introduce Nokia CMT driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20131216121524.GA6177@earth.universe>
Date:	Mon, 16 Dec 2013 13:15:24 +0100
From:	Sebastian Reichel <sre@...ian.org>
To:	Linus Walleij <linus.walleij@...aro.org>
Cc:	Shubhrajyoti Datta <omaplinuxkernel@...il.com>,
	Carlos Chinea <cch.devel@...il.com>,
	Tony Lindgren <tony@...mide.com>,
	Grant Likely <grant.likely@...aro.org>,
	Rob Herring <rob.herring@...xeda.com>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Stephen Warren <swarren@...dotorg.org>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Rob Landley <rob@...dley.net>,
	Benoît Cousson <bcousson@...libre.com>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Linux-OMAP <linux-omap@...r.kernel.org>,
	Pali Rohár <pali.rohar@...il.com>,
	Ивайло Димитров 
	<freemangordon@....bg>,
	Joni Lapilainen <joni.lapilainen@...il.com>,
	Aaro Koskinen <aaro.koskinen@....fi>
Subject: Re: [RFCv4 06/11] misc: Introduce Nokia CMT driver

Hi Linus,

On Mon, Dec 16, 2013 at 10:48:06AM +0100, Linus Walleij wrote:
> On Mon, Dec 16, 2013 at 12:27 AM, Sebastian Reichel <sre@...ian.org> 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@...ian.org>
> 
> 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.

I like the idea.

Probably the remaining gpio exporting code can be converted into
some generic gpio-sysfs-export driver as well.

What do you think about the following?

/*
 * driver, which provides generic reset notifications
 */
cmt_reset: reset-notifier {
    compatible = "linux,reset-notification";

    interrupts = <gpio>;
};

/*
 * driver, which exports the specified gpios in sysfs with the
 * supplied names. The device will be named according to the
 * label
 */
cmt_gpios: gpio-sysfs-export {
    compatible = "linux,gpio-sysfs-export";
    label = "nokia-cmt";

    gpios = <A>, <B>, ...;
    gpio-names = "A", "B", ...;
};

> > +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".

I probably should have reviewed this better. This is the original
text from Nokia's driver.

> > +++ b/drivers/misc/nokia-cmt.c
> > @@ -0,0 +1,298 @@
> > +/*
> > + * CMT support.
> 
> So CMT = ...?

CMT = Cellular Modem

> > +/**
> > + * 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.

I will fix this in v5 (in case that its not removed completly).

> > +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

Will be removed in v5.

> > +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?

No particular reason. I just took this over from Nokia's code.

-- Sebastian

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)