Re: [PATCHv3 1/2] tda18250: support for new silicon tuner

[Mauro Carvalho Chehab <mchehab@s-opensource.com>]

Em Mon, 30 Oct 2017 05:31:40 +0200
Olli Salonen <olli.salonen@iki.fi> escreveu:

> Hello Michael,
> 
> Many thanks for taking the time to review the patches.
> 
> On 27 October 2017 at 13:27, Michael Ira Krufky <mkrufky@linuxtv.org> wrote:
> >> +static int tda18250_sleep(struct dvb_frontend *fe)
> >> +{
> >> +       struct i2c_client *client = fe->tuner_priv;
> >> +       struct tda18250_dev *dev = i2c_get_clientdata(client);
> >> +       int ret;
> >> +
> >> +       dev_dbg(&client->dev, "\n");
> >> +
> >> +       /* power down LNA */
> >> +       ret = regmap_write_bits(dev->regmap, R0C_AGC11, 0x80, 0x00);
> >> +       if (ret)
> >> +               return ret;
> >> +
> >> +       ret = tda18250_power_control(fe, TDA18250_POWER_STANDBY);
> >> +       return ret;
> >> +}  
> >
> > Do we know for sure if the IF_FREQUENCY is preserved after returning
> > from a sleep?   It might be a good idea to set `dev->if_frequency = 0`
> > within `tda18250_sleep` to be sure that it gets set again on the next
> > tune, but you may want to check the specification first, if its
> > available.
> >
> > This is not a show-stopper -- We can merge this as-is and this can be
> > handled in a follow-up patch.  
> 
> I will look into this and send a patch on top of this one if needed.
> 
> Thank you for pointing it out.

There is a show-stopper issue here, though: it lacks adding an
entry for the driver at MAINTAINERS file :-)

Please add it.

While here, please look at the checkpatch warnings:

WARNING: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
#746: FILE: drivers/media/tuners/tda18250.c:678:
+ msleep(5);

and the 80 column ones. I was unable to see, at the places it complained,
a reason why not limit the lines to 80 columns.

Regards,

Thanks,
Mauro