This patch set adds handling of the i2c_board_info struct to the saa7115 driver.
The main goal of this patch is to give the different devices with the gm7113c
chip an opportunity to configure the chip to their needs.
I've only implemented the overrides I know are necessary to get the stk1160
and the smi2021 drivers to work.
The first patch in the series sets the saa7113 init table to the defaults
according to the datasheet. Maybe it's better to add a new initialization
table for the gm7113c chip to avoid breaking devices that depend on the
settings as they are now?
That would introduce some unneeded code duplication.
Jon Arne Jørgensen (3):
saa7115: Set saa7113 init to values from datasheet
saa7115: [gm7113c] Remove unneeded register change
saa7115: Implement i2c_board_info.platform data
drivers/media/i2c/saa7115.c | 91 ++++++++++++++++++++++++------------
include/media/saa7115.h | 109 ++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 170 insertions(+), 30 deletions(-)
--
1.8.2.3
Change all default values in the initial setup table to match the table
in the datasheet.
Signed-off-by: Jon Arne Jørgensen <[email protected]>
---
drivers/media/i2c/saa7115.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/media/i2c/saa7115.c b/drivers/media/i2c/saa7115.c
index d6f589a..4403679 100644
--- a/drivers/media/i2c/saa7115.c
+++ b/drivers/media/i2c/saa7115.c
@@ -223,12 +223,12 @@ static const unsigned char saa7111_init[] = {
static const unsigned char saa7113_init[] = {
R_01_INC_DELAY, 0x08,
R_02_INPUT_CNTL_1, 0xc2,
- R_03_INPUT_CNTL_2, 0x30,
+ R_03_INPUT_CNTL_2, 0x33,
R_04_INPUT_CNTL_3, 0x00,
R_05_INPUT_CNTL_4, 0x00,
- R_06_H_SYNC_START, 0x89,
+ R_06_H_SYNC_START, 0xe9,
R_07_H_SYNC_STOP, 0x0d,
- R_08_SYNC_CNTL, 0x88,
+ R_08_SYNC_CNTL, 0x98,
R_09_LUMA_CNTL, 0x01,
R_0A_LUMA_BRIGHT_CNTL, 0x80,
R_0B_LUMA_CONTRAST_CNTL, 0x47,
@@ -236,11 +236,11 @@ static const unsigned char saa7113_init[] = {
R_0D_CHROMA_HUE_CNTL, 0x00,
R_0E_CHROMA_CNTL_1, 0x01,
R_0F_CHROMA_GAIN_CNTL, 0x2a,
- R_10_CHROMA_CNTL_2, 0x08,
+ R_10_CHROMA_CNTL_2, 0x00,
R_11_MODE_DELAY_CNTL, 0x0c,
- R_12_RT_SIGNAL_CNTL, 0x07,
+ R_12_RT_SIGNAL_CNTL, 0x01,
R_13_RT_X_PORT_OUT_CNTL, 0x00,
- R_14_ANAL_ADC_COMPAT_CNTL, 0x00,
+ R_14_ANAL_ADC_COMPAT_CNTL, 0x00, /* RESERVED */
R_15_VGATE_START_FID_CHG, 0x00,
R_16_VGATE_STOP, 0x00,
R_17_MISC_VGATE_CONF_AND_MSB, 0x00,
--
1.8.2.3
Implement i2c_board_info.platform_data handling in the driver so we can
make device specific changes to the chips we support.
Signed-off-by: Jon Arne Jørgensen <[email protected]>
---
drivers/media/i2c/saa7115.c | 62 +++++++++++++++++++++++--
include/media/saa7115.h | 109 ++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 166 insertions(+), 5 deletions(-)
diff --git a/drivers/media/i2c/saa7115.c b/drivers/media/i2c/saa7115.c
index ccfaac9..8e915c7 100644
--- a/drivers/media/i2c/saa7115.c
+++ b/drivers/media/i2c/saa7115.c
@@ -228,7 +228,7 @@ static const unsigned char saa7113_init[] = {
R_05_INPUT_CNTL_4, 0x00,
R_06_H_SYNC_START, 0xe9,
R_07_H_SYNC_STOP, 0x0d,
- R_08_SYNC_CNTL, 0x98,
+ R_08_SYNC_CNTL, SAA7113_R08_DEFAULT,
R_09_LUMA_CNTL, 0x01,
R_0A_LUMA_BRIGHT_CNTL, 0x80,
R_0B_LUMA_CONTRAST_CNTL, 0x47,
@@ -236,11 +236,10 @@ static const unsigned char saa7113_init[] = {
R_0D_CHROMA_HUE_CNTL, 0x00,
R_0E_CHROMA_CNTL_1, 0x01,
R_0F_CHROMA_GAIN_CNTL, 0x2a,
- R_10_CHROMA_CNTL_2, 0x00,
+ R_10_CHROMA_CNTL_2, SAA7113_R10_DEFAULT,
R_11_MODE_DELAY_CNTL, 0x0c,
- R_12_RT_SIGNAL_CNTL, 0x01,
- R_13_RT_X_PORT_OUT_CNTL, 0x00,
- R_14_ANAL_ADC_COMPAT_CNTL, 0x00, /* RESERVED */
+ R_12_RT_SIGNAL_CNTL, SAA7113_R12_DEFAULT,
+ R_13_RT_X_PORT_OUT_CNTL, SAA7113_R13_DEFAULT,
R_15_VGATE_START_FID_CHG, 0x00,
R_16_VGATE_STOP, 0x00,
R_17_MISC_VGATE_CONF_AND_MSB, 0x00,
@@ -1583,6 +1582,53 @@ static const struct v4l2_subdev_ops saa711x_ops = {
/* ----------------------------------------------------------------------- */
+static void saa7115_load_platform_data(struct saa711x_state *state,
+ struct saa7115_platform_data *data)
+{
+ struct v4l2_subdev *sd = &state->sd;
+ u8 work;
+
+ switch (state->ident) {
+ case V4L2_IDENT_GM7113C:
+ if (data->saa7113_r08_htc !=
+ (SAA7113_R08_DEFAULT & SAA7113_R08_HTC_MASK)) {
+ work = saa711x_read(sd, R_08_SYNC_CNTL);
+ saa711x_write(sd, R_08_SYNC_CNTL, (work & 0xe7) |
+ (data->saa7113_r08_htc << 3));
+ }
+ if (data->saa7113_r10_ofts !=
+ (SAA7113_R10_DEFAULT & SAA7113_R10_OFTS_MASK)) {
+ work = saa711x_read(sd, R_10_CHROMA_CNTL_2);
+ saa711x_write(sd, R_10_CHROMA_CNTL_2, (work & 0x3f) |
+ (data->saa7113_r10_ofts << 6));
+ }
+ if (data->saa7113_r10_vrln !=
+ (SAA7113_R10_DEFAULT & SAA7113_R10_VRLN_MASK)) {
+ work = saa711x_read(sd, R_10_CHROMA_CNTL_2);
+ saa711x_write(sd, R_10_CHROMA_CNTL_2, (work & 0xf7) |
+ (1 << 3));
+ }
+ if (data->saa7113_r12_rts0 !=
+ (SAA7113_R12_DEFAULT & SAA7113_R12_RTS0_MASK)) {
+ work = saa711x_read(sd, R_12_RT_SIGNAL_CNTL);
+ saa711x_write(sd, R_12_RT_SIGNAL_CNTL, (work & 0xf0) |
+ data->saa7113_r12_rts0);
+ }
+ if (data->saa7113_r12_rts1 !=
+ (SAA7113_R12_DEFAULT & SAA7113_R12_RTS1_MASK)) {
+ work = saa711x_read(sd, R_12_RT_SIGNAL_CNTL);
+ saa711x_write(sd, R_12_RT_SIGNAL_CNTL, (work & 0x0f) |
+ (data->saa7113_r12_rts1 << 4));
+ }
+ if (data->saa7113_r13_adlsb !=
+ (SAA7113_R13_DEFAULT & SAA7113_R13_ADLSB_MASK)) {
+ work = saa711x_read(sd, R_13_RT_X_PORT_OUT_CNTL);
+ saa711x_write(sd, R_13_RT_X_PORT_OUT_CNTL,
+ (work & 0x7f) | (1 << 7));
+ }
+ }
+}
+
/**
* saa711x_detect_chip - Detects the saa711x (or clone) variant
* @client: I2C client structure.
@@ -1769,6 +1815,12 @@ static int saa711x_probe(struct i2c_client *client,
}
if (state->ident > V4L2_IDENT_SAA7111A)
saa711x_writeregs(sd, saa7115_init_misc);
+
+ if (client->dev.platform_data) {
+ struct saa7115_platform_data *data = client->dev.platform_data;
+ saa7115_load_platform_data(state, data);
+ }
+
saa711x_set_v4lstd(sd, V4L2_STD_NTSC);
v4l2_ctrl_handler_setup(hdl);
diff --git a/include/media/saa7115.h b/include/media/saa7115.h
index 4079186..7bb4a11 100644
--- a/include/media/saa7115.h
+++ b/include/media/saa7115.h
@@ -64,5 +64,114 @@
#define SAA7115_FREQ_FL_APLL (1 << 2) /* SA 3A[3], APLL, SAA7114/5 only */
#define SAA7115_FREQ_FL_DOUBLE_ASCLK (1 << 3) /* SA 39, LRDIV, SAA7114/5 only */
+/* SAA7113 (and GM7113) Register settings */
+/* Vertical noise reduction */
+#define SAA7113_R08_VNOI_NORMAL (0x0 << 0)
+#define SAA7113_R08_VNOI_FAST (0x1 << 0)
+#define SAA7113_R08_VNOI_FREE (0x2 << 0) /* NOTE: AUFD must be disabled */
+#define SAA7113_R08_VNOI_BYPS (0x3 << 0)
+/* Horizontal PLL */
+#define SAA7113_R08_PLL_CLOSED (0x0 << 2)
+#define SAA7113_R08_PLL_OPEN (0x1 << 2) /* Horizontal freq. fixed */
+/* Horizontal time constant */
+#define SAA7113_R08_HTC_TV (0x0 << 3)
+#define SAA7113_R08_HTC_VTR (0x1 << 3)
+#define SAA7113_R08_HTC_FLM (0x3 << 3) /* Fast locking mode */
+#define SAA7113_R08_HTC_MASK (0x3 << 3)
+/* Forced ODD/EVEN toggle FOET */
+#define SAA7113_R08_FOET_INTERLACED (0x0 << 5)
+#define SAA7113_R08_FOET_FORCE (0x1 << 5)
+/* Field selection FSEL */
+#define SAA7113_R08_FSEL_50HZ_625 (0x0 << 6)
+#define SAA7113_R08_FSEL_60HZ_525 (0x1 << 6)
+/* Automatic field detection AUFD */
+#define SAA7113_R08_AUFD_DISABLE (0x0 << 7)
+#define SAA7113_R08_AUFD_ENABLE (0x1 << 7)
+
+/* Luminance delay compensation */
+#define SAA7113_R10_YDEL_MASK 0x7
+/* VRLN Pulse position and length */
+#define SAA7113_R10_VRLN_240_286 (0x0 << 3)
+#define SAA7113_R10_VRLN_242_288 (0x1 << 3)
+#define SAA7113_R10_VRLN_MASK (0x1 << 3)
+/* Fine position of HS */
+#define SAA7113_R10_HDEL_MASK (0x3 << 4)
+/* Output format selection */
+#define SAA7113_R10_OFTS_ITU656 (0x0 << 6)
+#define SAA7113_R10_OFTS_VREF (0x1 << 6)
+#define SAA7113_R10_OFTS_DATA_TYPE (0x2 << 6)
+#define SAA7113_R10_OFTS_MASK (0x3 << 6)
+
+/* RTS0/1 Output control */
+#define SAA7113_R12_RTS0_ADC_LSB (0x1 << 0)
+#define SAA7113_R12_RTS0_GPSW0 (0x2 << 0)
+#define SAA7113_R12_RTS0_HL (0x3 << 0)
+#define SAA7113_R12_RTS0_VL (0x4 << 0)
+#define SAA7113_R12_RTS0_DL (0x5 << 0)
+#define SAA7113_R12_RTS0_PLIN (0x6 << 0)
+#define SAA7113_R12_RTS0_HREF_HS (0x7 << 0)
+#define SAA7113_R12_RTS0_HS (0x8 << 0)
+#define SAA7113_R12_RTS0_HQ (0x9 << 0)
+#define SAA7113_R12_RTS0_ODD (0xa << 0)
+#define SAA7113_R12_RTS0_VS (0xb << 0)
+#define SAA7113_R12_RTS0_V123 (0xc << 0)
+#define SAA7113_R12_RTS0_VGATE (0xd << 0)
+#define SAA7113_R12_RTS0_VREF (0xe << 0)
+#define SAA7113_R12_RTS0_FID (0xf << 0)
+#define SAA7113_R12_RTS0_MASK (0xf << 0)
+#define SAA7113_R12_RTS1_ADC_LSB (0x1 << 4)
+#define SAA7113_R12_RTS1_GPSW1 (0x2 << 4)
+#define SAA7113_R12_RTS1_HL (0x3 << 4)
+#define SAA7113_R12_RTS1_VL (0x4 << 4)
+#define SAA7113_R12_RTS1_DL (0x5 << 4)
+#define SAA7113_R12_RTS1_PLIN (0x6 << 4)
+#define SAA7113_R12_RTS1_HREF_HS (0x7 << 4)
+#define SAA7113_R12_RTS1_HS (0x8 << 4)
+#define SAA7113_R12_RTS1_HQ (0x9 << 4)
+#define SAA7113_R12_RTS1_ODD (0xa << 4)
+#define SAA7113_R12_RTS1_VS (0xb << 4)
+#define SAA7113_R12_RTS1_V123 (0xc << 4)
+#define SAA7113_R12_RTS1_VGATE (0xd << 4)
+#define SAA7113_R12_RTS1_VREF (0xe << 4)
+#define SAA7113_R12_RTS1_FID (0xf << 4)
+#define SAA7113_R12_RTS1_MASK (0xf << 4)
+
+/* Analog test select */
+#define SAA7113_R13_AOSL_ITP1 (0x0 << 0)
+#define SAA7113_R13_AOSL_AD1 (0x1 << 0)
+#define SAA7113_R13_AOSL_AD2 (0x2 << 0)
+#define SAA7113_R13_AOSL_ITP2 (0x3 << 0)
+/* Field ID polarity */
+#define SAA7113_R13_FIDP_DEFAULT (0x0 << 3)
+#define SAA7113_R13_FIDP_INVERTED (0x1 << 3)
+/* Status byte functionality */
+#define SAA7113_R13_OLDSB_DEFAUL (0x0 << 4)
+#define SAA7113_R13_OLDSB_COMPAT (0x1 << 4)
+/* Analog-to-digital converter output bits on VPO7 to VPO0 in bypass mode */
+#define SAA7113_R13_ADLSB_MSB (0x0 << 7)
+#define SAA7113_R13_ADLSB_LSB (0x1 << 7)
+#define SAA7113_R13_ADLSB_MASK (0x1 << 7)
+
+
+/* Defaults according to datasheet */
+#define SAA7113_R08_DEFAULT (SAA7113_R08_AUFD_ENABLE | \
+ SAA7113_R08_HTC_FLM)
+#define SAA7113_R10_DEFAULT 0x0
+#define SAA7113_R12_DEFAULT SAA7113_R12_RTS0_ADC_LSB
+#define SAA7113_R13_DEFAULT 0x0
+
+struct saa7115_platform_data {
+ /* Horizontal time constant */
+ u8 saa7113_r08_htc;
+
+ u8 saa7113_r10_vrln;
+ u8 saa7113_r10_ofts;
+
+ u8 saa7113_r12_rts0;
+ u8 saa7113_r12_rts1;
+
+ u8 saa7113_r13_adlsb;
+};
+
#endif
--
1.8.2.3
On video std change, the driver would disable the automatic field
detection on the gm7113c chip, and force either 50Hz or 60Hz.
Don't do this any more.
Signed-off-by: Jon Arne Jørgensen <[email protected]>
---
drivers/media/i2c/saa7115.c | 25 ++-----------------------
1 file changed, 2 insertions(+), 23 deletions(-)
diff --git a/drivers/media/i2c/saa7115.c b/drivers/media/i2c/saa7115.c
index 4403679..ccfaac9 100644
--- a/drivers/media/i2c/saa7115.c
+++ b/drivers/media/i2c/saa7115.c
@@ -453,23 +453,6 @@ static const unsigned char saa7115_cfg_50hz_video[] = {
/* ============== SAA7715 VIDEO templates (end) ======= */
-/* ============== GM7113C VIDEO templates ============= */
-static const unsigned char gm7113c_cfg_60hz_video[] = {
- R_08_SYNC_CNTL, 0x68, /* 0xBO: auto detection, 0x68 = NTSC */
- R_0E_CHROMA_CNTL_1, 0x07, /* video autodetection is on */
-
- 0x00, 0x00
-};
-
-static const unsigned char gm7113c_cfg_50hz_video[] = {
- R_08_SYNC_CNTL, 0x28, /* 0x28 = PAL */
- R_0E_CHROMA_CNTL_1, 0x07,
-
- 0x00, 0x00
-};
-
-/* ============== GM7113C VIDEO templates (end) ======= */
-
static const unsigned char saa7115_cfg_vbi_on[] = {
R_80_GLOBAL_CNTL_1, 0x00, /* reset tasks */
@@ -955,16 +938,12 @@ static void saa711x_set_v4lstd(struct v4l2_subdev *sd, v4l2_std_id std)
// This works for NTSC-M, SECAM-L and the 50Hz PAL variants.
if (std & V4L2_STD_525_60) {
v4l2_dbg(1, debug, sd, "decoder set standard 60 Hz\n");
- if (state->ident == V4L2_IDENT_GM7113C)
- saa711x_writeregs(sd, gm7113c_cfg_60hz_video);
- else
+ if (state->ident != V4L2_IDENT_GM7113C)
saa711x_writeregs(sd, saa7115_cfg_60hz_video);
saa711x_set_size(sd, 720, 480);
} else {
v4l2_dbg(1, debug, sd, "decoder set standard 50 Hz\n");
- if (state->ident == V4L2_IDENT_GM7113C)
- saa711x_writeregs(sd, gm7113c_cfg_50hz_video);
- else
+ if (state->ident != V4L2_IDENT_GM7113C)
saa711x_writeregs(sd, saa7115_cfg_50hz_video);
saa711x_set_size(sd, 720, 576);
}
--
1.8.2.3
On Wed, May 29, 2013 at 10:41:15PM +0200, Jon Arne Jørgensen wrote:
> This patch set adds handling of the i2c_board_info struct to the saa7115 driver.
> The main goal of this patch is to give the different devices with the gm7113c
> chip an opportunity to configure the chip to their needs.
>
> I've only implemented the overrides I know are necessary to get the stk1160
> and the smi2021 drivers to work.
>
> The first patch in the series sets the saa7113 init table to the defaults
> according to the datasheet. Maybe it's better to add a new initialization
> table for the gm7113c chip to avoid breaking devices that depend on the
> settings as they are now?
> That would introduce some unneeded code duplication.
>
Hi,
I just realized that there are som grave errors in patch 3 in the
series.
I'll fix them and repost this series.
Best regards,
Jon Arne Jørgensen
> Jon Arne Jørgensen (3):
> saa7115: Set saa7113 init to values from datasheet
> saa7115: [gm7113c] Remove unneeded register change
> saa7115: Implement i2c_board_info.platform data
>
> drivers/media/i2c/saa7115.c | 91 ++++++++++++++++++++++++------------
> include/media/saa7115.h | 109 ++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 170 insertions(+), 30 deletions(-)
>
> --
> 1.8.2.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 29, 2013 at 10:41:18PM +0200, Jon Arne Jørgensen wrote:
> Implement i2c_board_info.platform_data handling in the driver so we can
> make device specific changes to the chips we support.
>
> Signed-off-by: Jon Arne Jørgensen <[email protected]>
> ---
> drivers/media/i2c/saa7115.c | 62 +++++++++++++++++++++++--
> include/media/saa7115.h | 109 ++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 166 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/media/i2c/saa7115.c b/drivers/media/i2c/saa7115.c
> index ccfaac9..8e915c7 100644
> --- a/drivers/media/i2c/saa7115.c
> +++ b/drivers/media/i2c/saa7115.c
> @@ -228,7 +228,7 @@ static const unsigned char saa7113_init[] = {
> R_05_INPUT_CNTL_4, 0x00,
> R_06_H_SYNC_START, 0xe9,
> R_07_H_SYNC_STOP, 0x0d,
> - R_08_SYNC_CNTL, 0x98,
> + R_08_SYNC_CNTL, SAA7113_R08_DEFAULT,
> R_09_LUMA_CNTL, 0x01,
> R_0A_LUMA_BRIGHT_CNTL, 0x80,
> R_0B_LUMA_CONTRAST_CNTL, 0x47,
> @@ -236,11 +236,10 @@ static const unsigned char saa7113_init[] = {
> R_0D_CHROMA_HUE_CNTL, 0x00,
> R_0E_CHROMA_CNTL_1, 0x01,
> R_0F_CHROMA_GAIN_CNTL, 0x2a,
> - R_10_CHROMA_CNTL_2, 0x00,
> + R_10_CHROMA_CNTL_2, SAA7113_R10_DEFAULT,
> R_11_MODE_DELAY_CNTL, 0x0c,
> - R_12_RT_SIGNAL_CNTL, 0x01,
> - R_13_RT_X_PORT_OUT_CNTL, 0x00,
> - R_14_ANAL_ADC_COMPAT_CNTL, 0x00, /* RESERVED */
> + R_12_RT_SIGNAL_CNTL, SAA7113_R12_DEFAULT,
> + R_13_RT_X_PORT_OUT_CNTL, SAA7113_R13_DEFAULT,
> R_15_VGATE_START_FID_CHG, 0x00,
> R_16_VGATE_STOP, 0x00,
> R_17_MISC_VGATE_CONF_AND_MSB, 0x00,
> @@ -1583,6 +1582,53 @@ static const struct v4l2_subdev_ops saa711x_ops = {
>
> /* ----------------------------------------------------------------------- */
>
> +static void saa7115_load_platform_data(struct saa711x_state *state,
> + struct saa7115_platform_data *data)
> +{
> + struct v4l2_subdev *sd = &state->sd;
> + u8 work;
> +
> + switch (state->ident) {
> + case V4L2_IDENT_GM7113C:
> + if (data->saa7113_r08_htc !=
> + (SAA7113_R08_DEFAULT & SAA7113_R08_HTC_MASK)) {
> + work = saa711x_read(sd, R_08_SYNC_CNTL);
> + saa711x_write(sd, R_08_SYNC_CNTL, (work & 0xe7) |
> + (data->saa7113_r08_htc << 3));
> + }
> + if (data->saa7113_r10_ofts !=
> + (SAA7113_R10_DEFAULT & SAA7113_R10_OFTS_MASK)) {
> + work = saa711x_read(sd, R_10_CHROMA_CNTL_2);
> + saa711x_write(sd, R_10_CHROMA_CNTL_2, (work & 0x3f) |
> + (data->saa7113_r10_ofts << 6));
> + }
> + if (data->saa7113_r10_vrln !=
> + (SAA7113_R10_DEFAULT & SAA7113_R10_VRLN_MASK)) {
> + work = saa711x_read(sd, R_10_CHROMA_CNTL_2);
> + saa711x_write(sd, R_10_CHROMA_CNTL_2, (work & 0xf7) |
> + (1 << 3));
> + }
> + if (data->saa7113_r12_rts0 !=
> + (SAA7113_R12_DEFAULT & SAA7113_R12_RTS0_MASK)) {
> + work = saa711x_read(sd, R_12_RT_SIGNAL_CNTL);
> + saa711x_write(sd, R_12_RT_SIGNAL_CNTL, (work & 0xf0) |
> + data->saa7113_r12_rts0);
> + }
> + if (data->saa7113_r12_rts1 !=
> + (SAA7113_R12_DEFAULT & SAA7113_R12_RTS1_MASK)) {
> + work = saa711x_read(sd, R_12_RT_SIGNAL_CNTL);
> + saa711x_write(sd, R_12_RT_SIGNAL_CNTL, (work & 0x0f) |
> + (data->saa7113_r12_rts1 << 4));
> + }
> + if (data->saa7113_r13_adlsb !=
> + (SAA7113_R13_DEFAULT & SAA7113_R13_ADLSB_MASK)) {
> + work = saa711x_read(sd, R_13_RT_X_PORT_OUT_CNTL);
> + saa711x_write(sd, R_13_RT_X_PORT_OUT_CNTL,
> + (work & 0x7f) | (1 << 7));
> + }
> + }
> +}
> +
I've made some grave mistakes here.
Will fix and repost.
> /**
> * saa711x_detect_chip - Detects the saa711x (or clone) variant
> * @client: I2C client structure.
> @@ -1769,6 +1815,12 @@ static int saa711x_probe(struct i2c_client *client,
> }
> if (state->ident > V4L2_IDENT_SAA7111A)
> saa711x_writeregs(sd, saa7115_init_misc);
> +
> + if (client->dev.platform_data) {
> + struct saa7115_platform_data *data = client->dev.platform_data;
> + saa7115_load_platform_data(state, data);
> + }
> +
> saa711x_set_v4lstd(sd, V4L2_STD_NTSC);
> v4l2_ctrl_handler_setup(hdl);
>
> diff --git a/include/media/saa7115.h b/include/media/saa7115.h
> index 4079186..7bb4a11 100644
> --- a/include/media/saa7115.h
> +++ b/include/media/saa7115.h
> @@ -64,5 +64,114 @@
> #define SAA7115_FREQ_FL_APLL (1 << 2) /* SA 3A[3], APLL, SAA7114/5 only */
> #define SAA7115_FREQ_FL_DOUBLE_ASCLK (1 << 3) /* SA 39, LRDIV, SAA7114/5 only */
>
> +/* SAA7113 (and GM7113) Register settings */
> +/* Vertical noise reduction */
> +#define SAA7113_R08_VNOI_NORMAL (0x0 << 0)
> +#define SAA7113_R08_VNOI_FAST (0x1 << 0)
> +#define SAA7113_R08_VNOI_FREE (0x2 << 0) /* NOTE: AUFD must be disabled */
> +#define SAA7113_R08_VNOI_BYPS (0x3 << 0)
> +/* Horizontal PLL */
> +#define SAA7113_R08_PLL_CLOSED (0x0 << 2)
> +#define SAA7113_R08_PLL_OPEN (0x1 << 2) /* Horizontal freq. fixed */
> +/* Horizontal time constant */
> +#define SAA7113_R08_HTC_TV (0x0 << 3)
> +#define SAA7113_R08_HTC_VTR (0x1 << 3)
> +#define SAA7113_R08_HTC_FLM (0x3 << 3) /* Fast locking mode */
> +#define SAA7113_R08_HTC_MASK (0x3 << 3)
> +/* Forced ODD/EVEN toggle FOET */
> +#define SAA7113_R08_FOET_INTERLACED (0x0 << 5)
> +#define SAA7113_R08_FOET_FORCE (0x1 << 5)
> +/* Field selection FSEL */
> +#define SAA7113_R08_FSEL_50HZ_625 (0x0 << 6)
> +#define SAA7113_R08_FSEL_60HZ_525 (0x1 << 6)
> +/* Automatic field detection AUFD */
> +#define SAA7113_R08_AUFD_DISABLE (0x0 << 7)
> +#define SAA7113_R08_AUFD_ENABLE (0x1 << 7)
> +
> +/* Luminance delay compensation */
> +#define SAA7113_R10_YDEL_MASK 0x7
> +/* VRLN Pulse position and length */
> +#define SAA7113_R10_VRLN_240_286 (0x0 << 3)
> +#define SAA7113_R10_VRLN_242_288 (0x1 << 3)
> +#define SAA7113_R10_VRLN_MASK (0x1 << 3)
> +/* Fine position of HS */
> +#define SAA7113_R10_HDEL_MASK (0x3 << 4)
> +/* Output format selection */
> +#define SAA7113_R10_OFTS_ITU656 (0x0 << 6)
> +#define SAA7113_R10_OFTS_VREF (0x1 << 6)
> +#define SAA7113_R10_OFTS_DATA_TYPE (0x2 << 6)
> +#define SAA7113_R10_OFTS_MASK (0x3 << 6)
> +
> +/* RTS0/1 Output control */
> +#define SAA7113_R12_RTS0_ADC_LSB (0x1 << 0)
> +#define SAA7113_R12_RTS0_GPSW0 (0x2 << 0)
> +#define SAA7113_R12_RTS0_HL (0x3 << 0)
> +#define SAA7113_R12_RTS0_VL (0x4 << 0)
> +#define SAA7113_R12_RTS0_DL (0x5 << 0)
> +#define SAA7113_R12_RTS0_PLIN (0x6 << 0)
> +#define SAA7113_R12_RTS0_HREF_HS (0x7 << 0)
> +#define SAA7113_R12_RTS0_HS (0x8 << 0)
> +#define SAA7113_R12_RTS0_HQ (0x9 << 0)
> +#define SAA7113_R12_RTS0_ODD (0xa << 0)
> +#define SAA7113_R12_RTS0_VS (0xb << 0)
> +#define SAA7113_R12_RTS0_V123 (0xc << 0)
> +#define SAA7113_R12_RTS0_VGATE (0xd << 0)
> +#define SAA7113_R12_RTS0_VREF (0xe << 0)
> +#define SAA7113_R12_RTS0_FID (0xf << 0)
> +#define SAA7113_R12_RTS0_MASK (0xf << 0)
> +#define SAA7113_R12_RTS1_ADC_LSB (0x1 << 4)
> +#define SAA7113_R12_RTS1_GPSW1 (0x2 << 4)
> +#define SAA7113_R12_RTS1_HL (0x3 << 4)
> +#define SAA7113_R12_RTS1_VL (0x4 << 4)
> +#define SAA7113_R12_RTS1_DL (0x5 << 4)
> +#define SAA7113_R12_RTS1_PLIN (0x6 << 4)
> +#define SAA7113_R12_RTS1_HREF_HS (0x7 << 4)
> +#define SAA7113_R12_RTS1_HS (0x8 << 4)
> +#define SAA7113_R12_RTS1_HQ (0x9 << 4)
> +#define SAA7113_R12_RTS1_ODD (0xa << 4)
> +#define SAA7113_R12_RTS1_VS (0xb << 4)
> +#define SAA7113_R12_RTS1_V123 (0xc << 4)
> +#define SAA7113_R12_RTS1_VGATE (0xd << 4)
> +#define SAA7113_R12_RTS1_VREF (0xe << 4)
> +#define SAA7113_R12_RTS1_FID (0xf << 4)
> +#define SAA7113_R12_RTS1_MASK (0xf << 4)
> +
> +/* Analog test select */
> +#define SAA7113_R13_AOSL_ITP1 (0x0 << 0)
> +#define SAA7113_R13_AOSL_AD1 (0x1 << 0)
> +#define SAA7113_R13_AOSL_AD2 (0x2 << 0)
> +#define SAA7113_R13_AOSL_ITP2 (0x3 << 0)
> +/* Field ID polarity */
> +#define SAA7113_R13_FIDP_DEFAULT (0x0 << 3)
> +#define SAA7113_R13_FIDP_INVERTED (0x1 << 3)
> +/* Status byte functionality */
> +#define SAA7113_R13_OLDSB_DEFAUL (0x0 << 4)
> +#define SAA7113_R13_OLDSB_COMPAT (0x1 << 4)
> +/* Analog-to-digital converter output bits on VPO7 to VPO0 in bypass mode */
> +#define SAA7113_R13_ADLSB_MSB (0x0 << 7)
> +#define SAA7113_R13_ADLSB_LSB (0x1 << 7)
> +#define SAA7113_R13_ADLSB_MASK (0x1 << 7)
> +
> +
> +/* Defaults according to datasheet */
> +#define SAA7113_R08_DEFAULT (SAA7113_R08_AUFD_ENABLE | \
> + SAA7113_R08_HTC_FLM)
> +#define SAA7113_R10_DEFAULT 0x0
> +#define SAA7113_R12_DEFAULT SAA7113_R12_RTS0_ADC_LSB
> +#define SAA7113_R13_DEFAULT 0x0
> +
> +struct saa7115_platform_data {
> + /* Horizontal time constant */
> + u8 saa7113_r08_htc;
> +
> + u8 saa7113_r10_vrln;
> + u8 saa7113_r10_ofts;
> +
> + u8 saa7113_r12_rts0;
> + u8 saa7113_r12_rts1;
> +
> + u8 saa7113_r13_adlsb;
> +};
> +
> #endif
>
> --
> 1.8.2.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Em Wed, 29 May 2013 22:41:16 +0200
Jon Arne Jørgensen <[email protected]> escreveu:
> Change all default values in the initial setup table to match the table
> in the datasheet.
This is not a good idea, as it can produce undesired side effects
on the existing drivers that depend on it, and can't be easily
tested.
Please, don't change the current "default". It is, of course, OK
to change them if needed via the information provided inside the
platform data.
Regards,
Mauro
>
> Signed-off-by: Jon Arne Jørgensen <[email protected]>
> ---
> drivers/media/i2c/saa7115.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/i2c/saa7115.c b/drivers/media/i2c/saa7115.c
> index d6f589a..4403679 100644
> --- a/drivers/media/i2c/saa7115.c
> +++ b/drivers/media/i2c/saa7115.c
> @@ -223,12 +223,12 @@ static const unsigned char saa7111_init[] = {
> static const unsigned char saa7113_init[] = {
> R_01_INC_DELAY, 0x08,
> R_02_INPUT_CNTL_1, 0xc2,
> - R_03_INPUT_CNTL_2, 0x30,
> + R_03_INPUT_CNTL_2, 0x33,
> R_04_INPUT_CNTL_3, 0x00,
> R_05_INPUT_CNTL_4, 0x00,
> - R_06_H_SYNC_START, 0x89,
> + R_06_H_SYNC_START, 0xe9,
> R_07_H_SYNC_STOP, 0x0d,
> - R_08_SYNC_CNTL, 0x88,
> + R_08_SYNC_CNTL, 0x98,
> R_09_LUMA_CNTL, 0x01,
> R_0A_LUMA_BRIGHT_CNTL, 0x80,
> R_0B_LUMA_CONTRAST_CNTL, 0x47,
> @@ -236,11 +236,11 @@ static const unsigned char saa7113_init[] = {
> R_0D_CHROMA_HUE_CNTL, 0x00,
> R_0E_CHROMA_CNTL_1, 0x01,
> R_0F_CHROMA_GAIN_CNTL, 0x2a,
> - R_10_CHROMA_CNTL_2, 0x08,
> + R_10_CHROMA_CNTL_2, 0x00,
> R_11_MODE_DELAY_CNTL, 0x0c,
> - R_12_RT_SIGNAL_CNTL, 0x07,
> + R_12_RT_SIGNAL_CNTL, 0x01,
> R_13_RT_X_PORT_OUT_CNTL, 0x00,
> - R_14_ANAL_ADC_COMPAT_CNTL, 0x00,
> + R_14_ANAL_ADC_COMPAT_CNTL, 0x00, /* RESERVED */
> R_15_VGATE_START_FID_CHG, 0x00,
> R_16_VGATE_STOP, 0x00,
> R_17_MISC_VGATE_CONF_AND_MSB, 0x00,
--
Cheers,
Mauro
Em Wed, 29 May 2013 22:41:18 +0200
Jon Arne Jørgensen <[email protected]> escreveu:
> Implement i2c_board_info.platform_data handling in the driver so we can
> make device specific changes to the chips we support.
>
...
> +struct saa7115_platform_data {
> + /* Horizontal time constant */
> + u8 saa7113_r08_htc;
> +
> + u8 saa7113_r10_vrln;
> + u8 saa7113_r10_ofts;
> +
> + u8 saa7113_r12_rts0;
> + u8 saa7113_r12_rts1;
> +
> + u8 saa7113_r13_adlsb;
> +};
While this works, it makes harder to analyze what's changed there,
as the above nomenclature is too obfuscated.
The better would be if you could, instead, name the bits (or bytes)
that will require different data, like (I just got some random
bits from reg08, on saa7113 datasheet - I didn't actually checked
what bits are you using):
unsigned pll_closed: 1;
unsigned fast_mode: 1;
unsigned fast_locking: 1;
--
Cheers,
Mauro
Mauro Carvalho Chehab <[email protected]> wrote:
>Em Wed, 29 May 2013 22:41:16 +0200
>Jon Arne Jørgensen <[email protected]> escreveu:
>
>> Change all default values in the initial setup table to match the
>table
>> in the datasheet.
>
>This is not a good idea, as it can produce undesired side effects
>on the existing drivers that depend on it, and can't be easily
>tested.
>
>Please, don't change the current "default". It is, of course, OK
>to change them if needed via the information provided inside the
>platform data.
>
>Regards,
>Mauro
>>
>> Signed-off-by: Jon Arne Jørgensen <[email protected]>
>> ---
>> drivers/media/i2c/saa7115.c | 12 ++++++------
>> 1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/media/i2c/saa7115.c
>b/drivers/media/i2c/saa7115.c
>> index d6f589a..4403679 100644
>> --- a/drivers/media/i2c/saa7115.c
>> +++ b/drivers/media/i2c/saa7115.c
>> @@ -223,12 +223,12 @@ static const unsigned char saa7111_init[] = {
>> static const unsigned char saa7113_init[] = {
>> R_01_INC_DELAY, 0x08,
>> R_02_INPUT_CNTL_1, 0xc2,
>> - R_03_INPUT_CNTL_2, 0x30,
>> + R_03_INPUT_CNTL_2, 0x33,
>> R_04_INPUT_CNTL_3, 0x00,
>> R_05_INPUT_CNTL_4, 0x00,
>> - R_06_H_SYNC_START, 0x89,
>> + R_06_H_SYNC_START, 0xe9,
>> R_07_H_SYNC_STOP, 0x0d,
>> - R_08_SYNC_CNTL, 0x88,
>> + R_08_SYNC_CNTL, 0x98,
>> R_09_LUMA_CNTL, 0x01,
>> R_0A_LUMA_BRIGHT_CNTL, 0x80,
>> R_0B_LUMA_CONTRAST_CNTL, 0x47,
>> @@ -236,11 +236,11 @@ static const unsigned char saa7113_init[] = {
>> R_0D_CHROMA_HUE_CNTL, 0x00,
>> R_0E_CHROMA_CNTL_1, 0x01,
>> R_0F_CHROMA_GAIN_CNTL, 0x2a,
>> - R_10_CHROMA_CNTL_2, 0x08,
>> + R_10_CHROMA_CNTL_2, 0x00,
>> R_11_MODE_DELAY_CNTL, 0x0c,
>> - R_12_RT_SIGNAL_CNTL, 0x07,
>> + R_12_RT_SIGNAL_CNTL, 0x01,
>> R_13_RT_X_PORT_OUT_CNTL, 0x00,
>> - R_14_ANAL_ADC_COMPAT_CNTL, 0x00,
>> + R_14_ANAL_ADC_COMPAT_CNTL, 0x00, /* RESERVED */
>> R_15_VGATE_START_FID_CHG, 0x00,
>> R_16_VGATE_STOP, 0x00,
>> R_17_MISC_VGATE_CONF_AND_MSB, 0x00,
>
>
>--
>
>Cheers,
>Mauro
>--
>To unsubscribe from this list: send the line "unsubscribe linux-media"
>in
>the body of a message to [email protected]
>More majordomo info at http://vger.kernel.org/majordomo-info.html
I was going to make a comment along the same line as Mauro.
Please leave the driver defaults alone. It is almost impossible to regression test all the different devices with a SAA7113 chip, to ensure the change doesn't cause someone's device to not work properly.
Regards,
Andy
On Wed, May 29, 2013 at 10:19:49PM -0400, Andy Walls wrote:
> Mauro Carvalho Chehab <[email protected]> wrote:
>
> >Em Wed, 29 May 2013 22:41:16 +0200
> >Jon Arne Jørgensen <[email protected]> escreveu:
> >
> >> Change all default values in the initial setup table to match the
> >table
> >> in the datasheet.
> >
> >This is not a good idea, as it can produce undesired side effects
> >on the existing drivers that depend on it, and can't be easily
> >tested.
> >
> >Please, don't change the current "default". It is, of course, OK
> >to change them if needed via the information provided inside the
> >platform data.
> >
> >Regards,
> >Mauro
> >>
> >> Signed-off-by: Jon Arne Jørgensen <[email protected]>
> >> ---
> >> drivers/media/i2c/saa7115.c | 12 ++++++------
> >> 1 file changed, 6 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/media/i2c/saa7115.c
> >b/drivers/media/i2c/saa7115.c
> >> index d6f589a..4403679 100644
> >> --- a/drivers/media/i2c/saa7115.c
> >> +++ b/drivers/media/i2c/saa7115.c
> >> @@ -223,12 +223,12 @@ static const unsigned char saa7111_init[] = {
> >> static const unsigned char saa7113_init[] = {
> >> R_01_INC_DELAY, 0x08,
> >> R_02_INPUT_CNTL_1, 0xc2,
> >> - R_03_INPUT_CNTL_2, 0x30,
> >> + R_03_INPUT_CNTL_2, 0x33,
> >> R_04_INPUT_CNTL_3, 0x00,
> >> R_05_INPUT_CNTL_4, 0x00,
> >> - R_06_H_SYNC_START, 0x89,
> >> + R_06_H_SYNC_START, 0xe9,
> >> R_07_H_SYNC_STOP, 0x0d,
> >> - R_08_SYNC_CNTL, 0x88,
> >> + R_08_SYNC_CNTL, 0x98,
> >> R_09_LUMA_CNTL, 0x01,
> >> R_0A_LUMA_BRIGHT_CNTL, 0x80,
> >> R_0B_LUMA_CONTRAST_CNTL, 0x47,
> >> @@ -236,11 +236,11 @@ static const unsigned char saa7113_init[] = {
> >> R_0D_CHROMA_HUE_CNTL, 0x00,
> >> R_0E_CHROMA_CNTL_1, 0x01,
> >> R_0F_CHROMA_GAIN_CNTL, 0x2a,
> >> - R_10_CHROMA_CNTL_2, 0x08,
> >> + R_10_CHROMA_CNTL_2, 0x00,
> >> R_11_MODE_DELAY_CNTL, 0x0c,
> >> - R_12_RT_SIGNAL_CNTL, 0x07,
> >> + R_12_RT_SIGNAL_CNTL, 0x01,
> >> R_13_RT_X_PORT_OUT_CNTL, 0x00,
> >> - R_14_ANAL_ADC_COMPAT_CNTL, 0x00,
> >> + R_14_ANAL_ADC_COMPAT_CNTL, 0x00, /* RESERVED */
> >> R_15_VGATE_START_FID_CHG, 0x00,
> >> R_16_VGATE_STOP, 0x00,
> >> R_17_MISC_VGATE_CONF_AND_MSB, 0x00,
> >
> >
> >--
> >
> >Cheers,
> >Mauro
> >--
> >To unsubscribe from this list: send the line "unsubscribe linux-media"
> >in
> >the body of a message to [email protected]
> >More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> I was going to make a comment along the same line as Mauro.
> Please leave the driver defaults alone. It is almost impossible to regression test all the different devices with a SAA7113 chip, to ensure the change doesn't cause someone's device to not work properly.
>
You guys are totally right.
What if I clone the original saa7113_init table into a new one, and make
the driver use the new one if the calling driver sets platform_data.
Something like this?
switch (state->ident) {
case V4L2_IDENT_SAA7111:
case V4L2_IDENT_SAA7111A:
saa711x_writeregs(sd, saa7111_init);
break;
case V4L2_IDENT_GM7113C:
case V4L2_IDENT_SAA7113:
- saa711x_writeregs(sd, saa7113_init);
+ if (client->dev.platform_data)
+ saa711x_writeregs(sd, saa7113_new_init);
+ else
+ saa711x_writeregs(sd, saa7113_init);
break;
default:
state->crystal_freq = SAA7115_FREQ_32_11_MHZ;
saa711x_writeregs(sd, saa7115_init_auto_input);
}
if (state->ident > V4L2_IDENT_SAA7111A)
saa711x_writeregs(sd, saa7115_init_misc);
if (client->dev.platform_data) {
struct saa7115_platform_data *data = client->dev.platform_data;
saa7115_load_platform_data(state, data);
}
It's not strictly necessary, but it feels a lot cleaner?
Would you accept this into the kernel, or would it just increase
maintenance?
Best regards
Jon Arne Jørgensen
> Regards,
> Andy
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 30 May 2013 07:21:36 +0200
Jon Arne Jørgensen <[email protected]> wrote:
> On Wed, May 29, 2013 at 10:19:49PM -0400, Andy Walls wrote:
> > Mauro Carvalho Chehab <[email protected]> wrote:
> >
> > >Em Wed, 29 May 2013 22:41:16 +0200
> > >Jon Arne Jørgensen <[email protected]> escreveu:
> > >
> > >> Change all default values in the initial setup table to match the
> > >table
> > >> in the datasheet.
> > >
> > >This is not a good idea, as it can produce undesired side effects
> > >on the existing drivers that depend on it, and can't be easily
> > >tested.
> > >
> > >Please, don't change the current "default". It is, of course, OK
> > >to change them if needed via the information provided inside the
> > >platform data.
> >
> > I was going to make a comment along the same line as Mauro.
> > Please leave the driver defaults alone. It is almost impossible to
> > regression test all the different devices with a SAA7113 chip, to
> > ensure the change doesn't cause someone's device to not work
> > properly.
> >
>
> You guys are totally right.
>
> What if I clone the original saa7113_init table into a new one, and
> make the driver use the new one if the calling driver sets
> platform_data.
>
> Something like this?
>
> switch (state->ident) {
> case V4L2_IDENT_SAA7111:
> case V4L2_IDENT_SAA7111A:
> saa711x_writeregs(sd, saa7111_init);
> break;
> case V4L2_IDENT_GM7113C:
> case V4L2_IDENT_SAA7113:
> - saa711x_writeregs(sd, saa7113_init);
> + if (client->dev.platform_data)
> + saa711x_writeregs(sd, saa7113_new_init);
> + else
> + saa711x_writeregs(sd, saa7113_init);
I would rather have the platform_data provide the new table. Or if you
think bulk of the table will be the same for most users, then perhaps
add there an enum saying which table to use - and name the tables
according to the chip variant it applies to.
- Timo
On Thu, May 30, 2013 at 08:33:32AM +0300, Timo Teras wrote:
> On Thu, 30 May 2013 07:21:36 +0200
> Jon Arne Jørgensen <[email protected]> wrote:
>
> > On Wed, May 29, 2013 at 10:19:49PM -0400, Andy Walls wrote:
> > > Mauro Carvalho Chehab <[email protected]> wrote:
> > >
> > > >Em Wed, 29 May 2013 22:41:16 +0200
> > > >Jon Arne Jørgensen <[email protected]> escreveu:
> > > >
> > > >> Change all default values in the initial setup table to match the
> > > >table
> > > >> in the datasheet.
> > > >
> > > >This is not a good idea, as it can produce undesired side effects
> > > >on the existing drivers that depend on it, and can't be easily
> > > >tested.
> > > >
> > > >Please, don't change the current "default". It is, of course, OK
> > > >to change them if needed via the information provided inside the
> > > >platform data.
> > >
> > > I was going to make a comment along the same line as Mauro.
> > > Please leave the driver defaults alone. It is almost impossible to
> > > regression test all the different devices with a SAA7113 chip, to
> > > ensure the change doesn't cause someone's device to not work
> > > properly.
> > >
> >
> > You guys are totally right.
> >
> > What if I clone the original saa7113_init table into a new one, and
> > make the driver use the new one if the calling driver sets
> > platform_data.
> >
> > Something like this?
> >
> > switch (state->ident) {
> > case V4L2_IDENT_SAA7111:
> > case V4L2_IDENT_SAA7111A:
> > saa711x_writeregs(sd, saa7111_init);
> > break;
> > case V4L2_IDENT_GM7113C:
> > case V4L2_IDENT_SAA7113:
> > - saa711x_writeregs(sd, saa7113_init);
> > + if (client->dev.platform_data)
> > + saa711x_writeregs(sd, saa7113_new_init);
> > + else
> > + saa711x_writeregs(sd, saa7113_init);
>
> I would rather have the platform_data provide the new table. Or if you
> think bulk of the table will be the same for most users, then perhaps
> add there an enum saying which table to use - and name the tables
> according to the chip variant it applies to.
>
I think the bulk of the table will be the same for all drivers.
It's one bit here and one bit there that needs changing.
As the driver didn't support platform data.
Changing to a new init table for the drivers that implement
platform_data shouldn't cause any regressions.
Best regards
Jon Arne Jørgensen
> - Timo
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Em Thu, 30 May 2013 21:00:01 +0200
Jon Arne Jørgensen <[email protected]> escreveu:
> On Thu, May 30, 2013 at 08:33:32AM +0300, Timo Teras wrote:
> > On Thu, 30 May 2013 07:21:36 +0200
> > Jon Arne Jørgensen <[email protected]> wrote:
> >
> > > On Wed, May 29, 2013 at 10:19:49PM -0400, Andy Walls wrote:
> > > > Mauro Carvalho Chehab <[email protected]> wrote:
> > > >
> > > > >Em Wed, 29 May 2013 22:41:16 +0200
> > > > >Jon Arne Jørgensen <[email protected]> escreveu:
> > > > >
> > > > >> Change all default values in the initial setup table to match the
> > > > >table
> > > > >> in the datasheet.
> > > > >
> > > > >This is not a good idea, as it can produce undesired side effects
> > > > >on the existing drivers that depend on it, and can't be easily
> > > > >tested.
> > > > >
> > > > >Please, don't change the current "default". It is, of course, OK
> > > > >to change them if needed via the information provided inside the
> > > > >platform data.
> > > >
> > > > I was going to make a comment along the same line as Mauro.
> > > > Please leave the driver defaults alone. It is almost impossible to
> > > > regression test all the different devices with a SAA7113 chip, to
> > > > ensure the change doesn't cause someone's device to not work
> > > > properly.
> > > >
> > >
> > > You guys are totally right.
> > >
> > > What if I clone the original saa7113_init table into a new one, and
> > > make the driver use the new one if the calling driver sets
> > > platform_data.
> > >
> > > Something like this?
> > >
> > > switch (state->ident) {
> > > case V4L2_IDENT_SAA7111:
> > > case V4L2_IDENT_SAA7111A:
> > > saa711x_writeregs(sd, saa7111_init);
> > > break;
> > > case V4L2_IDENT_GM7113C:
> > > case V4L2_IDENT_SAA7113:
> > > - saa711x_writeregs(sd, saa7113_init);
> > > + if (client->dev.platform_data)
> > > + saa711x_writeregs(sd, saa7113_new_init);
> > > + else
> > > + saa711x_writeregs(sd, saa7113_init);
> >
> > I would rather have the platform_data provide the new table. Or if you
> > think bulk of the table will be the same for most users, then perhaps
> > add there an enum saying which table to use - and name the tables
> > according to the chip variant it applies to.
> >
>
> I think the bulk of the table will be the same for all drivers.
> It's one bit here and one bit there that needs changing.
> As the driver didn't support platform data.
> Changing to a new init table for the drivers that implement
> platform_data shouldn't cause any regressions.
There are several things that are very bad on passing a table via
platform data:
1) you're adding saa711x-specific data at the bridge driver,
so, the saa711x code is spread on several places at the
long term;
2) some part of the saa711x code may override the data there,
as it is not aware about what bits should be preserved from
the new device;
3) due (2), latter changes on the code are more likely to
cause regressions;
4) also due to (2), some hacks can be needed, in order to warn
saa711x to handle some things differently.
That's why it is a way better to add meaningful parameters telling what
bits are needed for the driver to work with the bridge. That's also
why we do this with all other drivers.
Regards,
Mauro
On Fri, 31 May 2013 10:08:27 -0300
Mauro Carvalho Chehab <[email protected]> wrote:
> Em Thu, 30 May 2013 21:00:01 +0200
> Jon Arne Jørgensen <[email protected]> escreveu:
>
> > On Thu, May 30, 2013 at 08:33:32AM +0300, Timo Teras wrote:
> > > I would rather have the platform_data provide the new table. Or
> > > if you think bulk of the table will be the same for most users,
> > > then perhaps add there an enum saying which table to use - and
> > > name the tables according to the chip variant it applies to.
> > >
> >
> > I think the bulk of the table will be the same for all drivers.
> > It's one bit here and one bit there that needs changing.
> > As the driver didn't support platform data.
> > Changing to a new init table for the drivers that implement
> > platform_data shouldn't cause any regressions.
>
> There are several things that are very bad on passing a table via
> platform data:
Sorry, my wording was self-conflicting. The intention was to
suggest providing an enum saying which table to use. Not that the
platform data would provide the whole table.
> 1) you're adding saa711x-specific data at the bridge driver,
> so, the saa711x code is spread on several places at the
> long term;
>
> 2) some part of the saa711x code may override the data there,
> as it is not aware about what bits should be preserved from
> the new device;
>
> 3) due (2), latter changes on the code are more likely to
> cause regressions;
>
> 4) also due to (2), some hacks can be needed, in order to warn
> saa711x to handle some things differently.
Agreed.
> That's why it is a way better to add meaningful parameters telling
> what bits are needed for the driver to work with the bridge. That's
> also why we do this with all other drivers.
Based on the latest patch, more of these bits need to be controlled
individually than I figured. So yes, individual meaningful bits do make
the most sense.
Thanks,
Timo
On Fri, May 31, 2013 at 10:08:27AM -0300, Mauro Carvalho Chehab wrote:
> Em Thu, 30 May 2013 21:00:01 +0200
> Jon Arne Jørgensen <[email protected]> escreveu:
>
> > On Thu, May 30, 2013 at 08:33:32AM +0300, Timo Teras wrote:
> > > On Thu, 30 May 2013 07:21:36 +0200
> > > Jon Arne Jørgensen <[email protected]> wrote:
> > >
> > > > On Wed, May 29, 2013 at 10:19:49PM -0400, Andy Walls wrote:
> > > > > Mauro Carvalho Chehab <[email protected]> wrote:
> > > > >
> > > > > >Em Wed, 29 May 2013 22:41:16 +0200
> > > > > >Jon Arne Jørgensen <[email protected]> escreveu:
> > > > > >
> > > > > >> Change all default values in the initial setup table to match the
> > > > > >table
> > > > > >> in the datasheet.
> > > > > >
> > > > > >This is not a good idea, as it can produce undesired side effects
> > > > > >on the existing drivers that depend on it, and can't be easily
> > > > > >tested.
> > > > > >
> > > > > >Please, don't change the current "default". It is, of course, OK
> > > > > >to change them if needed via the information provided inside the
> > > > > >platform data.
> > > > >
> > > > > I was going to make a comment along the same line as Mauro.
> > > > > Please leave the driver defaults alone. It is almost impossible to
> > > > > regression test all the different devices with a SAA7113 chip, to
> > > > > ensure the change doesn't cause someone's device to not work
> > > > > properly.
> > > > >
> > > >
> > > > You guys are totally right.
> > > >
> > > > What if I clone the original saa7113_init table into a new one, and
> > > > make the driver use the new one if the calling driver sets
> > > > platform_data.
> > > >
> > > > Something like this?
> > > >
> > > > switch (state->ident) {
> > > > case V4L2_IDENT_SAA7111:
> > > > case V4L2_IDENT_SAA7111A:
> > > > saa711x_writeregs(sd, saa7111_init);
> > > > break;
> > > > case V4L2_IDENT_GM7113C:
> > > > case V4L2_IDENT_SAA7113:
> > > > - saa711x_writeregs(sd, saa7113_init);
> > > > + if (client->dev.platform_data)
> > > > + saa711x_writeregs(sd, saa7113_new_init);
> > > > + else
> > > > + saa711x_writeregs(sd, saa7113_init);
> > >
> > > I would rather have the platform_data provide the new table. Or if you
> > > think bulk of the table will be the same for most users, then perhaps
> > > add there an enum saying which table to use - and name the tables
> > > according to the chip variant it applies to.
> > >
> >
> > I think the bulk of the table will be the same for all drivers.
> > It's one bit here and one bit there that needs changing.
> > As the driver didn't support platform data.
> > Changing to a new init table for the drivers that implement
> > platform_data shouldn't cause any regressions.
>
> There are several things that are very bad on passing a table via
> platform data:
>
> 1) you're adding saa711x-specific data at the bridge driver,
> so, the saa711x code is spread on several places at the
> long term;
>
> 2) some part of the saa711x code may override the data there,
> as it is not aware about what bits should be preserved from
> the new device;
>
> 3) due (2), latter changes on the code are more likely to
> cause regressions;
>
> 4) also due to (2), some hacks can be needed, in order to warn
> saa711x to handle some things differently.
>
> That's why it is a way better to add meaningful parameters telling what
> bits are needed for the driver to work with the bridge. That's also
> why we do this with all other drivers.
>
I agree with you here.
I never planned to pass the table via platform data.
I've just posted a new version of this patch set.
Please have a look.
Best regards
Jon Arne Jørgensen
> Regards,
> Mauro