On Sat, Feb 06, 2016 at 11:44:20AM -0700, Sagar Dharia wrote:
> +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;
> + 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);
> + }
> +}
I'm not 100% clear why we're scheduling this into a workqueue, it'd
probably help to at least explain what's going on in the code for future
reference.
> +}
> +/**
There's an awful lot of places in this which look like they're missing
blank lines.
> +ret_assigned_laddr:
> + mutex_unlock(&ctrl->m_ctrl);
> + if (exists || ret)
> + return ret;
> +
> + pr_info("setting slimbus l-addr:%x, ea:%x,%x,%x,%x\n",
> + *laddr, e_addr->manf_id, e_addr->prod_code,
> + e_addr->dev_index, e_addr->instance);
Not a dev_ print?
> + slim = slim_query_device(ctrl, e_addr);
> + if (!slim) {
> + ret = -ENOMEM;
-ENOMEM?
> +static void __exit slimbus_exit(void)
> +{
> + bus_unregister(&slimbus_type);
> +}
> +
> +static int __init slimbus_init(void)
> +{
> + return bus_register(&slimbus_type);
> +}
> +postcore_initcall(slimbus_init);
> +module_exit(slimbus_exit);
Put the annotatations next to their functions.
> +MODULE_DESCRIPTION("Slimbus module");
> +MODULE_ALIAS("platform:slimbus");
This isn't a platform driver, it shouldn't have this alias.
> diff --git a/include/linux/slimbus.h b/include/linux/slimbus.h
> new file mode 100644
> index 0000000..2a78f79
> --- /dev/null
> +++ b/include/linux/slimbus.h
Probably good to add a module_slimbus_device() macro like other buses
have.
Attachment:
signature.asc
Description: PGP signature
