On Fri, Oct 06, 2017 at 05:51:30PM +0200, srinivas.kandaga...@linaro.org wrote: > From: Sagar Dharia <sdha...@codeaurora.org> > > SLIMbus (Serial Low Power Interchip Media Bus) is a specification > developed by MIPI (Mobile Industry Processor Interface) alliance. > SLIMbus is a 2-wire implementation, which is used to communicate with > peripheral components like audio-codec. > SLIMbus uses Time-Division-Multiplexing to accommodate multiple data > channels, and control channel. Control channel has messages to do > device-enumeration, messages to send/receive control-data to/from > slimbus devices, messages for port/channel management, and messages to > do bandwidth allocation. > The framework supports multiple instances of the bus (1 controller per > bus), and multiple slave devices per controller. > > This patch does device enumeration, logical address assignment, > informing device when the device reports present/absent etc. > Reporting present may need the driver to do the needful (e.g. turning > on voltage regulators powering the device). Additionally device is > probed when it reports present if that device doesn't need any such > steps mentioned above. > > Signed-off-by: Sagar Dharia <sdha...@codeaurora.org> > Signed-off-by: Srinivas Kandagatla <srinivas.kandaga...@linaro.org> > --- > Documentation/devicetree/bindings/slimbus/bus.txt | 57 ++ > Documentation/slimbus/summary | 109 ++++ > drivers/Kconfig | 2 + > drivers/Makefile | 1 + > drivers/slimbus/Kconfig | 11 + > drivers/slimbus/Makefile | 5 + > drivers/slimbus/slim-core.c | 695 > ++++++++++++++++++++++ > include/linux/mod_devicetable.h | 13 + > include/linux/slimbus.h | 299 ++++++++++ > 9 files changed, 1192 insertions(+)
thats a lot of code for review, consider splitting it up further for better reviews > create mode 100644 Documentation/devicetree/bindings/slimbus/bus.txt > create mode 100644 Documentation/slimbus/summary > create mode 100644 drivers/slimbus/Kconfig > create mode 100644 drivers/slimbus/Makefile > create mode 100644 drivers/slimbus/slim-core.c how about core.c (https://lkml.org/lkml/2017/7/12/430) > +static const struct slim_device_id *slim_match(const struct slim_device_id > *id, > + const struct slim_device *sbdev) > +{ > + while (id->manf_id != 0 || id->prod_code != 0) { > + if (id->manf_id == sbdev->e_addr.manf_id && > + id->prod_code == sbdev->e_addr.prod_code && > + id->dev_index == sbdev->e_addr.dev_index) > + return id; > + id++; > + } > + return NULL; > +} > + > +static int slim_device_match(struct device *dev, struct device_driver *drv) > +{ > + struct slim_device *sbdev = to_slim_device(dev); > + struct slim_driver *sbdrv = to_slim_driver(drv); > + > + /* Attempt an OF style match first */ > + if (of_driver_match_device(dev, drv)) > + return 1; is of_driver_match_device() a must have here? (I dont completely understand DT so pardon my ignorance). Since we have devices with ids can we use that alone for matching? > + > + /* Then try to match against the id table */ > + if (sbdrv->id_table) > + return slim_match(sbdrv->id_table, sbdev) != NULL; > + > + return 0; > +} > + rather than jumping now to reporting APIs, can we club all bus_type parts to one place (patch) so that it is easier to review logically > +struct sb_report_wd { > + struct work_struct wd; > + struct slim_device *sbdev; > + bool report; > +}; > + > +static void slim_report(struct work_struct *work) > +{ > + struct slim_driver *sbdrv; > + struct sb_report_wd *sbw = container_of(work, struct sb_report_wd, wd); > + struct slim_device *sbdev = sbw->sbdev; > + > + 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 > + */ I think ppl commented about this style, so lets fix those issues > + 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); what do the device_up/down calls signify here? > +static int slim_device_probe(struct device *dev) > +{ > + struct slim_device *sbdev; > + struct slim_driver *sbdrv; > + int status = 0; > + > + sbdev = to_slim_device(dev); > + sbdrv = to_slim_driver(dev->driver); > + > + sbdev->driver = sbdrv; > + > + if (sbdrv->probe) > + status = sbdrv->probe(sbdev); > + > + if (status) > + sbdev->driver = NULL; > + else if (sbdrv->device_up) > + schedule_slim_report(sbdev->ctrl, sbdev, true); can you please explain what this is trying to do? > +int __slim_driver_register(struct slim_driver *drv, struct module *owner) > +{ > + drv->driver.bus = &slimbus_type; > + drv->driver.owner = owner; > + return driver_register(&drv->driver); > +} > +EXPORT_SYMBOL_GPL(__slim_driver_register); any reason to use __ for this API? > +static 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); > + 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); I dont think the comment is quite correct, you register a device not probe! > +/* OF helpers for SLIMbus */ > +static void of_register_slim_devices(struct slim_controller *ctrl) > +{ > + struct device *dev = &ctrl->dev; > + struct device_node *node; > + > + if (!ctrl->dev.of_node) > + return; > + > + for_each_child_of_node(ctrl->dev.of_node, node) { > + struct slim_device *slim; > + const char *compat = NULL; > + char *p, *tok; > + int reg[2], ret; > + > + slim = kzalloc(sizeof(*slim), GFP_KERNEL); > + if (!slim) > + continue; > + > + slim->dev.of_node = of_node_get(node); > + > + compat = of_get_property(node, "compatible", NULL); > + if (!compat) > + continue; > + > + p = kasprintf(GFP_KERNEL, "%s", compat + strlen("slim")); > + > + tok = strsep(&p, ","); > + if (!tok) { > + dev_err(dev, "No valid Manufacturer ID found\n"); > + kfree(p); > + continue; > + } > + slim->e_addr.manf_id = str2hex(tok); > + > + tok = strsep(&p, ","); > + if (!tok) { > + dev_err(dev, "No valid Product ID found\n"); > + kfree(p); > + continue; > + } > + slim->e_addr.prod_code = str2hex(tok); > + kfree(p); > + > + ret = of_property_read_u32_array(node, "reg", reg, 2); > + if (ret) { > + dev_err(dev, "Device and Instance id not found:%d\n", > + ret); > + continue; > + } > + slim->e_addr.dev_index = reg[0]; > + slim->e_addr.instance = reg[1]; > + > + ret = slim_add_device(ctrl, slim); okay this is good stuff. So we scan the DT for slimbus devices and register them here. Same stuff we can do with ACPI :) then why do we need the of register stuff I commented earlier. A Slimbus device can work irrespective of firmware type and registers using various ids. The platform will scan firmware (dt/acpi) create devices and load drivers against them generically. Apart from this code we ideally should not have any DT parts in the bus, do you agree? > + if (ret) > + dev_err(dev, "of_slim device register err:%d\n", ret); > + } > +} > + > +/** > + * 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); what are these ids used for? > + 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); one more device_register?? Can you explain why > +/** > + * struct slim_addrt: slimbus address used internally by the slimbus > framework. > + * @valid: If the device is present. Valid is set to false when device > reports > + * absent. > + * @eaddr: Enumeration address > + * @laddr: It is possible that controller will set a predefined logical > address > + * rather than the one assigned by framework. (i.e. logical address may > + * not be same as index into this table). This entry will store the > + * logical address value for this enumeration address. > + */ > +struct slim_addrt { addrt? why not just addr? > + bool valid; > + struct slim_eaddr eaddr; > + u8 laddr; > +}; > + > +/* SLIMbus message types. Related to interpretation of message code. */ > +#define SLIM_MSG_MT_CORE 0x0 > +#define SLIM_MSG_MT_DEST_REFERRED_CLASS 0x1 > +#define SLIM_MSG_MT_DEST_REFERRED_USER 0x2 > +#define SLIM_MSG_MT_SRC_REFERRED_CLASS 0x5 > +#define SLIM_MSG_MT_SRC_REFERRED_USER 0x6 BIT() GENMASK() please here and other places where they define bits in spec > +/** > + * struct slim_driver: Slimbus 'generic device' (slave) device driver > + * (similar to 'spi_device' on SPI) > + * @probe: Binds this driver to a slimbus device. > + * @remove: Unbinds this driver from the slimbus device. > + * @shutdown: Standard shutdown callback used during powerdown/halt. > + * @suspend: Standard suspend callback used during system suspend > + * @resume: Standard resume callback used during system resume > + * @device_up: This callback is called when the device reports present and > + * gets a logical address assigned to it > + * @device_down: This callback is called when device reports absent, or the > + * bus goes down. Device will report present when bus is up and > + * device_up callback will be called again when that happens do we need two callback, why not a status or notify callback with argument for up/down? -- ~Vinod
