Re: [Patch v6 1/7] slimbus: Device management on SLIMbus

[Bjorn Andersson]

On Wed 18 Oct 09:38 PDT 2017, Srinivas Kandagatla wrote:
> On 17/10/17 07:23, Bjorn Andersson wrote:
> > On Fri 06 Oct 08:51 PDT 2017, srinivas.kandaga...@linaro.org wrote:
[..]
> > > +static int slim_device_remove(struct device *dev)
> > > +{
> > > + struct slim_device *sbdev;
> > > + struct slim_driver *sbdrv;
> > > + int status = 0;
> > > +
> > > + sbdev = to_slim_device(dev);
> > > + if (!dev->driver)
> > > + return 0;
> > > +
> > > + sbdrv = to_slim_driver(dev->driver);
> > > + if (sbdrv->remove)
> > > + status = sbdrv->remove(sbdev);
> > > +
> > > + mutex_lock(>report_lock);
> > > + sbdev->notified = false;
> > > + if (status == 0)
> > > + sbdev->driver = NULL;
> > > + mutex_unlock(>report_lock);
> > > + return status;
> > 
> > device_unregister() will call device_del() which will end up in
> > __device_release_driver() which will call this function. Upon returning
> > from this function the core expect the bus to have cleaned up after the
> > dev (normally by calling drv->remove(dev)).
> > 
> > It will completely ignore the return value and continue tearing down the
> > rest of the core resources, e.g. three lines down it will
> > devres_release_all().
> > 
> > 
> > So you have the option of sleeping, while waiting for stuff to be
> > aborted/finished and then you need to clean things up.
> > 
> > The slim_device object itself will stick around until all references are
> > dropped though.
> 
> So you are suggesting that we make slim_driver remove not return anything?
> 

Yes.

Having the opportunity of failing remove() causes driver writers to
expect that they can fail, with the result of things not being torn
down properly.

Note that right after remove() returns devm_* resources will be
released, so anything that is not torn down and for some reason later
access allocated resources would cause issues.

Regards,
Bjorn

Re: [Patch v6 1/7] slimbus: Device management on SLIMbus

[Bjorn Andersson]

On Wed 18 Oct 09:38 PDT 2017, Srinivas Kandagatla wrote:
> On 17/10/17 07:23, Bjorn Andersson wrote:
> > On Fri 06 Oct 08:51 PDT 2017, srinivas.kandaga...@linaro.org wrote:
[..]
> > > +static int slim_device_remove(struct device *dev)
> > > +{
> > > + struct slim_device *sbdev;
> > > + struct slim_driver *sbdrv;
> > > + int status = 0;
> > > +
> > > + sbdev = to_slim_device(dev);
> > > + if (!dev->driver)
> > > + return 0;
> > > +
> > > + sbdrv = to_slim_driver(dev->driver);
> > > + if (sbdrv->remove)
> > > + status = sbdrv->remove(sbdev);
> > > +
> > > + mutex_lock(>report_lock);
> > > + sbdev->notified = false;
> > > + if (status == 0)
> > > + sbdev->driver = NULL;
> > > + mutex_unlock(>report_lock);
> > > + return status;
> > 
> > device_unregister() will call device_del() which will end up in
> > __device_release_driver() which will call this function. Upon returning
> > from this function the core expect the bus to have cleaned up after the
> > dev (normally by calling drv->remove(dev)).
> > 
> > It will completely ignore the return value and continue tearing down the
> > rest of the core resources, e.g. three lines down it will
> > devres_release_all().
> > 
> > 
> > So you have the option of sleeping, while waiting for stuff to be
> > aborted/finished and then you need to clean things up.
> > 
> > The slim_device object itself will stick around until all references are
> > dropped though.
> 
> So you are suggesting that we make slim_driver remove not return anything?
> 

Yes.

Having the opportunity of failing remove() causes driver writers to
expect that they can fail, with the result of things not being torn
down properly.

Note that right after remove() returns devm_* resources will be
released, so anything that is not torn down and for some reason later
access allocated resources would cause issues.

Regards,
Bjorn

Re: [Patch v6 1/7] slimbus: Device management on SLIMbus

[Rishi Bhattacharya]

Re: [Patch v6 1/7] slimbus: Device management on SLIMbus

[Rishi Bhattacharya]

Re: [Patch v6 1/7] slimbus: Device management on SLIMbus

[Stephen Boyd]

On 10/06, srinivas.kandaga...@linaro.org wrote:
> +
> +SLIMbus example for Qualcomm's slimbus manager component:
> +
> + slim@2808 {
> + compatible = "qcom,slim-msm";
> + reg = <0x2808 0x2000>,
> + interrupts = <0 33 0>;
> + clocks = < SLIMBUS_SRC>, < AUDIO_SLIMBUS_CLK>;
> + clock-names = "iface_clk", "core_clk";

Please remove "_clk" from here.

> + #address-cells = <2>;
> + #size-cells = <0>;
> +
> + codec: wcd9310@1{
> + compatible = "slim217,60"";
> + reg = <1 0>;
> + };
> + };

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [Patch v6 1/7] slimbus: Device management on SLIMbus

[Stephen Boyd]

On 10/06, srinivas.kandaga...@linaro.org wrote:
> +
> +SLIMbus example for Qualcomm's slimbus manager component:
> +
> + slim@2808 {
> + compatible = "qcom,slim-msm";
> + reg = <0x2808 0x2000>,
> + interrupts = <0 33 0>;
> + clocks = < SLIMBUS_SRC>, < AUDIO_SLIMBUS_CLK>;
> + clock-names = "iface_clk", "core_clk";

Please remove "_clk" from here.

> + #address-cells = <2>;
> + #size-cells = <0>;
> +
> + codec: wcd9310@1{
> + compatible = "slim217,60"";
> + reg = <1 0>;
> + };
> + };

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [Patch v6 1/7] slimbus: Device management on SLIMbus

[Mark Brown]

On Fri, Oct 06, 2017 at 05:51:30PM +0200, srinivas.kandaga...@linaro.org wrote:

> ---
>  Documentation/devicetree/bindings/slimbus/bus.txt |  57 ++
>  Documentation/slimbus/summary | 109 

This is a 40k patch which is a bit offputting for review.  Splitting the
docs out would help this a bit.

> +static int slim_boot_child(struct device *dev, void *unused)
> +{
> + struct slim_driver *sbdrv;
> + struct slim_device *sbdev = to_slim_device(dev);
> +
> + if (sbdev && sbdev->dev.driver) {
> + sbdrv = to_slim_driver(sbdev->dev.driver);
> + if (sbdrv->boot_device)
> + sbdrv->boot_device(sbdev);
> + }
> + return 0;
> +}

We silently don't boot a device if it hasn't got a driver - is that the
right thing?  It feels like the silencing should be in the calling
function.

> +ret_assigned_laddr:
> + mutex_unlock(>m_ctrl);
> + if (exists || ret)
> + return ret;
> +
> + dev_info(>dev, "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);

dev_dbg()?


signature.asc
Description: PGP signature

Re: [Patch v6 1/7] slimbus: Device management on SLIMbus

[Mark Brown]

On Fri, Oct 06, 2017 at 05:51:30PM +0200, srinivas.kandaga...@linaro.org wrote:

> ---
>  Documentation/devicetree/bindings/slimbus/bus.txt |  57 ++
>  Documentation/slimbus/summary | 109 

This is a 40k patch which is a bit offputting for review.  Splitting the
docs out would help this a bit.

> +static int slim_boot_child(struct device *dev, void *unused)
> +{
> + struct slim_driver *sbdrv;
> + struct slim_device *sbdev = to_slim_device(dev);
> +
> + if (sbdev && sbdev->dev.driver) {
> + sbdrv = to_slim_driver(sbdev->dev.driver);
> + if (sbdrv->boot_device)
> + sbdrv->boot_device(sbdev);
> + }
> + return 0;
> +}

We silently don't boot a device if it hasn't got a driver - is that the
right thing?  It feels like the silencing should be in the calling
function.

> +ret_assigned_laddr:
> + mutex_unlock(>m_ctrl);
> + if (exists || ret)
> + return ret;
> +
> + dev_info(>dev, "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);

dev_dbg()?


signature.asc
Description: PGP signature

Re: [Patch v6 1/7] slimbus: Device management on SLIMbus

[Srinivas Kandagatla]

Thanks for the Review Bjorn,

On 17/10/17 07:23, Bjorn Andersson wrote:

On Fri 06 Oct 08:51 PDT 2017, srinivas.kandaga...@linaro.org wrote:
[..]

diff --git a/drivers/slimbus/slim-core.c b/drivers/slimbus/slim-core.c

[..]

+/**
+ * Report callbacks(device_up, device_down) are implemented by slimbus-devices.
+ * The calls are scheduled into a workqueue to avoid holding up controller
+ * initialization/tear-down.
+ */
+static void schedule_slim_report(struct slim_controller *ctrl,
+struct slim_device *sb, bool report)
+{
+   struct sb_report_wd *sbw;
+
+   dev_dbg(>dev, "report:%d for slave:%s\n", report, sb->name);


This is the only place where sb->name is used in this driver. If you
instead invoke dev_*() on >dev you should get prettier output and
can drop the double storage of the device name.


Makes sense, we could get rid of sb->name storage too.




+
+   sbw = kmalloc(sizeof(*sbw), GFP_KERNEL);
+   if (!sbw)
+   return;
+
+   INIT_WORK(>wd, slim_report);
+   sbw->sbdev = sb;
+   sbw->report = report;
+   if (!queue_work(ctrl->wq, >wd)) {


When a controller is torn down destroy_workqueue() is called after all
child devices has been unregistered, so this work might be scheduled
after "sb" is gone, if I get this properly.


I agree, That is possible!
We should probably flush the workqueue before we start removing the clients.




+   dev_err(>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  *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);


So a driver can have a probe() and device_up() or just any one of them?

And probe() is called when the controller enumerates all devices
mentioned in DT and then device_up() is called at that point in time and
when it's advertised on the bus?

Is there a reason for this split model?

yes, Some of the devices need to be powered up before they become 
usable, so probe is used to do the initial power up of the device.




+
+   if (status)
+   sbdev->driver = NULL;
+   else if (sbdrv->device_up)
+   schedule_slim_report(sbdev->ctrl, sbdev, true);
+
+   return status;
+}
+
+static int slim_device_remove(struct device *dev)
+{
+   struct slim_device *sbdev;
+   struct slim_driver *sbdrv;
+   int status = 0;
+
+   sbdev = to_slim_device(dev);
+   if (!dev->driver)
+   return 0;
+
+   sbdrv = to_slim_driver(dev->driver);
+   if (sbdrv->remove)
+   status = sbdrv->remove(sbdev);
+
+   mutex_lock(>report_lock);
+   sbdev->notified = false;
+   if (status == 0)
+   sbdev->driver = NULL;
+   mutex_unlock(>report_lock);
+   return status;


device_unregister() will call device_del() which will end up in
__device_release_driver() which will call this function. Upon returning
from this function the core expect the bus to have cleaned up after the
dev (normally by calling drv->remove(dev)).

It will completely ignore the return value and continue tearing down the
rest of the core resources, e.g. three lines down it will
devres_release_all().


So you have the option of sleeping, while waiting for stuff to be
aborted/finished and then you need to clean things up.

The slim_device object itself will stick around until all references are
dropped though.


So you are suggesting that we make slim_driver remove not return anything?




+}
+
+struct bus_type slimbus_type = {
+   .name   = "slimbus",
+   .match  = slim_device_match,
+   .probe  = slim_device_probe,
+   .remove = slim_device_remove,
+};
+EXPORT_SYMBOL_GPL(slimbus_type);
+
+/**
+ * slim_driver_register: Client driver registration with slimbus
+ * @drv:Client driver to be associated with client-device.
+ * @owner: owning module/driver
+ * This API will register the client driver with the slimbus
+ * It is called from the driver's module-init function.
+ */
+int __slim_driver_register(struct slim_driver *drv, struct module *owner)
+{
+   drv->driver.bus = _type;
+   drv->driver.owner = owner;
+   return driver_register(>driver);
+}
+EXPORT_SYMBOL_GPL(__slim_driver_register);
+
+/**
+ * slim_driver_unregister: Undo effect of slim_driver_register
+ * @drv: Client driver to be unregistered
+ */
+void slim_driver_unregister(struct slim_driver *drv)
+{
+   if (drv)


A driver invoking slim_driver_unregister(NULL) is broken, drop this
check and let it oops on the dereference instead.


Yep.




+   driver_unregister(>driver);
+}

Re: [Patch v6 1/7] slimbus: Device management on SLIMbus

[Srinivas Kandagatla]

Thanks for the Review Bjorn,

On 17/10/17 07:23, Bjorn Andersson wrote:

On Fri 06 Oct 08:51 PDT 2017, srinivas.kandaga...@linaro.org wrote:
[..]

diff --git a/drivers/slimbus/slim-core.c b/drivers/slimbus/slim-core.c

[..]

+/**
+ * Report callbacks(device_up, device_down) are implemented by slimbus-devices.
+ * The calls are scheduled into a workqueue to avoid holding up controller
+ * initialization/tear-down.
+ */
+static void schedule_slim_report(struct slim_controller *ctrl,
+struct slim_device *sb, bool report)
+{
+   struct sb_report_wd *sbw;
+
+   dev_dbg(>dev, "report:%d for slave:%s\n", report, sb->name);


This is the only place where sb->name is used in this driver. If you
instead invoke dev_*() on >dev you should get prettier output and
can drop the double storage of the device name.


Makes sense, we could get rid of sb->name storage too.




+
+   sbw = kmalloc(sizeof(*sbw), GFP_KERNEL);
+   if (!sbw)
+   return;
+
+   INIT_WORK(>wd, slim_report);
+   sbw->sbdev = sb;
+   sbw->report = report;
+   if (!queue_work(ctrl->wq, >wd)) {


When a controller is torn down destroy_workqueue() is called after all
child devices has been unregistered, so this work might be scheduled
after "sb" is gone, if I get this properly.


I agree, That is possible!
We should probably flush the workqueue before we start removing the clients.




+   dev_err(>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  *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);


So a driver can have a probe() and device_up() or just any one of them?

And probe() is called when the controller enumerates all devices
mentioned in DT and then device_up() is called at that point in time and
when it's advertised on the bus?

Is there a reason for this split model?

yes, Some of the devices need to be powered up before they become 
usable, so probe is used to do the initial power up of the device.




+
+   if (status)
+   sbdev->driver = NULL;
+   else if (sbdrv->device_up)
+   schedule_slim_report(sbdev->ctrl, sbdev, true);
+
+   return status;
+}
+
+static int slim_device_remove(struct device *dev)
+{
+   struct slim_device *sbdev;
+   struct slim_driver *sbdrv;
+   int status = 0;
+
+   sbdev = to_slim_device(dev);
+   if (!dev->driver)
+   return 0;
+
+   sbdrv = to_slim_driver(dev->driver);
+   if (sbdrv->remove)
+   status = sbdrv->remove(sbdev);
+
+   mutex_lock(>report_lock);
+   sbdev->notified = false;
+   if (status == 0)
+   sbdev->driver = NULL;
+   mutex_unlock(>report_lock);
+   return status;


device_unregister() will call device_del() which will end up in
__device_release_driver() which will call this function. Upon returning
from this function the core expect the bus to have cleaned up after the
dev (normally by calling drv->remove(dev)).

It will completely ignore the return value and continue tearing down the
rest of the core resources, e.g. three lines down it will
devres_release_all().


So you have the option of sleeping, while waiting for stuff to be
aborted/finished and then you need to clean things up.

The slim_device object itself will stick around until all references are
dropped though.


So you are suggesting that we make slim_driver remove not return anything?




+}
+
+struct bus_type slimbus_type = {
+   .name   = "slimbus",
+   .match  = slim_device_match,
+   .probe  = slim_device_probe,
+   .remove = slim_device_remove,
+};
+EXPORT_SYMBOL_GPL(slimbus_type);
+
+/**
+ * slim_driver_register: Client driver registration with slimbus
+ * @drv:Client driver to be associated with client-device.
+ * @owner: owning module/driver
+ * This API will register the client driver with the slimbus
+ * It is called from the driver's module-init function.
+ */
+int __slim_driver_register(struct slim_driver *drv, struct module *owner)
+{
+   drv->driver.bus = _type;
+   drv->driver.owner = owner;
+   return driver_register(>driver);
+}
+EXPORT_SYMBOL_GPL(__slim_driver_register);
+
+/**
+ * slim_driver_unregister: Undo effect of slim_driver_register
+ * @drv: Client driver to be unregistered
+ */
+void slim_driver_unregister(struct slim_driver *drv)
+{
+   if (drv)


A driver invoking slim_driver_unregister(NULL) is broken, drop this
check and let it oops on the dereference instead.


Yep.




+   driver_unregister(>driver);
+}

Re: [Patch v6 1/7] slimbus: Device management on SLIMbus

[Bjorn Andersson]

On Fri 06 Oct 08:51 PDT 2017, srinivas.kandaga...@linaro.org wrote:
[..]
> diff --git a/drivers/slimbus/slim-core.c b/drivers/slimbus/slim-core.c
[..]
> +/**
> + * Report callbacks(device_up, device_down) are implemented by 
> slimbus-devices.
> + * The calls are scheduled into a workqueue to avoid holding up controller
> + * initialization/tear-down.
> + */
> +static void schedule_slim_report(struct slim_controller *ctrl,
> +  struct slim_device *sb, bool report)
> +{
> + struct sb_report_wd *sbw;
> +
> + dev_dbg(>dev, "report:%d for slave:%s\n", report, sb->name);

This is the only place where sb->name is used in this driver. If you
instead invoke dev_*() on >dev you should get prettier output and
can drop the double storage of the device name.

> +
> + sbw = kmalloc(sizeof(*sbw), GFP_KERNEL);
> + if (!sbw)
> + return;
> +
> + INIT_WORK(>wd, slim_report);
> + sbw->sbdev = sb;
> + sbw->report = report;
> + if (!queue_work(ctrl->wq, >wd)) {

When a controller is torn down destroy_workqueue() is called after all
child devices has been unregistered, so this work might be scheduled
after "sb" is gone, if I get this properly.

> + dev_err(>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  *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);

So a driver can have a probe() and device_up() or just any one of them?

And probe() is called when the controller enumerates all devices
mentioned in DT and then device_up() is called at that point in time and
when it's advertised on the bus?

Is there a reason for this split model?

> +
> + if (status)
> + sbdev->driver = NULL;
> + else if (sbdrv->device_up)
> + schedule_slim_report(sbdev->ctrl, sbdev, true);
> +
> + return status;
> +}
> +
> +static int slim_device_remove(struct device *dev)
> +{
> + struct slim_device *sbdev;
> + struct slim_driver *sbdrv;
> + int status = 0;
> +
> + sbdev = to_slim_device(dev);
> + if (!dev->driver)
> + return 0;
> +
> + sbdrv = to_slim_driver(dev->driver);
> + if (sbdrv->remove)
> + status = sbdrv->remove(sbdev);
> +
> + mutex_lock(>report_lock);
> + sbdev->notified = false;
> + if (status == 0)
> + sbdev->driver = NULL;
> + mutex_unlock(>report_lock);
> + return status;

device_unregister() will call device_del() which will end up in
__device_release_driver() which will call this function. Upon returning
from this function the core expect the bus to have cleaned up after the
dev (normally by calling drv->remove(dev)).

It will completely ignore the return value and continue tearing down the
rest of the core resources, e.g. three lines down it will
devres_release_all().


So you have the option of sleeping, while waiting for stuff to be
aborted/finished and then you need to clean things up.

The slim_device object itself will stick around until all references are
dropped though.

> +}
> +
> +struct bus_type slimbus_type = {
> + .name   = "slimbus",
> + .match  = slim_device_match,
> + .probe  = slim_device_probe,
> + .remove = slim_device_remove,
> +};
> +EXPORT_SYMBOL_GPL(slimbus_type);
> +
> +/**
> + * slim_driver_register: Client driver registration with slimbus
> + * @drv:Client driver to be associated with client-device.
> + * @owner: owning module/driver
> + * This API will register the client driver with the slimbus
> + * It is called from the driver's module-init function.
> + */
> +int __slim_driver_register(struct slim_driver *drv, struct module *owner)
> +{
> + drv->driver.bus = _type;
> + drv->driver.owner = owner;
> + return driver_register(>driver);
> +}
> +EXPORT_SYMBOL_GPL(__slim_driver_register);
> +
> +/**
> + * slim_driver_unregister: Undo effect of slim_driver_register
> + * @drv: Client driver to be unregistered
> + */
> +void slim_driver_unregister(struct slim_driver *drv)
> +{
> + if (drv)

A driver invoking slim_driver_unregister(NULL) is broken, drop this
check and let it oops on the dereference instead.

> + driver_unregister(>driver);
> +}
> +EXPORT_SYMBOL_GPL(slim_driver_unregister);
> +
> +static struct slim_controller *slim_ctrl_get(struct slim_controller *ctrl)
> +{
> + if (!ctrl || !get_device(>dev))

ctrl can't be NULL here. In all code paths leading here it's
dereferenced multiple times already.

> + return NULL;
> +
> + return ctrl;
> +}
> +
> +static void slim_ctrl_put(struct slim_controller *ctrl)
> +{
> +   

Re: [Patch v6 1/7] slimbus: Device management on SLIMbus

[Bjorn Andersson]

On Fri 06 Oct 08:51 PDT 2017, srinivas.kandaga...@linaro.org wrote:
[..]
> diff --git a/drivers/slimbus/slim-core.c b/drivers/slimbus/slim-core.c
[..]
> +/**
> + * Report callbacks(device_up, device_down) are implemented by 
> slimbus-devices.
> + * The calls are scheduled into a workqueue to avoid holding up controller
> + * initialization/tear-down.
> + */
> +static void schedule_slim_report(struct slim_controller *ctrl,
> +  struct slim_device *sb, bool report)
> +{
> + struct sb_report_wd *sbw;
> +
> + dev_dbg(>dev, "report:%d for slave:%s\n", report, sb->name);

This is the only place where sb->name is used in this driver. If you
instead invoke dev_*() on >dev you should get prettier output and
can drop the double storage of the device name.

> +
> + sbw = kmalloc(sizeof(*sbw), GFP_KERNEL);
> + if (!sbw)
> + return;
> +
> + INIT_WORK(>wd, slim_report);
> + sbw->sbdev = sb;
> + sbw->report = report;
> + if (!queue_work(ctrl->wq, >wd)) {

When a controller is torn down destroy_workqueue() is called after all
child devices has been unregistered, so this work might be scheduled
after "sb" is gone, if I get this properly.

> + dev_err(>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  *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);

So a driver can have a probe() and device_up() or just any one of them?

And probe() is called when the controller enumerates all devices
mentioned in DT and then device_up() is called at that point in time and
when it's advertised on the bus?

Is there a reason for this split model?

> +
> + if (status)
> + sbdev->driver = NULL;
> + else if (sbdrv->device_up)
> + schedule_slim_report(sbdev->ctrl, sbdev, true);
> +
> + return status;
> +}
> +
> +static int slim_device_remove(struct device *dev)
> +{
> + struct slim_device *sbdev;
> + struct slim_driver *sbdrv;
> + int status = 0;
> +
> + sbdev = to_slim_device(dev);
> + if (!dev->driver)
> + return 0;
> +
> + sbdrv = to_slim_driver(dev->driver);
> + if (sbdrv->remove)
> + status = sbdrv->remove(sbdev);
> +
> + mutex_lock(>report_lock);
> + sbdev->notified = false;
> + if (status == 0)
> + sbdev->driver = NULL;
> + mutex_unlock(>report_lock);
> + return status;

device_unregister() will call device_del() which will end up in
__device_release_driver() which will call this function. Upon returning
from this function the core expect the bus to have cleaned up after the
dev (normally by calling drv->remove(dev)).

It will completely ignore the return value and continue tearing down the
rest of the core resources, e.g. three lines down it will
devres_release_all().


So you have the option of sleeping, while waiting for stuff to be
aborted/finished and then you need to clean things up.

The slim_device object itself will stick around until all references are
dropped though.

> +}
> +
> +struct bus_type slimbus_type = {
> + .name   = "slimbus",
> + .match  = slim_device_match,
> + .probe  = slim_device_probe,
> + .remove = slim_device_remove,
> +};
> +EXPORT_SYMBOL_GPL(slimbus_type);
> +
> +/**
> + * slim_driver_register: Client driver registration with slimbus
> + * @drv:Client driver to be associated with client-device.
> + * @owner: owning module/driver
> + * This API will register the client driver with the slimbus
> + * It is called from the driver's module-init function.
> + */
> +int __slim_driver_register(struct slim_driver *drv, struct module *owner)
> +{
> + drv->driver.bus = _type;
> + drv->driver.owner = owner;
> + return driver_register(>driver);
> +}
> +EXPORT_SYMBOL_GPL(__slim_driver_register);
> +
> +/**
> + * slim_driver_unregister: Undo effect of slim_driver_register
> + * @drv: Client driver to be unregistered
> + */
> +void slim_driver_unregister(struct slim_driver *drv)
> +{
> + if (drv)

A driver invoking slim_driver_unregister(NULL) is broken, drop this
check and let it oops on the dereference instead.

> + driver_unregister(>driver);
> +}
> +EXPORT_SYMBOL_GPL(slim_driver_unregister);
> +
> +static struct slim_controller *slim_ctrl_get(struct slim_controller *ctrl)
> +{
> + if (!ctrl || !get_device(>dev))

ctrl can't be NULL here. In all code paths leading here it's
dereferenced multiple times already.

> + return NULL;
> +
> + return ctrl;
> +}
> +
> +static void slim_ctrl_put(struct slim_controller *ctrl)
> +{
> +   

Re: [Patch v6 1/7] slimbus: Device management on SLIMbus

[Srinivas Kandagatla]

On 13/10/17 20:26, Rob Herring wrote:

On Fri, Oct 06, 2017 at 05:51:30PM +0200, srinivas.kandaga...@linaro.org wrote:

From: Sagar Dharia 

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 
Signed-off-by: Srinivas Kandagatla 
---
  Documentation/devicetree/bindings/slimbus/bus.txt |  57 ++


Split to separate patch.


Will do this in next patch.
It was suggested by Arnd in v5, may be just of_parse code can be merged 
in this patch and the bindings made as separate patch.






  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(+)
  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
  create mode 100644 include/linux/slimbus.h

diff --git a/Documentation/devicetree/bindings/slimbus/bus.txt 
b/Documentation/devicetree/bindings/slimbus/bus.txt
new file mode 100644
index 000..cb658bb
--- /dev/null
+++ b/Documentation/devicetree/bindings/slimbus/bus.txt
@@ -0,0 +1,57 @@
+SLIM(Serial Low Power Interchip Media Bus) bus
+
+SLIMbus is a 2-wire bus, and is used to communicate with peripheral
+components like audio-codec.
+
+Controller is a normal device using binding for whatever bus it is
+on (e.g. platform bus).


I can't have a PCI based slimbus controller? "platform bus" is a
Linuxism.

I agree, Will remove this Linuxism from the bindings.




+Required property for SLIMbus controller node:
+- compatible   - name of SLIMbus controller following generic names
+   recommended practice.


generic names aren't recommended. Allowed with some conditons, yes.


+- #address-cells - should be 2


You used 4 for your controller.


It should be actually 2. I will fix such mismatches in the other patches.




+- #size-cells  - should be 0
+
+No other properties are required in the SLIMbus controller bus node.


That's not a useful statement. Almost every controller probably has
other required properties.

Yep, will get rid of this in next patch.



+
+Child nodes:
+Every SLIMbus controller node can contain zero or more child nodes
+representing slave devices on the bus. Every SLIMbus slave device is
+uniquely determined by the enumeration address containing 4 fields:
+Manufacturer ID, Product code, Device index, and Instance value for
+the device.
+If child node is not present and it is instantiated after device
+discovery (slave device reporting itself present).
+
+In some cases it may be necessary to describe non-probeable device
+details such as non-standard ways of powering up a device. In
+such cases, child nodes for those devices will be present as
+slaves of the slimbus-controller, as detailed below.
+
+Required property for SLIMbus child node if it is present:
+- reg  - Is Duplex (Device index, Instance ID) from Enumeration
+ Address.
+ Device Index Uniquely identifies multiple Devices within
+ a single Component.
+ Instance ID Is for the cases where multiple Devices of the
+ same type or Class are attached to the bus.
+
+- compatible   -"slimMID,PID". The textual representation of Manufacturer ID,
+ Product Code, shall be in lower case hexadecimal with leading
+ zeroes suppressed
+
+SLIMbus example for Qualcomm's slimbus manager 

Re: [Patch v6 1/7] slimbus: Device management on SLIMbus

[Srinivas Kandagatla]

On 13/10/17 20:26, Rob Herring wrote:

On Fri, Oct 06, 2017 at 05:51:30PM +0200, srinivas.kandaga...@linaro.org wrote:

From: Sagar Dharia 

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 
Signed-off-by: Srinivas Kandagatla 
---
  Documentation/devicetree/bindings/slimbus/bus.txt |  57 ++


Split to separate patch.


Will do this in next patch.
It was suggested by Arnd in v5, may be just of_parse code can be merged 
in this patch and the bindings made as separate patch.






  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(+)
  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
  create mode 100644 include/linux/slimbus.h

diff --git a/Documentation/devicetree/bindings/slimbus/bus.txt 
b/Documentation/devicetree/bindings/slimbus/bus.txt
new file mode 100644
index 000..cb658bb
--- /dev/null
+++ b/Documentation/devicetree/bindings/slimbus/bus.txt
@@ -0,0 +1,57 @@
+SLIM(Serial Low Power Interchip Media Bus) bus
+
+SLIMbus is a 2-wire bus, and is used to communicate with peripheral
+components like audio-codec.
+
+Controller is a normal device using binding for whatever bus it is
+on (e.g. platform bus).


I can't have a PCI based slimbus controller? "platform bus" is a
Linuxism.

I agree, Will remove this Linuxism from the bindings.




+Required property for SLIMbus controller node:
+- compatible   - name of SLIMbus controller following generic names
+   recommended practice.


generic names aren't recommended. Allowed with some conditons, yes.


+- #address-cells - should be 2


You used 4 for your controller.


It should be actually 2. I will fix such mismatches in the other patches.




+- #size-cells  - should be 0
+
+No other properties are required in the SLIMbus controller bus node.


That's not a useful statement. Almost every controller probably has
other required properties.

Yep, will get rid of this in next patch.



+
+Child nodes:
+Every SLIMbus controller node can contain zero or more child nodes
+representing slave devices on the bus. Every SLIMbus slave device is
+uniquely determined by the enumeration address containing 4 fields:
+Manufacturer ID, Product code, Device index, and Instance value for
+the device.
+If child node is not present and it is instantiated after device
+discovery (slave device reporting itself present).
+
+In some cases it may be necessary to describe non-probeable device
+details such as non-standard ways of powering up a device. In
+such cases, child nodes for those devices will be present as
+slaves of the slimbus-controller, as detailed below.
+
+Required property for SLIMbus child node if it is present:
+- reg  - Is Duplex (Device index, Instance ID) from Enumeration
+ Address.
+ Device Index Uniquely identifies multiple Devices within
+ a single Component.
+ Instance ID Is for the cases where multiple Devices of the
+ same type or Class are attached to the bus.
+
+- compatible   -"slimMID,PID". The textual representation of Manufacturer ID,
+ Product Code, shall be in lower case hexadecimal with leading
+ zeroes suppressed
+
+SLIMbus example for Qualcomm's slimbus manager component:
+
+   slim@2808 {
+   compatible = "qcom,slim-msm";

Re: [Patch v6 1/7] slimbus: Device management on SLIMbus

[Rob Herring]

On Fri, Oct 06, 2017 at 05:51:30PM +0200, srinivas.kandaga...@linaro.org wrote:
> From: Sagar Dharia 
> 
> 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 
> Signed-off-by: Srinivas Kandagatla 
> ---
>  Documentation/devicetree/bindings/slimbus/bus.txt |  57 ++

Split to separate patch.

>  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(+)
>  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
>  create mode 100644 include/linux/slimbus.h
> 
> diff --git a/Documentation/devicetree/bindings/slimbus/bus.txt 
> b/Documentation/devicetree/bindings/slimbus/bus.txt
> new file mode 100644
> index 000..cb658bb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/slimbus/bus.txt
> @@ -0,0 +1,57 @@
> +SLIM(Serial Low Power Interchip Media Bus) bus
> +
> +SLIMbus is a 2-wire bus, and is used to communicate with peripheral
> +components like audio-codec.
> +
> +Controller is a normal device using binding for whatever bus it is
> +on (e.g. platform bus).

I can't have a PCI based slimbus controller? "platform bus" is a 
Linuxism.

> +Required property for SLIMbus controller node:
> +- compatible - name of SLIMbus controller following generic names
> + recommended practice.

generic names aren't recommended. Allowed with some conditons, yes.

> +- #address-cells - should be 2

You used 4 for your controller.

> +- #size-cells- should be 0
> +
> +No other properties are required in the SLIMbus controller bus node.

That's not a useful statement. Almost every controller probably has 
other required properties.

> +
> +Child nodes:
> +Every SLIMbus controller node can contain zero or more child nodes
> +representing slave devices on the bus. Every SLIMbus slave device is
> +uniquely determined by the enumeration address containing 4 fields:
> +Manufacturer ID, Product code, Device index, and Instance value for
> +the device.
> +If child node is not present and it is instantiated after device
> +discovery (slave device reporting itself present).
> +
> +In some cases it may be necessary to describe non-probeable device
> +details such as non-standard ways of powering up a device. In
> +such cases, child nodes for those devices will be present as
> +slaves of the slimbus-controller, as detailed below.
> +
> +Required property for SLIMbus child node if it is present:
> +- reg- Is Duplex (Device index, Instance ID) from Enumeration
> +   Address.
> +   Device Index Uniquely identifies multiple Devices within
> +   a single Component.
> +   Instance ID Is for the cases where multiple Devices of the
> +   same type or Class are attached to the bus.
> +
> +- compatible -"slimMID,PID". The textual representation of Manufacturer ID,
> +   Product Code, shall be in lower case hexadecimal with leading
> +   zeroes suppressed
> +
> +SLIMbus example for Qualcomm's slimbus manager component:
> +
> + slim@2808 {
> + compatible = "qcom,slim-msm";
> + reg = <0x2808 0x2000>,
> + interrupts = <0 33 0>;
> + clocks = < SLIMBUS_SRC>, < AUDIO_SLIMBUS_CLK>;
> +   

Re: [Patch v6 1/7] slimbus: Device management on SLIMbus

[Rob Herring]

On Fri, Oct 06, 2017 at 05:51:30PM +0200, srinivas.kandaga...@linaro.org wrote:
> From: Sagar Dharia 
> 
> 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 
> Signed-off-by: Srinivas Kandagatla 
> ---
>  Documentation/devicetree/bindings/slimbus/bus.txt |  57 ++

Split to separate patch.

>  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(+)
>  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
>  create mode 100644 include/linux/slimbus.h
> 
> diff --git a/Documentation/devicetree/bindings/slimbus/bus.txt 
> b/Documentation/devicetree/bindings/slimbus/bus.txt
> new file mode 100644
> index 000..cb658bb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/slimbus/bus.txt
> @@ -0,0 +1,57 @@
> +SLIM(Serial Low Power Interchip Media Bus) bus
> +
> +SLIMbus is a 2-wire bus, and is used to communicate with peripheral
> +components like audio-codec.
> +
> +Controller is a normal device using binding for whatever bus it is
> +on (e.g. platform bus).

I can't have a PCI based slimbus controller? "platform bus" is a 
Linuxism.

> +Required property for SLIMbus controller node:
> +- compatible - name of SLIMbus controller following generic names
> + recommended practice.

generic names aren't recommended. Allowed with some conditons, yes.

> +- #address-cells - should be 2

You used 4 for your controller.

> +- #size-cells- should be 0
> +
> +No other properties are required in the SLIMbus controller bus node.

That's not a useful statement. Almost every controller probably has 
other required properties.

> +
> +Child nodes:
> +Every SLIMbus controller node can contain zero or more child nodes
> +representing slave devices on the bus. Every SLIMbus slave device is
> +uniquely determined by the enumeration address containing 4 fields:
> +Manufacturer ID, Product code, Device index, and Instance value for
> +the device.
> +If child node is not present and it is instantiated after device
> +discovery (slave device reporting itself present).
> +
> +In some cases it may be necessary to describe non-probeable device
> +details such as non-standard ways of powering up a device. In
> +such cases, child nodes for those devices will be present as
> +slaves of the slimbus-controller, as detailed below.
> +
> +Required property for SLIMbus child node if it is present:
> +- reg- Is Duplex (Device index, Instance ID) from Enumeration
> +   Address.
> +   Device Index Uniquely identifies multiple Devices within
> +   a single Component.
> +   Instance ID Is for the cases where multiple Devices of the
> +   same type or Class are attached to the bus.
> +
> +- compatible -"slimMID,PID". The textual representation of Manufacturer ID,
> +   Product Code, shall be in lower case hexadecimal with leading
> +   zeroes suppressed
> +
> +SLIMbus example for Qualcomm's slimbus manager component:
> +
> + slim@2808 {
> + compatible = "qcom,slim-msm";
> + reg = <0x2808 0x2000>,
> + interrupts = <0 33 0>;
> + clocks = < SLIMBUS_SRC>, < AUDIO_SLIMBUS_CLK>;
> + clock-names = "iface_clk", "core_clk";
> + #address-cells 

Re: [alsa-devel] [Patch v6 1/7] slimbus: Device management on SLIMbus

[Srinivas Kandagatla]

On 12/10/17 12:01, Sanyog Kale wrote:

On Fri, Oct 06, 2017 at 05:51:30PM +0200, srinivas.kandaga...@linaro.org wrote:

From: Sagar Dharia 

+Per specification, Slimbus uses "clock gears" to do power management based on
+current frequency and bandwidth requirements. There are 10 clock gears and each
+gear changes the Slimbus frequency to be twice its previous gear.


Does the spec mandate 10 clock gears or its controller property?


Clock Gear Construct is part of SLIMbus Specs to alter clk frequency.



+Device notifications to the driver:
+---
+Since SLIMbus devices have mechanisms for reporting their presence, the
+framework allows drivers to bind when corresponding devices report their
+presence on the bus.
+However, it is possible that the driver needs to be probed
+first so that it can enable corresponding SLIMbus devie (e.g. power it up 
and/or
+take it out of reset). To support that behavior, the framework allows drivers
+to probe first as well  (e.g. using standard DeviceTree compatbility field).
+This creates the necessity for the driver to know when the device is functional
+(i.e. reported present). device_up callback is used for that reason when the
+device reports present and is assigned a logical address by the controller.
+
+Similarly, SLIMbus devices 'report absent' when they go down. A 'device_down'
+callback notifies the driver when the device reports absent and its logical
+address assignment is invalidated by the controller.


Is the same logical address assign when it reports present again?


Currently, Code as it is will pick the first available logical address. 
If required we can add logic in future to assign same logical address.

For now the code is simple.




+ *
+ * Controller here performs duties of the manager device, and 'interface
+ * device'. Interface device is responsible for monitoring the bus and
+ * reporting information such as loss-of-synchronization, data
+ * slot-collision.
+ * @dev: Device interface to this driver
+ * @nr: Board-specific number identifier for this controller/bus
+ * @list: Link with other slimbus controllers
+ * @name: Name for this controller
+ * @min_cg: Minimum clock gear supported by this controller (default value: 1)
+ * @max_cg: Maximum clock gear supported by this controller (default value: 10)
+ * @clkgear: Current clock gear in which this bus is running
+ * @a_framer: Active framer which is clocking the bus managed by this 
controller
+ * @m_ctrl: Mutex protecting controller data structures
+ * @addrt: Logical address table
+ * @num_dev: Number of active slimbus slaves on this bus
+ * @wq: Workqueue per controller used to notify devices when they report 
present
+ * @xfer_msg: Transfer a message on this controller (this can be a broadcast
+ * control/status message like data channel setup, or a unicast message
+ * like value element read/write.


xfer_msg element is not present in structure.



Thanks, it will be fixed in next version.

thanks,
Srini

Re: [alsa-devel] [Patch v6 1/7] slimbus: Device management on SLIMbus

[Srinivas Kandagatla]

On 12/10/17 12:01, Sanyog Kale wrote:

On Fri, Oct 06, 2017 at 05:51:30PM +0200, srinivas.kandaga...@linaro.org wrote:

From: Sagar Dharia 

+Per specification, Slimbus uses "clock gears" to do power management based on
+current frequency and bandwidth requirements. There are 10 clock gears and each
+gear changes the Slimbus frequency to be twice its previous gear.


Does the spec mandate 10 clock gears or its controller property?


Clock Gear Construct is part of SLIMbus Specs to alter clk frequency.



+Device notifications to the driver:
+---
+Since SLIMbus devices have mechanisms for reporting their presence, the
+framework allows drivers to bind when corresponding devices report their
+presence on the bus.
+However, it is possible that the driver needs to be probed
+first so that it can enable corresponding SLIMbus devie (e.g. power it up 
and/or
+take it out of reset). To support that behavior, the framework allows drivers
+to probe first as well  (e.g. using standard DeviceTree compatbility field).
+This creates the necessity for the driver to know when the device is functional
+(i.e. reported present). device_up callback is used for that reason when the
+device reports present and is assigned a logical address by the controller.
+
+Similarly, SLIMbus devices 'report absent' when they go down. A 'device_down'
+callback notifies the driver when the device reports absent and its logical
+address assignment is invalidated by the controller.


Is the same logical address assign when it reports present again?


Currently, Code as it is will pick the first available logical address. 
If required we can add logic in future to assign same logical address.

For now the code is simple.




+ *
+ * Controller here performs duties of the manager device, and 'interface
+ * device'. Interface device is responsible for monitoring the bus and
+ * reporting information such as loss-of-synchronization, data
+ * slot-collision.
+ * @dev: Device interface to this driver
+ * @nr: Board-specific number identifier for this controller/bus
+ * @list: Link with other slimbus controllers
+ * @name: Name for this controller
+ * @min_cg: Minimum clock gear supported by this controller (default value: 1)
+ * @max_cg: Maximum clock gear supported by this controller (default value: 10)
+ * @clkgear: Current clock gear in which this bus is running
+ * @a_framer: Active framer which is clocking the bus managed by this 
controller
+ * @m_ctrl: Mutex protecting controller data structures
+ * @addrt: Logical address table
+ * @num_dev: Number of active slimbus slaves on this bus
+ * @wq: Workqueue per controller used to notify devices when they report 
present
+ * @xfer_msg: Transfer a message on this controller (this can be a broadcast
+ * control/status message like data channel setup, or a unicast message
+ * like value element read/write.


xfer_msg element is not present in structure.



Thanks, it will be fixed in next version.

thanks,
Srini

Re: [alsa-devel] [Patch v6 1/7] slimbus: Device management on SLIMbus

[Sanyog Kale]

On Fri, Oct 06, 2017 at 05:51:30PM +0200, srinivas.kandaga...@linaro.org wrote:
> From: Sagar Dharia 
> 
> 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 
> Signed-off-by: Srinivas Kandagatla 
> ---
>  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(+)
>  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
>  create mode 100644 include/linux/slimbus.h
> 
> diff --git a/Documentation/devicetree/bindings/slimbus/bus.txt 
> b/Documentation/devicetree/bindings/slimbus/bus.txt
> new file mode 100644
> index 000..cb658bb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/slimbus/bus.txt
> @@ -0,0 +1,57 @@
> +SLIM(Serial Low Power Interchip Media Bus) bus
> +
> +SLIMbus is a 2-wire bus, and is used to communicate with peripheral
> +components like audio-codec.
> +
> +Controller is a normal device using binding for whatever bus it is
> +on (e.g. platform bus).
> +Required property for SLIMbus controller node:
> +- compatible - name of SLIMbus controller following generic names
> + recommended practice.
> +- #address-cells - should be 2
> +- #size-cells- should be 0
> +
> +No other properties are required in the SLIMbus controller bus node.
> +
> +Child nodes:
> +Every SLIMbus controller node can contain zero or more child nodes
> +representing slave devices on the bus. Every SLIMbus slave device is
> +uniquely determined by the enumeration address containing 4 fields:
> +Manufacturer ID, Product code, Device index, and Instance value for
> +the device.
> +If child node is not present and it is instantiated after device
> +discovery (slave device reporting itself present).
> +
> +In some cases it may be necessary to describe non-probeable device
> +details such as non-standard ways of powering up a device. In
> +such cases, child nodes for those devices will be present as
> +slaves of the slimbus-controller, as detailed below.
> +
> +Required property for SLIMbus child node if it is present:
> +- reg- Is Duplex (Device index, Instance ID) from Enumeration
> +   Address.
> +   Device Index Uniquely identifies multiple Devices within
> +   a single Component.
> +   Instance ID Is for the cases where multiple Devices of the
> +   same type or Class are attached to the bus.
> +
> +- compatible -"slimMID,PID". The textual representation of Manufacturer ID,
> +   Product Code, shall be in lower case hexadecimal with leading
> +   zeroes suppressed
> +
> +SLIMbus example for Qualcomm's slimbus manager component:
> +
> + slim@2808 {
> + compatible = "qcom,slim-msm";
> + reg = <0x2808 0x2000>,
> + interrupts = <0 33 0>;
> + clocks = < SLIMBUS_SRC>, < AUDIO_SLIMBUS_CLK>;
> + clock-names = "iface_clk", "core_clk";
> + #address-cells = <2>;
> + #size-cells = <0>;
> +
> + codec: wcd9310@1{
> + compatible = "slim217,60"";
> + reg = <1 0>;
> + };
> + };
> diff --git 

Re: [alsa-devel] [Patch v6 1/7] slimbus: Device management on SLIMbus

[Sanyog Kale]

On Fri, Oct 06, 2017 at 05:51:30PM +0200, srinivas.kandaga...@linaro.org wrote:
> From: Sagar Dharia 
> 
> 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 
> Signed-off-by: Srinivas Kandagatla 
> ---
>  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(+)
>  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
>  create mode 100644 include/linux/slimbus.h
> 
> diff --git a/Documentation/devicetree/bindings/slimbus/bus.txt 
> b/Documentation/devicetree/bindings/slimbus/bus.txt
> new file mode 100644
> index 000..cb658bb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/slimbus/bus.txt
> @@ -0,0 +1,57 @@
> +SLIM(Serial Low Power Interchip Media Bus) bus
> +
> +SLIMbus is a 2-wire bus, and is used to communicate with peripheral
> +components like audio-codec.
> +
> +Controller is a normal device using binding for whatever bus it is
> +on (e.g. platform bus).
> +Required property for SLIMbus controller node:
> +- compatible - name of SLIMbus controller following generic names
> + recommended practice.
> +- #address-cells - should be 2
> +- #size-cells- should be 0
> +
> +No other properties are required in the SLIMbus controller bus node.
> +
> +Child nodes:
> +Every SLIMbus controller node can contain zero or more child nodes
> +representing slave devices on the bus. Every SLIMbus slave device is
> +uniquely determined by the enumeration address containing 4 fields:
> +Manufacturer ID, Product code, Device index, and Instance value for
> +the device.
> +If child node is not present and it is instantiated after device
> +discovery (slave device reporting itself present).
> +
> +In some cases it may be necessary to describe non-probeable device
> +details such as non-standard ways of powering up a device. In
> +such cases, child nodes for those devices will be present as
> +slaves of the slimbus-controller, as detailed below.
> +
> +Required property for SLIMbus child node if it is present:
> +- reg- Is Duplex (Device index, Instance ID) from Enumeration
> +   Address.
> +   Device Index Uniquely identifies multiple Devices within
> +   a single Component.
> +   Instance ID Is for the cases where multiple Devices of the
> +   same type or Class are attached to the bus.
> +
> +- compatible -"slimMID,PID". The textual representation of Manufacturer ID,
> +   Product Code, shall be in lower case hexadecimal with leading
> +   zeroes suppressed
> +
> +SLIMbus example for Qualcomm's slimbus manager component:
> +
> + slim@2808 {
> + compatible = "qcom,slim-msm";
> + reg = <0x2808 0x2000>,
> + interrupts = <0 33 0>;
> + clocks = < SLIMBUS_SRC>, < AUDIO_SLIMBUS_CLK>;
> + clock-names = "iface_clk", "core_clk";
> + #address-cells = <2>;
> + #size-cells = <0>;
> +
> + codec: wcd9310@1{
> + compatible = "slim217,60"";
> + reg = <1 0>;
> + };
> + };
> diff --git a/Documentation/slimbus/summary b/Documentation/slimbus/summary
> new file mode 100644
> index 

Re: [alsa-devel] [Patch v6 1/7] slimbus: Device management on SLIMbus

[Srinivas Kandagatla]

On 11/10/17 11:21, Vinod Koul wrote:

ah which cases would those be. I was expecting that you would have a
platform_device as a slimbus controller which would call slim_register?

As of now there is only one controller which uses platform driver, but in
future there might be more, but this is something which makes the slimbus
core more flexible.

even if you have more controllers wouldn't we have similar number of platform
devices. Each instance of the link/controller would have its device node.


Yep, I will give at try and see in next version!


thanks,
srini



Thanks
-- ~Vinod

Re: [alsa-devel] [Patch v6 1/7] slimbus: Device management on SLIMbus

[Srinivas Kandagatla]

On 11/10/17 11:21, Vinod Koul wrote:

ah which cases would those be. I was expecting that you would have a
platform_device as a slimbus controller which would call slim_register?

As of now there is only one controller which uses platform driver, but in
future there might be more, but this is something which makes the slimbus
core more flexible.

even if you have more controllers wouldn't we have similar number of platform
devices. Each instance of the link/controller would have its device node.


Yep, I will give at try and see in next version!


thanks,
srini



Thanks
-- ~Vinod

Re: [alsa-devel] [Patch v6 1/7] slimbus: Device management on SLIMbus

[Mark Brown]

On Tue, Oct 10, 2017 at 01:34:48PM +0100, Srinivas Kandagatla wrote:
> On 10/10/17 11:05, Charles Keepax wrote:

Please delete unneeded context from mails when replying.  Doing this
makes it much easier to find your reply in the message, helping ensure
it won't be missed by people scrolling through the irrelevant quoted
material.

> > This does sort of make sense but kinda makes the code a bit ugly
> > parsing the MID and PID. Some parts will support SLIMBus and also
> > other control interfaces, which means they would need to add an
> > additional compatible string just for SLIMBus. It also breaks
> > the normal conventions of vendor,part and finally it makes the
> > compatible strings really unreadable which will be a bit annoying
> > when looking at DTs.

> This change was made inline to the comments provided in previous version of
> the patch https://lkml.org/lkml/2016/5/3/576

I'm not sure I really agree with Rob here - while Slimbus is notionally
discoverable I don't think I ever saw a practical system which relied on
that for enumeration.  In real systems the discoverability is more of a
complexity to be worked around than anything else.


signature.asc
Description: PGP signature

Re: [alsa-devel] [Patch v6 1/7] slimbus: Device management on SLIMbus

[Mark Brown]

On Tue, Oct 10, 2017 at 01:34:48PM +0100, Srinivas Kandagatla wrote:
> On 10/10/17 11:05, Charles Keepax wrote:

Please delete unneeded context from mails when replying.  Doing this
makes it much easier to find your reply in the message, helping ensure
it won't be missed by people scrolling through the irrelevant quoted
material.

> > This does sort of make sense but kinda makes the code a bit ugly
> > parsing the MID and PID. Some parts will support SLIMBus and also
> > other control interfaces, which means they would need to add an
> > additional compatible string just for SLIMBus. It also breaks
> > the normal conventions of vendor,part and finally it makes the
> > compatible strings really unreadable which will be a bit annoying
> > when looking at DTs.

> This change was made inline to the comments provided in previous version of
> the patch https://lkml.org/lkml/2016/5/3/576

I'm not sure I really agree with Rob here - while Slimbus is notionally
discoverable I don't think I ever saw a practical system which relied on
that for enumeration.  In real systems the discoverability is more of a
complexity to be worked around than anything else.


signature.asc
Description: PGP signature

Re: [alsa-devel] [Patch v6 1/7] slimbus: Device management on SLIMbus

[Vinod Koul]

On Wed, Oct 11, 2017 at 10:42:23AM +0100, Srinivas Kandagatla wrote:
> On 11/10/17 05:07, Vinod Koul wrote:
> >On Tue, Oct 10, 2017 at 06:21:34PM +0100, Srinivas Kandagatla wrote:
> >>On 10/10/17 17:49, Vinod Koul wrote:

> >>+/**
> >>+ * slim_register_controller: Controller bring-up and registration.
> >>...
> >>+
> >>+   mutex_init(>m_ctrl);
> >>+   ret = device_register(>dev);
> >
> >one more device_register?? Can you explain why
> >
> 
> This is a device for each controller.
> >>>
> >>>wont the controller have its own platform_device?
> >>
> >>Reason could be that slim_register controller can be called from any code
> >>not just platform devices..
> >
> >ah which cases would those be. I was expecting that you would have a
> >platform_device as a slimbus controller which would call slim_register?
> As of now there is only one controller which uses platform driver, but in
> future there might be more, but this is something which makes the slimbus
> core more flexible.

even if you have more controllers wouldn't we have similar number of platform
devices. Each instance of the link/controller would have its device node.

Thanks
-- 
~Vinod

Re: [alsa-devel] [Patch v6 1/7] slimbus: Device management on SLIMbus

[Vinod Koul]

On Wed, Oct 11, 2017 at 10:42:23AM +0100, Srinivas Kandagatla wrote:
> On 11/10/17 05:07, Vinod Koul wrote:
> >On Tue, Oct 10, 2017 at 06:21:34PM +0100, Srinivas Kandagatla wrote:
> >>On 10/10/17 17:49, Vinod Koul wrote:

> >>+/**
> >>+ * slim_register_controller: Controller bring-up and registration.
> >>...
> >>+
> >>+   mutex_init(>m_ctrl);
> >>+   ret = device_register(>dev);
> >
> >one more device_register?? Can you explain why
> >
> 
> This is a device for each controller.
> >>>
> >>>wont the controller have its own platform_device?
> >>
> >>Reason could be that slim_register controller can be called from any code
> >>not just platform devices..
> >
> >ah which cases would those be. I was expecting that you would have a
> >platform_device as a slimbus controller which would call slim_register?
> As of now there is only one controller which uses platform driver, but in
> future there might be more, but this is something which makes the slimbus
> core more flexible.

even if you have more controllers wouldn't we have similar number of platform
devices. Each instance of the link/controller would have its device node.

Thanks
-- 
~Vinod

Re: [alsa-devel] [Patch v6 1/7] slimbus: Device management on SLIMbus

[Srinivas Kandagatla]

On 11/10/17 05:07, Vinod Koul wrote:

On Tue, Oct 10, 2017 at 06:21:34PM +0100, Srinivas Kandagatla wrote:

On 10/10/17 17:49, Vinod Koul wrote:



+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?


It is scheduling a device_up() callback in workqueue for reporting
discovered device.


any reason for that? Would the device not announce itself on the bus and
then you can synchronously update the device.

You are correct,  Device should announce itself in all cases. core should
only call this callback only when device is announced, it does not make
sense for this call in slim_device_probe(). Will remove it from here in next
version.


Okay great. Btw do you need a notify being scheduled in those cases? I guess
your controller would get an interrupt and you will handle that in bottom
half and then cll this, so why not call in the bottom half?

That makes sense, I will optimize this path, It looks like there are 2 
workqueues in this path. We should be able to get rid of one work-queue.





+/**
+ * slim_register_controller: Controller bring-up and registration.

...

+
+   mutex_init(>m_ctrl);
+   ret = device_register(>dev);


one more device_register?? Can you explain why



This is a device for each controller.


wont the controller have its own platform_device?


Reason could be that slim_register controller can be called from any code
not just platform devices..


ah which cases would those be. I was expecting that you would have a
platform_device as a slimbus controller which would call slim_register?
As of now there is only one controller which uses platform driver, but 
in future there might be more, but this is something which makes the 
slimbus core more flexible.

Re: [alsa-devel] [Patch v6 1/7] slimbus: Device management on SLIMbus

[Srinivas Kandagatla]

On 11/10/17 05:07, Vinod Koul wrote:

On Tue, Oct 10, 2017 at 06:21:34PM +0100, Srinivas Kandagatla wrote:

On 10/10/17 17:49, Vinod Koul wrote:



+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?


It is scheduling a device_up() callback in workqueue for reporting
discovered device.


any reason for that? Would the device not announce itself on the bus and
then you can synchronously update the device.

You are correct,  Device should announce itself in all cases. core should
only call this callback only when device is announced, it does not make
sense for this call in slim_device_probe(). Will remove it from here in next
version.


Okay great. Btw do you need a notify being scheduled in those cases? I guess
your controller would get an interrupt and you will handle that in bottom
half and then cll this, so why not call in the bottom half?

That makes sense, I will optimize this path, It looks like there are 2 
workqueues in this path. We should be able to get rid of one work-queue.





+/**
+ * slim_register_controller: Controller bring-up and registration.

...

+
+   mutex_init(>m_ctrl);
+   ret = device_register(>dev);


one more device_register?? Can you explain why



This is a device for each controller.


wont the controller have its own platform_device?


Reason could be that slim_register controller can be called from any code
not just platform devices..


ah which cases would those be. I was expecting that you would have a
platform_device as a slimbus controller which would call slim_register?
As of now there is only one controller which uses platform driver, but 
in future there might be more, but this is something which makes the 
slimbus core more flexible.

Re: [alsa-devel] [Patch v6 1/7] slimbus: Device management on SLIMbus

[Vinod Koul]

On Tue, Oct 10, 2017 at 06:21:34PM +0100, Srinivas Kandagatla wrote:
> On 10/10/17 17:49, Vinod Koul wrote:

> +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?
> >>
> >>It is scheduling a device_up() callback in workqueue for reporting
> >>discovered device.
> >
> >any reason for that? Would the device not announce itself on the bus and
> >then you can synchronously update the device.
> You are correct,  Device should announce itself in all cases. core should
> only call this callback only when device is announced, it does not make
> sense for this call in slim_device_probe(). Will remove it from here in next
> version.

Okay great. Btw do you need a notify being scheduled in those cases? I guess
your controller would get an interrupt and you will handle that in bottom
half and then cll this, so why not call in the bottom half?

> +/**
> + * slim_register_controller: Controller bring-up and registration.
> ...
> +
> + mutex_init(>m_ctrl);
> + ret = device_register(>dev);
> >>>
> >>>one more device_register?? Can you explain why
> >>>
> >>
> >>This is a device for each controller.
> >
> >wont the controller have its own platform_device?
> 
> Reason could be that slim_register controller can be called from any code
> not just platform devices..

ah which cases would those be. I was expecting that you would have a
platform_device as a slimbus controller which would call slim_register?

-- 
~Vinod

Re: [alsa-devel] [Patch v6 1/7] slimbus: Device management on SLIMbus

[Vinod Koul]

On Tue, Oct 10, 2017 at 06:21:34PM +0100, Srinivas Kandagatla wrote:
> On 10/10/17 17:49, Vinod Koul wrote:

> +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?
> >>
> >>It is scheduling a device_up() callback in workqueue for reporting
> >>discovered device.
> >
> >any reason for that? Would the device not announce itself on the bus and
> >then you can synchronously update the device.
> You are correct,  Device should announce itself in all cases. core should
> only call this callback only when device is announced, it does not make
> sense for this call in slim_device_probe(). Will remove it from here in next
> version.

Okay great. Btw do you need a notify being scheduled in those cases? I guess
your controller would get an interrupt and you will handle that in bottom
half and then cll this, so why not call in the bottom half?

> +/**
> + * slim_register_controller: Controller bring-up and registration.
> ...
> +
> + mutex_init(>m_ctrl);
> + ret = device_register(>dev);
> >>>
> >>>one more device_register?? Can you explain why
> >>>
> >>
> >>This is a device for each controller.
> >
> >wont the controller have its own platform_device?
> 
> Reason could be that slim_register controller can be called from any code
> not just platform devices..

ah which cases would those be. I was expecting that you would have a
platform_device as a slimbus controller which would call slim_register?

-- 
~Vinod

Re: [alsa-devel] [Patch v6 1/7] slimbus: Device management on SLIMbus

[Srinivas Kandagatla]

On 10/10/17 17:49, Vinod Koul wrote:

On Tue, Oct 10, 2017 at 01:34:55PM +0100, Srinivas Kandagatla wrote:

  9 files changed, 1192 insertions(+)


thats a lot of code for review, consider splitting it up further for better
reviews


Its was suggested that parts of dtbindings and of_* wrapper merged into this
patch.  In V5 review comments. https://lkml.org/lkml/2016/4/28/175


yes but it can still be split :)


Will give it a go in next version!!



+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

Yes, we need this to match the compatible string from device tree vs driver
itself, most of the bus driver do this in bus match functions.



DT so pardon my ignorance). Since we have devices with ids can we use that
alone for matching?


Two cases to consider here,
1> If the device is up and discoverable.
2> Device is not discoverable yet, as it needs some power up sequence.


In first case comparing with ID table makes sense.

But second case we would want to probe the device(for power sequencing)
before we can discover the device on bus.


This code as it is supports both DT and id_table.


Why not only id_table, see below:




Yes, we make id_table as mandatory field for all slimbus drivers.



+   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?


up would be called when a device is discovered on the bus, and down on when
the device disappeared on slimbus.


+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?


It is scheduling a device_up() callback in workqueue for reporting
discovered device.


any reason for that? Would the device not announce itself on the bus and
then you can synchronously update the device.
You are correct,  Device should announce itself in all cases. core 
should only call this callback only when device is announced, it does 
not make sense for this call in slim_device_probe(). Will remove it from 
here in next version.






+int __slim_driver_register(struct slim_driver *drv, struct module *owner)
+{
+   drv->driver.bus = _type;
+   drv->driver.owner = owner;
+   return driver_register(>driver);
+}
+EXPORT_SYMBOL_GPL(__slim_driver_register);


any reason to use __ for this API?


This is made inline with __platfrom_driver_register() suggested in v5
review.


I guess Greg is best person to make this call :)



Forgot to put original comment in v5 by Arnd: 
https://lkml.org/lkml/2016/4/28/179




+static void of_register_slim_devices(struct slim_controller *ctrl)
+{
+   struct device *dev = >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(, ",");
+   if (!tok) {
+   dev_err(dev, "No 

Re: [alsa-devel] [Patch v6 1/7] slimbus: Device management on SLIMbus

[Srinivas Kandagatla]

On 10/10/17 17:49, Vinod Koul wrote:

On Tue, Oct 10, 2017 at 01:34:55PM +0100, Srinivas Kandagatla wrote:

  9 files changed, 1192 insertions(+)


thats a lot of code for review, consider splitting it up further for better
reviews


Its was suggested that parts of dtbindings and of_* wrapper merged into this
patch.  In V5 review comments. https://lkml.org/lkml/2016/4/28/175


yes but it can still be split :)


Will give it a go in next version!!



+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

Yes, we need this to match the compatible string from device tree vs driver
itself, most of the bus driver do this in bus match functions.



DT so pardon my ignorance). Since we have devices with ids can we use that
alone for matching?


Two cases to consider here,
1> If the device is up and discoverable.
2> Device is not discoverable yet, as it needs some power up sequence.


In first case comparing with ID table makes sense.

But second case we would want to probe the device(for power sequencing)
before we can discover the device on bus.


This code as it is supports both DT and id_table.


Why not only id_table, see below:




Yes, we make id_table as mandatory field for all slimbus drivers.



+   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?


up would be called when a device is discovered on the bus, and down on when
the device disappeared on slimbus.


+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?


It is scheduling a device_up() callback in workqueue for reporting
discovered device.


any reason for that? Would the device not announce itself on the bus and
then you can synchronously update the device.
You are correct,  Device should announce itself in all cases. core 
should only call this callback only when device is announced, it does 
not make sense for this call in slim_device_probe(). Will remove it from 
here in next version.






+int __slim_driver_register(struct slim_driver *drv, struct module *owner)
+{
+   drv->driver.bus = _type;
+   drv->driver.owner = owner;
+   return driver_register(>driver);
+}
+EXPORT_SYMBOL_GPL(__slim_driver_register);


any reason to use __ for this API?


This is made inline with __platfrom_driver_register() suggested in v5
review.


I guess Greg is best person to make this call :)



Forgot to put original comment in v5 by Arnd: 
https://lkml.org/lkml/2016/4/28/179




+static void of_register_slim_devices(struct slim_controller *ctrl)
+{
+   struct device *dev = >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(, ",");
+   if (!tok) {
+   dev_err(dev, "No 

Re: [alsa-devel] [Patch v6 1/7] slimbus: Device management on SLIMbus

[Vinod Koul]

On Tue, Oct 10, 2017 at 01:34:55PM +0100, Srinivas Kandagatla wrote:
> >>  9 files changed, 1192 insertions(+)
> >
> >thats a lot of code for review, consider splitting it up further for better
> >reviews
> 
> Its was suggested that parts of dtbindings and of_* wrapper merged into this
> patch.  In V5 review comments. https://lkml.org/lkml/2016/4/28/175

yes but it can still be split :)


> >>+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
> Yes, we need this to match the compatible string from device tree vs driver
> itself, most of the bus driver do this in bus match functions.
> 
> 
> >DT so pardon my ignorance). Since we have devices with ids can we use that
> >alone for matching?
> 
> Two cases to consider here,
> 1> If the device is up and discoverable.
> 2> Device is not discoverable yet, as it needs some power up sequence.
> 
> 
> In first case comparing with ID table makes sense.
> 
> But second case we would want to probe the device(for power sequencing)
> before we can discover the device on bus.
> 
> 
> This code as it is supports both DT and id_table.

Why not only id_table, see below:

> >>+   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?
> >
> up would be called when a device is discovered on the bus, and down on when
> the device disappeared on slimbus.
> 
> >>+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?
> 
> It is scheduling a device_up() callback in workqueue for reporting
> discovered device.

any reason for that? Would the device not announce itself on the bus and
then you can synchronously update the device.

> >>+int __slim_driver_register(struct slim_driver *drv, struct module *owner)
> >>+{
> >>+   drv->driver.bus = _type;
> >>+   drv->driver.owner = owner;
> >>+   return driver_register(>driver);
> >>+}
> >>+EXPORT_SYMBOL_GPL(__slim_driver_register);
> >
> >any reason to use __ for this API?
> 
> This is made inline with __platfrom_driver_register() suggested in v5
> review.

I guess Greg is best person to make this call :)

> >>+static void of_register_slim_devices(struct slim_controller *ctrl)
> >>+{
> >>+   struct device *dev = >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(, ",");
> >>+   if (!tok) {
> >>+   dev_err(dev, "No valid Manufacturer ID found\n");
> >>+   kfree(p);
> >>+   continue;
> >>+   }
> >>+   slim->e_addr.manf_id = str2hex(tok);
> >>+
> >>+   tok = strsep(, ",");
> >>+   if (!tok) {
> >>+   dev_err(dev, "No valid 

Re: [alsa-devel] [Patch v6 1/7] slimbus: Device management on SLIMbus

[Vinod Koul]

On Tue, Oct 10, 2017 at 01:34:55PM +0100, Srinivas Kandagatla wrote:
> >>  9 files changed, 1192 insertions(+)
> >
> >thats a lot of code for review, consider splitting it up further for better
> >reviews
> 
> Its was suggested that parts of dtbindings and of_* wrapper merged into this
> patch.  In V5 review comments. https://lkml.org/lkml/2016/4/28/175

yes but it can still be split :)


> >>+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
> Yes, we need this to match the compatible string from device tree vs driver
> itself, most of the bus driver do this in bus match functions.
> 
> 
> >DT so pardon my ignorance). Since we have devices with ids can we use that
> >alone for matching?
> 
> Two cases to consider here,
> 1> If the device is up and discoverable.
> 2> Device is not discoverable yet, as it needs some power up sequence.
> 
> 
> In first case comparing with ID table makes sense.
> 
> But second case we would want to probe the device(for power sequencing)
> before we can discover the device on bus.
> 
> 
> This code as it is supports both DT and id_table.

Why not only id_table, see below:

> >>+   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?
> >
> up would be called when a device is discovered on the bus, and down on when
> the device disappeared on slimbus.
> 
> >>+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?
> 
> It is scheduling a device_up() callback in workqueue for reporting
> discovered device.

any reason for that? Would the device not announce itself on the bus and
then you can synchronously update the device.

> >>+int __slim_driver_register(struct slim_driver *drv, struct module *owner)
> >>+{
> >>+   drv->driver.bus = _type;
> >>+   drv->driver.owner = owner;
> >>+   return driver_register(>driver);
> >>+}
> >>+EXPORT_SYMBOL_GPL(__slim_driver_register);
> >
> >any reason to use __ for this API?
> 
> This is made inline with __platfrom_driver_register() suggested in v5
> review.

I guess Greg is best person to make this call :)

> >>+static void of_register_slim_devices(struct slim_controller *ctrl)
> >>+{
> >>+   struct device *dev = >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(, ",");
> >>+   if (!tok) {
> >>+   dev_err(dev, "No valid Manufacturer ID found\n");
> >>+   kfree(p);
> >>+   continue;
> >>+   }
> >>+   slim->e_addr.manf_id = str2hex(tok);
> >>+
> >>+   tok = strsep(, ",");
> >>+   if (!tok) {
> >>+   dev_err(dev, "No valid 

Re: [alsa-devel] [Patch v6 1/7] slimbus: Device management on SLIMbus

[Charles Keepax]

On Tue, Oct 10, 2017 at 01:34:48PM +0100, Srinivas Kandagatla wrote:
> Thanks for the review comments.
> 
> On 10/10/17 11:05, Charles Keepax wrote:
> > On Fri, Oct 06, 2017 at 05:51:30PM +0200, srinivas.kandaga...@linaro.org 
> > wrote:
> > > +Required property for SLIMbus child node if it is present:
> > > +- reg- Is Duplex (Device index, Instance ID) from Enumeration
> > > +   Address.
> > > +   Device Index Uniquely identifies multiple Devices within
> > > +   a single Component.
> > > +   Instance ID Is for the cases where multiple Devices of the
> > > +   same type or Class are attached to the bus.
> > > +
> > > +- compatible -"slimMID,PID". The textual representation of 
> > > Manufacturer ID,
> > > +   Product Code, shall be in lower case hexadecimal with leading
> > > +   zeroes suppressed
> > 
> > This does sort of make sense but kinda makes the code a bit ugly
> > parsing the MID and PID. Some parts will support SLIMBus and also
> > other control interfaces, which means they would need to add an
> > additional compatible string just for SLIMBus. It also breaks
> > the normal conventions of vendor,part and finally it makes the
> > compatible strings really unreadable which will be a bit annoying
> > when looking at DTs.
> > 
> This change was made inline to the comments provided in previous version of
> the patch https://lkml.org/lkml/2016/5/3/576
> 
> > I think the MID and PID should just be included in the reg field
> > and just leave this as a standard compatible.
> 
> AFAIK, reg field should only contain index and instance, which was also
> discussed  at https://lkml.org/lkml/2016/5/3/747
> 

Thanks for the links, if people are determined this is the way
to go I can live with it. I guess my primary objection is the
fact my parts are going to have different compatibles depending
on if they are on I2C/SPI or on SLIMBus (we have many parts that
support all three on the same chip) which feels like it violates
the principle of least surprise. Will wait to see if Arnd or Rob
have anymore thoughts to offer.

> > > +/* Helper to get hex Manufacturer ID and Product id from compatible */
> > > +static unsigned long str2hex(unsigned char *str)
> > > +{
> > > + int value = 0;
> > > +
> > > + while (*str) {
> > > + char c = *str++;
> > > +
> > > + value = value << 4;
> > > + if (c >= '0' && c <= '9')
> > > + value |= (c - '0');
> > > + if (c >= 'a' && c <= 'f')
> > > + value |= (c - 'a' + 10);
> > > +
> > > + }
> > > +
> > > + return value;
> > > +}
> > 
> > Isn't this just reimplementing kstrtoul?
> > 
> I would say partly, I think kstrtoul will only parse string as hex if its
> prefixed with "0x" But the compatible does not have 0x prefix..
> we could probably do some prefixing before passing to kstrtoul to remove
> above function.. I will try that and see!
> 

I am pretty sure kstrtoul is happy without the 0x as long as you
specify the base as 16 which I guess you should be doing here.

Thanks,
Charles

Re: [alsa-devel] [Patch v6 1/7] slimbus: Device management on SLIMbus

[Charles Keepax]

On Tue, Oct 10, 2017 at 01:34:48PM +0100, Srinivas Kandagatla wrote:
> Thanks for the review comments.
> 
> On 10/10/17 11:05, Charles Keepax wrote:
> > On Fri, Oct 06, 2017 at 05:51:30PM +0200, srinivas.kandaga...@linaro.org 
> > wrote:
> > > +Required property for SLIMbus child node if it is present:
> > > +- reg- Is Duplex (Device index, Instance ID) from Enumeration
> > > +   Address.
> > > +   Device Index Uniquely identifies multiple Devices within
> > > +   a single Component.
> > > +   Instance ID Is for the cases where multiple Devices of the
> > > +   same type or Class are attached to the bus.
> > > +
> > > +- compatible -"slimMID,PID". The textual representation of 
> > > Manufacturer ID,
> > > +   Product Code, shall be in lower case hexadecimal with leading
> > > +   zeroes suppressed
> > 
> > This does sort of make sense but kinda makes the code a bit ugly
> > parsing the MID and PID. Some parts will support SLIMBus and also
> > other control interfaces, which means they would need to add an
> > additional compatible string just for SLIMBus. It also breaks
> > the normal conventions of vendor,part and finally it makes the
> > compatible strings really unreadable which will be a bit annoying
> > when looking at DTs.
> > 
> This change was made inline to the comments provided in previous version of
> the patch https://lkml.org/lkml/2016/5/3/576
> 
> > I think the MID and PID should just be included in the reg field
> > and just leave this as a standard compatible.
> 
> AFAIK, reg field should only contain index and instance, which was also
> discussed  at https://lkml.org/lkml/2016/5/3/747
> 

Thanks for the links, if people are determined this is the way
to go I can live with it. I guess my primary objection is the
fact my parts are going to have different compatibles depending
on if they are on I2C/SPI or on SLIMBus (we have many parts that
support all three on the same chip) which feels like it violates
the principle of least surprise. Will wait to see if Arnd or Rob
have anymore thoughts to offer.

> > > +/* Helper to get hex Manufacturer ID and Product id from compatible */
> > > +static unsigned long str2hex(unsigned char *str)
> > > +{
> > > + int value = 0;
> > > +
> > > + while (*str) {
> > > + char c = *str++;
> > > +
> > > + value = value << 4;
> > > + if (c >= '0' && c <= '9')
> > > + value |= (c - '0');
> > > + if (c >= 'a' && c <= 'f')
> > > + value |= (c - 'a' + 10);
> > > +
> > > + }
> > > +
> > > + return value;
> > > +}
> > 
> > Isn't this just reimplementing kstrtoul?
> > 
> I would say partly, I think kstrtoul will only parse string as hex if its
> prefixed with "0x" But the compatible does not have 0x prefix..
> we could probably do some prefixing before passing to kstrtoul to remove
> above function.. I will try that and see!
> 

I am pretty sure kstrtoul is happy without the 0x as long as you
specify the base as 16 which I guess you should be doing here.

Thanks,
Charles

Re: [alsa-devel] [Patch v6 1/7] slimbus: Device management on SLIMbus

[Srinivas Kandagatla]

Thanks for your review comments,

On 10/10/17 11:45, Vinod Koul wrote:

On Fri, Oct 06, 2017 at 05:51:30PM +0200, srinivas.kandaga...@linaro.org wrote:

From: Sagar Dharia 

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 
Signed-off-by: Srinivas Kandagatla 
---
  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


Its was suggested that parts of dtbindings and of_* wrapper merged into 
this patch.  In V5 review comments. https://lkml.org/lkml/2016/4/28/175






  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)


Makes sense, will do that in next version.


+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
Yes, we need this to match the compatible string from device tree vs 
driver itself, most of the bus driver do this in bus match functions.




DT so pardon my ignorance). Since we have devices with ids can we use that
alone for matching?


Two cases to consider here,
1> If the device is up and discoverable.
2> Device is not discoverable yet, as it needs some power up sequence.


In first case comparing with ID table makes sense.

But second case we would want to probe the device(for power sequencing) 
before we can discover the device on bus.



This code as it is supports both DT and id_table.


+
+   /* 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


Let me try that in next version.


+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(>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 && 

Re: [alsa-devel] [Patch v6 1/7] slimbus: Device management on SLIMbus

[Srinivas Kandagatla]

Thanks for your review comments,

On 10/10/17 11:45, Vinod Koul wrote:

On Fri, Oct 06, 2017 at 05:51:30PM +0200, srinivas.kandaga...@linaro.org wrote:

From: Sagar Dharia 

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 
Signed-off-by: Srinivas Kandagatla 
---
  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


Its was suggested that parts of dtbindings and of_* wrapper merged into 
this patch.  In V5 review comments. https://lkml.org/lkml/2016/4/28/175






  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)


Makes sense, will do that in next version.


+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
Yes, we need this to match the compatible string from device tree vs 
driver itself, most of the bus driver do this in bus match functions.




DT so pardon my ignorance). Since we have devices with ids can we use that
alone for matching?


Two cases to consider here,
1> If the device is up and discoverable.
2> Device is not discoverable yet, as it needs some power up sequence.


In first case comparing with ID table makes sense.

But second case we would want to probe the device(for power sequencing) 
before we can discover the device on bus.



This code as it is supports both DT and id_table.


+
+   /* 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


Let me try that in next version.


+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(>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 = 

Re: [alsa-devel] [Patch v6 1/7] slimbus: Device management on SLIMbus

[Srinivas Kandagatla]

Thanks for the review comments.

On 10/10/17 11:05, Charles Keepax wrote:

On Fri, Oct 06, 2017 at 05:51:30PM +0200, srinivas.kandaga...@linaro.org wrote:

From: Sagar Dharia 

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 
Signed-off-by: Srinivas Kandagatla 
---
  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(+)
  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
  create mode 100644 include/linux/slimbus.h

diff --git a/Documentation/devicetree/bindings/slimbus/bus.txt 
b/Documentation/devicetree/bindings/slimbus/bus.txt
new file mode 100644
index 000..cb658bb
--- /dev/null
+++ b/Documentation/devicetree/bindings/slimbus/bus.txt
@@ -0,0 +1,57 @@
+SLIM(Serial Low Power Interchip Media Bus) bus
+
+SLIMbus is a 2-wire bus, and is used to communicate with peripheral
+components like audio-codec.
+
+Controller is a normal device using binding for whatever bus it is
+on (e.g. platform bus).
+Required property for SLIMbus controller node:
+- compatible   - name of SLIMbus controller following generic names
+   recommended practice.
+- #address-cells - should be 2
+- #size-cells  - should be 0
+
+No other properties are required in the SLIMbus controller bus node.
+
+Child nodes:
+Every SLIMbus controller node can contain zero or more child nodes
+representing slave devices on the bus. Every SLIMbus slave device is
+uniquely determined by the enumeration address containing 4 fields:
+Manufacturer ID, Product code, Device index, and Instance value for
+the device.
+If child node is not present and it is instantiated after device
+discovery (slave device reporting itself present).
+
+In some cases it may be necessary to describe non-probeable device
+details such as non-standard ways of powering up a device. In
+such cases, child nodes for those devices will be present as
+slaves of the slimbus-controller, as detailed below.
+
+Required property for SLIMbus child node if it is present:
+- reg  - Is Duplex (Device index, Instance ID) from Enumeration
+ Address.
+ Device Index Uniquely identifies multiple Devices within
+ a single Component.
+ Instance ID Is for the cases where multiple Devices of the
+ same type or Class are attached to the bus.
+
+- compatible   -"slimMID,PID". The textual representation of Manufacturer ID,
+ Product Code, shall be in lower case hexadecimal with leading
+ zeroes suppressed


This does sort of make sense but kinda makes the code a bit ugly
parsing the MID and PID. Some parts will support SLIMBus and also
other control interfaces, which means they would need to add an
additional compatible string just for SLIMBus. It also breaks
the normal conventions of vendor,part and finally it makes the
compatible strings really unreadable which will be a bit annoying
when looking at DTs.

This change was made inline to the comments provided in previous version 
of the patch https://lkml.org/lkml/2016/5/3/576



I think the MID and PID should just be included in the reg field
and just leave this as a standard compatible.


AFAIK, reg field should 

Re: [alsa-devel] [Patch v6 1/7] slimbus: Device management on SLIMbus

[Srinivas Kandagatla]

Thanks for the review comments.

On 10/10/17 11:05, Charles Keepax wrote:

On Fri, Oct 06, 2017 at 05:51:30PM +0200, srinivas.kandaga...@linaro.org wrote:

From: Sagar Dharia 

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 
Signed-off-by: Srinivas Kandagatla 
---
  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(+)
  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
  create mode 100644 include/linux/slimbus.h

diff --git a/Documentation/devicetree/bindings/slimbus/bus.txt 
b/Documentation/devicetree/bindings/slimbus/bus.txt
new file mode 100644
index 000..cb658bb
--- /dev/null
+++ b/Documentation/devicetree/bindings/slimbus/bus.txt
@@ -0,0 +1,57 @@
+SLIM(Serial Low Power Interchip Media Bus) bus
+
+SLIMbus is a 2-wire bus, and is used to communicate with peripheral
+components like audio-codec.
+
+Controller is a normal device using binding for whatever bus it is
+on (e.g. platform bus).
+Required property for SLIMbus controller node:
+- compatible   - name of SLIMbus controller following generic names
+   recommended practice.
+- #address-cells - should be 2
+- #size-cells  - should be 0
+
+No other properties are required in the SLIMbus controller bus node.
+
+Child nodes:
+Every SLIMbus controller node can contain zero or more child nodes
+representing slave devices on the bus. Every SLIMbus slave device is
+uniquely determined by the enumeration address containing 4 fields:
+Manufacturer ID, Product code, Device index, and Instance value for
+the device.
+If child node is not present and it is instantiated after device
+discovery (slave device reporting itself present).
+
+In some cases it may be necessary to describe non-probeable device
+details such as non-standard ways of powering up a device. In
+such cases, child nodes for those devices will be present as
+slaves of the slimbus-controller, as detailed below.
+
+Required property for SLIMbus child node if it is present:
+- reg  - Is Duplex (Device index, Instance ID) from Enumeration
+ Address.
+ Device Index Uniquely identifies multiple Devices within
+ a single Component.
+ Instance ID Is for the cases where multiple Devices of the
+ same type or Class are attached to the bus.
+
+- compatible   -"slimMID,PID". The textual representation of Manufacturer ID,
+ Product Code, shall be in lower case hexadecimal with leading
+ zeroes suppressed


This does sort of make sense but kinda makes the code a bit ugly
parsing the MID and PID. Some parts will support SLIMBus and also
other control interfaces, which means they would need to add an
additional compatible string just for SLIMBus. It also breaks
the normal conventions of vendor,part and finally it makes the
compatible strings really unreadable which will be a bit annoying
when looking at DTs.

This change was made inline to the comments provided in previous version 
of the patch https://lkml.org/lkml/2016/5/3/576



I think the MID and PID should just be included in the reg field
and just leave this as a standard compatible.


AFAIK, reg field should only contain index and instance, which was also 
discussed  at 

Re: [alsa-devel] [Patch v6 1/7] slimbus: Device management on SLIMbus

[Vinod Koul]

On Fri, Oct 06, 2017 at 05:51:30PM +0200, srinivas.kandaga...@linaro.org wrote:
> From: Sagar Dharia 
> 
> 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 
> Signed-off-by: Srinivas Kandagatla 
> ---
>  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(>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 

Re: [alsa-devel] [Patch v6 1/7] slimbus: Device management on SLIMbus

[Vinod Koul]

On Fri, Oct 06, 2017 at 05:51:30PM +0200, srinivas.kandaga...@linaro.org wrote:
> From: Sagar Dharia 
> 
> 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 
> Signed-off-by: Srinivas Kandagatla 
> ---
>  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(>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)
> +{
> 

Re: [alsa-devel] [Patch v6 1/7] slimbus: Device management on SLIMbus

[Charles Keepax]

On Fri, Oct 06, 2017 at 05:51:30PM +0200, srinivas.kandaga...@linaro.org wrote:
> From: Sagar Dharia 
> 
> 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 
> Signed-off-by: Srinivas Kandagatla 
> ---
>  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(+)
>  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
>  create mode 100644 include/linux/slimbus.h
> 
> diff --git a/Documentation/devicetree/bindings/slimbus/bus.txt 
> b/Documentation/devicetree/bindings/slimbus/bus.txt
> new file mode 100644
> index 000..cb658bb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/slimbus/bus.txt
> @@ -0,0 +1,57 @@
> +SLIM(Serial Low Power Interchip Media Bus) bus
> +
> +SLIMbus is a 2-wire bus, and is used to communicate with peripheral
> +components like audio-codec.
> +
> +Controller is a normal device using binding for whatever bus it is
> +on (e.g. platform bus).
> +Required property for SLIMbus controller node:
> +- compatible - name of SLIMbus controller following generic names
> + recommended practice.
> +- #address-cells - should be 2
> +- #size-cells- should be 0
> +
> +No other properties are required in the SLIMbus controller bus node.
> +
> +Child nodes:
> +Every SLIMbus controller node can contain zero or more child nodes
> +representing slave devices on the bus. Every SLIMbus slave device is
> +uniquely determined by the enumeration address containing 4 fields:
> +Manufacturer ID, Product code, Device index, and Instance value for
> +the device.
> +If child node is not present and it is instantiated after device
> +discovery (slave device reporting itself present).
> +
> +In some cases it may be necessary to describe non-probeable device
> +details such as non-standard ways of powering up a device. In
> +such cases, child nodes for those devices will be present as
> +slaves of the slimbus-controller, as detailed below.
> +
> +Required property for SLIMbus child node if it is present:
> +- reg- Is Duplex (Device index, Instance ID) from Enumeration
> +   Address.
> +   Device Index Uniquely identifies multiple Devices within
> +   a single Component.
> +   Instance ID Is for the cases where multiple Devices of the
> +   same type or Class are attached to the bus.
> +
> +- compatible -"slimMID,PID". The textual representation of Manufacturer ID,
> +   Product Code, shall be in lower case hexadecimal with leading
> +   zeroes suppressed

This does sort of make sense but kinda makes the code a bit ugly
parsing the MID and PID. Some parts will support SLIMBus and also
other control interfaces, which means they would need to add an
additional compatible string just for SLIMBus. It also breaks
the normal conventions of vendor,part and finally it makes the
compatible strings really unreadable which will be a bit annoying
when looking at DTs.

I think the MID and PID should just be included in the reg field
and just leave this as a standard compatible.

> +/**

This doesn't appear to be a kernel doc comment, so only /*.

> + * 

Re: [alsa-devel] [Patch v6 1/7] slimbus: Device management on SLIMbus

[Charles Keepax]

On Fri, Oct 06, 2017 at 05:51:30PM +0200, srinivas.kandaga...@linaro.org wrote:
> From: Sagar Dharia 
> 
> 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 
> Signed-off-by: Srinivas Kandagatla 
> ---
>  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(+)
>  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
>  create mode 100644 include/linux/slimbus.h
> 
> diff --git a/Documentation/devicetree/bindings/slimbus/bus.txt 
> b/Documentation/devicetree/bindings/slimbus/bus.txt
> new file mode 100644
> index 000..cb658bb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/slimbus/bus.txt
> @@ -0,0 +1,57 @@
> +SLIM(Serial Low Power Interchip Media Bus) bus
> +
> +SLIMbus is a 2-wire bus, and is used to communicate with peripheral
> +components like audio-codec.
> +
> +Controller is a normal device using binding for whatever bus it is
> +on (e.g. platform bus).
> +Required property for SLIMbus controller node:
> +- compatible - name of SLIMbus controller following generic names
> + recommended practice.
> +- #address-cells - should be 2
> +- #size-cells- should be 0
> +
> +No other properties are required in the SLIMbus controller bus node.
> +
> +Child nodes:
> +Every SLIMbus controller node can contain zero or more child nodes
> +representing slave devices on the bus. Every SLIMbus slave device is
> +uniquely determined by the enumeration address containing 4 fields:
> +Manufacturer ID, Product code, Device index, and Instance value for
> +the device.
> +If child node is not present and it is instantiated after device
> +discovery (slave device reporting itself present).
> +
> +In some cases it may be necessary to describe non-probeable device
> +details such as non-standard ways of powering up a device. In
> +such cases, child nodes for those devices will be present as
> +slaves of the slimbus-controller, as detailed below.
> +
> +Required property for SLIMbus child node if it is present:
> +- reg- Is Duplex (Device index, Instance ID) from Enumeration
> +   Address.
> +   Device Index Uniquely identifies multiple Devices within
> +   a single Component.
> +   Instance ID Is for the cases where multiple Devices of the
> +   same type or Class are attached to the bus.
> +
> +- compatible -"slimMID,PID". The textual representation of Manufacturer ID,
> +   Product Code, shall be in lower case hexadecimal with leading
> +   zeroes suppressed

This does sort of make sense but kinda makes the code a bit ugly
parsing the MID and PID. Some parts will support SLIMBus and also
other control interfaces, which means they would need to add an
additional compatible string just for SLIMBus. It also breaks
the normal conventions of vendor,part and finally it makes the
compatible strings really unreadable which will be a bit annoying
when looking at DTs.

I think the MID and PID should just be included in the reg field
and just leave this as a standard compatible.

> +/**

This doesn't appear to be a kernel doc comment, so only /*.

> + * Report callbacks(device_up, device_down) are implemented by 
> slimbus-devices.

Re: [Patch v6 1/7] slimbus: Device management on SLIMbus

[Srinivas Kandagatla]

Thanks for the comments.

On 07/10/17 05:14, Jonathan Neuschäfer wrote:

Hi, I have some more or less trivial comments below.

On Fri, Oct 06, 2017 at 05:51:30PM +0200, srinivas.kandaga...@linaro.org wrote:

From: Sagar Dharia 

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 
Signed-off-by: Srinivas Kandagatla 
---

[...]

+SLIMbus example for Qualcomm's slimbus manager component:
+
+   slim@2808 {
+   compatible = "qcom,slim-msm";
+   reg = <0x2808 0x2000>,
+   interrupts = <0 33 0>;
+   clocks = < SLIMBUS_SRC>, < AUDIO_SLIMBUS_CLK>;
+   clock-names = "iface_clk", "core_clk";
+   #address-cells = <2>;
+   #size-cells = <0>;
+
+   codec: wcd9310@1{
+   compatible = "slim217,60"";

^ spurious quote?


+   reg = <1 0>;
+   };
+   };
diff --git a/Documentation/slimbus/summary b/Documentation/slimbus/summary
new file mode 100644
index 000..e7f90bb
--- /dev/null
+++ b/Documentation/slimbus/summary


Should this file have a .rst extension, like other Restructured Text
files?


Will try to sort this out in next version.




@@ -0,0 +1,109 @@
+Overview of Linux kernel SLIMbus support
+

[...]

+Device notifications to the driver:
+---
+Since SLIMbus devices have mechanisms for reporting their presence, the
+framework allows drivers to bind when corresponding devices report their
+presence on the bus.
+However, it is possible that the driver needs to be probed
+first so that it can enable corresponding SLIMbus devie (e.g. power it up 
and/or


s/devie/device/ I guess


+take it out of reset). To support that behavior, the framework allows drivers
+to probe first as well  (e.g. using standard DeviceTree compatbility field).
+This creates the necessity for the driver to know when the device is functional
+(i.e. reported present). device_up callback is used for that reason when the
+device reports present and is assigned a logical address by the controller.

[...]

+/**
+ * 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 {
+   boolvalid;
+   struct slim_eaddr   eaddr;
+   u8  laddr;
+};


I wonder if valid should be moved after eaddr, to reduce the need for
padding. AFAICS, struct slim_eaddr is 6 bytes long and requires 2-byte
alignment, so if valid is one byte long, there would be one byte of
padding after it, slightly bloating struct slim_addrt, unnecessarily.

Makes sense!!




+/**
+ * struct slim_controller: Controls every instance of SLIMbus
+ * (similar to 'master' on SPI)
+ * 'Manager device' is responsible for  device management, bandwidth
+ * allocation, channel setup, and port associations per channel.
+ * Device management means Logical address assignment/removal based on
+ * enumeration (report-present, report-absent) if a device.





s/if a device/of a device/ ?


Yep, will fix this in next version.



+ * Bandwidth allocation is done dynamically by the manager based on active
+ * channels on the bus, message-bandwidth requests made by slimbus devices.
+ * Based on current bandwidth usage, manager chooses a frequency to run
+ * the bus at (in steps of 'clock-gear', 1 through 10, each 

Re: [Patch v6 1/7] slimbus: Device management on SLIMbus

[Srinivas Kandagatla]

Thanks for the comments.

On 07/10/17 05:14, Jonathan Neuschäfer wrote:

Hi, I have some more or less trivial comments below.

On Fri, Oct 06, 2017 at 05:51:30PM +0200, srinivas.kandaga...@linaro.org wrote:

From: Sagar Dharia 

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 
Signed-off-by: Srinivas Kandagatla 
---

[...]

+SLIMbus example for Qualcomm's slimbus manager component:
+
+   slim@2808 {
+   compatible = "qcom,slim-msm";
+   reg = <0x2808 0x2000>,
+   interrupts = <0 33 0>;
+   clocks = < SLIMBUS_SRC>, < AUDIO_SLIMBUS_CLK>;
+   clock-names = "iface_clk", "core_clk";
+   #address-cells = <2>;
+   #size-cells = <0>;
+
+   codec: wcd9310@1{
+   compatible = "slim217,60"";

^ spurious quote?


+   reg = <1 0>;
+   };
+   };
diff --git a/Documentation/slimbus/summary b/Documentation/slimbus/summary
new file mode 100644
index 000..e7f90bb
--- /dev/null
+++ b/Documentation/slimbus/summary


Should this file have a .rst extension, like other Restructured Text
files?


Will try to sort this out in next version.




@@ -0,0 +1,109 @@
+Overview of Linux kernel SLIMbus support
+

[...]

+Device notifications to the driver:
+---
+Since SLIMbus devices have mechanisms for reporting their presence, the
+framework allows drivers to bind when corresponding devices report their
+presence on the bus.
+However, it is possible that the driver needs to be probed
+first so that it can enable corresponding SLIMbus devie (e.g. power it up 
and/or


s/devie/device/ I guess


+take it out of reset). To support that behavior, the framework allows drivers
+to probe first as well  (e.g. using standard DeviceTree compatbility field).
+This creates the necessity for the driver to know when the device is functional
+(i.e. reported present). device_up callback is used for that reason when the
+device reports present and is assigned a logical address by the controller.

[...]

+/**
+ * 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 {
+   boolvalid;
+   struct slim_eaddr   eaddr;
+   u8  laddr;
+};


I wonder if valid should be moved after eaddr, to reduce the need for
padding. AFAICS, struct slim_eaddr is 6 bytes long and requires 2-byte
alignment, so if valid is one byte long, there would be one byte of
padding after it, slightly bloating struct slim_addrt, unnecessarily.

Makes sense!!




+/**
+ * struct slim_controller: Controls every instance of SLIMbus
+ * (similar to 'master' on SPI)
+ * 'Manager device' is responsible for  device management, bandwidth
+ * allocation, channel setup, and port associations per channel.
+ * Device management means Logical address assignment/removal based on
+ * enumeration (report-present, report-absent) if a device.





s/if a device/of a device/ ?


Yep, will fix this in next version.



+ * Bandwidth allocation is done dynamically by the manager based on active
+ * channels on the bus, message-bandwidth requests made by slimbus devices.
+ * Based on current bandwidth usage, manager chooses a frequency to run
+ * the bus at (in steps of 'clock-gear', 1 through 10, each clock gear
+ * representing twice the frequency than the previous gear).
+ * 

Re: [Patch v6 1/7] slimbus: Device management on SLIMbus

[Jonathan Neuschäfer]

Hi, I have some more or less trivial comments below.

On Fri, Oct 06, 2017 at 05:51:30PM +0200, srinivas.kandaga...@linaro.org wrote:
> From: Sagar Dharia 
> 
> 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 
> Signed-off-by: Srinivas Kandagatla 
> ---
[...]
> +SLIMbus example for Qualcomm's slimbus manager component:
> +
> + slim@2808 {
> + compatible = "qcom,slim-msm";
> + reg = <0x2808 0x2000>,
> + interrupts = <0 33 0>;
> + clocks = < SLIMBUS_SRC>, < AUDIO_SLIMBUS_CLK>;
> + clock-names = "iface_clk", "core_clk";
> + #address-cells = <2>;
> + #size-cells = <0>;
> +
> + codec: wcd9310@1{
> + compatible = "slim217,60"";
^ spurious quote?

> + reg = <1 0>;
> + };
> + };
> diff --git a/Documentation/slimbus/summary b/Documentation/slimbus/summary
> new file mode 100644
> index 000..e7f90bb
> --- /dev/null
> +++ b/Documentation/slimbus/summary

Should this file have a .rst extension, like other Restructured Text
files?

> @@ -0,0 +1,109 @@
> +Overview of Linux kernel SLIMbus support
> +
[...]
> +Device notifications to the driver:
> +---
> +Since SLIMbus devices have mechanisms for reporting their presence, the
> +framework allows drivers to bind when corresponding devices report their
> +presence on the bus.
> +However, it is possible that the driver needs to be probed
> +first so that it can enable corresponding SLIMbus devie (e.g. power it up 
> and/or

s/devie/device/ I guess

> +take it out of reset). To support that behavior, the framework allows drivers
> +to probe first as well  (e.g. using standard DeviceTree compatbility field).
> +This creates the necessity for the driver to know when the device is 
> functional
> +(i.e. reported present). device_up callback is used for that reason when the
> +device reports present and is assigned a logical address by the controller.
[...]
> +/**
> + * 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 {
> + boolvalid;
> + struct slim_eaddr   eaddr;
> + u8  laddr;
> +};

I wonder if valid should be moved after eaddr, to reduce the need for
padding. AFAICS, struct slim_eaddr is 6 bytes long and requires 2-byte
alignment, so if valid is one byte long, there would be one byte of
padding after it, slightly bloating struct slim_addrt, unnecessarily.

> +/**
> + * struct slim_controller: Controls every instance of SLIMbus
> + *   (similar to 'master' on SPI)
> + *   'Manager device' is responsible for  device management, bandwidth
> + *   allocation, channel setup, and port associations per channel.
> + *   Device management means Logical address assignment/removal based on
> + *   enumeration (report-present, report-absent) if a device.

s/if a device/of a device/ ?

> + *   Bandwidth allocation is done dynamically by the manager based on active
> + *   channels on the bus, message-bandwidth requests made by slimbus devices.
> + *   Based on current bandwidth usage, manager chooses a frequency to run
> + *   the bus at (in steps of 'clock-gear', 1 through 10, each clock gear
> + *   representing twice the frequency than the previous 

Re: [Patch v6 1/7] slimbus: Device management on SLIMbus

[Jonathan Neuschäfer]

Hi, I have some more or less trivial comments below.

On Fri, Oct 06, 2017 at 05:51:30PM +0200, srinivas.kandaga...@linaro.org wrote:
> From: Sagar Dharia 
> 
> 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 
> Signed-off-by: Srinivas Kandagatla 
> ---
[...]
> +SLIMbus example for Qualcomm's slimbus manager component:
> +
> + slim@2808 {
> + compatible = "qcom,slim-msm";
> + reg = <0x2808 0x2000>,
> + interrupts = <0 33 0>;
> + clocks = < SLIMBUS_SRC>, < AUDIO_SLIMBUS_CLK>;
> + clock-names = "iface_clk", "core_clk";
> + #address-cells = <2>;
> + #size-cells = <0>;
> +
> + codec: wcd9310@1{
> + compatible = "slim217,60"";
^ spurious quote?

> + reg = <1 0>;
> + };
> + };
> diff --git a/Documentation/slimbus/summary b/Documentation/slimbus/summary
> new file mode 100644
> index 000..e7f90bb
> --- /dev/null
> +++ b/Documentation/slimbus/summary

Should this file have a .rst extension, like other Restructured Text
files?

> @@ -0,0 +1,109 @@
> +Overview of Linux kernel SLIMbus support
> +
[...]
> +Device notifications to the driver:
> +---
> +Since SLIMbus devices have mechanisms for reporting their presence, the
> +framework allows drivers to bind when corresponding devices report their
> +presence on the bus.
> +However, it is possible that the driver needs to be probed
> +first so that it can enable corresponding SLIMbus devie (e.g. power it up 
> and/or

s/devie/device/ I guess

> +take it out of reset). To support that behavior, the framework allows drivers
> +to probe first as well  (e.g. using standard DeviceTree compatbility field).
> +This creates the necessity for the driver to know when the device is 
> functional
> +(i.e. reported present). device_up callback is used for that reason when the
> +device reports present and is assigned a logical address by the controller.
[...]
> +/**
> + * 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 {
> + boolvalid;
> + struct slim_eaddr   eaddr;
> + u8  laddr;
> +};

I wonder if valid should be moved after eaddr, to reduce the need for
padding. AFAICS, struct slim_eaddr is 6 bytes long and requires 2-byte
alignment, so if valid is one byte long, there would be one byte of
padding after it, slightly bloating struct slim_addrt, unnecessarily.

> +/**
> + * struct slim_controller: Controls every instance of SLIMbus
> + *   (similar to 'master' on SPI)
> + *   'Manager device' is responsible for  device management, bandwidth
> + *   allocation, channel setup, and port associations per channel.
> + *   Device management means Logical address assignment/removal based on
> + *   enumeration (report-present, report-absent) if a device.

s/if a device/of a device/ ?

> + *   Bandwidth allocation is done dynamically by the manager based on active
> + *   channels on the bus, message-bandwidth requests made by slimbus devices.
> + *   Based on current bandwidth usage, manager chooses a frequency to run
> + *   the bus at (in steps of 'clock-gear', 1 through 10, each clock gear
> + *   representing twice the frequency than the previous gear).
> + *   Manager is also responsible for entering (and exiting) 

[Patch v6 1/7] slimbus: Device management on SLIMbus

[srinivas . kandagatla]

From: Sagar Dharia 

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 
Signed-off-by: Srinivas Kandagatla 
---
 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(+)
 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
 create mode 100644 include/linux/slimbus.h

diff --git a/Documentation/devicetree/bindings/slimbus/bus.txt 
b/Documentation/devicetree/bindings/slimbus/bus.txt
new file mode 100644
index 000..cb658bb
--- /dev/null
+++ b/Documentation/devicetree/bindings/slimbus/bus.txt
@@ -0,0 +1,57 @@
+SLIM(Serial Low Power Interchip Media Bus) bus
+
+SLIMbus is a 2-wire bus, and is used to communicate with peripheral
+components like audio-codec.
+
+Controller is a normal device using binding for whatever bus it is
+on (e.g. platform bus).
+Required property for SLIMbus controller node:
+- compatible   - name of SLIMbus controller following generic names
+   recommended practice.
+- #address-cells - should be 2
+- #size-cells  - should be 0
+
+No other properties are required in the SLIMbus controller bus node.
+
+Child nodes:
+Every SLIMbus controller node can contain zero or more child nodes
+representing slave devices on the bus. Every SLIMbus slave device is
+uniquely determined by the enumeration address containing 4 fields:
+Manufacturer ID, Product code, Device index, and Instance value for
+the device.
+If child node is not present and it is instantiated after device
+discovery (slave device reporting itself present).
+
+In some cases it may be necessary to describe non-probeable device
+details such as non-standard ways of powering up a device. In
+such cases, child nodes for those devices will be present as
+slaves of the slimbus-controller, as detailed below.
+
+Required property for SLIMbus child node if it is present:
+- reg  - Is Duplex (Device index, Instance ID) from Enumeration
+ Address.
+ Device Index Uniquely identifies multiple Devices within
+ a single Component.
+ Instance ID Is for the cases where multiple Devices of the
+ same type or Class are attached to the bus.
+
+- compatible   -"slimMID,PID". The textual representation of Manufacturer ID,
+ Product Code, shall be in lower case hexadecimal with leading
+ zeroes suppressed
+
+SLIMbus example for Qualcomm's slimbus manager component:
+
+   slim@2808 {
+   compatible = "qcom,slim-msm";
+   reg = <0x2808 0x2000>,
+   interrupts = <0 33 0>;
+   clocks = < SLIMBUS_SRC>, < AUDIO_SLIMBUS_CLK>;
+   clock-names = "iface_clk", "core_clk";
+   #address-cells = <2>;
+   #size-cells = <0>;
+
+   codec: wcd9310@1{
+   compatible = "slim217,60"";
+   reg = <1 0>;
+   };
+   };
diff --git a/Documentation/slimbus/summary b/Documentation/slimbus/summary
new file mode 100644
index 000..e7f90bb
--- /dev/null
+++ b/Documentation/slimbus/summary
@@ -0,0 +1,109 @@
+Overview of Linux kernel SLIMbus support
+
+
+What is 

[Patch v6 1/7] slimbus: Device management on SLIMbus

[srinivas . kandagatla]

From: Sagar Dharia 

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 
Signed-off-by: Srinivas Kandagatla 
---
 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(+)
 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
 create mode 100644 include/linux/slimbus.h

diff --git a/Documentation/devicetree/bindings/slimbus/bus.txt 
b/Documentation/devicetree/bindings/slimbus/bus.txt
new file mode 100644
index 000..cb658bb
--- /dev/null
+++ b/Documentation/devicetree/bindings/slimbus/bus.txt
@@ -0,0 +1,57 @@
+SLIM(Serial Low Power Interchip Media Bus) bus
+
+SLIMbus is a 2-wire bus, and is used to communicate with peripheral
+components like audio-codec.
+
+Controller is a normal device using binding for whatever bus it is
+on (e.g. platform bus).
+Required property for SLIMbus controller node:
+- compatible   - name of SLIMbus controller following generic names
+   recommended practice.
+- #address-cells - should be 2
+- #size-cells  - should be 0
+
+No other properties are required in the SLIMbus controller bus node.
+
+Child nodes:
+Every SLIMbus controller node can contain zero or more child nodes
+representing slave devices on the bus. Every SLIMbus slave device is
+uniquely determined by the enumeration address containing 4 fields:
+Manufacturer ID, Product code, Device index, and Instance value for
+the device.
+If child node is not present and it is instantiated after device
+discovery (slave device reporting itself present).
+
+In some cases it may be necessary to describe non-probeable device
+details such as non-standard ways of powering up a device. In
+such cases, child nodes for those devices will be present as
+slaves of the slimbus-controller, as detailed below.
+
+Required property for SLIMbus child node if it is present:
+- reg  - Is Duplex (Device index, Instance ID) from Enumeration
+ Address.
+ Device Index Uniquely identifies multiple Devices within
+ a single Component.
+ Instance ID Is for the cases where multiple Devices of the
+ same type or Class are attached to the bus.
+
+- compatible   -"slimMID,PID". The textual representation of Manufacturer ID,
+ Product Code, shall be in lower case hexadecimal with leading
+ zeroes suppressed
+
+SLIMbus example for Qualcomm's slimbus manager component:
+
+   slim@2808 {
+   compatible = "qcom,slim-msm";
+   reg = <0x2808 0x2000>,
+   interrupts = <0 33 0>;
+   clocks = < SLIMBUS_SRC>, < AUDIO_SLIMBUS_CLK>;
+   clock-names = "iface_clk", "core_clk";
+   #address-cells = <2>;
+   #size-cells = <0>;
+
+   codec: wcd9310@1{
+   compatible = "slim217,60"";
+   reg = <1 0>;
+   };
+   };
diff --git a/Documentation/slimbus/summary b/Documentation/slimbus/summary
new file mode 100644
index 000..e7f90bb
--- /dev/null
+++ b/Documentation/slimbus/summary
@@ -0,0 +1,109 @@
+Overview of Linux kernel SLIMbus support
+
+
+What is SLIMbus?
+
+SLIMbus (Serial Low Power Interchip Media Bus) is a