Re: [PATCH] media: dvb_dummy_fe.c: add members to dvb_dummy_fe_state

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

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