Hi,
just few minor things.
On Wed, 2011-03-30 at 09:27 +0200, John Crispin wrote:
> +/* the NOR flash is connected to the same external bus unit (EBU) as PCI.
> + * To make PCI work we need to enable the endianess swapping of the addr
> + * written to the EBU. this however has some limitations and breaks when
> + * using NOR. it does not really matter if the onflash data is in a swapped
> + * order, however cfi sequences also fail. to workaround this we need to use
> + * a complex map. We essentially software swap all addresses during probe
> + * and then swizzle the unlock addresses.
> + */
Would be nice to clean-up the comment a little bit and use capital
letters at the start of the sentences. Also, I think the style for
multi-line comments is:
/*
* bla
* bla
*/
> +static void
> +ltq_copy_from(struct map_info *map, void *to,
> + unsigned long from, ssize_t len)
> +{
> + unsigned char *f = (unsigned char *) (map->virt + from);
> + unsigned char *t = (unsigned char *) to;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&ebu_lock, flags);
> + while (len--)
> + *t++ = *f++;
> + spin_unlock_irqrestore(&ebu_lock, flags);
Can you use memcpy here instead?
> +}
> +
> +static void
> +ltq_copy_to(struct map_info *map, unsigned long to,
> + const void *from, ssize_t len)
> +{
> + unsigned char *f = (unsigned char *) from;
> + unsigned char *t = (unsigned char *) (map->virt + to);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&ebu_lock, flags);
> + while (len--)
> + *t++ = *f++;
Can you use memcpy here instead?
> + spin_unlock_irqrestore(&ebu_lock, flags);
> +}
> +
> +static const char const *part_probe_types[] = {
> + "cmdlinepart", NULL };
This can be one-liner, no need to make 2 lines.
> +
> +static struct map_info ltq_map = {
> + .name = "ltq_nor",
> + .bankwidth = 2,
> + .read = ltq_read16,
> + .write = ltq_write16,
> + .copy_from = ltq_copy_from,
> + .copy_to = ltq_copy_to,
> +};
> +
> +static int __init
> +ltq_mtd_probe(struct platform_device *pdev)
> +{
> + struct physmap_flash_data *ltq_mtd_data =
> + dev_get_platdata(&pdev->dev);
This can be one line - it takes only 79 characters.
> + struct mtd_info *ltq_mtd = NULL;
> + struct mtd_partition *parts = NULL;
> + struct resource *res = 0;
You do not need this initialization.
> + int nr_parts = 0;
> + struct cfi_private *cfi;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res) {
> + dev_err(&pdev->dev, "failed to get memory resource");
> + return -ENOENT;
> + }
> + res = devm_request_mem_region(&pdev->dev, res->start,
> + resource_size(res), dev_name(&pdev->dev));
> + if (!res) {
> + dev_err(&pdev->dev, "failed to request mem resource");
> + return -EBUSY;
> + }
> +
> + ltq_map.phys = res->start;
> + ltq_map.size = resource_size(res);
> + ltq_map.virt = devm_ioremap_nocache(&pdev->dev, ltq_map.phys,
> + ltq_map.size);
> + if (!ltq_map.virt) {
> + dev_err(&pdev->dev, "failed to ioremap!\n");
> + return -EIO;
> + }
> +
> + ltq_mtd_probing = 1;
> + ltq_mtd = do_map_probe("cfi_probe", <q_map);
> + ltq_mtd_probing = 0;
> + if (!ltq_mtd) {
> + iounmap(ltq_map.virt);
> + dev_err(&pdev->dev, "probing failed\n");
> + return -ENXIO;
> + }
> + ltq_mtd->owner = THIS_MODULE;
> +
> + cfi = ltq_map.fldrv_priv;
> + cfi->addr_unlock1 ^= 1;
> + cfi->addr_unlock2 ^= 1;
> +
> + nr_parts = parse_mtd_partitions(ltq_mtd, part_probe_types, &parts, 0);
> + if (nr_parts > 0) {
> + dev_info(&pdev->dev,
> + "using %d partitions from cmdline", nr_parts);
> + } else {
> + nr_parts = ltq_mtd_data->nr_parts;
> + parts = ltq_mtd_data->parts;
> + }
> +
> + add_mtd_partitions(ltq_mtd, parts, nr_parts);
This function may return an error.
> + return 0;
> +}
> +
> +static struct platform_driver ltq_mtd_driver = {
> + .driver = {
> + .name = "ltq_nor",
> + .owner = THIS_MODULE,
> + },
> +};
> +
> +int __init
> +init_ltq_mtd(void)
> +{
> + int ret = platform_driver_probe(<q_mtd_driver, ltq_mtd_probe);
> +
> + if (ret)
> + printk(KERN_INFO "ltq_nor: error registering platfom driver");
I think errors should be printed with KERN_ERR level. Anyway, use
pr_err() here instead pleas.
--
Best Regards,
Artem Bityutskiy (ÐÑÑÑÐ ÐÐÑÑÑÐÐÐ)
