Hi Sylwester,
On Wed, Nov 6, 2013 at 4:53 PM, Sylwester Nawrocki
<s.nawrocki@xxxxxxxxxxx> wrote:
> 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.
>
Yes the toolchains are different and ideally the firmware also should
use a __packed
attribute which possibly is not happening.
>>>> 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.
>
Ok
> 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.
>
Yes its better to use 4byte data structures. Will change that.
> 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.
>
Ok.
>>> 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.
>
Ok
> 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.
>
Ok will address these comments in follow up patches.
Thanks & Regards
Arun
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
