Hi Sylwester and Arun, On Wed, Nov 06, 2013 at 12:23:07PM +0100, Sylwester Nawrocki 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. Agreed. > >>> 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. As long as you can somehow ensure these will never end up to FPU registers, I think that should be fine. Just copying the struct elsewhere using memcpy() will be good, I believe. > 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. Good point; I agree. > 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. I guess it wouldn't hurt to use uint32_t there instead of u32 (and __u32). entirely up to you. -- Kind regards, Sakari Ailus e-mail: sakari.ailus@xxxxxx XMPP: sailus@xxxxxxxxxxxxxx -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html