Linux Kernel Mentees Archive on lore.kernel.org
 help / color / Atom feed
* [Linux-kernel-mentees] [PATCH] media: dvb_dummy_fe.c: add members to dvb_dummy_fe_state
@ 2019-11-30  4:54 Daniel W. S. Almeida
  2019-12-01  8:07 ` Mauro Carvalho Chehab
  0 siblings, 1 reply; 4+ messages in thread
From: Daniel W. S. Almeida @ 2019-11-30  4:54 UTC (permalink / raw)
  To: mchehab, gregkh, rfontana, kstewart, tglx
  Cc: linux-kernel, linux-media, linux-kernel-mentees, Daniel W. S. Almeida

From: "Daniel W. S. Almeida" <dwlsalmeida@gmail.com>

Add members to dvb_dummy_fe_state in order to match with other frontends.

Signed-off-by: Daniel W. S. Almeida <dwlsalmeida@gmail.com>
---
 drivers/media/dvb-frontends/dvb_dummy_fe.c | 26 +++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/media/dvb-frontends/dvb_dummy_fe.c b/drivers/media/dvb-frontends/dvb_dummy_fe.c
index 1ccb58c67e8e..80e6a3bf76e0 100644
--- a/drivers/media/dvb-frontends/dvb_dummy_fe.c
+++ b/drivers/media/dvb-frontends/dvb_dummy_fe.c
@@ -15,18 +15,29 @@
 
 DVB_DEFINE_MOD_OPT_ADAPTER_NR(adapter_nr);
 
+struct dvb_dummy_fe_config {};
+
 struct dvb_dummy_fe_state {
 	struct dvb_frontend frontend;
+	struct mutex lock;
+	struct dvb_adapter adapter;
+	struct dvb_frontend frontend;
+	struct dvb_dummy_fe_config config;
+
+	enum fe_status frontend_status;
+	u32 current_frequency;
+
+	bool sleeping;
 };
 
+
+
 static int dvb_dummy_fe_read_status(struct dvb_frontend *fe,
 				    enum fe_status *status)
 {
-	*status = FE_HAS_SIGNAL
-		| FE_HAS_CARRIER
-		| FE_HAS_VITERBI
-		| FE_HAS_SYNC
-		| FE_HAS_LOCK;
+	struct dvb_dummy_fe_state *state = fe->demodulator_priv;
+
+	*status = state->frontend_status;
 
 	return 0;
 }
@@ -79,6 +90,11 @@ static int dvb_dummy_fe_set_frontend(struct dvb_frontend *fe)
 
 static int dvb_dummy_fe_sleep(struct dvb_frontend* fe)
 {
+
+	struct dvb_dummy_fe_state *state = fe->demodulator_priv;
+
+	state->sleeping = true;
+
 	return 0;
 }
 
-- 
2.24.0

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH] media: dvb_dummy_fe.c: add members to dvb_dummy_fe_state
  2019-11-30  4:54 [Linux-kernel-mentees] [PATCH] media: dvb_dummy_fe.c: add members to dvb_dummy_fe_state Daniel W. S. Almeida
@ 2019-12-01  8:07 ` Mauro Carvalho Chehab
  2019-12-02  4:49   ` Daniel W. S. Almeida
  0 siblings, 1 reply; 4+ messages in thread
From: Mauro Carvalho Chehab @ 2019-12-01  8:07 UTC (permalink / raw)
  To: Daniel W. S. Almeida
  Cc: kstewart, linux-kernel, rfontana, tglx, linux-kernel-mentees,
	linux-media

Em Sat, 30 Nov 2019 01:54:20 -0300
"Daniel W. S. Almeida" <dwlsalmeida@gmail.com> escreveu:

> From: "Daniel W. S. Almeida" <dwlsalmeida@gmail.com>
> 
> Add members to dvb_dummy_fe_state in order to match with other frontends.
> 
> Signed-off-by: Daniel W. S. Almeida <dwlsalmeida@gmail.com>
> ---
>  drivers/media/dvb-frontends/dvb_dummy_fe.c | 26 +++++++++++++++++-----
>  1 file changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/dvb-frontends/dvb_dummy_fe.c b/drivers/media/dvb-frontends/dvb_dummy_fe.c
> index 1ccb58c67e8e..80e6a3bf76e0 100644
> --- a/drivers/media/dvb-frontends/dvb_dummy_fe.c
> +++ b/drivers/media/dvb-frontends/dvb_dummy_fe.c
> @@ -15,18 +15,29 @@
>  
>  DVB_DEFINE_MOD_OPT_ADAPTER_NR(adapter_nr);
>  
> +struct dvb_dummy_fe_config {};
> +
>  struct dvb_dummy_fe_state {
>  	struct dvb_frontend frontend;
> +	struct mutex lock;
> +	struct dvb_adapter adapter;
> +	struct dvb_frontend frontend;
> +	struct dvb_dummy_fe_config config;
> +
> +	enum fe_status frontend_status;
> +	u32 current_frequency;

While the above will very likely makes sense, once we add the missing
functionality at the dummy frontend, please don't add fields at the
struct while they're not used, as this makes harder for reviewers to be
sure that we're not adding bloatware at the code.

> +
> +	bool sleeping;
>  };
>  
> +
> +
>  static int dvb_dummy_fe_read_status(struct dvb_frontend *fe,
>  				    enum fe_status *status)
>  {
> -	*status = FE_HAS_SIGNAL
> -		| FE_HAS_CARRIER
> -		| FE_HAS_VITERBI
> -		| FE_HAS_SYNC
> -		| FE_HAS_LOCK;
> +	struct dvb_dummy_fe_state *state = fe->demodulator_priv;
> +
> +	*status = state->frontend_status;

That sounds wrong to me, at least on this patch as-is. Please remember that
we want one logical change per patch.

It means that, if you add a state->frontend_status at the driver, the
patch should implement the entire logic for it.

In other words, when the device is not tuned, status should return 0 and
when the device is tuned, it should return:

  FE_HAS_SIGNAL | FE_HAS_CARRIER | FE_HAS_VITERBI | FE_HAS_SYNC | FE_HAS_LOCK


So, while it is OK to move the status into a var at state, you need also
to modify the set_frontend part of the code for it to properly initalize
the state->frontend_status var.

>  
>  	return 0;
>  }
> @@ -79,6 +90,11 @@ static int dvb_dummy_fe_set_frontend(struct dvb_frontend *fe)
>  
>  static int dvb_dummy_fe_sleep(struct dvb_frontend* fe)
>  {
> +
> +	struct dvb_dummy_fe_state *state = fe->demodulator_priv;
> +
> +	state->sleeping = true;
> +

Hmm...what's the sense of adding it? Where are you setting it to false?
Where are you using the sleeping state?

>  	return 0;
>  }
>  

Cheers,
Mauro
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH] media: dvb_dummy_fe.c: add members to dvb_dummy_fe_state
  2019-12-01  8:07 ` Mauro Carvalho Chehab
