On Fri March 1 2013 12:02:14 Tomasz Stanislawski wrote:
> Hi Hans,
> Please refer to the comments below.
>
> On 02/16/2013 10:28 AM, Hans Verkuil wrote:
> > From: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> >
> > This just adds dv_timings support without modifying existing dv_preset
> > support, although I had to refactor a little bit in order to share
> > hdmiphy_find_conf() between the preset and timings code.
> >
> > Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> > Cc: Tomasz Stanislawski <t.stanislaws@xxxxxxxxxxx>
> > Cc: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
> > ---
> > drivers/media/platform/s5p-tv/hdmiphy_drv.c | 48 ++++++++++++++++++++++-----
> > 1 file changed, 39 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/media/platform/s5p-tv/hdmiphy_drv.c b/drivers/media/platform/s5p-tv/hdmiphy_drv.c
> > index 80717ce..85b4211 100644
> > --- a/drivers/media/platform/s5p-tv/hdmiphy_drv.c
> > +++ b/drivers/media/platform/s5p-tv/hdmiphy_drv.c
> > @@ -197,14 +197,9 @@ static unsigned long hdmiphy_preset_to_pixclk(u32 preset)
> > return 0;
> > }
> >
> > -static const u8 *hdmiphy_find_conf(u32 preset, const struct hdmiphy_conf *conf)
> > +static const u8 *hdmiphy_find_conf(unsigned long pixclk,
> > + const struct hdmiphy_conf *conf)
> > {
> > - unsigned long pixclk;
> > -
> > - pixclk = hdmiphy_preset_to_pixclk(preset);
> > - if (!pixclk)
> > - return NULL;
> > -
> > for (; conf->pixclk; ++conf)
> > if (conf->pixclk == pixclk)
> > return conf->data;
> > @@ -220,15 +215,49 @@ static int hdmiphy_s_power(struct v4l2_subdev *sd, int on)
> > static int hdmiphy_s_dv_preset(struct v4l2_subdev *sd,
> > struct v4l2_dv_preset *preset)
> > {
> > - const u8 *data;
> > + const u8 *data = NULL;
> > u8 buffer[32];
> > int ret;
> > struct hdmiphy_ctx *ctx = sd_to_ctx(sd);
> > struct i2c_client *client = v4l2_get_subdevdata(sd);
> > + unsigned long pixclk;
> > struct device *dev = &client->dev;
> >
> > dev_info(dev, "s_dv_preset(preset = %d)\n", preset->preset);
> > - data = hdmiphy_find_conf(preset->preset, ctx->conf_tab);
> > +
> > + pixclk = hdmiphy_preset_to_pixclk(preset->preset);
>
> Just nitpicking.
> The pixclk might be 0 is the preset is not supported hdmiphy.
> For some platforms not all frequencies are supported.
> Anyway, if pixclk is 0 then hdmiphy_find_conf will detect it.
I'll add a comment clarifying this. I actually was relying on find_conf
to handle the 0 case.
> > + data = hdmiphy_find_conf(pixclk, ctx->conf_tab);
> > + if (!data) {
> > + dev_err(dev, "format not supported\n");
> > + return -EINVAL;
> > + }
> > +
> > + /* storing configuration to the device */
> > + memcpy(buffer, data, 32);
> > + ret = i2c_master_send(client, buffer, 32);
> > + if (ret != 32) {
> > + dev_err(dev, "failed to configure HDMIPHY via I2C\n");
> > + return -EIO;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int hdmiphy_s_dv_timings(struct v4l2_subdev *sd,
> > + struct v4l2_dv_timings *timings)
> > +{
> > + const u8 *data;
> > + u8 buffer[32];
> > + int ret;
> > + struct hdmiphy_ctx *ctx = sd_to_ctx(sd);
> > + struct i2c_client *client = v4l2_get_subdevdata(sd);
> > + struct device *dev = &client->dev;
> > + unsigned long pixclk = timings->bt.pixelclock;
> > +
> > + dev_info(dev, "s_dv_timings\n");
>
> Using test against V4L2_DV_FL_REDUCED_FPS and 74250000 looks little hacky to me.
> Why there is no such a check for 148500000 and 27000000 (REDUCED -> INCREASED?).
Because that was not supported by the original code. I have no idea whether 1080p59.94
is supported or not, or what pixelclock should be used in that case. So I faithfully
reproduced the original functionality.
BTW, it is 'reduced': 74.25 MHz is for 30 fps, 74.176 is for 29.97 fps. Also, REDUCED_FPS
doesn't apply to the 27 MHz case which is always 29.97 fps in the NTSC resolution
case.
> Maybe it will be better to past both timings->bt.pixelclock and timings->bt.flags
> as parameters for hdmiphy_find_conf function. The hdmiphy_find_conf could
> perform pixclk adjustment or some fallback policy based on the flags.
I really prefer to keep the same functionality. I don't want to make changes
to the existing functionality, partially to make it easier to prevent
regressions, partially because I don't have the setup anyway to test this if
I add functionality.
But feel free to provide an additional patch doing this :-)
Regards,
Hans
>
> > + if ((timings->bt.flags & V4L2_DV_FL_REDUCED_FPS) && pixclk == 74250000)
> > + pixclk = 74176000;
> > + data = hdmiphy_find_conf(pixclk, ctx->conf_tab);
> > if (!data) {
> > dev_err(dev, "format not supported\n");
> > return -EINVAL;
> > @@ -271,6 +300,7 @@ static const struct v4l2_subdev_core_ops hdmiphy_core_ops = {
> >
> > static const struct v4l2_subdev_video_ops hdmiphy_video_ops = {
> > .s_dv_preset = hdmiphy_s_dv_preset,
> > + .s_dv_timings = hdmiphy_s_dv_timings,
> > .s_stream = hdmiphy_s_stream,
> > };
> >
> >
>
--
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
