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

Daniel W. S. Almeida dwlsalmeida at gmail.com
Mon Dec 2 04:49:38 UTC 2019


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.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linuxfoundation.org/pipermail/linux-kernel-mentees/attachments/20191202/fcbc5545/attachment-0001.html>


More information about the Linux-kernel-mentees mailing list