Re: [PATCH 2/3] iio: make use of iio_device_attach_kfifo_buffer() where straightforward
How do you think about a patch subject like “iio: Increase use of iio_device_attach_kfifo_buffer()”? > This change does that. I suggest to improve also this commit message. * Would you like to consider a wording like “Convert a specific function call combination to a better programming interface.”? * Do you imagine any more software fine-tuning because of related collateral evolution? Regards, Markus ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/3] iio: make use of iio_device_attach_kfifo_buffer() where straightforward
On Tue, 2020-04-07 at 11:26 +0200, Markus Elfring wrote: > [External] > > How do you think about a patch subject like “iio: Increase use of > iio_device_attach_kfifo_buffer()”? > > > > This change does that. > > I suggest to improve also this commit message. > > * Would you like to consider a wording like “Convert a specific function call > combination to a better programming interface.”? > > * Do you imagine any more software fine-tuning because of related > collateral evolution? > I'll see. This patchset is kind of stopped. Will need a rework for it. > Regards, > Markus ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Information
My name is Reem E. Al-Hashimi, the Emirates Minister of State and Managing Director of United Arab Emirates (Dubai) World Expo 2020 Committee. I am writing you to stand as my partner to receive my share of gratification from foreign companies whom I helped during the bidding exercise towards the Dubai World Expo 2020 Committee and also i want to use this funds assist Corona virus Symptoms and Causes. Am a single Arab women and serving as a minister, there is a limit to my personal income and investment level and For this reason, I cannot receive such a huge sum back to my country or my personal account, so an agreement was reached with the foreign companies to direct the gratifications to an open beneficiary account with a financial institution where it will be possible for me to instruct further transfer of the fund to a third party account for investment purpose which is the reason i contacted you to receive the fund as my partner for investment in your country. The amount is valued at Euro 47,745,533.00 with a financial institution waiting my instruction for further transfer to a destination account as soon as I have your information indicating interest to receive and invest the fund, I will compensate you with 30% of the total amount and you will also get benefit from the investment. If you can handle the fund in a good investment. reply on this email only: reemalhash...@daum.net Regards, Ms. Reem ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging: rtl8192e: remove set but not used variable 'tmpRegC'
On Tue, Apr 07, 2020 at 08:36:04AM -0400, Wang Hai wrote:
> Fixes gcc '-Wunused-but-set-variable' warning:
>
> drivers/staging/rtl8192e/rtl8192e/r8192E_dev.c: In function
> rtl92e_start_adapter:
> drivers/staging/rtl8192e/rtl8192e/r8192E_dev.c:693:15: warning: variable
> ‘tmpRegC’ set but not used [-Wunused-but-set-variable]
>
> commit 94a799425eee ("rtl8192e: Split into two directories")
> involved this, remove it.
>
> Reported-by: Hulk Robot
> Signed-off-by: Wang Hai
> ---
> drivers/staging/rtl8192e/rtl8192e/r8192E_dev.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/rtl8192e/rtl8192e/r8192E_dev.c
> b/drivers/staging/rtl8192e/rtl8192e/r8192E_dev.c
> index ddcd788..ff934ae 100644
> --- a/drivers/staging/rtl8192e/rtl8192e/r8192E_dev.c
> +++ b/drivers/staging/rtl8192e/rtl8192e/r8192E_dev.c
> @@ -690,7 +690,7 @@ bool rtl92e_start_adapter(struct net_device *dev)
> u8 tmpvalue;
> u8 ICVersion, SwitchingRegulatorOutput;
> bool bfirmwareok = true;
> - u32 tmpRegA, tmpRegC, TempCCk;
> + u32 tmpRegA, TempCCk;
> int i = 0;
> u32 retry_times = 0;
>
> @@ -889,8 +889,8 @@ bool rtl92e_start_adapter(struct net_device *dev)
> if (priv->IC_Cut >= IC_VersionCut_D) {
> tmpRegA = rtl92e_get_bb_reg(dev, rOFDM0_XATxIQImbalance,
> bMaskDWord);
> - tmpRegC = rtl92e_get_bb_reg(dev, rOFDM0_XCTxIQImbalance,
> - bMaskDWord);
> + rtl92e_get_bb_reg(dev, rOFDM0_XCTxIQImbalance,
> + bMaskDWord);
Delete the call as well.
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/3] staging: vt6656: Use define instead of magic number for tx_rate
On Mon, Apr 06, 2020 at 07:58:08PM +0200, Greg Kroah-Hartman wrote:
> On Mon, Apr 06, 2020 at 06:38:36PM +0200, Oscar Carter wrote:
> > On Mon, Apr 06, 2020 at 04:22:12PM +0200, Greg Kroah-Hartman wrote:
> > > On Sat, Apr 04, 2020 at 04:13:59PM +0200, Oscar Carter wrote:
> > > > Use the define RATE_11M present in the file "device.h" instead of the
> > > > magic number 3. So the code is more clear.
> > > >
> > > > Signed-off-by: Oscar Carter
> > > > Reviewed-by: Dan Carpenter
> > > > ---
> > > > drivers/staging/vt6656/baseband.c | 3 ++-
> > > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/staging/vt6656/baseband.c
> > > > b/drivers/staging/vt6656/baseband.c
> > > > index 3e4bd637849a..a785f91c1566 100644
> > > > --- a/drivers/staging/vt6656/baseband.c
> > > > +++ b/drivers/staging/vt6656/baseband.c
> > > > @@ -24,6 +24,7 @@
> > > >
> > > > #include
> > > > #include
> > > > +#include "device.h"
> > > > #include "mac.h"
> > > > #include "baseband.h"
> > > > #include "rf.h"
> > > > @@ -141,7 +142,7 @@ unsigned int vnt_get_frame_time(u8 preamble_type,
> > > > u8 pkt_type,
> > > >
> > > > rate = (unsigned int)vnt_frame_time[tx_rate];
> > > >
> > > > - if (tx_rate <= 3) {
> > > > + if (tx_rate <= RATE_11M) {
> > > > if (preamble_type == 1)
> > > > preamble = 96;
> > > > else
> > > > --
> > > > 2.20.1
> > >
> > > This doesn't apply to my tree :(
> > >
> > Sorry, but I don't understand what it means. This meant that I need to
> > rebase
> > this patch against your staging-next branch of your staging tree ?
>
> Yes, and 3/3 as well, because I dropped the 1/3 patch here.
>
Ok, I will create a new patch series version rebased against your staging-next
branch and I will send it.
> thanks,
>
> greg k-h
thanks,
oscar carter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v5 4/9] media: adv748x: add definitions for audio output related registers
Hi Alex,
On 02/04/2020 19:34, Alex Riesen wrote:
> Signed-off-by: Alexander Riesen
> ---
> 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_100x10/* io_reg_10 */
> #define ADV748X_IO_10_CSI4_ENBIT(7)
> #define ADV748X_IO_10_CSI1_ENBIT(6)
> @@ -248,7 +253,21 @@ struct adv748x_state {
> #define ADV748X_IO_REG_FF0xff
> #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_MASKGENMASK(4, 0)
> +#define ADV748X_HDMI_I2SOUTMODE_SHIFT5
> +#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_MASKGENMASK(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_REGENBIT(5)
> @@ -260,6 +279,16 @@ struct adv748x_state {
> #define ADV748X_HDMI_F1H10x0b/* 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_SPEED0x0f
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_PORCH0x20/* 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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 1/2] staging: vt6656: Use define instead of magic number for tx_rate
Use the define RATE_11M present in the file "device.h" instead of the
magic number 3. So the code is more clear.
Reviewed-by: Dan Carpenter
Signed-off-by: Oscar Carter
---
drivers/staging/vt6656/baseband.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/vt6656/baseband.c
b/drivers/staging/vt6656/baseband.c
index a19a563d8bcc..092e56668a09 100644
--- a/drivers/staging/vt6656/baseband.c
+++ b/drivers/staging/vt6656/baseband.c
@@ -24,6 +24,7 @@
#include
#include
+#include "device.h"
#include "mac.h"
#include "baseband.h"
#include "rf.h"
@@ -141,7 +142,7 @@ unsigned int vnt_get_frame_time(u8 preamble_type, u8
pkt_type,
rate = (unsigned int)vnt_frame_time[tx_rate];
- if (tx_rate <= 3) {
+ if (tx_rate <= RATE_11M) {
if (preamble_type == 1)
preamble = 96;
else
--
2.20.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 2/2] staging: vt6656: Remove unnecessary local variable initialization
Don't initialize the rate variable as it is set a few lines later.
Reviewed-by: Dan Carpenter
Signed-off-by: Oscar Carter
---
drivers/staging/vt6656/baseband.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/vt6656/baseband.c
b/drivers/staging/vt6656/baseband.c
index 092e56668a09..5d9bc97916a5 100644
--- a/drivers/staging/vt6656/baseband.c
+++ b/drivers/staging/vt6656/baseband.c
@@ -135,7 +135,7 @@ unsigned int vnt_get_frame_time(u8 preamble_type, u8
pkt_type,
{
unsigned int frame_time;
unsigned int preamble;
- unsigned int rate = 0;
+ unsigned int rate;
if (tx_rate > RATE_54M)
return 0;
--
2.20.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 0/2] staging: vt6656: Cleanup of the vnt_get_frame_time function
This patch series makes a cleanup of the vnt_get_frame_time function. The first patch makes use of the define RATE_11M instead of a magic number. The second patch remove unnecessary local variable initialization. Changelog v1 -> v2 - Not use the ARRAY_SIZE macro to compare against the tx_rate variable. Oscar Carter (2): staging: vt6656: Use define instead of magic number for tx_rate staging: vt6656: Remove unnecessary local variable initialization drivers/staging/vt6656/baseband.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) -- 2.20.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v3 0/2] staging: vt6656: Cleanup of the vnt_get_frame_time function
This patch series makes a cleanup of the vnt_get_frame_time function. The first patch makes use of the define RATE_11M instead of a magic number. The second patch remove unnecessary local variable initialization. Changelog v1 -> v2 - Not use the ARRAY_SIZE macro to compare against the tx_rate variable. Changelog v2 -> v3 - Use the version number in the subject line of patch 1/2 and 2/2. Oscar Carter (2): staging: vt6656: Use define instead of magic number for tx_rate staging: vt6656: Remove unnecessary local variable initialization drivers/staging/vt6656/baseband.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) -- 2.20.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v3 1/2] staging: vt6656: Use define instead of magic number for tx_rate
Use the define RATE_11M present in the file "device.h" instead of the
magic number 3. So the code is more clear.
Reviewed-by: Dan Carpenter
Signed-off-by: Oscar Carter
---
drivers/staging/vt6656/baseband.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/vt6656/baseband.c
b/drivers/staging/vt6656/baseband.c
index a19a563d8bcc..092e56668a09 100644
--- a/drivers/staging/vt6656/baseband.c
+++ b/drivers/staging/vt6656/baseband.c
@@ -24,6 +24,7 @@
#include
#include
+#include "device.h"
#include "mac.h"
#include "baseband.h"
#include "rf.h"
@@ -141,7 +142,7 @@ unsigned int vnt_get_frame_time(u8 preamble_type, u8
pkt_type,
rate = (unsigned int)vnt_frame_time[tx_rate];
- if (tx_rate <= 3) {
+ if (tx_rate <= RATE_11M) {
if (preamble_type == 1)
preamble = 96;
else
--
2.20.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v3 2/2] staging: vt6656: Remove unnecessary local variable initialization
Don't initialize the rate variable as it is set a few lines later.
Reviewed-by: Dan Carpenter
Signed-off-by: Oscar Carter
---
drivers/staging/vt6656/baseband.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/vt6656/baseband.c
b/drivers/staging/vt6656/baseband.c
index 092e56668a09..5d9bc97916a5 100644
--- a/drivers/staging/vt6656/baseband.c
+++ b/drivers/staging/vt6656/baseband.c
@@ -135,7 +135,7 @@ unsigned int vnt_get_frame_time(u8 preamble_type, u8
pkt_type,
{
unsigned int frame_time;
unsigned int preamble;
- unsigned int rate = 0;
+ unsigned int rate;
if (tx_rate > RATE_54M)
return 0;
--
2.20.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 0/2] staging: vt6656: Cleanup of the vnt_get_frame_time function
On Tue, Apr 07, 2020 at 06:29:57PM +0200, Oscar Carter wrote: > This patch series makes a cleanup of the vnt_get_frame_time function. > > The first patch makes use of the define RATE_11M instead of a magic > number. The second patch remove unnecessary local variable initialization. > > Changelog v1 -> v2 > - Not use the ARRAY_SIZE macro to compare against the tx_rate variable. > > Oscar Carter (2): > staging: vt6656: Use define instead of magic number for tx_rate > staging: vt6656: Remove unnecessary local variable initialization > > drivers/staging/vt6656/baseband.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > -- > 2.20.1 > Don't review this patch series as I have sent a new version. Sorry. Thanks Oscar Carter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v5 4/9] media: adv748x: add definitions for audio output related registers
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_CONTROLS0x0e
> > +#define ADV748X_IO_PAD_CONTROLS_TRI_AUDBIT(5)
> > +#define ADV748X_IO_PAD_CONTROLS_PDN_AUDBIT(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_3BIT(3)
>
> Or
> #define ADV748X_IO_PAD_CONTROLS_RESVDBIT(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_MASKGENMASK(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_MASKGENMASK(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_6D0x6d/* hdmi_reg_6d */
> > +#define ADV748X_I2S_TDM_MODE_ENABLE BIT(7)
Alignment corrected.
Regards,
Alex
___
Re: [PATCH v5 4/9] media: adv748x: add definitions for audio output related registers
Hi Alex,
With all the changes you've described below:
Reviewed-by: Kieran Bingham
On 07/04/2020 18:13, Alex Riesen wrote:
> 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_CONTROLS0x0e
>>> +#define ADV748X_IO_PAD_CONTROLS_TRI_AUDBIT(5)
>>> +#define ADV748X_IO_PAD_CONTROLS_PDN_AUDBIT(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).
I'm fine describing those extra BIT()s correctly.
>
>>> @@ -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_MASKGENMASK(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_MASKGENMASK(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. Besid
Re: [PATCH v5 4/9] media: adv748x: add definitions for audio output related registers
Kieran Bingham, Tue, Apr 07, 2020 20:44:04 +0200:
> Hi Alex,
>
> With all the changes you've described below:
>
> Reviewed-by: Kieran Bingham
>
Thanks. Will be in v6 like this below:
diff --git a/drivers/media/i2c/adv748x/adv748x.h
b/drivers/media/i2c/adv748x/adv748x.h
index 0a9d78c2870b..e1d8cb01ebf8 100644
--- a/drivers/media/i2c/adv748x/adv748x.h
+++ b/drivers/media/i2c/adv748x/adv748x.h
@@ -226,6 +226,16 @@ struct adv748x_state {
#define ADV748X_IO_VID_STD 0x05
+#define ADV748X_IO_PAD_CONTROLS0x0e
+#define ADV748X_IO_PAD_CONTROLS_TRI_LLCBIT(7)
+#define ADV748X_IO_PAD_CONTROLS_TRI_PIXBIT(6)
+#define ADV748X_IO_PAD_CONTROLS_TRI_AUDBIT(5)
+#define ADV748X_IO_PAD_CONTROLS_TRI_SPIBIT(4)
+#define ADV748X_IO_PAD_CONTROLS_BIT_3 BIT(3)
+#define ADV748X_IO_PAD_CONTROLS_PDN_PIXBIT(2)
+#define ADV748X_IO_PAD_CONTROLS_PDN_AUDBIT(1)
+#define ADV748X_IO_PAD_CONTROLS_PDN_SPIBIT(0)
+
#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 +258,20 @@ 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_MASKGENMASK(2, 0)
+
/* HDMI RX Map */
+#define ADV748X_HDMI_I2S 0x03/* I2S mode and width */
+#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)
+#define ADV748X_HDMI_I2SOUTMODE_I2S0
+#define ADV748X_HDMI_I2SOUTMODE_RIGHT_J1
+#define ADV748X_HDMI_I2SOUTMODE_LEFT_J 2
+#define ADV748X_HDMI_I2SOUTMODE_SPDIF 3
+
#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 +283,16 @@ struct adv748x_state {
#define ADV748X_HDMI_F1H1 0x0b/* field1 height_1 */
#define ADV748X_HDMI_F1H1_INTERLACED BIT(5)
+#define ADV748X_HDMI_AUDIO_MUTE_SPEED 0x0f
+#define ADV748X_HDMI_AUDIO_MUTE_SPEED_MASK GENMASK(4, 0)
+#define ADV748X_MAN_AUDIO_DL_BYPASSBIT(7)
+#define ADV748X_AUDIO_DELAY_LINE_BYPASSBIT(6)
+
+#define ADV748X_HDMI_MUTE_CTRL 0x1a
+#define ADV748X_HDMI_MUTE_CTRL_MUTE_AUDIO BIT(4)
+#define ADV748X_HDMI_MUTE_CTRL_WAIT_UNMUTE_MASKGENMASK(3, 1)
+#define ADV748X_HDMI_MUTE_CTRL_NOT_AUTO_UNMUTE BIT(0)
+
#define ADV748X_HDMI_HFRONT_PORCH 0x20/* hsync_front_porch_1 */
#define ADV748X_HDMI_HFRONT_PORCH_MASK 0x1fff
@@ -281,6 +314,9 @@ struct adv748x_state {
#define ADV748X_HDMI_TMDS_10x51/* hdmi_reg_51 */
#define ADV748X_HDMI_TMDS_20x52/* hdmi_reg_52 */
+#define ADV748X_HDMI_REG_6D0x6d/* hdmi_reg_6d */
+#define ADV748X_I2S_TDM_MODE_ENABLEBIT(7)
+
/* HDMI RX Repeater Map */
#define ADV748X_REPEATER_EDID_SZ 0x70/* primary_edid_size */
#define ADV748X_REPEATER_EDID_SZ_SHIFT 4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
CURSOS BONIFICABLES DESDE CASA (Empleados activos y en ERTE)
Buenos días Se encuentra abierto el plazo de inscripción de Cursos Bonificables para empleados en activo y en situación de ERTE. Todos los cursos son totalmente Bonificables con cargo al Crédito de Formación 2020 que dispone las empresa. Se realizan desde casa en modalidad individual E-learning a través de la plataforma web y con total flexibilidad horaria. Deseáis que os mandemos la información? Saludos cordiales. Alex Pons Director departamento formación. FOESCO Formación Estatal Continua. Entidad Organizadora: B171823AP www.foesco.com e-mail: cur...@foesco.net Tel: 910 323 794 (Horario de 9h a 15h y de 17h a 20h de Lunes a Viernes) FOESCO ofrece formación a empresas y trabajadores en activo a través de cursos bonificados por la Fundación Estatal para la Formación en el Empleo (antiguo FORCEM) que gestiona las acciones formativas de FORMACIÓN CONTINUA para trabajadores y se rige por la ley 30/2015 de 9 de Septiembre. Si no desea recibir mas información de FOESCO responda a este correo con la palabra BAJA en el asunto. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
