From: Alex Riesen <alexander.riesen@cetitec.com> To: Kieran Bingham <kieran.bingham@ideasonboard.com> Cc: Geert Uytterhoeven <geert@linux-m68k.org>, Mauro Carvalho Chehab <mchehab@kernel.org>, Hans Verkuil <hverkuil-cisco@xs4all.nl>, Laurent Pinchart <laurent.pinchart@ideasonboard.com>, Rob Herring <robh+dt@kernel.org>, Mark Rutland <mark.rutland@arm.com>, Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>, devel@driverdev.osuosl.org, linux-media@vger.kernel.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, linux-renesas-soc@vger.kernel.org Subject: Re: [PATCH v5 4/9] media: adv748x: add definitions for audio output related registers Date: Tue, 7 Apr 2020 19:13:27 +0200 [thread overview] Message-ID: <20200407171327.GA4711@pflmari> (raw) In-Reply-To: <a0ff0a59-bd6e-044b-5669-679126c23323@ideasonboard.com> Hi Kieran, Kieran Bingham, Tue, Apr 07, 2020 18:21:00 +0200: > On 02/04/2020 19:34, Alex Riesen wrote: > > 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? I wish I knew... I afraid this is a left-over from the early development attempts. It is obviously a mask of some bits. I don't even use the _CONTROLS1 anymore. Removed the #define. > 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) I would prefer _BIT_3, if only to stay as opaque as the documentation. > (Unless you have documentation that better describes it?) Mine matches what you described above. Do you mind if I describe the other bits of the register even though the driver does not use them? Just for completeness sake (and while I still have access to the documentation). > > @@ -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 It was derived from the map and prefix of its bit-fields: i2soutmode and i2sbitwidth. I too felt the name hdmi_register_03h lacking of depth. > > +#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) No problem. Reformatted with two spaces. > > +#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? Alignment corrected. > 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 Well, I see no reason for them to stand out like this, so perhaps I better add the prefix. I didn't add the prefix initially because they weren't names of fields or registers, but names of values of a field (i2soutmode of that hdmi_register_03h). But I see there is a precedent for such already: ADV748X_CP_{CON,SAT,BRI}_{MIN,DEF,MAX}, so prefix is okay. > > @@ -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? Done. > > +#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... Yes, please. Besides, they aren't even obviously related to the audio mute speed. And I corrected the alignment. > > +#define ADV748X_HDMI_REG_6D 0x6d /* hdmi_reg_6d */ > > +#define ADV748X_I2S_TDM_MODE_ENABLE BIT(7) Alignment corrected. Regards, Alex
WARNING: multiple messages have this Message-ID (diff)
From: Alex Riesen <alexander.riesen@cetitec.com> To: Kieran Bingham <kieran.bingham@ideasonboard.com> Cc: Mark Rutland <mark.rutland@arm.com>, devel@driverdev.osuosl.org, Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-renesas-soc@vger.kernel.org, Rob Herring <robh+dt@kernel.org>, Geert Uytterhoeven <geert@linux-m68k.org>, Laurent Pinchart <laurent.pinchart@ideasonboard.com>, Hans Verkuil <hverkuil-cisco@xs4all.nl>, Mauro Carvalho Chehab <mchehab@kernel.org>, linux-media@vger.kernel.org Subject: Re: [PATCH v5 4/9] media: adv748x: add definitions for audio output related registers Date: Tue, 7 Apr 2020 19:13:27 +0200 [thread overview] Message-ID: <20200407171327.GA4711@pflmari> (raw) In-Reply-To: <a0ff0a59-bd6e-044b-5669-679126c23323@ideasonboard.com> Hi Kieran, Kieran Bingham, Tue, Apr 07, 2020 18:21:00 +0200: > On 02/04/2020 19:34, Alex Riesen wrote: > > 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? I wish I knew... I afraid this is a left-over from the early development attempts. It is obviously a mask of some bits. I don't even use the _CONTROLS1 anymore. Removed the #define. > 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) I would prefer _BIT_3, if only to stay as opaque as the documentation. > (Unless you have documentation that better describes it?) Mine matches what you described above. Do you mind if I describe the other bits of the register even though the driver does not use them? Just for completeness sake (and while I still have access to the documentation). > > @@ -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 It was derived from the map and prefix of its bit-fields: i2soutmode and i2sbitwidth. I too felt the name hdmi_register_03h lacking of depth. > > +#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) No problem. Reformatted with two spaces. > > +#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? Alignment corrected. > 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 Well, I see no reason for them to stand out like this, so perhaps I better add the prefix. I didn't add the prefix initially because they weren't names of fields or registers, but names of values of a field (i2soutmode of that hdmi_register_03h). But I see there is a precedent for such already: ADV748X_CP_{CON,SAT,BRI}_{MIN,DEF,MAX}, so prefix is okay. > > @@ -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? Done. > > +#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... Yes, please. Besides, they aren't even obviously related to the audio mute speed. And I corrected the alignment. > > +#define ADV748X_HDMI_REG_6D 0x6d /* hdmi_reg_6d */ > > +#define ADV748X_I2S_TDM_MODE_ENABLE BIT(7) Alignment corrected. Regards, Alex _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
next prev parent reply other threads:[~2020-04-07 17:13 UTC|newest] Thread overview: 56+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-04-02 18:33 [PATCH v5 0/9] media: adv748x: add support for HDMI audio Alex Riesen 2020-04-02 18:33 ` Alex Riesen 2020-04-02 18:34 ` [PATCH v5 1/9] media: adv748x: fix end-of-line terminators in diagnostic statements Alex Riesen 2020-04-02 18:34 ` Alex Riesen 2020-04-03 10:43 ` Kieran Bingham 2020-04-03 10:43 ` Kieran Bingham 2020-04-03 11:01 ` Alex Riesen 2020-04-03 11:01 ` Alex Riesen 2020-04-02 18:34 ` [PATCH v5 2/9] media: adv748x: include everything adv748x.h needs into the file Alex Riesen 2020-04-02 18:34 ` Alex Riesen 2020-04-03 10:48 ` Kieran Bingham 2020-04-03 10:48 ` Kieran Bingham 2020-04-03 11:02 ` Alex Riesen 2020-04-03 11:02 ` Alex Riesen 2020-04-02 18:34 ` [PATCH v5 3/9] media: adv748x: reduce amount of code for bitwise modifications of device registers Alex Riesen 2020-04-02 18:34 ` Alex Riesen 2020-04-03 19:09 ` Kieran Bingham 2020-04-03 19:09 ` Kieran Bingham 2020-04-02 18:34 ` [PATCH v5 4/9] media: adv748x: add definitions for audio output related registers Alex Riesen 2020-04-02 18:34 ` Alex Riesen 2020-04-07 16:21 ` Kieran Bingham 2020-04-07 16:21 ` Kieran Bingham 2020-04-07 17:13 ` Alex Riesen [this message] 2020-04-07 17:13 ` Alex Riesen 2020-04-07 18:44 ` Kieran Bingham 2020-04-07 18:44 ` Kieran Bingham 2020-04-07 18:55 ` Alex Riesen 2020-04-07 18:55 ` Alex Riesen 2020-04-02 18:34 ` [PATCH v5 5/9] media: adv748x: add support for HDMI audio Alex Riesen 2020-04-02 18:34 ` Alex Riesen 2020-04-02 18:34 ` [PATCH v5 6/9] media: adv748x: prepare/enable mclk when the audio is used Alex Riesen 2020-04-02 18:34 ` Alex Riesen 2020-06-18 16:23 ` Kieran Bingham 2020-06-18 16:23 ` Kieran Bingham 2020-06-19 8:51 ` Alex Riesen 2020-06-19 8:51 ` Alex Riesen 2020-04-02 18:34 ` [PATCH v5 7/9] media: adv748x: only activate DAI if it is described in device tree Alex Riesen 2020-04-02 18:34 ` Alex Riesen 2020-06-18 16:17 ` Kieran Bingham 2020-06-18 16:17 ` Kieran Bingham 2020-06-19 9:10 ` Alex Riesen 2020-06-19 9:10 ` Alex Riesen 2020-04-02 18:35 ` [PATCH v5 8/9] dt-bindings: adv748x: add information about serial audio interface (I2S/TDM) Alex Riesen 2020-04-02 18:35 ` Alex Riesen 2020-06-18 16:27 ` Kieran Bingham 2020-06-18 16:27 ` Kieran Bingham 2020-04-02 18:35 ` [PATCH v5 9/9] arm64: dts: renesas: salvator: add a connection from adv748x codec (HDMI input) to the R-Car SoC Alex Riesen 2020-04-02 18:35 ` Alex Riesen 2020-06-18 16:32 ` Kieran Bingham 2020-06-18 16:32 ` Kieran Bingham 2020-06-19 9:34 ` Alex Riesen 2020-06-19 9:34 ` Alex Riesen 2020-08-25 14:57 ` Kieran Bingham 2020-08-25 14:57 ` Kieran Bingham 2020-09-14 8:17 ` Alex Riesen 2020-09-14 8:17 ` Alex Riesen
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20200407171327.GA4711@pflmari \ --to=alexander.riesen@cetitec.com \ --cc=devel@driverdev.osuosl.org \ --cc=devicetree@vger.kernel.org \ --cc=geert@linux-m68k.org \ --cc=hverkuil-cisco@xs4all.nl \ --cc=kieran.bingham@ideasonboard.com \ --cc=kuninori.morimoto.gx@renesas.com \ --cc=laurent.pinchart@ideasonboard.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-media@vger.kernel.org \ --cc=linux-renesas-soc@vger.kernel.org \ --cc=mark.rutland@arm.com \ --cc=mchehab@kernel.org \ --cc=robh+dt@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.