Hi Sakari, I reverted v10 and just added a comparison to see if clock-frequency is 19.2Mhz https://patchwork.kernel.org/patch/9784827/ -Hyungwoo -----Original Message----- > From: Sakari Ailus [mailto:sakari.ailus@xxxxxx] > Sent: Tuesday, June 13, 2017 8:29 AM > To: Yang, Hyungwoo <hyungwoo.yang@xxxxxxxxx> > Cc: linux-media@xxxxxxxxxxxxxxx; sakari.ailus@xxxxxxxxxxxxxxx; Zheng, Jian Xu <jian.xu.zheng@xxxxxxxxx>; tfiga@xxxxxxxxxxxx; Hsu, Cedric <cedric.hsu@xxxxxxxxx> > Subject: Re: [PATCH v10 1/1] [media] i2c: add support for OV13858 sensor > > Hi Hyungwoo, > > On Tue, Jun 13, 2017 at 02:29:25PM +0000, Yang, Hyungwoo wrote: > > > > > > Here is the _DSD for 19.2Mhz > > If you attached it, the list server most likely removed the attachment. > Could you send it again in-line? > > > > > > > > > > > > > i've inlined my comments. > > > > > > > > -----Original Message----- > > > From: Sakari Ailus [mailto:sakari.ailus@xxxxxx] > > > Sent: Tuesday, June 13, 2017 3:18 AM > > > To: Yang, Hyungwoo <hyungwoo.yang@xxxxxxxxx> > > > Cc: linux-media@xxxxxxxxxxxxxxx; sakari.ailus@xxxxxxxxxxxxxxx; > > > Zheng, Jian Xu <jian.xu.zheng@xxxxxxxxx>; tfiga@xxxxxxxxxxxx; Hsu, > > > Cedric <cedric.hsu@xxxxxxxxx> > > > Subject: Re: [PATCH v10 1/1] [media] i2c: add support for OV13858 > > > sensor > > > > > > Hi Hyungwoo, > > > > > > On Mon, Jun 12, 2017 at 05:56:00PM -0700, Hyungwoo Yang wrote: > ... > > > > + if (ret) > > > > + return ret; > > > > + ov13858->num_of_skip_frames = val; > > > > + > > > > + device_for_each_child_node(dev, child) { > > > > + if (!fwnode_property_present(child, "link")) > > > > + continue; > > > > > > You shouldn't need these here. > > > > > > > ?? just get child and see if the expected property is found ? > > Acked > > Your child nodes shouldn't look like that, nor an individual driver should need to be directly interested in them. > > Please see Documentation/acpi/dsd/graph.txt . > > > > > + if (ret) { > > > > + dev_err(dev, "link-rate error : %d\n", ret); > > > > + goto error; > > > > + } > > > > + link_freq_menu_items[freq_id] = val; > > > > + > > > > + freq_cfg = &link_freq_configs[freq_id]; > > > > + ret = fwnode_property_read_u32(fwn_freq, "pixel-rate", &val); > > > > > > This is something a sensor driver needs to know. It may not be present in device firmware. > > > > > > > Real frequency of the link can be different from input clock frequency > > so pixel reate which is dependent on link frequency should be here. > > The pixel rate does depend on the link frequency, but you can either calculate it in the driver OR make it specific your sensor configuration. > > > > > > > + if (ret) { > > > > + dev_err(dev, "pixel-rate error : %d\n", ret); > > > > + goto error; > > > > + } > > > > + freq_cfg->pixel_rate = val; > > > > + > > > > + num_of_values = fwnode_property_read_u32_array(fwn_freq, > > > > + "pll-regs", > > > > > > This is something that could go to device firmware but I don't really see a reason for that at the moment. Can this continue to be a part of the driver for now? > > > > > > Could you also check that the external clock frequency matches with what your register lists assume? The property should be called "clock-frequency" > > > and it's a u32. > > > > Are you saying, for now, let's remove pll reg/value pairs from _DSD > > and just support only one input clock frequency ? > > That would help getting the driver merged during this merge window. It's always possible to add support for different configurations later on. > > -- > Kind regards, > > Sakari Ailus > e-mail: sakari.ailus@xxxxxx XMPP: sailus@xxxxxxxxxxxxxx >
