Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754079AbaBRXEo (ORCPT ); Tue, 18 Feb 2014 18:04:44 -0500 Received: from smtp.codeaurora.org ([198.145.11.231]:35836 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753677AbaBRXEi (ORCPT ); Tue, 18 Feb 2014 18:04:38 -0500 Date: Tue, 18 Feb 2014 17:02:10 -0600 From: Josh Cartwright To: Johannes Thumshirn Cc: Greg Kroah-Hartman , Jonathan Cameron , linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/3] drivers: Introduce MEN Chameleon Bus Message-ID: <20140218230210.GK31116@joshc.qualcomm.com> References: <1392737654-22682-1-git-send-email-johannes.thumshirn@men.de> <1392737654-22682-2-git-send-email-johannes.thumshirn@men.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1392737654-22682-2-git-send-email-johannes.thumshirn@men.de> User-Agent: Mutt/1.5.21 (2012-12-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hello Johannes, On Tue, Feb 18, 2014 at 04:34:12PM +0100, Johannes Thumshirn wrote: [..] > +++ b/drivers/mcb/mcb-core.c > @@ -0,0 +1,420 @@ > +/* > + * MEN Chameleon Bus. > + * > + * Copyright (C) 2013 MEN Mikroelektronik GmbH (www.men.de) > + * Author: Johannes Thumshirn > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms of the GNU General Public License as published by the Free > + * Software Foundation; version 2 of the License. > + */ > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +static DECLARE_RWSEM(mcb_bus_sem); Why a rwsemaphore and not a simple mutex? It doesn't appear like you attempt to handle any multiple-reader usecases. > +static DEFINE_IDA(mcb_ida); > + [..] > +static void mcb_release_dev(struct device *dev) > +{ > + struct mcb_device *mdev = to_mcb_device(dev); > + > + down_write(&mcb_bus_sem); > + list_del(&mdev->bus_list); > + up_write(&mcb_bus_sem); Why even maintain your own list of devices? Doesn't the bus_type already do so for you? (And provides conveniences such as bus_for_each_dev, etc). > + > + mcb_bus_put(mdev->bus); > + kfree(mdev); > +} > + > +/** > + * mcb_device_register() - Register a mcb_device > + * @bus: The @mcb_bus of the device > + * @dev: The @mcb_device > + * > + * Register a specific @mcb_device at a @mcb_bus and the system itself. > + */ > +int mcb_device_register(struct mcb_bus *bus, struct mcb_device *dev) > +{ > + int ret; > + int device_id; > + > + device_initialize(&dev->dev); > + dev->dev.bus = &mcb_bus_type; > + dev->dev.parent = bus->dev.parent; > + dev->dev.release = mcb_release_dev; > + > + Nit: extraneous line. > + device_id = dev->id; > + dev_set_name(&dev->dev, "mcb%d-16z%03d-%d:%d:%d", > + bus->bus_nr, device_id, dev->inst, dev->group, dev->var); > + > + down_write(&mcb_bus_sem); > + list_add_tail(&dev->bus_list, &bus->devices); > + up_write(&mcb_bus_sem); Again, unsure why you maintain your own list :(. > + > + ret = device_add(&dev->dev); > + if (ret < 0) { > + pr_err("Failed registering device 16z%03d on bus mcb%d (%d)\n", > + device_id, bus->bus_nr, ret); > + goto out; > + } > + > + return 0; > + > +out: > + > + return ret; > +} [..] > +/** > + * mcb_get_irq() - Get device's IRQ number > + * @dev: The @mcb_device the IRQ is for > + * > + * Get the IRQ number of a given @mcb_device. > + */ > +int mcb_get_irq(struct mcb_device *dev) > +{ > + struct resource *irq = &dev->irq; > + > + return irq ? irq->start : -ENXIO; How could irq ever be NULL? > +} > +EXPORT_SYMBOL_GPL(mcb_get_irq); > + > +static int mcb_init(void) > +{ > + ida_init(&mcb_ida); This isn't explicitly necessary (DEFINE_IDA statically initializes the ida). > + return bus_register(&mcb_bus_type); > +} > + > +static void mcb_exit(void) > +{ > + bus_unregister(&mcb_bus_type); > + ida_destroy(&mcb_ida); This is also unnecessary. > +} > + > +/* mcb must be initialized after PCI but before the chameleon drivers. > + * That means we must use some initcall between subsys_initcall and > + * device_initcall. > + */ > +fs_initcall(mcb_init); > +module_exit(mcb_exit); > + > +MODULE_DESCRIPTION("MEN Chameleon Bus Driver"); > +MODULE_AUTHOR("Johannes Thumshirn "); > +MODULE_LICENSE("GPL v2"); > diff --git a/drivers/mcb/mcb-internal.h b/drivers/mcb/mcb-internal.h > new file mode 100644 > index 0000000..ca253e3 > --- /dev/null > +++ b/drivers/mcb/mcb-internal.h > @@ -0,0 +1,118 @@ > +#ifndef __MCB_INTERNAL > +#define __MCB_INTERNAL > + > +#include > + > +#define PCI_VENDOR_ID_MEN 0x1a88 > +#define PCI_DEVICE_ID_MEN_CHAMELEON 0x4d45 This seems misplaced. Perhaps it should really be in the PCI carrier patch? > +#define CHAMELEON_FILENAME_LEN 12 > +#define CHAMELEONV2_MAGIC 0xabce > + [..] > +int parse_chameleon_cells(struct mcb_bus *bus, phys_addr_t mapbase, > + void __iomem *base); Nit: it'd be nicer if you consistently used the chameleon_ prefix. That is, rename this chameleon_parse_cells(...). [..] > +int parse_chameleon_cells(struct mcb_bus *bus, phys_addr_t mapbase, > + void __iomem *base) > +{ > + char __iomem *p = base; > + struct chameleon_fpga_header *header; > + uint32_t dtype; > + int num_cells = 0; > + int ret = 0; > + u32 hsize; > + > + hsize = sizeof(struct chameleon_fpga_header); > + > + header = kzalloc(hsize, GFP_KERNEL); > + if (!header) > + return -ENOMEM; > + > + /* Extract header information */ > + memcpy_fromio(header, p, hsize); > + /* We only support chameleon v2 at the moment */ > + header->magic = le16_to_cpu(header->magic); > + if (header->magic != CHAMELEONV2_MAGIC) { > + pr_err("Unsupported chameleon version 0x%x\n", > + header->magic); > + kfree(header); > + return -ENODEV; > + } > + p += hsize; > + > + pr_debug("header->revision = %d\n", header->revision); > + pr_debug("header->model = 0x%x ('%c')\n", header->model, > + header->model); > + pr_debug("header->minor = %d\n", header->minor); > + pr_debug("header->bus_type = 0x%x\n", header->bus_type); > + > + > + pr_debug("header->magic = 0x%x\n", header->magic); > + pr_debug("header->filename = \"%.*s\"\n", CHAMELEON_FILENAME_LEN, > + header->filename); > + > + for_each_chameleon_cell(dtype, p) { > + switch (dtype) { > + case CHAMELEON_DTYPE_GENERAL: > + ret = parse_chameleon_gdd(bus, mapbase, p); Same comment about using chameleon_ prefix. > + if (ret < 0) > + goto out; > + p += sizeof(struct chameleon_gdd); > + break; > + case CHAMELEON_DTYPE_BRIDGE: > + parse_chameleon_bdd(bus, mapbase, p); And this one, too :). [..] > +++ b/include/linux/mcb.h > @@ -0,0 +1,113 @@ > +/* > + * MEN Chameleon Bus. > + * > + * Copyright (C) 2014 MEN Mikroelektronik GmbH (www.men.de) > + * Author: Johannes Thumshirn > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms of the GNU General Public License as published by the Free > + * Software Foundation; version 2 of the License. > + */ > +#ifndef _LINUX_MCB_H > +#define _LINUX_MCB_H > + > +#include > +#include > +#include > + > +struct mcb_driver; > + > +/** > + * struct mcb_bus - MEN Chameleon Bus > + * > + * @parent: Pointer to carrier device > + * @cores: Number of cores available on this bus Looks like these comments could be updated... > + * @bus_nr: mcb bus number > + */ > +struct mcb_bus { > + struct list_head node; > + struct list_head devices; > + struct list_head children; > + struct device dev; > + int bus_nr; > +}; > +#define to_mcb_bus(b) container_of((b), struct mcb_bus, dev) > + > +/** > + * struct mcb_device - MEN Chameleon Bus device > + * > + * @bus_list: internal list handling for bus code > + * @dev: device in kernel representation > + * @bus: mcb bus the device is plugged to > + * @subordinate: subordinate MCBus in case of bridge > + * @is_added: flag to check if device is added to bus > + * @driver: associated mcb_driver > + * @id: mcb device id > + * @inst: instance in Chameleon table > + * @group: group in Chameleon table > + * @var: variant in Chameleon table > + * @bar: BAR in Chameleon table > + * @rev: revision in Chameleon table > + * @irq: IRQ resource > + * @memory: memory resource > + */ > +struct mcb_device { > + struct list_head bus_list; > + struct device dev; > + struct mcb_bus *bus; > + struct mcb_bus *subordinate; > + bool is_added; > + struct mcb_driver *driver; > + u16 id; > + int inst; > + int group; > + int var; > + int bar; > + int rev; > + struct resource irq; > + struct resource mem; > +}; > +#define to_mcb_device(x) container_of((x), struct mcb_device, dev) > + > +struct mcb_driver { This could probably use a kerneldoc. > + struct list_head node; How is this node used? > + struct device_driver driver; > + const struct mcb_device_id *id_table; > + int (*probe)(struct mcb_device *mdev, const struct mcb_device_id *id); > + void (*remove)(struct mcb_device *mdev); > + void (*shutdown)(struct mcb_device *mdev); > +}; -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/