[PATCH linux dev-4.7 v2 1/7] drivers/fsi: Add hub master scan detect

[Jeremy Kerr]

Hi Chris,

> @@ -252,7 +271,31 @@ static int fsi_slave_scan(struct fsi_slave *slave)
>  		 * Unused address areas are marked by a zero type value; this
>  		 * skips the defined address areas
>  		 */
> -		if (type != 0 && slots != 0) {
> +		if (type != 0) {
> +
> +			if (type == FSI_ENGID_HUB_MASTER) {

I'd split this into a separate part of the above 'if' statement. Or,
convert it to a switch.

> +				hub = kzalloc(sizeof(*hub), GFP_KERNEL);
> +				if (!hub)
> +					return -ENOMEM;
> +
> +				hub->base = FSI_HUB_LINK_OFFSET;
> +				hub->control_regs = engine_addr;
> +				hub->slave = slave;
> +				rc = hub_master_init(hub);
> +
> +				continue;
> +			}
> +			if (type == FSI_ENGID_HUB_LINK) {
> +				if (!hub)
> +					continue;
> +
> +				hub->master.n_links++;
> +				if (hub->master.n_links ==
> +						(FSI_HUB_MASTER_MAX_LINKS * 2))
> +					rc = fsi_master_register(&hub->master);
> +
> +				continue;
> +			}

This logic doesn't make a lot of sense. You're counting the number of
link slots, but then registering the master when that count hits a
predefined number. I'd suggest either:

 - just setting n_links to the predefined number; or

 - keeping a separate variable for n_links, and then once we've finished
   scanning the configuration table (after the for loop)

    if (hub) {
	    hub->n_links = conf_link_count / 2;
	    fsi_master_register(&hub->master);
    }

I'd much prefer the second option, as that actually uses the link count
from the config table.

>  int fsi_master_register(struct fsi_master *master)
>  {
> -	if (!master || !master->dev)
> +	if (!master)
>  		return -EINVAL;

I'm worried if this is needed - we should be creating a device for all
masters.

Cheers,


Jeremy