Re: [RFC PATCH v2] fpga: Introduce new fpga subsystem

[Michal Simek]

Hi Joe,

On 10/02/2013 06:06 PM, Joe Perches wrote:
> On Wed, 2013-10-02 at 17:35 +0200, Michal Simek wrote:
>> This new fpga subsystem core should unify all fpga drivers/managers which
>> do the same things. Load configuration data to fpga or another programmable
>> logic through common interface. It doesn't matter if it is MMIO device,
>> gpio bitbanging, etc. connection. The point is to have the same
>> interface for these drivers.
> 
> Does this interface support partial reprogramming/configuration
> for FPGAs that can do that?

Currently we have two interfaces and third one is around (char driver)
that's why I didn't look at this support.
But for Xilinx devices init and complete phase is different.

We can use one more flag parameter for init and complete functions.

It means we can simple extend config states with write_init_partial, etc
Or the second option is to create new sysfs file to indicate
that we will work with partial bitstreams.

> trivial notes:
> 
> There are a _lot_ of dev_dbg statements.
> 
> I hope some of these would be removed one day,
> especially the function tracing style ones, because
> there's already a generic kernel mechanism for that.
> 
> Maybe perf/trace support could be added eventually.

Are you talking about trace_printk?

>> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> []
>> +/**
>> + * fpga_mgr_status_write - Write fpga manager status
>> + * @dev: Pointer to the device structure
>> + * @attr: Pointer to the device attribute structure
>> + * @buf: Pointer to the buffer location
>> + * @count: Number of characters in @buf
>> + *
>> + * Returns the number of bytes copied to @buf, a negative error number otherwise
>> + */
>> +static ssize_t fpga_mgr_status_write(struct device *dev,
>> +				     struct device_attribute *attr,
>> +				     const char *buf, size_t count)
>> +{
>> +	struct fpga_manager *mgr = dev_get_drvdata(dev);
>> +	int ret;
>> +
>> +	if (test_and_set_bit_lock(FPGA_MGR_DEV_BUSY, &mgr->flags))
>> +		return -EBUSY;
>> +
>> +	ret = strcmp(buf, "write_init");
>> +	if (!ret) {
>> +		ret = fpga_mgr_write_init(mgr);
>> +		goto out;
>> +	}
>> +
>> +	ret = strcmp(buf, "write_complete");
>> +	if (!ret) {
>> +		ret = fpga_mgr_write_complete(mgr);
>> +		goto out;
>> +	}
>> +
>> +	ret = strcmp(buf, "read_init");
>> +	if (!ret) {
>> +		ret = fpga_mgr_read_init(mgr);
>> +		goto out;
>> +	}
>> +
>> +	ret = strcmp(buf, "read_complete");
>> +	if (!ret) {
>> +		ret = fpga_mgr_read_complete(mgr);
>> +		goto out;
>> +	}
>> +
>> +	ret = -EINVAL;
>> +out:
>> +	clear_bit_unlock(FPGA_MGR_DEV_BUSY, &mgr->flags);
>> +
>> +	return ret ? : count;
>> +}
> 
> I think this style is awkward and this would be
> better written as:
> 
> 	if (!strcmp(buf, "write_init"))
> 		ret = fpga_mgr_write_init(mgr);
> 	else if (!strcmp(buf, "write_complete"))
> 		ret = fpga_mgr_write_complete(mgr);
> 	else if (!strcmp(buf, "read_init"))
> 		ret = fpga_mgr_read_init(mgr);
> 	else if (!strcmp(buf, "read_complete"))
> 		ret = fpga_mgr_read_complete(mgr);
> 	else
> 		ret = -EINVAL;
> 
> 	clear_bit_unlock(FPGA_MGR_DEV_BUSY, &mgr->flags);
> 
> 	if (ret)
> 		return ret;
> 
> 	return count;
> }
> 
> Maybe use (strcmp(...) == 0) if you prefer that.
> Both styles are commonly used in linux.


sure.


> Probably all of the "return ret ?: count;" uses
> would be more easily understood on 3 lines.

This structure is also quite common in the kernel
git grep "? :"  | wc -l
415

git grep "?:"  | wc -l
541

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform

Attachment: signature.asc
Description: OpenPGP digital signature