Messages in this thread |  | | Subject | Re: [PATCH v5 4/9] media: adv748x: add definitions for audio output related registers | From | Kieran Bingham <> | Date | Tue, 7 Apr 2020 17:21:00 +0100 |
| |
Hi Alex,
On 02/04/2020 19:34, Alex Riesen wrote: > Signed-off-by: Alexander Riesen <alexander.riesen@cetitec.com> > --- > drivers/media/i2c/adv748x/adv748x.h | 32 +++++++++++++++++++++++++++++ > 1 file changed, 32 insertions(+) > > diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h > index 0a9d78c2870b..1a1ea70086c6 100644 > --- a/drivers/media/i2c/adv748x/adv748x.h > +++ b/drivers/media/i2c/adv748x/adv748x.h > @@ -226,6 +226,11 @@ struct adv748x_state { > > #define ADV748X_IO_VID_STD 0x05 > > +#define ADV748X_IO_PAD_CONTROLS 0x0e > +#define ADV748X_IO_PAD_CONTROLS_TRI_AUD BIT(5) > +#define ADV748X_IO_PAD_CONTROLS_PDN_AUD BIT(1) > +#define ADV748X_IO_PAD_CONTROLS1 0x1d
What is CONTROLS1 (1d) referenced from here?
There's no 'field' matching for this register, and the 'bits' (0, 2, 3, 4) correspond to "pdn_spi, pdn_pix, '-', tri_spi"
Perhaps we need to define those bit fields accordingly and reference them where they get used directly?
Perhaps calling bit 3 as: #define ADV748X_IO_PAD_CONTROLS_BIT_3 BIT(3)
Or #define ADV748X_IO_PAD_CONTROLS_RESVD BIT(3)
(Unless you have documentation that better describes it?)
> + > #define ADV748X_IO_10 0x10 /* io_reg_10 */ > #define ADV748X_IO_10_CSI4_EN BIT(7) > #define ADV748X_IO_10_CSI1_EN BIT(6) > @@ -248,7 +253,21 @@ struct adv748x_state { > #define ADV748X_IO_REG_FF 0xff > #define ADV748X_IO_REG_FF_MAIN_RESET 0xff > > +/* DPLL Map */ > +#define ADV748X_DPLL_MCLK_FS 0xb5 > +#define ADV748X_DPLL_MCLK_FS_N_MASK GENMASK(2, 0) > + > /* HDMI RX Map */ > +#define ADV748X_HDMI_I2S 0x03 /* I2S mode and width */
Looks like a more appropriate name than the datasheets "hdmi_register_03h" :-D
> +#define ADV748X_HDMI_I2SBITWIDTH_MASK GENMASK(4, 0) > +#define ADV748X_HDMI_I2SOUTMODE_SHIFT 5 > +#define ADV748X_HDMI_I2SOUTMODE_MASK \ > + GENMASK(6, ADV748X_HDMI_I2SOUTMODE_SHIFT)
I'd be very tempted to ignore the 80char limit here and put that on the line above ... or find a way to remove the 1 character...
In fact, given the entry there - how about just leaving this as:
#define ADV748X_HDMI_I2SOUTMODE_MASK GENMASK(6, 5)
> +#define ADV748X_I2SOUTMODE_I2S 0 > +#define ADV748X_I2SOUTMODE_RIGHT_J 1 > +#define ADV748X_I2SOUTMODE_LEFT_J 2 > +#define ADV748X_I2SOUTMODE_SPDIF 3
Can we align these value in the column with the other values?
And as much as I hate long define names, it seems a bit odd that these suddenly lack the HDMI_ part of the define prefix...
Should we either remove the HDMI_ from ADV748X_HDMI_I2SBITWIDTH_MASK ADV748X_HDMI_I2SOUTMODE_SHIFT ADV748X_HDMI_I2SOUTMODE_MASK
or add it to ADV748X_I2SOUTMODE_I2S ADV748X_I2SOUTMODE_RIGHT_J ADV748X_I2SOUTMODE_LEFT_J ADV748X_I2SOUTMODE_SPDIF
?
> + > #define ADV748X_HDMI_LW1 0x07 /* line width_1 */ > #define ADV748X_HDMI_LW1_VERT_FILTER BIT(7) > #define ADV748X_HDMI_LW1_DE_REGEN BIT(5) > @@ -260,6 +279,16 @@ struct adv748x_state { > #define ADV748X_HDMI_F1H1 0x0b /* field1 height_1 */ > #define ADV748X_HDMI_F1H1_INTERLACED BIT(5) > > +#define ADV748X_HDMI_MUTE_CTRL 0x1a > +#define ADV748X_HDMI_MUTE_CTRL_MUTE_AUDIO BIT(4) > +#define ADV748X_HDMI_MUTE_CTRL_WAIT_UNMUTE_MASK GENMASK(3, 1) > +#define ADV748X_HDMI_MUTE_CTRL_NOT_AUTO_UNMUTE BIT(0) > + > +#define ADV748X_HDMI_AUDIO_MUTE_SPEED 0x0f
Can we keep the register definitions in address order please?
> +#define ADV748X_HDMI_AUDIO_MUTE_SPEED_MASK GENMASK(4, 0) > +#define ADV748X_MAN_AUDIO_DL_BYPASS BIT(7) > +#define ADV748X_AUDIO_DELAY_LINE_BYPASS BIT(6)
Those bits do not describe the register they are in, not sure how to address that without exceptionally long names though.. :-(
So perhaps how you've got them might be the best option...
> + > #define ADV748X_HDMI_HFRONT_PORCH 0x20 /* hsync_front_porch_1 */ > #define ADV748X_HDMI_HFRONT_PORCH_MASK 0x1fff > > @@ -281,6 +310,9 @@ struct adv748x_state { > #define ADV748X_HDMI_TMDS_1 0x51 /* hdmi_reg_51 */ > #define ADV748X_HDMI_TMDS_2 0x52 /* hdmi_reg_52 */ > > +#define ADV748X_HDMI_REG_6D 0x6d /* hdmi_reg_6d */ > +#define ADV748X_I2S_TDM_MODE_ENABLE BIT(7) > + > /* HDMI RX Repeater Map */ > #define ADV748X_REPEATER_EDID_SZ 0x70 /* primary_edid_size */ > #define ADV748X_REPEATER_EDID_SZ_SHIFT 4 >
-- Regards -- Kieran
|  |