From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sebastian Reichel Subject: Re: [RFCv4 06/11] misc: Introduce Nokia CMT driver Date: Mon, 16 Dec 2013 13:15:24 +0100 Message-ID: <20131216121524.GA6177@earth.universe> References: <1387150085-23173-1-git-send-email-sre@debian.org> <1387150085-23173-7-git-send-email-sre@debian.org> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="KsGdsel6WgEHnImy" Return-path: Content-Disposition: inline In-Reply-To: Sender: linux-omap-owner@vger.kernel.org To: Linus Walleij Cc: Shubhrajyoti Datta , Carlos Chinea , Tony Lindgren , Grant Likely , Rob Herring , Pawel Moll , Mark Rutland , Stephen Warren , Ian Campbell , Rob Landley , =?iso-8859-1?Q?Beno=EEt?= Cousson , "devicetree@vger.kernel.org" , "linux-kernel@vger.kernel.org" , Linux-OMAP , Pali =?iso-8859-1?Q?Roh=E1r?= , =?utf-8?B?0JjQstCw0LnQu9C+INCU0LjQvNC40YLRgNC+0LI=?= , Joni Lapilainen , Aaro Koskinen List-Id: devicetree@vger.kernel.org --KsGdsel6WgEHnImy Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 wrot= e: > > 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 >=20 > If the driver provides reset notifications, should it rather be > in drivers/reset? >=20 > I'm thinking of a generic GPIO reset driver with a generic > notification mechanism, which seems to be what this is. >=20 > 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 =3D "linux,reset-notification"; interrupts =3D ; }; /* * 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 =3D "linux,gpio-sysfs-export"; label =3D "nokia-cmt"; gpios =3D , , ...; gpio-names =3D "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. >=20 > 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. >=20 > So CMT =3D ...? CMT =3D 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; > > +}; >=20 > 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 =3D ERR_PTR(-ENODEV); > > + > > + list_for_each_entry(p, &cmt_list, node) > > + if (strcmp(name, dev_name(p->device)) =3D=3D 0) { > > + cmt =3D p; > > + break; > > + } > > + > > + return cmt; > > +} > > +EXPORT_SYMBOL_GPL(cmt_get); >=20 > 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 =3D (struct cmt_device *)cmtdev; > > + > > + tasklet_schedule(&cmt->cmt_rst_ind_tasklet); > > + > > + return IRQ_HANDLED; > > +} >=20 > 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 --KsGdsel6WgEHnImy Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.15 (GNU/Linux) iQIcBAEBCAAGBQJSru7cAAoJENju1/PIO/qa6qkP+gKKi+BUkOKDO5LwVej51IfF Jc8tpX1k8OpI2JgALP4uX8sQEUn/TCO4djGyPZU+GNbbZW8MLMAxrh1MIkm/zOrk zyHI0ifIhke4uexPgeuJ9lqobqeSzy+J0BcJkw4bWDG4XJIMBEGXzViQzVzI+8EB ODKKriYvqdaTTetPLq102cb7E7v/P3wqXVRTmUU6F9d7L/ACuhf/YGzvt5iel7UV aPJpsV7wp0XlG0+yGDVFjdhrv4rDBRhDw5a4hyz8b6VRGNBGYKGNTaWGSnCSxyEO D/F7WmDM3A3JR3J0+yCOtGm4D6HrQq2F0xlb6g620pa5S3VcxE9H1JHDyH/vt80Z /kolz7AUSbvGRdeTCRU92phThazEoWzKWQy2g5D+dN8g9rFRXO4bey8mmTrC42AF cl9Mw2T1aPRrRkRL8DEZo+MRukeYIOATicbf7g435of5KvwHdJBIUHHipHCR7j3I e9jh7gG+Hpvzjr3L5rZve60JaTif4dKOlT20HHbqjx0Sk0e3D/ugUK+bDAU153IR g7g+q70DEkFEhB9WDJvK4074owV+ilfXuer8xsULronfiwZPYcU1EFjGgxTkY2OZ VsxBASde3usnxCWISzLRWMnd+dtMiht9WN/sP5gxLHiifND5KuRrim0ESq85DuhJ Y1LUlolhtDdIJAnwZqjk =lNxa -----END PGP SIGNATURE----- --KsGdsel6WgEHnImy--