Re: [RFC PATCH 2/4] drivers/bus: Freescale Management Complex (fsl-mc) bus driver

From: Arnd Bergmann
Date: Sat Aug 16 2014 - 08:06:18 EST


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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/