linux-media.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Santiago Hormazabal <santiagohssl@gmail.com>
To: Joe Perches <joe@perches.com>
Cc: linux-media@vger.kernel.org, devicetree@vger.kernel.org,
	Rob Herring <robh+dt@kernel.org>,
	Ezequiel Garcia <ezequiel@collabora.com>,
	Hans Verkuil <hverkuil@xs4all.nl>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/3] media: Add support for the AM/FM radio chip KT0913 from KT Micro.
Date: Thu, 3 Sep 2020 16:51:13 -0300	[thread overview]
Message-ID: <CAHkAS4ZME-iFCuqTOkZT8r7UUEaB-Wp49Btq5HEWtS4pFGFD2Q@mail.gmail.com> (raw)
In-Reply-To: <6b225c10b6c71ffbc79c236b64dcc83fc33cc21b.camel@perches.com>

Hi Joe,
Thanks for the feedback.

On Thu, 3 Sep 2020 at 13:45, Joe Perches <joe@perches.com> 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.

  reply	other threads:[~2020-09-03 19:51 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-31 22:05 [PATCH 0/3] KT0913 FM/AM driver Santiago Hormazabal
2020-08-31 22:05 ` [PATCH 1/3] dt-bindings: vendor-prefixes: Add KT Micro Santiago Hormazabal
2020-09-14 18:27   ` Rob Herring
2020-08-31 22:06 ` [PATCH 2/3] media: Add support for the AM/FM radio chip KT0913 from " Santiago Hormazabal
2020-09-03 16:45   ` Joe Perches
2020-09-03 19:51     ` Santiago Hormazabal [this message]
2020-09-09 14:06   ` Hans Verkuil
2020-08-31 22:06 ` [PATCH 3/3] media: kt0913: device tree binding Santiago Hormazabal
2020-09-03 16:11   ` Rob Herring

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAHkAS4ZME-iFCuqTOkZT8r7UUEaB-Wp49Btq5HEWtS4pFGFD2Q@mail.gmail.com \
    --to=santiagohssl@gmail.com \
    --cc=devicetree@vger.kernel.org \
    --cc=ezequiel@collabora.com \
    --cc=hverkuil@xs4all.nl \
    --cc=joe@perches.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=mchehab@kernel.org \
    --cc=robh+dt@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).