Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <82828e89ccf4173de4e5e52dcecacc4d5168315c.1584720678.git.alexander.riesen@cetitec.com>
References: <cover.1584720678.git.alexander.riesen@cetitec.com> <82828e89ccf4173de4e5e52dcecacc4d5168315c.1584720678.git.alexander.riesen@cetitec.com>
Subject: Re: [PATCH v3 05/11] media: adv748x: add support for HDMI audio
From:   Stephen Boyd <sboyd@kernel.org>
Cc:     Geert Uytterhoeven <geert@linux-m68k.org>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Hans Verkuil <hverkuil-cisco@xs4all.nl>,
        Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
        Rob Herring <robh+dt@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>,
        devel@driverdev.osuosl.org, linux-media@vger.kernel.org,
        linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
        linux-renesas-soc@vger.kernel.org,
        linux-clk <linux-clk@vger.kernel.org>
To:     Alex Riesen <alexander.riesen@cetitec.com>,
        Kieran Bingham <kieran.bingham@ideasonboard.com>
Date:   Fri, 20 Mar 2020 18:09:31 -0700
Message-ID: <158475297119.125146.8177273856843293558@swboyd.mtv.corp.google.com>
User-Agent: alot/0.9
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Quoting Alex Riesen (2020-03-20 09:12:00)
> diff --git a/drivers/media/i2c/adv748x/adv748x-dai.c b/drivers/media/i2c/=
adv748x/adv748x-dai.c
> new file mode 100644
> index 000000000000..6fce7d000423
> --- /dev/null
> +++ b/drivers/media/i2c/adv748x/adv748x-dai.c
> @@ -0,0 +1,265 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Driver for Analog Devices ADV748X HDMI receiver with AFE
> + * The implementation of DAI.
> + */
> +
> +#include "adv748x.h"
> +
> +#include <linux/clk.h>

Is this include used? Please try to make a clk provider or a clk
consumer and not both unless necessary.

> +#include <linux/clk-provider.h>
> +#include <sound/pcm_params.h>
> +
> +#define state_of(soc_dai) \
> +       adv748x_dai_to_state(container_of((soc_dai)->driver, \
> +                                         struct adv748x_dai, \
> +                                         drv))
> +
> +static const char ADV748X_DAI_NAME[] =3D "adv748x-i2s";
> +
[...]
> +       .set_sysclk =3D adv748x_dai_set_sysclk,
> +       .set_fmt =3D adv748x_dai_set_fmt,
> +       .startup =3D adv748x_dai_startup,
> +       .hw_params =3D adv748x_dai_hw_params,
> +       .mute_stream =3D adv748x_dai_mute_stream,
> +       .shutdown =3D adv748x_dai_shutdown,
> +};
> +
> +static int adv748x_of_xlate_dai_name(struct snd_soc_component *component,
> +                                     struct of_phandle_args *args,
> +                                     const char **dai_name)
> +{
> +       if (dai_name)
> +               *dai_name =3D ADV748X_DAI_NAME;
> +       return 0;
> +}
> +
> +static const struct snd_soc_component_driver adv748x_codec =3D {
> +       .of_xlate_dai_name =3D adv748x_of_xlate_dai_name,
> +};
> +
> +int adv748x_dai_init(struct adv748x_dai *dai)
> +{
> +       int ret;
> +       struct adv748x_state *state =3D adv748x_dai_to_state(dai);
> +
> +       dai->mclk_name =3D kasprintf(GFP_KERNEL, "%s.%s-i2s-mclk",
> +                                  state->dev->driver->name,
> +                                  dev_name(state->dev));
> +       if (!dai->mclk_name) {
> +               ret =3D -ENOMEM;
> +               adv_err(state, "No memory for MCLK\n");
> +               goto fail;
> +       }
> +       dai->mclk =3D clk_register_fixed_rate(state->dev,

Please register with clk_hw_register_fixed_rate() instead.

> +                                           dai->mclk_name,
> +                                           NULL /* parent_name */,
> +                                           0 /* flags */,
> +                                           12288000 /* rate */);

Not sure these comments are useful.

> +       if (IS_ERR_OR_NULL(dai->mclk)) {
> +               ret =3D PTR_ERR(dai->mclk);
> +               adv_err(state, "Failed to register MCLK (%d)\n", ret);
> +               goto fail;
> +       }
> +       ret =3D of_clk_add_provider(state->dev->of_node, of_clk_src_simpl=
e_get,
> +                                 dai->mclk);
> +       if (ret < 0) {
> +               adv_err(state, "Failed to add MCLK provider (%d)\n", ret);
> +               goto unreg_mclk;
> +       }
> +       dai->drv.name =3D ADV748X_DAI_NAME;
> +       dai->drv.ops =3D &adv748x_dai_ops;
> +       dai->drv.capture =3D (struct snd_soc_pcm_stream){

Can this be const?

> +               .stream_name    =3D "Capture",
> +               .channels_min   =3D 8,
> +               .channels_max   =3D 8,
> +               .rates =3D SNDRV_PCM_RATE_48000,
> +               .formats =3D SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_U=
24_LE,
> +       };
> +
> +       ret =3D devm_snd_soc_register_component(state->dev, &adv748x_code=
c,
> +                                             &dai->drv, 1);
> +       if (ret < 0) {
> +               adv_err(state, "Failed to register the codec (%d)\n", ret=
);
> +               goto cleanup_mclk;
> +       }
> +       return 0;
> +
> +cleanup_mclk:
> +       of_clk_del_provider(state->dev->of_node);
> +unreg_mclk:
> +       clk_unregister_fixed_rate(dai->mclk);
> +fail:
> +       return ret;
> +}
> +
> +void adv748x_dai_cleanup(struct adv748x_dai *dai)
> +{
> +       struct adv748x_state *state =3D adv748x_dai_to_state(dai);
> +
> +       of_clk_del_provider(state->dev->of_node);
> +       clk_unregister_fixed_rate(dai->mclk);
> +       kfree(dai->mclk_name);
> +}
> +

Please drop extra newline at end of file.