@ 2019-12-02  4:49   ` Daniel W. S. Almeida
  2019-12-02  5:50     ` Mauro Carvalho Chehab
  0 siblings, 1 reply; 4+ messages in thread
From: Daniel W. S. Almeida @ 2019-12-02  4:49 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: kstewart, linux-kernel, rfontana, tglx, linux-kernel-mentees,
	linux-media

[-- Attachment #1.1: Type: text/plain, Size: 1592 bytes --]

Hi Mauro, thanks for checking out my work!

> please don't add fields at the
> struct while they're not used, as this makes harder for reviewers to be
> sure that we're not adding bloatware at the code.

OK.

> Please remember that
> we want one logical change per patch.
>
> It means that, if you add a state->frontend_status at the driver, the
> patch should implement the entire logic for it.
I will keep this in mind.

>   static int dvb_dummy_fe_sleep(struct dvb_frontend* fe)
>   {
> +
> +	struct dvb_dummy_fe_state *state = fe->demodulator_priv;
> +
> +	state->sleeping = true;
> +

> Hmm...what's the sense of adding it? Where are you setting it to false?
> Where are you using the sleeping state?

I noted some drivers seem to instruct the hardware into a low power state. Take helene, for instance:

static int helene_sleep(struct dvb_frontend *fe)
{
	struct helene_priv *priv = fe->tuner_priv;

	dev_dbg(&priv->i2c->dev, "%s()\n", __func__);
	helene_enter_power_save(priv);
	return 0;
}

static int helene_enter_power_save(struct helene_priv *priv)
{
	dev_dbg(&priv->i2c->dev, "%s()\n", __func__);
	if (priv->state == STATE_SLEEP)
		return 0;

	/* Standby setting for CPU */
	helene_write_reg(priv, 0x88, 0x0);

	/* Standby setting for internal logic block */
	helene_write_reg(priv, 0x87, 0xC0);

	priv->state = STATE_SLEEP;
	return 0;
}

As we are emulating some piece of hardware I thought it would be enough to add a simple bool to indicate that the device was sleeping.

This patch was a bit convoluted on my part. Let's iron out the issues you pointed out in v2.

- Daniel.


[-- Attachment #1.2: Type: text/html, Size: 2141 bytes --]

<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
  </head>
  <body>
    <pre class="moz-quote-pre" wrap="">Hi Mauro, thanks for checking out my work!

<blockquote type="cite"><pre class="moz-quote-pre" wrap="">please don't add fields at the
struct while they're not used, as this makes harder for reviewers to be
sure that we're not adding bloatware at the code.</pre>
</blockquote>
OK.

<blockquote type="cite"><pre class="moz-quote-pre" wrap="">Please remember that
we want one logical change per patch.

It means that, if you add a state-&gt;frontend_status at the driver, the
patch should implement the entire logic for it.</pre>
</blockquote>I will keep this in mind.

<blockquote type="cite"><pre class="moz-quote-pre" wrap=""> static int dvb_dummy_fe_sleep(struct dvb_frontend* fe)
 {
+
+	struct dvb_dummy_fe_state *state = fe-&gt;demodulator_priv;
+
+	state-&gt;sleeping = true;
+</pre></blockquote>
<blockquote type="cite"><pre class="moz-quote-pre" wrap="">Hmm...what's the sense of adding it? Where are you setting it to false?
Where are you using the sleeping state?
</pre>
</blockquote>
I noted some drivers seem to instruct the hardware into a low power state. Take helene, for instance:

static int helene_sleep(struct dvb_frontend *fe)
{
	struct helene_priv *priv = fe-&gt;tuner_priv;

	dev_dbg(&amp;priv-&gt;i2c-&gt;dev, "%s()\n", __func__);
	helene_enter_power_save(priv);
	return 0;
}
<style type="text/css">p, li { white-space: pre-wrap; }
</style>
static int helene_enter_power_save(struct helene_priv *priv)
{
	dev_dbg(&amp;priv-&gt;i2c-&gt;dev, "%s()\n", __func__);
	if (priv-&gt;state == STATE_SLEEP)
		return 0;

	/* Standby setting for CPU */
	helene_write_reg(priv, 0x88, 0x0);

	/* Standby setting for internal logic block */
	helene_write_reg(priv, 0x87, 0xC0);

	priv-&gt;state = STATE_SLEEP;
	return 0;
}

As we are emulating some piece of hardware I thought it would be enough to add a simple bool to indicate that the device was sleeping.

This patch was a bit convoluted on my part. Let's iron out the issues you pointed out in v2.

- Daniel.
</pre>
  </body>
</html>

[-- Attachment #2: Type: text/plain, Size: 201 bytes --]

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH] media: dvb_dummy_fe.c: add members to dvb_dummy_fe_state
  2019-12-02  4:49   ` Daniel W. S. Almeida
@ 2019-12-02  5:50     ` Mauro Carvalho Chehab
  0 siblings, 0 replies; 4+ messages in thread
From: Mauro Carvalho Chehab @ 2019-12-02  5:50 UTC (permalink / raw)
  To: Daniel W. S. Almeida
  Cc: kstewart, linux-kernel, rfontana, tglx, linux-kernel-mentees,
	linux-media

Em Mon, 2 Dec 2019 01:49:38 -0300
"Daniel W. S. Almeida" <dwlsalmeida@gmail.com> escreveu:

> Hi Mauro, thanks for checking out my work!
> 
> > please don't add fields at the
> > struct while they're not used, as this makes harder for reviewers to be
> > sure that we're not adding bloatware at the code.  
> 
> OK.
> 
> > Please remember that
> > we want one logical change per patch.
> >
> > It means that, if you add a state->frontend_status at the driver, the
> > patch should implement the entire logic for it.  
> I will keep this in mind.
> 
> >   static int dvb_dummy_fe_sleep(struct dvb_frontend* fe)
> >   {
> > +
> > +	struct dvb_dummy_fe_state *state = fe->demodulator_priv;
> > +
> > +	state->sleeping = true;
> > +  
> 
> > Hmm...what's the sense of adding it? Where are you setting it to false?
> > Where are you using the sleeping state?  
> 
> I noted some drivers seem to instruct the hardware into a low power state. Take helene, for instance:
> 
> static int helene_sleep(struct dvb_frontend *fe)
> {
> 	struct helene_priv *priv = fe->tuner_priv;
> 
> 	dev_dbg(&priv->i2c->dev, "%s()\n", __func__);
> 	helene_enter_power_save(priv);
> 	return 0;
> }
> 
> static int helene_enter_power_save(struct helene_priv *priv)
> {
> 	dev_dbg(&priv->i2c->dev, "%s()\n", __func__);
> 	if (priv->state == STATE_SLEEP)
> 		return 0;
> 
> 	/* Standby setting for CPU */
> 	helene_write_reg(priv, 0x88, 0x0);
> 
> 	/* Standby setting for internal logic block */
> 	helene_write_reg(priv, 0x87, 0xC0);
> 
> 	priv->state = STATE_SLEEP;
> 	return 0;
> }
> 
> As we are emulating some piece of hardware I thought it would be enough to add a simple bool to indicate that the device was sleeping.

Yeah, that could have some value. Yet, if you're doing that, I suggest
to add a function to change the device power state, as this is what a
real driver would do. Something similar to:

static inline void change_power_state(struct state *st, bool on) 
{
	/* A real driver would have commands here to wake/sleep the dev */

	st->power = on;
}

In any case, a real device on sleeping mode will not be tuning any
channels, so all stats will reflect that, e. g:

	- frontend status will return 0;
	- stats that depends on having a lock will be set with:
		FE_SCALE_NOT_AVAILABLE
	  tip: most stats are like that
	- stats like signal strength should probably return 0.

Not so sure what SNR will return, but probably it should return
FE_SCALE_NOT_AVAILABLE too, as this is usually calculated indirectly
once the device is locked.

On other words, only signal strength and stats should return a value
with would be 0 on both cases. All other stats should return
FE_SCALE_NOT_AVAILABLE.

Btw, as this depends on having the stats implemented, I would suggest you to
first finish the tuning and stats part of the driver. Only after having those
patches, apply the power mode patch.

> 
> This patch was a bit convoluted on my part. Let's iron out the issues you pointed out in v2.
> 
> - Daniel.

Cheers,
Mauro
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-30  4:54 [Linux-kernel-mentees] [PATCH] media: dvb_dummy_fe.c: add members to dvb_dummy_fe_state Daniel W. S. Almeida
2019-12-01  8:07 ` Mauro Carvalho Chehab
2019-12-02  4:49   ` Daniel W. S. Almeida
2019-12-02  5:50     ` Mauro Carvalho Chehab

Linux Kernel Mentees Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-kernel-mentees/0 linux-kernel-mentees/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-kernel-mentees linux-kernel-mentees/ https://lore.kernel.org/linux-kernel-mentees \
		linux-kernel-mentees@lists.linuxfoundation.org
	public-inbox-index linux-kernel-mentees

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.linuxfoundation.lists.linux-kernel-mentees


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git