Re: [PATCH v11 03/12] [media] exynos5-fimc-is: Add common driver header files

[Sylwester Nawrocki <s.nawrocki@samsung.com>]

Hi,

On 05/11/13 14:16, Arun Kumar K wrote:
>>> +struct is_common_reg {
>>> +     u32 hicmd;
>>> +     u32 hic_sensorid;
>>> +     u32 hic_param[4];
>>> +
>>> +     u32 reserved1[3];
[...]
>>> +     u32 meta_iflag;
>>> +     u32 meta_sensor_id;
>>> +     u32 meta_param1;
>>> +
>>> +     u32 reserved9[1];
>>> +
>>> +     u32 fcount;
>>
>> If these structs define an interface that's not used by the driver only it
>> might be a good idea to use __packed to ensure no padding is added.
>>
> 
> The same structure is used as is in the firmware code and so it is retained
> in the driver.

I agree it makes sense to use __packed attribute to ensure no padding is
added by the compiler. The firmware source and the driver will likely be 
compiled with different toolchains, and in both cases we should ensure
no padding is added.

>>> diff --git a/drivers/media/platform/exynos5-is/fimc-is-metadata.h b/drivers/media/platform/exynos5-is/fimc-is-metadata.h
>>> new file mode 100644
>>> index 0000000..02367c4
>>> --- /dev/null
>>> +++ b/drivers/media/platform/exynos5-is/fimc-is-metadata.h
>>> @@ -0,0 +1,767 @@
[..]
>>> +enum metadata_mode {
>>> +     METADATA_MODE_NONE,
>>> +     METADATA_MODE_FULL
>>> +};
>>> +
>>> +struct camera2_request_ctl {
>>> +     uint32_t                id;
>>> +     enum metadata_mode      metadatamode;
>>> +     uint8_t                 outputstreams[16];
>>> +     uint32_t                framecount;
>>> +};
>>> +
>>> +struct camera2_request_dm {
>>> +     uint32_t                id;
>>> +     enum metadata_mode      metadatamode;
>>> +     uint32_t                framecount;
>>> +};
[...]
>>> +struct camera2_lens_ctl {
>>> +     uint32_t                                focus_distance;
>>> +     float                                   aperture;
>>
>> Floating point numbers? Really? :-)
>>
> 
> Yes as mentioned, the same structure is used by the firmware and
> so it is used as is in the kernel.

These floating numbers are pretty painful, but I don't think they can
be avoided unless the firmware is changed. I hope there is no need to 
touch those in the kernel.

There are already precedents of using floating point numbers in driver's
public interface, e.g. some gpu/drm drivers. 

I noticed there is another issue in this firmware/kernel interface, i.e. 
some data structures contain enums in them, e.g.

struct camera2_lens_ctl {
	uint32_t				focus_distance;
	float					aperture;
	float					focal_length;
	float					filter_density;
	enum optical_stabilization_mode		optical_stabilization_mode;
};

It looks like a mistake in the interface design, as size of an enum is
implementation specific.

I guess size of those enum types is supposed to be 4 bytes ? Presumably
you should, e.g. use fixed data type like uin32_t or __u32 instead of those 
enums. It looks pretty fragile as it is now.

In addition all those data structures should be declared with __packed
attribute, to ensure a specific data structure layout and to avoid 
an unexpected padding.

>> diff --git a/drivers/media/platform/exynos5-is/fimc-is-param.h b/drivers/media/platform/exynos5-is/fimc-is-param.h
>> new file mode 100644
>> index 0000000..015cc13
>> --- /dev/null
>> +++ b/drivers/media/platform/exynos5-is/fimc-is-param.h
> ...
>> +struct param_control {
>> +	u32 cmd;
> 
> You use uint32_t in some other headers. It's not wrong to use both C99 and
> Linux types but I'd try to stick to either one.

I tend to agree with that, it's probably better to use one convention, u32
for kernel internal structures and __u32 for any public interfaces. I don't
think it is e requirement but would be nice to keep it more consistent.

Even if we wanted to keep the firmware defined data structures in sync with
the Linux driver, there are already some Linux types used within the firmware
interface. if I understood things correctly.

>> +	u32 bypass;
>> +	u32 buffer_address;
>> +	u32 buffer_number;
>> +	/* 0: continuous, 1: single */
>> +	u32 run_mode;
>> +	u32 reserved[PARAMETER_MAX_MEMBER - 6];
>> +	u32 err;
>> +};

Can you please address those issues in follow up patches ?
I will be sending these patches for inclusion in the media tree,
I would prefer to avoid keeping it on the ML for more than those
7 months already passed.

--
Regards,
Sylwester