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

[Christopher Bostic <cbostic@linux.vnet.ibm.com>]

On 2/21/17 7:02 PM, Jeremy Kerr wrote:
> 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.

Hi Jeremy,

Will change.

>> +				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.

Will implement the second option

>>   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.

Will add a dev for struct fsi_master.

Thanks

> Cheers,
>
>
> Jeremy
>