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.