[Linux-kernel-mentees] [PATCH] media: dvb_dummy_fe.c: add members to dvb_dummy_fe_state

[Linux-kernel-mentees] [PATCH] media: dvb_dummy_fe.c: add members to dvb_dummy_fe_state

[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

Re: [Linux-kernel-mentees] [PATCH] media: dvb_dummy_fe.c: add members to dvb_dummy_fe_state

[Mauro Carvalho Chehab]

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

Re: [Linux-kernel-mentees] [PATCH] media: dvb_dummy_fe.c: add members to dvb_dummy_fe_state

[Daniel W. S. Almeida]

[-- 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 --]

[-- 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

Re: [Linux-kernel-mentees] [PATCH] media: dvb_dummy_fe.c: add members to dvb_dummy_fe_state

[Mauro Carvalho Chehab]

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