On Wed, Nov 15, 2017 at 02:10:34PM +0000, 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 = &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?
> +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(&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
> +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?
> + sbdev->is_laddr_valid = true;
shouldn't non-zero value signify that?
--
~Vinod
