Re: [PATCH 2/3] media: Add support for the AM/FM radio chip KT0913 from KT Micro.

[Santiago Hormazabal]

Hi Joe,
Thanks for the feedback.

On Thu, 3 Sep 2020 at 13:45, Joe Perches <joe@xxxxxxxxxxx> wrote:
>
> On Mon, 2020-08-31 at 19:06 -0300, Santiago Hormazabal wrote:
> > This chip requires almost no support components and can used over I2C.
> > The driver uses the I2C bus and exposes the controls as a V4L2 radio.
> > Tested with a module that contains this chip (from SZZSJDZ.com,
> > part number ZJ-801B, even tho the company seems defunct now), and an H2+
> > AllWinner SoC running a kernel built off 07d999f of the media_tree.
>
> Thanks.
>
> style trivia:
>
> []
> > diff --git a/drivers/media/radio/radio-kt0913.c b/drivers/media/radio/radio-kt0913.c
> []
> > +static const struct reg_sequence kt0913_init_regs_to_defaults[] = {
> > +     /* Standby disabled, volume 0dB */
> > +     { KT0913_REG_RXCFG, 0x881f },
>
> These might be more legible on single lines,
> ignoring the 80 column limits.
>
> > +     /* FM Channel spacing = 50kHz, Right & Left unmuted */
> > +     { KT0913_REG_SEEK, 0x000b },
>
> etc...
>
I agree, didn't know I could exceed the limit in these situations.

> []
>
> > +static int __kt0913_set_fm_frequency(struct kt0913_device *radio,
> > +                                  unsigned int frequency)
> > +{
> > +     return regmap_write(radio->regmap, KT0913_REG_TUNE,
> > +             KT0913_TUNE_FMTUNE_ON | (frequency / KT0913_FMCHAN_MUL));
>
> It might be nicer to align multi-line statements to the
> open parenthesis.
>
Yes, that's totally true. What about these cases where the other part
of the lines would exceed 80 chars? For instance, if I align the
second line to the open parenthesis of the first line, I'll be past
the 80 chars limit. Splitting it back again to fit that would make the
code not so much readable.

> []
>
> > +static int __kt0913_set_au_gain(struct kt0913_device *radio, s32 gain)
> > +{
> > +     switch (gain) {
> > +     case 6:
> > +             return regmap_update_bits(radio->regmap,
> > +                     KT0913_REG_AMSYSCFG, KT0913_AMSYSCFG_AU_GAIN_MASK,
> > +                     KT0913_AMSYSCFG_AU_GAIN_6DB);
> > +     case 3:
> > +             return regmap_update_bits(radio->regmap,
> > +                     KT0913_REG_AMSYSCFG, KT0913_AMSYSCFG_AU_GAIN_MASK,
> > +                     KT0913_AMSYSCFG_AU_GAIN_3DB);
> > +     case 0:
> > +             return regmap_update_bits(radio->regmap,
> > +                     KT0913_REG_AMSYSCFG, KT0913_AMSYSCFG_AU_GAIN_MASK,
> > +                     KT0913_AMSYSCFG_AU_GAIN_0DB);
> > +     case -3:
> > +             return regmap_update_bits(radio->regmap,
> > +                     KT0913_REG_AMSYSCFG, KT0913_AMSYSCFG_AU_GAIN_MASK,
> > +                     KT0913_AMSYSCFG_AU_GAIN_MIN_3DB);
> > +     default:
> > +             return -EINVAL;
> > +     }
> > +}
>
> It's generally more legible to write this with an intermediate
> variable holding the changed value.  It's also most commonly
> smaller object code.
>
> static int __kt0913_set_au_gain(struct kt0913_device *radio, s32 gain)
> {
>         int val;
>
>         switch (gain) {
>         case 6:
>                 val = KT0913_AMSYSCFG_AU_GAIN_6DB;
>                 break;
>         case 3:
>                 val = KT0913_AMSYSCFG_AU_GAIN_3DB;
>                 break;
>         case 0:
>                 val = KT0913_AMSYSCFG_AU_GAIN_0DB;
>                 break;
>         case -3:
>                 val = KT0913_AMSYSCFG_AU_GAIN_MIN_3DB;
>                 break;
>         default:
>                 return -EINVAL;
>         }
>
>         return regmap_update_bits(radio->regmap, KT0913_REG_AMSYSCFG,
>                                   KT0913_AMSYSCFG_AU_GAIN_MASK, val);
> }
>
>
I agree, thanks for your feedback.

I'll wait for your reply to fix the other issue you've mentioned, and
I'll fix the others in the meantime.

Thanks!

- Santiago H.