On Friday 15 August 2014 17:13:12 J. German Rivera wrote: > +struct fsl_mc_bus *fsl_mc_bus; > +EXPORT_SYMBOL(fsl_mc_bus);
This does not look like something that should be exported. Or even better, kill this structure entirely and just pass around pointers to the fsl_mc_device so you can deal with multiple root instances. > +static struct kmem_cache *mc_dev_cache; > + > +/** > + * fsl_mc_bus_match - device to driver matching callback > + * @dev: the MC object device structure to match against > + * @drv: the device driver to search for matching MC object device id > + * structures > + * > + * Returns 1 on success, 0 otherwise. > + */ > +static int fsl_mc_bus_match(struct device *dev, struct device_driver *drv) > +{ > + const struct fsl_mc_device_match_id *id; > + struct fsl_mc_device *mc_dev = to_fsl_mc_device(dev); > + struct fsl_mc_driver *mc_drv = to_fsl_mc_driver(drv); > + bool found = false; > + > + if (WARN_ON(mc_dev->magic != FSL_MC_DEVICE_MAGIC)) > + goto out; > + if (WARN_ON(mc_drv->magic != FSL_MC_DRIVER_MAGIC)) > + goto out; We normally don't do this magic number matching, just remove these and rely on the compile-time checks. > +struct bus_type fsl_mc_bus_type = { > + .name = "fsl-mc", > + .match = fsl_mc_bus_match, > + .uevent = fsl_mc_bus_uevent, > + .drv_groups = NULL, > + .dev_groups = NULL, > + .bus_groups = NULL, > + .pm = NULL, > +}; > +EXPORT_SYMBOL(fsl_mc_bus_type); No need to assign NULL members. Does it need to be exported to drivers? How about making it EXPORT_SYMBOL_GPL if it does? > +static int dprc_parse_dt_node(struct platform_device *pdev, > + phys_addr_t *mc_portal_phys_addr, > + uint32_t *mc_portal_size) > +{ > + struct resource res; > + struct device_node *pdev_of_node = pdev->dev.of_node; > + int error = -EINVAL; > + > + error = of_address_to_resource(pdev_of_node, 0, &res); > + if (error < 0) { > + FSL_MC_ERROR(&pdev->dev, > + "of_address_to_resource() failed for %s\n", > + pdev_of_node->full_name); > + goto out; > + } > + > + *mc_portal_phys_addr = res.start; > + *mc_portal_size = resource_size(&res); > + error = 0; > +out: > + return error; > +} Why not just call of_address_to_resource in the caller? > +/** > + * __fsl_mc_driver_register - registers a child device driver with the > + * MC bus > + * > + * This function is implicitly invoked from the registration function of > + * fsl_mc device drivers, which is generated by the > + * module_fsl_mc_driver() macro. > + */ > +int __fsl_mc_driver_register(struct fsl_mc_driver *mc_driver, > + struct module *owner) > +{ > + struct fsl_mc_device *root_mc_dev; Here the root_mc_dev variable isn't really used for much. > +static int fsl_mc_device_get_mmio_regions(struct fsl_mc_device *mc_dev, > + struct fsl_mc_device *container_dev) > +{ > + int i; > + int error; > + struct fsl_mc_device_region *regions; > + struct dprc_obj_desc *obj_desc = &mc_dev->obj_desc; > + struct device *parent_dev = mc_dev->dev.parent; > + > + regions = kmalloc_array(obj_desc->region_count, > + sizeof(regions[0]), GFP_KERNEL); Better use 'struct resource' for the resources than make your own type. > + mc_dev->icid = container_dev->icid; > + mc_dev->dma_mask = 0xffffffff; /* 32bit */ > + mc_dev->dev.dma_mask = &mc_dev->dma_mask; Is 32-bit DMA a fundamental limit of the bus? > + > +static const struct of_device_id fsl_mc_bus_match_table[] = { > + {.compatible = "fsl,qoriq-mc",}, > + {}, > +}; Please add a binding documentation for this device in Documentation/device-tree/ > +#define FSL_MC_MAGIC(_a, _b, _c, _d) \ > + (((uint32_t)(_a) << 24) | \ > + ((uint32_t)(_b) << 16) | \ > + ((uint32_t)(_c) << 8) | \ > + (uint32_t)(_d)) Can be dropped once you remove all the magic number matching > +/** > + * struct fsl_mc_device_region - MC object device MMIO region > + * @addr: base physical address > + * @size: size of the region in bytes > + */ > +struct fsl_mc_device_region { > + phys_addr_t paddr; > + uint32_t size; > +}; Can be removed when you move to 'struct resource' > +/** > + * struct fsl_mc_device - MC object device object > + * @magic: marker to verify identity of this structure remove > + * @flags: MC object device flags > + * @icid: Isolation context ID for the device > + * @mc_handle: MC handle for the corresponding MC object opened > + * @mc_io: Pointer to MC IO object assigned to this device or > + * NULL if none. > + * @driver: Pointer to the MC object device driver for this device Use container_of(&this->dev.driver, ...) instead > + * @container: Pointer to the DPRC device that contains this MC object device Why are there two devices for this? Should this just use dev->parent instead? > + * @dev_node: Node in the container's child list Same here: just use the device model's list management instead if you can, or explain why this is needed. > + * @obj_desc: MC description of the DPAA device > + * @num_regions: Number of MMIO regions for this MC object device Doesn't actually exist? > +#define FSL_MC_ERROR(_dev, _fmt, ...) \ > + do { \ > + if ((_dev) != NULL) \ > + dev_err(_dev, "%s:" __stringify(__LINE__) " " \ > + _fmt, __func__, ##__VA_ARGS__); \ > + else \ > + pr_err("%s:" __stringify(__LINE__) " " _fmt, \ > + __func__, ##__VA_ARGS__); \ > + } while (0) just use dev_err() directly, it handles the !_dev case already > +/** > + * struct fsl_mc_bus - Management Complex (MC) bus object > + * @magic: marker to verify identity of this structure > + * @pdev: platform device for this MC bus object > + * @root_mc_dev: pointer to root MC object device for this MC bus. > + */ > +struct fsl_mc_bus { > +# define FSL_MC_BUS_MAGIC FSL_MC_MAGIC('L', 'B', 'U', 'S') > + uint32_t magic; > + struct platform_device *pdev; > + struct fsl_mc_device *root_mc_dev; > +}; pdev should be root_mc_dev->dev->parent, and magic seems pointless, so no need for this structure at all. > +/** > + * struct fsl_mc_dprc - Data Path Resource Container (DPRC) object > + * @magic: marker to verify identity of this structure > + * @mc_dev: pointer to MC object device object for this DPRC > + * @mutex: mutex to serialize access to the container. > + * @child_device_count: have the count of devices in this DPRC > + * @child_list: anchor node of list of child devices on this DPRC > + */ > +struct fsl_mc_dprc { > +# define FSL_MC_DPRC_MAGIC FSL_MC_MAGIC('D', 'P', 'R', 'C') > + uint32_t magic; > + struct fsl_mc_device *mc_dev; > + struct mutex mutex; /* serializes access to fields below */ > + uint16_t child_device_count; /* Count of devices in this DPRC */ > + struct list_head child_list; > +}; It's not clear what this represents to me. mc_dev presumably already has a list of children, so why not just use a pointer to mc_dev and remove this structure entirely? Arnd -- 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/