Em Mon, 2 Dec 2019 01:49:38 -0300
"Daniel W. S. Almeida" <dwlsalmeida@xxxxxxxxx> 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
