lkml.org 
[lkml]   [2017]   [Nov]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v7 04/13] slimbus: core: Add slim controllers support
From
Date
thanks for the comments.


On 16/11/17 16:42, Vinod Koul wrote:
> On Wed, Nov 15, 2017 at 02:10:34PM +0000, srinivas.kandagatla@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 = &slimbus_bus;
>> + sbdev->dev.parent = ctrl->dev;
>> + sbdev->dev.release = slim_dev_release;
>> + sbdev->dev.driver = NULL;
>> + sbdev->ctrl = ctrl;
>> +
>> + dev_set_name(&sbdev->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(&ctrl->lock);
>> + sbdev->is_laddr_valid = false;
>> + mutex_unlock(&ctrl->lock);
>> +
>> + ida_simple_remove(&ctrl->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(&ctrl->lock);
>> + if (ctrl->get_laddr) {
>> + ret = ctrl->get_laddr(ctrl, &sbdev->e_addr, laddr);
>> + if (ret < 0)
>> + goto err;
>> + } else if (report_present) {
>> + ret = ida_simple_get(&ctrl->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, &sbdev->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.

>
\
 
 \ /
  Last update: 2017-11-17 17:18    [W:0.057 / U:5.732 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site