From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from metis.ext.pengutronix.de (metis.ext.pengutronix.de [92.198.50.35]) by ozlabs.org (Postfix) with ESMTP id EDBE8DE014 for ; Thu, 21 May 2009 10:46:36 +1000 (EST) Date: Thu, 21 May 2009 02:46:26 +0200 From: Wolfram Sang To: Grant Likely Subject: Re: [PATCH 2/2] maps/mtd-ram: add an of-platform driver Message-ID: <20090521004626.GA1635@pengutronix.de> References: <1232559685-2268-1-git-send-email-w.sang@pengutronix.de> <1232559685-2268-3-git-send-email-w.sang@pengutronix.de> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="nFreZHaLTZJo0R7j" In-Reply-To: Cc: linuxppc-dev@ozlabs.org, linux-mtd@lists.infradead.org, ben-linux@fluff.org List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , --nFreZHaLTZJo0R7j Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hello Grant, as I am just working on V2 (fixed even some additional things), two more questions came up: On Tue, May 19, 2009 at 08:57:15AM -0600, Grant Likely wrote: > On Wed, Jan 21, 2009 at 11:41 AM, Wolfram Sang wr= ote: > > Create an of-aware driver using the now exported generic functions from > > plat-ram.c. A typical oftree snipplet for it looks like this: > > > > sram@04e00000 { > > =A0 =A0 =A0 =A0compatible =3D "mtd-ram"; > > =A0 =A0 =A0 =A0reg =3D <0x04e00000 0x00200000>; > > =A0 =A0 =A0 =A0bank-width =3D <2>; > > }; > > > > Partitions on this device are not yet supported. >=20 > This is a new device tree binding. It must be documented in > Documentation/powerpc/dts-bindings and posted to > devicetree-discuss@ozlabs.org for review. >=20 > > diff --git a/drivers/mtd/maps/Makefile b/drivers/mtd/maps/Makefile > > index 6d9ba35..f8e3908 100644 > > --- a/drivers/mtd/maps/Makefile > > +++ b/drivers/mtd/maps/Makefile > > @@ -58,6 +58,7 @@ obj-$(CONFIG_MTD_WRSBC8260) =A0 +=3D wr_sbc82xx_flash= =2Eo > > =A0obj-$(CONFIG_MTD_DMV182) =A0 =A0 =A0 +=3D dmv182.o > > =A0obj-$(CONFIG_MTD_SHARP_SL) =A0 =A0 +=3D sharpsl-flash.o > > =A0obj-$(CONFIG_MTD_PLATRAM) =A0 =A0 =A0+=3D plat-ram.o > > +obj-$(CONFIG_MTD_OFRAM) =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0+=3D of-ram.o > > =A0obj-$(CONFIG_MTD_OMAP_NOR) =A0 =A0 +=3D omap_nor.o > > =A0obj-$(CONFIG_MTD_INTEL_VR_NOR) +=3D intel_vr_nor.o > > =A0obj-$(CONFIG_MTD_BFIN_ASYNC) =A0 +=3D bfin-async-flash.o > > diff --git a/drivers/mtd/maps/of-ram.c b/drivers/mtd/maps/of-ram.c > > new file mode 100644 > > index 0000000..4d334a9 > > --- /dev/null > > +++ b/drivers/mtd/maps/of-ram.c > > @@ -0,0 +1,120 @@ > > +/* > > + * drivers/mtd/maps/of-ram.c > > + * > > + * Generic of device based RAM map > > + * > > + * Copyright (C) 2009 Wolfram Sang, Pengutronix > > + * > > + * Using plat-ram.c by Ben Dooks > > + * > > + * This program is free software; you can redistribute it and/or modif= y it > > + * under the terms of the GNU General Public License version 2 as publ= ished by > > + * the Free Software Foundation. > > + */ > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +static int of_ram_probe(struct of_device *op, const struct of_device_i= d *match) >=20 > __devinit >=20 > > +{ > > + =A0 =A0 =A0 struct platdata_mtd_ram *pdata; > > + =A0 =A0 =A0 struct resource *resource; > > + =A0 =A0 =A0 const char *name; > > + =A0 =A0 =A0 const u32 *bankwidth; > > + =A0 =A0 =A0 int ret =3D -ENOMEM; > > + > > + =A0 =A0 =A0 pdata =3D kzalloc(sizeof(*pdata), GFP_KERNEL); > > + =A0 =A0 =A0 if (!pdata) > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto bad0; > > + =A0 =A0 =A0 op->dev.platform_data =3D pdata; > > + > > + =A0 =A0 =A0 name =3D of_get_property(op->node, "name", NULL); > > + =A0 =A0 =A0 if (!name) { > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 ret =3D -ENOENT; > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_dbg(&op->dev, "could not get node nam= e\n"); > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto bad1; > > + =A0 =A0 =A0 } Can I just use name =3D op->node->name here? I wonder because of_device in asm/of_device.h states 'node' as "to be obsoleted". And could I safely assume that all architectures will have the = node entry? At least, I could drop the error check, if I understand booting-without-of correctly? A name property will always be constructed, even if it is not explicitly given. > > + > > + =A0 =A0 =A0 resource =3D kzalloc(sizeof(struct resource), GFP_KERNEL); > > + =A0 =A0 =A0 if (!resource) > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto bad1; >=20 > Why isn't resource on the stack? >=20 > > + > > + =A0 =A0 =A0 if (of_address_to_resource(op->node, 0, resource)) { > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 ret =3D -ENXIO; > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_dbg(&op->dev, "could not create resou= rce\n"); > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto bad2; > > + =A0 =A0 =A0 } > > + > > + =A0 =A0 =A0 bankwidth =3D of_get_property(op->node, "bank-width", NUL= L); > > + =A0 =A0 =A0 if (*bankwidth =3D=3D 0) { > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 ret =3D -ENOENT; > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_dbg(&op->dev, "bank width not set\n"); > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto bad2; > > + =A0 =A0 =A0 } > > + =A0 =A0 =A0 pdata->bankwidth =3D *bankwidth; > > + > > + =A0 =A0 =A0 ret =3D __platram_probe(&op->dev, name, resource, pdata); > > + =A0 =A0 =A0 kfree(resource); > > + > > + =A0 =A0 =A0 if (ret) > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto bad1; > > + > > + =A0 =A0 =A0 return 0; > > + > > + bad2: > > + =A0 =A0 =A0 kfree(resource); > > + bad1: > > + =A0 =A0 =A0 op->dev.platform_data =3D NULL; > > + =A0 =A0 =A0 kfree(pdata); > > + bad0: > > + =A0 =A0 =A0 return ret; >=20 > I'd rather see more meaningful labels. ie "err_kalloc:" and > "err_platram_probe:". >=20 > > +} > > + > > +static int of_ram_remove(struct of_device *op) >=20 > __devexit >=20 > > +{ > > + =A0 =A0 =A0 int ret; > > + =A0 =A0 =A0 struct platdata_mtd_ram *pdata =3D op->dev.platform_data; > > + > > + =A0 =A0 =A0 ret =3D __platram_remove(&op->dev); > > + =A0 =A0 =A0 op->dev.platform_data =3D NULL; > > + =A0 =A0 =A0 kfree(pdata); > > + =A0 =A0 =A0 return ret; > > +} > > + > > +static struct of_device_id of_ram_match[] =3D { > > + =A0 =A0 =A0 { .compatible =3D "mtd-ram", }, > > + =A0 =A0 =A0 {}, > > +}; > > +MODULE_DEVICE_TABLE(of, of_ram_match); > > + > > +static struct of_platform_driver of_ram_driver =3D { >=20 > __devinitdata I assume you mean the match_table and not the of_platform_driver. Shouldn't= it even better be const and using __devinitconst? >=20 > > + =A0 =A0 =A0 .owner =3D THIS_MODULE, > > + =A0 =A0 =A0 .name =3D "of-ram", >=20 > This name is too generic for an of_platform device. 'mtd' needs to be > in there somewhere. >=20 > > + =A0 =A0 =A0 .match_table =3D of_ram_match, > > + =A0 =A0 =A0 .probe =3D of_ram_probe, > > + =A0 =A0 =A0 .remove =3D of_ram_remove, >=20 > __devexit_p() >=20 > > +}; > > + > > +static int __init of_ram_init(void) > > +{ > > + =A0 =A0 =A0 return of_register_platform_driver(&of_ram_driver); > > +} > > + > > +static void __exit of_ram_exit(void) > > +{ > > + =A0 =A0 =A0 of_unregister_platform_driver(&of_ram_driver); > > +} > > + > > +module_init(of_ram_init); >=20 > Put the module_init() statement directly after the of_ram_init() definiti= on. >=20 > > +module_exit(of_ram_exit); > > + > > +MODULE_AUTHOR("Wolfram Sang"); > > +MODULE_DESCRIPTION("MTD OF RAM map driver"); > > +MODULE_LICENSE("GPL v2"); > > -- > > 1.5.6.5 > > > > _______________________________________________ > > Linuxppc-dev mailing list > > Linuxppc-dev@ozlabs.org > > https://ozlabs.org/mailman/listinfo/linuxppc-dev > > >=20 >=20 >=20 > --=20 > Grant Likely, B.Sc., P.Eng. > Secret Lab Technologies Ltd. >=20 > ______________________________________________________ > Linux MTD discussion mailing list > http://lists.infradead.org/mailman/listinfo/linux-mtd/ --=20 Pengutronix e.K. | Wolfram Sang | Industrial Linux Solutions | http://www.pengutronix.de/ | --nFreZHaLTZJo0R7j Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (GNU/Linux) iEYEARECAAYFAkoUpGIACgkQD27XaX1/VRsi3QCfaeRmnWt0d66EN3hjcfNBwB8S gVQAnjuI+nXRdlAYu9FnrWWGa521ZMua =e0bD -----END PGP SIGNATURE----- --nFreZHaLTZJo0R7j--