Re: [PATCH v7 04/13] slimbus: core: Add slim controllers support

2017-11-26 Thread Vinod Koul 
On Mon, Nov 20, 2017 at 06:47:58AM +, Srinivas Kandagatla wrote:
> >>>thanks for the comments.
> >>>
> >>>
> >>>On 16/11/17 16:42, Vinod Koul wrote:
> On Wed, Nov 15, 2017 at 02:10:34PM +,srinivas.kandaga...@linaro.org  
> wrote:
> 
> >+static void slim_dev_release(struct device *dev)
> >+{
> >+struct slim_device *sbdev = to_slim_device(dev);
> >+
> >+put_device(sbdev->ctrl->dev);
> which device would that be?
> >>>This is controller device
> >>>
> >+static int slim_add_device(struct slim_controller *ctrl,
> >+   struct slim_device *sbdev,
> >+   struct device_node *node)
> >+{
> >+sbdev->dev.bus = _bus;
> >+sbdev->dev.parent = ctrl->dev;
> >+sbdev->dev.release = slim_dev_release;
> >+sbdev->dev.driver = NULL;
> >+sbdev->ctrl = ctrl;
> >+
> >+dev_set_name(>dev, "%x:%x:%x:%x",
> >+  sbdev->e_addr.manf_id,
> >+  sbdev->e_addr.prod_code,
> >+  sbdev->e_addr.dev_index,
> >+  sbdev->e_addr.instance);
> >+
> >+get_device(ctrl->dev);
> is this controller device and you ensuring it doesnt go away while you 
> have
> slaves on it?
> >>>Yes.
> >>I thought since you are marking ctrl->dev as parent, the device core should
> >>ensure that parent doesn't go off when you have child device?
> >>
> >>Greg, is that understanding correct, if so we may not need these calls.
> >That understanding should be correct, as the reference count is
> >incremented on the parent when a child is added.
> >
> >It would be trivial for this to be tested, and yes, I am pretty sure you
> >don't need this call.
> 
> Thanks for suggestion, I will remove this in next version.

I think it might be helpful to test the assumption as Greg noted :)

-- 
~Vinod

Re: [PATCH v7 04/13] slimbus: core: Add slim controllers support

2017-11-26 Thread Vinod Koul 
On Mon, Nov 20, 2017 at 06:47:58AM +, Srinivas Kandagatla wrote:
> >>>thanks for the comments.
> >>>
> >>>
> >>>On 16/11/17 16:42, Vinod Koul wrote:
> On Wed, Nov 15, 2017 at 02:10:34PM +,srinivas.kandaga...@linaro.org  
> wrote:
> 
> >+static void slim_dev_release(struct device *dev)
> >+{
> >+struct slim_device *sbdev = to_slim_device(dev);
> >+
> >+put_device(sbdev->ctrl->dev);
> which device would that be?
> >>>This is controller device
> >>>
> >+static int slim_add_device(struct slim_controller *ctrl,
> >+   struct slim_device *sbdev,
> >+   struct device_node *node)
> >+{
> >+sbdev->dev.bus = _bus;
> >+sbdev->dev.parent = ctrl->dev;
> >+sbdev->dev.release = slim_dev_release;
> >+sbdev->dev.driver = NULL;
> >+sbdev->ctrl = ctrl;
> >+
> >+dev_set_name(>dev, "%x:%x:%x:%x",
> >+  sbdev->e_addr.manf_id,
> >+  sbdev->e_addr.prod_code,
> >+  sbdev->e_addr.dev_index,
> >+  sbdev->e_addr.instance);
> >+
> >+get_device(ctrl->dev);
> is this controller device and you ensuring it doesnt go away while you 
> have
> slaves on it?
> >>>Yes.
> >>I thought since you are marking ctrl->dev as parent, the device core should
> >>ensure that parent doesn't go off when you have child device?
> >>
> >>Greg, is that understanding correct, if so we may not need these calls.
> >That understanding should be correct, as the reference count is
> >incremented on the parent when a child is added.
> >
> >It would be trivial for this to be tested, and yes, I am pretty sure you
> >don't need this call.
> 
> Thanks for suggestion, I will remove this in next version.

I think it might be helpful to test the assumption as Greg noted :)

-- 
~Vinod

Re: [PATCH v7 04/13] slimbus: core: Add slim controllers support

2017-11-19 Thread Srinivas Kandagatla 



On 17/11/17 08:13, Greg KH wrote:

On Fri, Nov 17, 2017 at 10:12:22AM +0530, Vinod Koul wrote:

On Thu, Nov 16, 2017 at 05:29:35PM +, Srinivas Kandagatla wrote:

thanks for the comments.


On 16/11/17 16:42, Vinod Koul wrote:

On Wed, Nov 15, 2017 at 02:10:34PM +,srinivas.kandaga...@linaro.org  wrote:


+static void slim_dev_release(struct device *dev)
+{
+   struct slim_device *sbdev = to_slim_device(dev);
+
+   put_device(sbdev->ctrl->dev);

which device would that be?

This is controller device


+static int slim_add_device(struct slim_controller *ctrl,
+  struct slim_device *sbdev,
+  struct device_node *node)
+{
+   sbdev->dev.bus = _bus;
+   sbdev->dev.parent = ctrl->dev;
+   sbdev->dev.release = slim_dev_release;
+   sbdev->dev.driver = NULL;
+   sbdev->ctrl = ctrl;
+
+   dev_set_name(>dev, "%x:%x:%x:%x",
+ sbdev->e_addr.manf_id,
+ sbdev->e_addr.prod_code,
+ sbdev->e_addr.dev_index,
+ sbdev->e_addr.instance);
+
+   get_device(ctrl->dev);

is this controller device and you ensuring it doesnt go away while you have
slaves on it?

Yes.

I thought since you are marking ctrl->dev as parent, the device core should
ensure that parent doesn't go off when you have child device?

Greg, is that understanding correct, if so we may not need these calls.

That understanding should be correct, as the reference count is
incremented on the parent when a child is added.

It would be trivial for this to be tested, and yes, I am pretty sure you
don't need this call.



Thanks for suggestion, I will remove this in next version.


thanks,
srini

Re: [PATCH v7 04/13] slimbus: core: Add slim controllers support

2017-11-19 Thread Srinivas Kandagatla 



On 17/11/17 08:13, Greg KH wrote:

On Fri, Nov 17, 2017 at 10:12:22AM +0530, Vinod Koul wrote:

On Thu, Nov 16, 2017 at 05:29:35PM +, Srinivas Kandagatla wrote:

thanks for the comments.


On 16/11/17 16:42, Vinod Koul wrote:

On Wed, Nov 15, 2017 at 02:10:34PM +,srinivas.kandaga...@linaro.org  wrote:


+static void slim_dev_release(struct device *dev)
+{
+   struct slim_device *sbdev = to_slim_device(dev);
+
+   put_device(sbdev->ctrl->dev);

which device would that be?

This is controller device


+static int slim_add_device(struct slim_controller *ctrl,
+  struct slim_device *sbdev,
+  struct device_node *node)
+{
+   sbdev->dev.bus = _bus;
+   sbdev->dev.parent = ctrl->dev;
+   sbdev->dev.release = slim_dev_release;
+   sbdev->dev.driver = NULL;
+   sbdev->ctrl = ctrl;
+
+   dev_set_name(>dev, "%x:%x:%x:%x",
+ sbdev->e_addr.manf_id,
+ sbdev->e_addr.prod_code,
+ sbdev->e_addr.dev_index,
+ sbdev->e_addr.instance);
+
+   get_device(ctrl->dev);

is this controller device and you ensuring it doesnt go away while you have
slaves on it?

Yes.

I thought since you are marking ctrl->dev as parent, the device core should
ensure that parent doesn't go off when you have child device?

Greg, is that understanding correct, if so we may not need these calls.

That understanding should be correct, as the reference count is
incremented on the parent when a child is added.

It would be trivial for this to be tested, and yes, I am pretty sure you
don't need this call.



Thanks for suggestion, I will remove this in next version.


thanks,
srini

Re: [PATCH v7 04/13] slimbus: core: Add slim controllers support

2017-11-17 Thread Greg KH 
On Fri, Nov 17, 2017 at 10:12:22AM +0530, Vinod Koul wrote:
> On Thu, Nov 16, 2017 at 05:29:35PM +, Srinivas Kandagatla wrote:
> > thanks for the comments.
> > 
> > 
> > On 16/11/17 16:42, Vinod Koul wrote:
> > >On Wed, Nov 15, 2017 at 02:10:34PM +, srinivas.kandaga...@linaro.org 
> > >wrote:
> > >
> > >>+static void slim_dev_release(struct device *dev)
> > >>+{
> > >>+ struct slim_device *sbdev = to_slim_device(dev);
> > >>+
> > >>+ put_device(sbdev->ctrl->dev);
> > >
> > >which device would that be?
> > This is controller device
> > 
> > >
> > >>+static int slim_add_device(struct slim_controller *ctrl,
> > >>+struct slim_device *sbdev,
> > >>+struct device_node *node)
> > >>+{
> > >>+ sbdev->dev.bus = _bus;
> > >>+ sbdev->dev.parent = ctrl->dev;
> > >>+ sbdev->dev.release = slim_dev_release;
> > >>+ sbdev->dev.driver = NULL;
> > >>+ sbdev->ctrl = ctrl;
> > >>+
> > >>+ dev_set_name(>dev, "%x:%x:%x:%x",
> > >>+   sbdev->e_addr.manf_id,
> > >>+   sbdev->e_addr.prod_code,
> > >>+   sbdev->e_addr.dev_index,
> > >>+   sbdev->e_addr.instance);
> > >>+
> > >>+ get_device(ctrl->dev);
> > >
> > >is this controller device and you ensuring it doesnt go away while you have
> > >slaves on it?
> > 
> > Yes.
> 
> I thought since you are marking ctrl->dev as parent, the device core should
> ensure that parent doesn't go off when you have child device?
> 
> Greg, is that understanding correct, if so we may not need these calls.

That understanding should be correct, as the reference count is
incremented on the parent when a child is added.

It would be trivial for this to be tested, and yes, I am pretty sure you
don't need this call.

thanks,

greg k-h

Re: [PATCH v7 04/13] slimbus: core: Add slim controllers support

2017-11-17 Thread Greg KH 
On Fri, Nov 17, 2017 at 10:12:22AM +0530, Vinod Koul wrote:
> On Thu, Nov 16, 2017 at 05:29:35PM +, Srinivas Kandagatla wrote:
> > thanks for the comments.
> > 
> > 
> > On 16/11/17 16:42, Vinod Koul wrote:
> > >On Wed, Nov 15, 2017 at 02:10:34PM +, srinivas.kandaga...@linaro.org 
> > >wrote:
> > >
> > >>+static void slim_dev_release(struct device *dev)
> > >>+{
> > >>+ struct slim_device *sbdev = to_slim_device(dev);
> > >>+
> > >>+ put_device(sbdev->ctrl->dev);
> > >
> > >which device would that be?
> > This is controller device
> > 
> > >
> > >>+static int slim_add_device(struct slim_controller *ctrl,
> > >>+struct slim_device *sbdev,
> > >>+struct device_node *node)
> > >>+{
> > >>+ sbdev->dev.bus = _bus;
> > >>+ sbdev->dev.parent = ctrl->dev;
> > >>+ sbdev->dev.release = slim_dev_release;
> > >>+ sbdev->dev.driver = NULL;
> > >>+ sbdev->ctrl = ctrl;
> > >>+
> > >>+ dev_set_name(>dev, "%x:%x:%x:%x",
> > >>+   sbdev->e_addr.manf_id,
> > >>+   sbdev->e_addr.prod_code,
> > >>+   sbdev->e_addr.dev_index,
> > >>+   sbdev->e_addr.instance);
> > >>+
> > >>+ get_device(ctrl->dev);
> > >
> > >is this controller device and you ensuring it doesnt go away while you have
> > >slaves on it?
> > 
> > Yes.
> 
> I thought since you are marking ctrl->dev as parent, the device core should
> ensure that parent doesn't go off when you have child device?
> 
> Greg, is that understanding correct, if so we may not need these calls.

That understanding should be correct, as the reference count is
incremented on the parent when a child is added.

It would be trivial for this to be tested, and yes, I am pretty sure you
don't need this call.

thanks,

greg k-h

Re: [PATCH v7 04/13] slimbus: core: Add slim controllers support

2017-11-16 Thread Vinod Koul 
On Thu, Nov 16, 2017 at 05:29:35PM +, Srinivas Kandagatla wrote:
> thanks for the comments.
> 
> 
> On 16/11/17 16:42, Vinod Koul wrote:
> >On Wed, Nov 15, 2017 at 02:10:34PM +, srinivas.kandaga...@linaro.org 
> >wrote:
> >
> >>+static void slim_dev_release(struct device *dev)
> >>+{
> >>+   struct slim_device *sbdev = to_slim_device(dev);
> >>+
> >>+   put_device(sbdev->ctrl->dev);
> >
> >which device would that be?
> This is controller device
> 
> >
> >>+static int slim_add_device(struct slim_controller *ctrl,
> >>+  struct slim_device *sbdev,
> >>+  struct device_node *node)
> >>+{
> >>+   sbdev->dev.bus = _bus;
> >>+   sbdev->dev.parent = ctrl->dev;
> >>+   sbdev->dev.release = slim_dev_release;
> >>+   sbdev->dev.driver = NULL;
> >>+   sbdev->ctrl = ctrl;
> >>+
> >>+   dev_set_name(>dev, "%x:%x:%x:%x",
> >>+ sbdev->e_addr.manf_id,
> >>+ sbdev->e_addr.prod_code,
> >>+ sbdev->e_addr.dev_index,
> >>+ sbdev->e_addr.instance);
> >>+
> >>+   get_device(ctrl->dev);
> >
> >is this controller device and you ensuring it doesnt go away while you have
> >slaves on it?
> 
> Yes.

I thought since you are marking ctrl->dev as parent, the device core should
ensure that parent doesn't go off when you have child device?

Greg, is that understanding correct, if so we may not need these calls.

-- 
~Vinod

Re: [PATCH v7 04/13] slimbus: core: Add slim controllers support

2017-11-16 Thread Vinod Koul 
On Thu, Nov 16, 2017 at 05:29:35PM +, Srinivas Kandagatla wrote:
> thanks for the comments.
> 
> 
> On 16/11/17 16:42, Vinod Koul wrote:
> >On Wed, Nov 15, 2017 at 02:10:34PM +, srinivas.kandaga...@linaro.org 
> >wrote:
> >
> >>+static void slim_dev_release(struct device *dev)
> >>+{
> >>+   struct slim_device *sbdev = to_slim_device(dev);
> >>+
> >>+   put_device(sbdev->ctrl->dev);
> >
> >which device would that be?
> This is controller device
> 
> >
> >>+static int slim_add_device(struct slim_controller *ctrl,
> >>+  struct slim_device *sbdev,
> >>+  struct device_node *node)
> >>+{
> >>+   sbdev->dev.bus = _bus;
> >>+   sbdev->dev.parent = ctrl->dev;
> >>+   sbdev->dev.release = slim_dev_release;
> >>+   sbdev->dev.driver = NULL;
> >>+   sbdev->ctrl = ctrl;
> >>+
> >>+   dev_set_name(>dev, "%x:%x:%x:%x",
> >>+ sbdev->e_addr.manf_id,
> >>+ sbdev->e_addr.prod_code,
> >>+ sbdev->e_addr.dev_index,
> >>+ sbdev->e_addr.instance);
> >>+
> >>+   get_device(ctrl->dev);
> >
> >is this controller device and you ensuring it doesnt go away while you have
> >slaves on it?
> 
> Yes.

I thought since you are marking ctrl->dev as parent, the device core should
ensure that parent doesn't go off when you have child device?

Greg, is that understanding correct, if so we may not need these calls.

-- 
~Vinod

Re: [PATCH v7 04/13] slimbus: core: Add slim controllers support

2017-11-16 Thread Srinivas Kandagatla 

thanks for the comments.


On 16/11/17 16:42, Vinod Koul wrote:

On Wed, Nov 15, 2017 at 02:10:34PM +, srinivas.kandaga...@linaro.org wrote:


+static void slim_dev_release(struct device *dev)
+{
+   struct slim_device *sbdev = to_slim_device(dev);
+
+   put_device(sbdev->ctrl->dev);


which device would that be?

This is controller device




+static int slim_add_device(struct slim_controller *ctrl,
+  struct slim_device *sbdev,
+  struct device_node *node)
+{
+   sbdev->dev.bus = _bus;
+   sbdev->dev.parent = ctrl->dev;
+   sbdev->dev.release = slim_dev_release;
+   sbdev->dev.driver = NULL;
+   sbdev->ctrl = ctrl;
+
+   dev_set_name(>dev, "%x:%x:%x:%x",
+ sbdev->e_addr.manf_id,
+ sbdev->e_addr.prod_code,
+ sbdev->e_addr.dev_index,
+ sbdev->e_addr.instance);
+
+   get_device(ctrl->dev);


is this controller device and you ensuring it doesnt go away while you have
slaves on it?


Yes.




+static struct slim_device *slim_alloc_device(struct slim_controller *ctrl,
+struct slim_eaddr *eaddr,
+struct device_node *node)
+{
+   struct slim_device *sbdev;
+   int ret;
+
+   sbdev = kzalloc(sizeof(struct slim_device), GFP_KERNEL);


Usual kernel way of doing is kzalloc(*sbdev)


I agree will fix it in next version.


+void slim_report_absent(struct slim_device *sbdev)
+{
+   struct slim_controller *ctrl = sbdev->ctrl;
+
+   if (!ctrl)
+   return;
+
+   /* invalidate logical addresses */
+   mutex_lock(>lock);
+   sbdev->is_laddr_valid = false;
+   mutex_unlock(>lock);
+
+   ida_simple_remove(>laddr_ida, sbdev->laddr);
+   slim_device_update_status(sbdev, SLIM_DEVICE_STATUS_DOWN);
+}
+EXPORT_SYMBOL_GPL(slim_report_absent);


Do you have APIs for report present too, if so why not add te status in
argument as you may have common handling
Yes, We do  have api for reporting too, I will give it a try to combine 
both.





+static int slim_device_alloc_laddr(struct slim_device *sbdev,
+  u8 *laddr, bool report_present)
+{
+   struct slim_controller *ctrl = sbdev->ctrl;
+   int ret;
+
+   mutex_lock(>lock);
+   if (ctrl->get_laddr) {
+   ret = ctrl->get_laddr(ctrl, >e_addr, laddr);
+   if (ret < 0)
+   goto err;
+   } else if (report_present) {
+   ret = ida_simple_get(>laddr_ida,
+0, SLIM_LA_MANAGER - 1, GFP_KERNEL);
+   if (ret < 0)
+   goto err;
+
+   *laddr = ret;
+   } else {
+   ret = -EINVAL;
+   goto err;
+   }
+
+   if (ctrl->set_laddr) {
+   ret = ctrl->set_laddr(ctrl, >e_addr, *laddr);
+   if (ret) {
+   ret = -EINVAL;
+   goto err;
+   }
+   }
+
+   sbdev->laddr = *laddr;


if you have this in sbdev, then why have this as an arg also?
Yes makes sens, laddr argument in this function is redundant, it can be 
removed totally.



+   sbdev->is_laddr_valid = true;


shouldn't non-zero value signify that?

0 is also a valid laddr.

Re: [PATCH v7 04/13] slimbus: core: Add slim controllers support

2017-11-16 Thread Srinivas Kandagatla 

thanks for the comments.


On 16/11/17 16:42, Vinod Koul wrote:

On Wed, Nov 15, 2017 at 02:10:34PM +, srinivas.kandaga...@linaro.org wrote:


+static void slim_dev_release(struct device *dev)
+{
+   struct slim_device *sbdev = to_slim_device(dev);
+
+   put_device(sbdev->ctrl->dev);


which device would that be?

This is controller device




+static int slim_add_device(struct slim_controller *ctrl,
+  struct slim_device *sbdev,
+  struct device_node *node)
+{
+   sbdev->dev.bus = _bus;
+   sbdev->dev.parent = ctrl->dev;
+   sbdev->dev.release = slim_dev_release;
+   sbdev->dev.driver = NULL;
+   sbdev->ctrl = ctrl;
+
+   dev_set_name(>dev, "%x:%x:%x:%x",
+ sbdev->e_addr.manf_id,
+ sbdev->e_addr.prod_code,
+ sbdev->e_addr.dev_index,
+ sbdev->e_addr.instance);
+
+   get_device(ctrl->dev);


is this controller device and you ensuring it doesnt go away while you have
slaves on it?


Yes.




+static struct slim_device *slim_alloc_device(struct slim_controller *ctrl,
+struct slim_eaddr *eaddr,
+struct device_node *node)
+{
+   struct slim_device *sbdev;
+   int ret;
+
+   sbdev = kzalloc(sizeof(struct slim_device), GFP_KERNEL);


Usual kernel way of doing is kzalloc(*sbdev)


I agree will fix it in next version.


+void slim_report_absent(struct slim_device *sbdev)
+{
+   struct slim_controller *ctrl = sbdev->ctrl;
+
+   if (!ctrl)
+   return;
+
+   /* invalidate logical addresses */
+   mutex_lock(>lock);
+   sbdev->is_laddr_valid = false;
+   mutex_unlock(>lock);
+
+   ida_simple_remove(>laddr_ida, sbdev->laddr);
+   slim_device_update_status(sbdev, SLIM_DEVICE_STATUS_DOWN);
+}
+EXPORT_SYMBOL_GPL(slim_report_absent);


Do you have APIs for report present too, if so why not add te status in
argument as you may have common handling
Yes, We do  have api for reporting too, I will give it a try to combine 
both.





+static int slim_device_alloc_laddr(struct slim_device *sbdev,
+  u8 *laddr, bool report_present)
+{
+   struct slim_controller *ctrl = sbdev->ctrl;
+   int ret;
+
+   mutex_lock(>lock);
+   if (ctrl->get_laddr) {
+   ret = ctrl->get_laddr(ctrl, >e_addr, laddr);
+   if (ret < 0)
+   goto err;
+   } else if (report_present) {
+   ret = ida_simple_get(>laddr_ida,
+0, SLIM_LA_MANAGER - 1, GFP_KERNEL);
+   if (ret < 0)
+   goto err;
+
+   *laddr = ret;
+   } else {
+   ret = -EINVAL;
+   goto err;
+   }
+
+   if (ctrl->set_laddr) {
+   ret = ctrl->set_laddr(ctrl, >e_addr, *laddr);
+   if (ret) {
+   ret = -EINVAL;
+   goto err;
+   }
+   }
+
+   sbdev->laddr = *laddr;


if you have this in sbdev, then why have this as an arg also?
Yes makes sens, laddr argument in this function is redundant, it can be 
removed totally.



+   sbdev->is_laddr_valid = true;


shouldn't non-zero value signify that?

0 is also a valid laddr.

Re: [PATCH v7 04/13] slimbus: core: Add slim controllers support

2017-11-16 Thread Vinod Koul 
On Wed, Nov 15, 2017 at 02:10:34PM +, srinivas.kandaga...@linaro.org wrote:

> +static void slim_dev_release(struct device *dev)
> +{
> + struct slim_device *sbdev = to_slim_device(dev);
> +
> + put_device(sbdev->ctrl->dev);

which device would that be?

> +static int slim_add_device(struct slim_controller *ctrl,
> +struct slim_device *sbdev,
> +struct device_node *node)
> +{
> + sbdev->dev.bus = _bus;
> + sbdev->dev.parent = ctrl->dev;
> + sbdev->dev.release = slim_dev_release;
> + sbdev->dev.driver = NULL;
> + sbdev->ctrl = ctrl;
> +
> + dev_set_name(>dev, "%x:%x:%x:%x",
> +   sbdev->e_addr.manf_id,
> +   sbdev->e_addr.prod_code,
> +   sbdev->e_addr.dev_index,
> +   sbdev->e_addr.instance);
> +
> + get_device(ctrl->dev);

is this controller device and you ensuring it doesnt go away while you have
slaves on it?

> +static struct slim_device *slim_alloc_device(struct slim_controller *ctrl,
> +  struct slim_eaddr *eaddr,
> +  struct device_node *node)
> +{
> + struct slim_device *sbdev;
> + int ret;
> +
> + sbdev = kzalloc(sizeof(struct slim_device), GFP_KERNEL);

Usual kernel way of doing is kzalloc(*sbdev)

> +void slim_report_absent(struct slim_device *sbdev)
> +{
> + struct slim_controller *ctrl = sbdev->ctrl;
> +
> + if (!ctrl)
> + return;
> +
> + /* invalidate logical addresses */
> + mutex_lock(>lock);
> + sbdev->is_laddr_valid = false;
> + mutex_unlock(>lock);
> +
> + ida_simple_remove(>laddr_ida, sbdev->laddr);
> + slim_device_update_status(sbdev, SLIM_DEVICE_STATUS_DOWN);
> +}
> +EXPORT_SYMBOL_GPL(slim_report_absent);

Do you have APIs for report present too, if so why not add te status in
argument as you may have common handling

> +static int slim_device_alloc_laddr(struct slim_device *sbdev,
> +u8 *laddr, bool report_present)
> +{
> + struct slim_controller *ctrl = sbdev->ctrl;
> + int ret;
> +
> + mutex_lock(>lock);
> + if (ctrl->get_laddr) {
> + ret = ctrl->get_laddr(ctrl, >e_addr, laddr);
> + if (ret < 0)
> + goto err;
> + } else if (report_present) {
> + ret = ida_simple_get(>laddr_ida,
> +  0, SLIM_LA_MANAGER - 1, GFP_KERNEL);
> + if (ret < 0)
> + goto err;
> +
> + *laddr = ret;
> + } else {
> + ret = -EINVAL;
> + goto err;
> + }
> +
> + if (ctrl->set_laddr) {
> + ret = ctrl->set_laddr(ctrl, >e_addr, *laddr);
> + if (ret) {
> + ret = -EINVAL;
> + goto err;
> + }
> + }
> +
> + sbdev->laddr = *laddr;

if you have this in sbdev, then why have this as an arg also?

> + sbdev->is_laddr_valid = true;

shouldn't non-zero value signify that?

-- 
~Vinod

Re: [PATCH v7 04/13] slimbus: core: Add slim controllers support

2017-11-16 Thread Vinod Koul 
On Wed, Nov 15, 2017 at 02:10:34PM +, srinivas.kandaga...@linaro.org wrote:

> +static void slim_dev_release(struct device *dev)
> +{
> + struct slim_device *sbdev = to_slim_device(dev);
> +
> + put_device(sbdev->ctrl->dev);

which device would that be?

> +static int slim_add_device(struct slim_controller *ctrl,
> +struct slim_device *sbdev,
> +struct device_node *node)
> +{
> + sbdev->dev.bus = _bus;
> + sbdev->dev.parent = ctrl->dev;
> + sbdev->dev.release = slim_dev_release;
> + sbdev->dev.driver = NULL;
> + sbdev->ctrl = ctrl;
> +
> + dev_set_name(>dev, "%x:%x:%x:%x",
> +   sbdev->e_addr.manf_id,
> +   sbdev->e_addr.prod_code,
> +   sbdev->e_addr.dev_index,
> +   sbdev->e_addr.instance);
> +
> + get_device(ctrl->dev);

is this controller device and you ensuring it doesnt go away while you have
slaves on it?

> +static struct slim_device *slim_alloc_device(struct slim_controller *ctrl,
> +  struct slim_eaddr *eaddr,
> +  struct device_node *node)
> +{
> + struct slim_device *sbdev;
> + int ret;
> +
> + sbdev = kzalloc(sizeof(struct slim_device), GFP_KERNEL);

Usual kernel way of doing is kzalloc(*sbdev)

> +void slim_report_absent(struct slim_device *sbdev)
> +{
> + struct slim_controller *ctrl = sbdev->ctrl;
> +
> + if (!ctrl)
> + return;
> +
> + /* invalidate logical addresses */
> + mutex_lock(>lock);
> + sbdev->is_laddr_valid = false;
> + mutex_unlock(>lock);
> +
> + ida_simple_remove(>laddr_ida, sbdev->laddr);
> + slim_device_update_status(sbdev, SLIM_DEVICE_STATUS_DOWN);
> +}
> +EXPORT_SYMBOL_GPL(slim_report_absent);

Do you have APIs for report present too, if so why not add te status in
argument as you may have common handling

> +static int slim_device_alloc_laddr(struct slim_device *sbdev,
> +u8 *laddr, bool report_present)
> +{
> + struct slim_controller *ctrl = sbdev->ctrl;
> + int ret;
> +
> + mutex_lock(>lock);
> + if (ctrl->get_laddr) {
> + ret = ctrl->get_laddr(ctrl, >e_addr, laddr);
> + if (ret < 0)
> + goto err;
> + } else if (report_present) {
> + ret = ida_simple_get(>laddr_ida,
> +  0, SLIM_LA_MANAGER - 1, GFP_KERNEL);
> + if (ret < 0)
> + goto err;
> +
> + *laddr = ret;
> + } else {
> + ret = -EINVAL;
> + goto err;
> + }
> +
> + if (ctrl->set_laddr) {
> + ret = ctrl->set_laddr(ctrl, >e_addr, *laddr);
> + if (ret) {
> + ret = -EINVAL;
> + goto err;
> + }
> + }
> +
> + sbdev->laddr = *laddr;

if you have this in sbdev, then why have this as an arg also?

> + sbdev->is_laddr_valid = true;

shouldn't non-zero value signify that?

-- 
~Vinod

[PATCH v7 04/13] slimbus: core: Add slim controllers support

2017-11-15 Thread srinivas . kandagatla 
From: Sagar Dharia 

This patch adds support to slim controllers in the slim core,
including some utility functions invoked by the controller and
slim device drivers.

Signed-off-by: Srinivas Kandagatla 
---
 drivers/slimbus/core.c| 312 ++
 drivers/slimbus/slimbus.h | 116 +
 include/linux/slimbus.h   |   7 ++
 3 files changed, 435 insertions(+)
 create mode 100644 drivers/slimbus/slimbus.h

diff --git a/drivers/slimbus/core.c b/drivers/slimbus/core.c
index 22edebf1a233..d31a9c4fe7e9 100644
--- a/drivers/slimbus/core.c
+++ b/drivers/slimbus/core.c
@@ -14,7 +14,11 @@
 #include 
 #include 
 #include 
+#include 
 #include 
+#include "slimbus.h"
+
+static DEFINE_IDA(ctrl_ida);
 
 static const struct slim_device_id *slim_match(const struct slim_device_id *id,
   const struct slim_device *sbdev)
@@ -106,6 +110,314 @@ void slim_driver_unregister(struct slim_driver *drv)
 }
 EXPORT_SYMBOL_GPL(slim_driver_unregister);
 
+static void slim_dev_release(struct device *dev)
+{
+   struct slim_device *sbdev = to_slim_device(dev);
+
+   put_device(sbdev->ctrl->dev);
+   kfree(sbdev);
+}
+
+static int slim_add_device(struct slim_controller *ctrl,
+  struct slim_device *sbdev,
+  struct device_node *node)
+{
+   sbdev->dev.bus = _bus;
+   sbdev->dev.parent = ctrl->dev;
+   sbdev->dev.release = slim_dev_release;
+   sbdev->dev.driver = NULL;
+   sbdev->ctrl = ctrl;
+
+   dev_set_name(>dev, "%x:%x:%x:%x",
+ sbdev->e_addr.manf_id,
+ sbdev->e_addr.prod_code,
+ sbdev->e_addr.dev_index,
+ sbdev->e_addr.instance);
+
+   get_device(ctrl->dev);
+
+   return device_register(>dev);
+}
+
+static struct slim_device *slim_alloc_device(struct slim_controller *ctrl,
+struct slim_eaddr *eaddr,
+struct device_node *node)
+{
+   struct slim_device *sbdev;
+   int ret;
+
+   sbdev = kzalloc(sizeof(struct slim_device), GFP_KERNEL);
+   if (!sbdev)
+   return NULL;
+
+   sbdev->e_addr = *eaddr;
+   ret = slim_add_device(ctrl, sbdev, node);
+   if (ret) {
+   kfree(sbdev);
+   return NULL;
+   }
+
+   return sbdev;
+}
+
+/*
+ * slim_register_controller() - Controller bring-up and registration.
+ *
+ * @ctrl: Controller to be registered.
+ *
+ * A controller is registered with the framework using this API.
+ * If devices on a controller were registered before controller,
+ * this will make sure that they get probed when controller is up
+ */
+int slim_register_controller(struct slim_controller *ctrl)
+{
+   int id;
+
+   id = ida_simple_get(_ida, 0, 0, GFP_KERNEL);
+   if (id < 0)
+   return id;
+
+   ctrl->id = id;
+
+   if (!ctrl->min_cg)
+   ctrl->min_cg = SLIM_MIN_CLK_GEAR;
+   if (!ctrl->max_cg)
+   ctrl->max_cg = SLIM_MAX_CLK_GEAR;
+
+   ida_init(>laddr_ida);
+   idr_init(>tid_idr);
+   mutex_init(>lock);
+
+   dev_dbg(ctrl->dev, "Bus [%s] registered:dev:%p\n",
+   ctrl->name, ctrl->dev);
+
+   return 0;
+}
+EXPORT_SYMBOL_GPL(slim_register_controller);
+
+/* slim_remove_device: Remove the effect of slim_add_device() */
+static void slim_remove_device(struct slim_device *sbdev)
+{
+   device_unregister(>dev);
+}
+
+static int slim_ctrl_remove_device(struct device *dev, void *null)
+{
+   slim_remove_device(to_slim_device(dev));
+   return 0;
+}
+
+/**
+ * slim_unregister_controller() - Controller tear-down.
+ *
+ * @ctrl: Controller to tear-down.
+ */
+int slim_unregister_controller(struct slim_controller *ctrl)
+{
+   /* Remove all clients */
+   device_for_each_child(ctrl->dev, NULL, slim_ctrl_remove_device);
+   ida_simple_remove(_ida, ctrl->id);
+
+   return 0;
+}
+EXPORT_SYMBOL_GPL(slim_unregister_controller);
+
+static void slim_device_update_status(struct slim_device *sbdev,
+ enum slim_device_status status)
+{
+   struct slim_driver *sbdrv;
+
+   if (sbdev->status == status)
+   return;
+
+   sbdev->status = status;
+   if (!sbdev->dev.driver)
+   return;
+
+   sbdrv = to_slim_driver(sbdev->dev.driver);
+   if (sbdrv->device_status)
+   sbdrv->device_status(sbdev, sbdev->status);
+}
+
+/**
+ * slim_report_absent() - Controller calls this function when a device
+ * reports absent, OR when the device cannot be communicated with
+ *
+ * @sbdev: Device that cannot be reached, or sent report absent
+ */
+void slim_report_absent(struct slim_device *sbdev)
+{
+   struct slim_controller

[PATCH v7 04/13] slimbus: core: Add slim controllers support

2017-11-15 Thread srinivas . kandagatla 
From: Sagar Dharia 

This patch adds support to slim controllers in the slim core,
including some utility functions invoked by the controller and
slim device drivers.

Signed-off-by: Srinivas Kandagatla 
---
 drivers/slimbus/core.c| 312 ++
 drivers/slimbus/slimbus.h | 116 +
 include/linux/slimbus.h   |   7 ++
 3 files changed, 435 insertions(+)
 create mode 100644 drivers/slimbus/slimbus.h

diff --git a/drivers/slimbus/core.c b/drivers/slimbus/core.c
index 22edebf1a233..d31a9c4fe7e9 100644
--- a/drivers/slimbus/core.c
+++ b/drivers/slimbus/core.c
@@ -14,7 +14,11 @@
 #include 
 #include 
 #include 
+#include 
 #include 
+#include "slimbus.h"
+
+static DEFINE_IDA(ctrl_ida);
 
 static const struct slim_device_id *slim_match(const struct slim_device_id *id,
   const struct slim_device *sbdev)
@@ -106,6 +110,314 @@ void slim_driver_unregister(struct slim_driver *drv)
 }
 EXPORT_SYMBOL_GPL(slim_driver_unregister);
 
+static void slim_dev_release(struct device *dev)
+{
+   struct slim_device *sbdev = to_slim_device(dev);
+
+   put_device(sbdev->ctrl->dev);
+   kfree(sbdev);
+}
+
+static int slim_add_device(struct slim_controller *ctrl,
+  struct slim_device *sbdev,
+  struct device_node *node)
+{
+   sbdev->dev.bus = _bus;
+   sbdev->dev.parent = ctrl->dev;
+   sbdev->dev.release = slim_dev_release;
+   sbdev->dev.driver = NULL;
+   sbdev->ctrl = ctrl;
+
+   dev_set_name(>dev, "%x:%x:%x:%x",
+ sbdev->e_addr.manf_id,
+ sbdev->e_addr.prod_code,
+ sbdev->e_addr.dev_index,
+ sbdev->e_addr.instance);
+
+   get_device(ctrl->dev);
+
+   return device_register(>dev);
+}
+
+static struct slim_device *slim_alloc_device(struct slim_controller *ctrl,
+struct slim_eaddr *eaddr,
+struct device_node *node)
+{
+   struct slim_device *sbdev;
+   int ret;
+
+   sbdev = kzalloc(sizeof(struct slim_device), GFP_KERNEL);
+   if (!sbdev)
+   return NULL;
+
+   sbdev->e_addr = *eaddr;
+   ret = slim_add_device(ctrl, sbdev, node);
+   if (ret) {
+   kfree(sbdev);
+   return NULL;
+   }
+
+   return sbdev;
+}
+
+/*
+ * slim_register_controller() - Controller bring-up and registration.
+ *
+ * @ctrl: Controller to be registered.
+ *
+ * A controller is registered with the framework using this API.
+ * If devices on a controller were registered before controller,
+ * this will make sure that they get probed when controller is up
+ */
+int slim_register_controller(struct slim_controller *ctrl)
+{
+   int id;
+
+   id = ida_simple_get(_ida, 0, 0, GFP_KERNEL);
+   if (id < 0)
+   return id;
+
+   ctrl->id = id;
+
+   if (!ctrl->min_cg)
+   ctrl->min_cg = SLIM_MIN_CLK_GEAR;
+   if (!ctrl->max_cg)
+   ctrl->max_cg = SLIM_MAX_CLK_GEAR;
+
+   ida_init(>laddr_ida);
+   idr_init(>tid_idr);
+   mutex_init(>lock);
+
+   dev_dbg(ctrl->dev, "Bus [%s] registered:dev:%p\n",
+   ctrl->name, ctrl->dev);
+
+   return 0;
+}
+EXPORT_SYMBOL_GPL(slim_register_controller);
+
+/* slim_remove_device: Remove the effect of slim_add_device() */
+static void slim_remove_device(struct slim_device *sbdev)
+{
+   device_unregister(>dev);
+}
+
+static int slim_ctrl_remove_device(struct device *dev, void *null)
+{
+   slim_remove_device(to_slim_device(dev));
+   return 0;
+}
+
+/**
+ * slim_unregister_controller() - Controller tear-down.
+ *
+ * @ctrl: Controller to tear-down.
+ */
+int slim_unregister_controller(struct slim_controller *ctrl)
+{
+   /* Remove all clients */
+   device_for_each_child(ctrl->dev, NULL, slim_ctrl_remove_device);
+   ida_simple_remove(_ida, ctrl->id);
+
+   return 0;
+}
+EXPORT_SYMBOL_GPL(slim_unregister_controller);
+
+static void slim_device_update_status(struct slim_device *sbdev,
+ enum slim_device_status status)
+{
+   struct slim_driver *sbdrv;
+
+   if (sbdev->status == status)
+   return;
+
+   sbdev->status = status;
+   if (!sbdev->dev.driver)
+   return;
+
+   sbdrv = to_slim_driver(sbdev->dev.driver);
+   if (sbdrv->device_status)
+   sbdrv->device_status(sbdev, sbdev->status);
+}
+
+/**
+ * slim_report_absent() - Controller calls this function when a device
+ * reports absent, OR when the device cannot be communicated with
+ *
+ * @sbdev: Device that cannot be reached, or sent report absent
+ */
+void slim_report_absent(struct slim_device *sbdev)
+{
+   struct slim_controller *ctrl = sbdev->ctrl;
+
+   if (!ctrl)
+