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
next prev parent reply other threads:[~2020-04-07 17:13 UTC|newest]
Thread overview: 28+ 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:34 ` [PATCH v5 1/9] media: adv748x: fix end-of-line terminators in diagnostic statements Alex Riesen
2020-04-03 10:43 ` Kieran Bingham
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-03 10:48 ` Kieran Bingham
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-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-07 16:21 ` Kieran Bingham
2020-04-07 17:13 ` Alex Riesen [this message]
2020-04-07 18:44 ` Kieran Bingham
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 ` [PATCH v5 6/9] media: adv748x: prepare/enable mclk when the audio is used Alex Riesen
2020-06-18 16:23 ` Kieran Bingham
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-06-18 16:17 ` Kieran Bingham
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-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-06-18 16:32 ` Kieran Bingham
2020-06-19 9:34 ` Alex Riesen
2020-08-25 14:57 ` Kieran Bingham
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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).