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

[Mauro Carvalho Chehab <mchehab@kernel.org>]

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