Hi Brian, On Wed, 2014-04-30 at 10:21 -0700, Brian Norris wrote: > A few more small comments. > > On Wed, Apr 16, 2014 at 09:17:15PM +0400, Sergei Ianovich wrote: > > +++ b/Documentation/devicetree/bindings/mtd/sram-lp8x4x.txt > > @@ -0,0 +1,22 @@ > > +512kB battery backed up SRAM on LP-8x4x industrial computers > > + > > +Required properties: > > +- compatible : should be "icpdas,sram-lp8x4x" > > + > > +- reg: physical base addresses and region lengths of > > + * IO memory range > > + * SRAM page selector > > Are these region types pretty static for this type of hardware? If not, > it helps to have a reg-names property in the DT, when there are 2 or > more register resources. The regions are fixed. The addresses are hard-wired. > > +- eeprom-gpios : should point to active-low write enable GPIO > > I'm curious: your driver doesn't actually utilize this binding. Is this > intentional? Is it actually optional? (I note that the example DT below > doesn't have this property...) Thanks for noticing. It's an artifact of copy-paste. I'll drop this. > > +++ b/arch/arm/configs/lp8x4x_defconfig > > @@ -57,6 +57,7 @@ CONFIG_MTD_CFI_ADV_OPTIONS=y > > CONFIG_MTD_CFI_GEOMETRY=y > > CONFIG_MTD_CFI_INTELEXT=y > > CONFIG_MTD_PHYSMAP_OF=y > > +CONFIG_MTD_SRAM_LP8X4X=y > > CONFIG_PROC_DEVICETREE=y > > CONFIG_BLK_DEV_LOOP=y > > CONFIG_BLK_DEV_LOOP_MIN_COUNT=2 > > I can't take the defconfig update via MTD; it will need to go via the > appropriate ARM tree (arm-soc?). So this hunk needs to move to another > patch. Sure. I'll remove this chunk and put it into main device patch. > > + match = of_match_device(of_flash_match, &pdev->dev); > > + if (!match) > > + return -EINVAL; > > Does this of_match_device() serve any particular purpose? Your driver > already matches against these IDs, and you're not actually retrieving > any of-data from the match, so this looks redundant. Point taken, I'll drop this. > > + > > + info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL); > > + if (!info) > > + return -ENOMEM; > > + > > + res_virt = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + info->virt = devm_ioremap_resource(&pdev->dev, res_virt); > > + if (IS_ERR(info->virt)) > > + return PTR_ERR(info->virt); > > + > > + res_bank = platform_get_resource(pdev, IORESOURCE_MEM, 1); > > + info->bank = devm_ioremap_resource(&pdev->dev, res_bank); > > + if (IS_ERR(info->bank)) > > + return PTR_ERR(info->bank); > > + > > + info->mtd.priv = info; > > + info->mtd.name = "SRAM"; > > Are you absolutely sure there is only ever a single SRAM device on a > given system? Because otherwise, you will get redundantly-named MTD's. > If the answer is no, you might consider a unique naming scheme. Like .999999 sure. This one is hard-wired. There is no extension slots to plug in any memory device. I'll post a new version with the rest of the series. Thanks for reviewing. -- 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
