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 <johannes.thumsh...@men.de>
> + *
> + * 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 <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +#include <linux/rwsem.h>
> +#include <linux/idr.h>
> +#include <linux/mcb.h>
> +
> +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 <johannes.thumsh...@men.de>");
> +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 <linux/types.h>
> +
> +#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 <johannes.thumsh...@men.de>
> + *
> + * 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 <linux/mod_devicetable.h>
> +#include <linux/device.h>
> +#include <linux/irqreturn.h>
> +
> +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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
