Hi Sagar,
I have some comments on it.
On Wed, 27 Apr 2016 17:58:04 -0600
Sagar Dharia <sdharia@xxxxxxxxxxxxxx> wrote:
[..]
> diff --git a/drivers/slimbus/slim-core.c b/drivers/slimbus/slim-core.c
> new file mode 100644
> index 0000000..6024f74
> --- /dev/null
> +++ b/drivers/slimbus/slim-core.c
> @@ -0,0 +1,684 @@
> +/* Copyright (c) 2011-2016, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
> +#include <linux/slab.h>
> +#include <linux/init.h>
> +#include <linux/completion.h>
> +#include <linux/idr.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/slimbus.h>
> +
> +static DEFINE_MUTEX(slim_lock);
> +static DEFINE_IDR(ctrl_idr);
> +
> +static bool slim_eaddr_equal(struct slim_eaddr *a, struct slim_eaddr *b)
> +{
> + return (a->manf_id == b->manf_id &&
> + a->prod_code == b->prod_code &&
> + a->dev_index == b->dev_index &&
> + a->instance == b->instance);
> +}
> +
> +static const struct slim_device_id *
> +slim_match(const struct slim_device_id *id, const struct slim_device *slim_dev)
Why does this not return bool? (and the name seems too generic,
it can be __slim_device_match() )
> +{
> + while (id->manf_id != 0 || id->prod_code != 0) {
> + if (id->manf_id == slim_dev->e_addr.manf_id &&
> + id->prod_code == slim_dev->e_addr.prod_code &&
> + id->dev_index == slim_dev->e_addr.dev_index)
> + return id;
> + id++;
> + }
> + return NULL;
> +}
> +
> +static int slim_device_match(struct device *dev, struct device_driver *driver)
> +{
> + struct slim_device *slim_dev;
> + struct slim_driver *drv = to_slim_driver(driver);
> +
> + slim_dev = to_slim_device(dev);
You can do this at definition line as slim_driver does.
> + if (drv->id_table)
> + return slim_match(drv->id_table, slim_dev) != NULL;
> + return 0;
> +}
> +
> +struct sb_report_wd {
> + struct work_struct wd;
> + struct slim_device *sb;
> + bool report;
> +};
> +
> +static void slim_report(struct work_struct *work)
> +{
> + struct slim_driver *sbdrv;
It would be better to give a consistent naming rule for local variables.
I see "sbdrv", "driver", "drv" etc. I think sbdrv and sbdev are good choice.
> + struct sb_report_wd *sbw = container_of(work, struct sb_report_wd, wd);
> + struct slim_device *sbdev = sbw->sb;
> +
> + mutex_lock(&sbdev->report_lock);
> + if (!sbdev->dev.driver)
> + goto report_exit;
> +
> + /* check if device-up or down needs to be called */
> + if ((!sbdev->reported && !sbdev->notified) ||
> + (sbdev->reported && sbdev->notified))
> + goto report_exit;
> +
> + sbdrv = to_slim_driver(sbdev->dev.driver);
> +
> + /**
> + * address no longer valid, means device reported absent, whereas
> + * address valid, means device reported present
> + */
> + if (sbdev->notified && !sbdev->reported) {
> + sbdev->notified = false;
> + if (sbdrv->device_down)
> + sbdrv->device_down(sbdev);
> + } else if (!sbdev->notified && sbdev->reported) {
> + sbdev->notified = true;
> + if (sbdrv->device_up)
> + sbdrv->device_up(sbdev);
> + }
> +report_exit:
> + mutex_unlock(&sbdev->report_lock);
> + kfree(sbw);
> +}
> +
> +/**
> + * Report callbacks(device_up, device_down) are implemented by slimbus-devices.
> + * The calls are scheduled into a workqueue to avoid holding up controller
> + * intialization/tear-down.
> + */
> +static void schedule_slim_report(struct slim_controller *ctrl,
> + struct slim_device *sb, bool report)
> +{
> + struct sb_report_wd *sbw;
> +
> + dev_dbg(&ctrl->dev, "report:%d for slave:%s\n", report, sb->name);
> +
> + sbw = kmalloc(sizeof(*sbw), GFP_KERNEL);
> + if (!sbw)
> + return;
> +
> + INIT_WORK(&sbw->wd, slim_report);
> + sbw->sb = sb;
Wouldn't we check whether the device is under another reporting or not?
Maybe we can cancel ongoing workqueue.
> + sbw->report = report;
> + if (!queue_work(ctrl->wq, &sbw->wd)) {
> + dev_err(&ctrl->dev, "failed to queue report:%d slave:%s\n",
> + report, sb->name);
> + kfree(sbw);
> + }
> +}
> +
> +static int slim_device_probe(struct device *dev)
> +{
> + struct slim_device *slim_dev;
> + struct slim_driver *driver;
"driver" looks generic name. sbdev and sbdrv are much better.
> + struct slim_controller *ctrl;
> + int status = 0;
> +
> + slim_dev = to_slim_device(dev);
> + driver = to_slim_driver(dev->driver);
> + if (!driver->id_table)
> + return -ENODEV;
> +
> + slim_dev->driver = driver;
> +
> + if (driver->probe)
> + status = driver->probe(slim_dev);
> + if (status) {
> + slim_dev->driver = NULL;
> + } else if (driver->device_up) {
> + ctrl = slim_dev->ctrl;
> + schedule_slim_report(ctrl, slim_dev, true);
Here, "crtl" seems a bit redundant.
> + }
> + return status;
> +}
> +
[..]
> +/**
> + * slim_add_device: Add a new device without register board info.
> + * @ctrl: Controller to which this device is to be added to.
> + * Called when device doesn't have an explicit client-driver to be probed, or
> + * the client-driver is a module installed dynamically.
> + */
> +int slim_add_device(struct slim_controller *ctrl, struct slim_device *sbdev)
> +{
> + sbdev->dev.bus = &slimbus_type;
> + sbdev->dev.parent = &ctrl->dev;
> + sbdev->dev.release = slim_dev_release;
> + sbdev->dev.driver = NULL;
> + sbdev->ctrl = ctrl;
> +
> + slim_ctrl_get(ctrl);
> + if (!sbdev->name) {
> + sbdev->name = kasprintf(GFP_KERNEL, "%x:%x:%x:%x",
> + sbdev->e_addr.manf_id,
> + sbdev->e_addr.prod_code,
> + sbdev->e_addr.dev_index,
> + sbdev->e_addr.instance);
> + if (!sbdev->name)
> + return -ENOMEM;
> + }
> + dev_set_name(&sbdev->dev, "%s", sbdev->name);
> + mutex_init(&sbdev->report_lock);
> +
> + /* probe slave on this controller */
> + return device_register(&sbdev->dev);
> +}
> +EXPORT_SYMBOL_GPL(slim_add_device);
> +
> +struct sbi_boardinfo {
> + struct list_head list;
> + struct slim_boardinfo board_info;
> +};
> +
> +static LIST_HEAD(board_list);
> +static LIST_HEAD(slim_ctrl_list);
> +static DEFINE_MUTEX(board_lock);
> +
> +/* If controller is not present, only add to boards list */
This comment would better be placed in slim_register_board_info()
since this function doesn't add boards to the list.
> +static void slim_match_ctrl_to_boardinfo(struct slim_controller *ctrl,
> + struct slim_boardinfo *bi)
> +{
> + int ret;
> +
> + if (ctrl->nr != bi->bus_num)
> + return;
> +
> + ret = slim_add_device(ctrl, bi->slim_slave);
> + if (ret != 0)
> + dev_err(ctrl->dev.parent, "can't create new device %s, ret:%d\n",
> + bi->slim_slave->name, ret);
> +}
> +
> +/**
> + * slim_register_board_info: Board-initialization routine.
> + * @info: List of all devices on all controllers present on the board.
> + * @n: number of entries.
> + * API enumerates respective devices on corresponding controller.
> + * Called from board-init function.
> + */
> +int slim_register_board_info(struct slim_boardinfo const *info, unsigned n)
> +{
> + struct sbi_boardinfo *bi;
> + int i;
> +
> + bi = kcalloc(n, sizeof(*bi), GFP_KERNEL);
> + if (!bi)
> + return -ENOMEM;
> +
> + for (i = 0; i < n; i++, bi++, info++) {
> + struct slim_controller *ctrl;
> +
> + memcpy(&bi->board_info, info, sizeof(*info));
> + mutex_lock(&board_lock);
> + list_add_tail(&bi->list, &board_list);
> + list_for_each_entry(ctrl, &slim_ctrl_list, list)
> + slim_match_ctrl_to_boardinfo(ctrl, &bi->board_info);
> + mutex_unlock(&board_lock);
> + }
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(slim_register_board_info);
> +
> +static void slim_ctrl_add_boarddevs(struct slim_controller *ctrl)
> +{
> + struct sbi_boardinfo *bi;
> +
> + mutex_lock(&board_lock);
> + list_add_tail(&ctrl->list, &slim_ctrl_list);
> + list_for_each_entry(bi, &board_list, list)
> + slim_match_ctrl_to_boardinfo(ctrl, &bi->board_info);
> + mutex_unlock(&board_lock);
> +}
> +
> +/**
> + * slim_register_controller: Controller bring-up and registration.
> + * @ctrl: Controller to be registered.
> + * A controller is registered with the framework using this API.
> + * If devices on a controller were registered before controller,
> + * this will make sure that they get probed when controller is up
> + */
> +int slim_register_controller(struct slim_controller *ctrl)
> +{
> + int id, ret = 0;
> +
> + mutex_lock(&slim_lock);
> + id = idr_alloc(&ctrl_idr, ctrl, ctrl->nr, -1, GFP_KERNEL);
> + mutex_unlock(&slim_lock);
> +
> + if (id < 0)
> + return id;
> +
> + ctrl->nr = id;
> +
> + dev_set_name(&ctrl->dev, "sb-%d", ctrl->nr);
> + ctrl->num_dev = 0;
> +
> + if (!ctrl->min_cg)
> + ctrl->min_cg = SLIM_MIN_CLK_GEAR;
> + if (!ctrl->max_cg)
> + ctrl->max_cg = SLIM_MAX_CLK_GEAR;
> +
> + mutex_init(&ctrl->m_ctrl);
> + ret = device_register(&ctrl->dev);
> + if (ret)
> + goto dev_reg_failed;
> +
> + dev_dbg(&ctrl->dev, "Bus [%s] registered:dev:%p\n",
> + ctrl->name, &ctrl->dev);
> +
> + ctrl->wq = create_singlethread_workqueue(dev_name(&ctrl->dev));
> + if (!ctrl->wq)
> + goto err_workq_failed;
> +
> + slim_ctrl_add_boarddevs(ctrl);
> + return 0;
> +
> +err_workq_failed:
> + device_unregister(&ctrl->dev);
> +dev_reg_failed:
> + mutex_lock(&slim_lock);
> + idr_remove(&ctrl_idr, ctrl->nr);
> + mutex_unlock(&slim_lock);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(slim_register_controller);
> +
> +/* slim_remove_device: Remove the effect of slim_add_device() */
> +void slim_remove_device(struct slim_device *sbdev)
> +{
> + device_unregister(&sbdev->dev);
> +}
> +EXPORT_SYMBOL_GPL(slim_remove_device);
> +
> +static int slim_ctrl_remove_device(struct device *dev, void *null)
> +{
> + slim_remove_device(to_slim_device(dev));
> + return 0;
> +}
> +
> +/**
> + * slim_del_controller: Controller tear-down.
> + * @ctrl: Controller to tear-down.
> + */
> +int slim_del_controller(struct slim_controller *ctrl)
> +{
> + struct slim_controller *found;
> +
> + /* First make sure that this bus was added */
> + mutex_lock(&slim_lock);
> + found = idr_find(&ctrl_idr, ctrl->nr);
> + mutex_unlock(&slim_lock);
> + if (found != ctrl)
> + return -EINVAL;
> +
> + /* Remove all clients */
> + device_for_each_child(&ctrl->dev, NULL, slim_ctrl_remove_device);
> +
> +
> + list_del(&ctrl->list);
This list operation should be protected by slim_lock as
slim_ctrl_add_boarddevs() does.
> + destroy_workqueue(ctrl->wq);
> +
> + /* free bus id */
> + mutex_lock(&slim_lock);
> + idr_remove(&ctrl_idr, ctrl->nr);
> + mutex_unlock(&slim_lock);
> +
> + device_unregister(&ctrl->dev);
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(slim_del_controller);
Thank you,
--
Masami Hiramatsu <masami.hiramatsu@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
