Re: [PATCH v6 2/2] mmc: host: meson-mx-sdhc: new driver for the Amlogic Meson SDHC host

From: Jerome Brunet <jbrunet@baylibre.com>
To: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Cc: DTML <devicetree@vger.kernel.org>,
	Ulf Hansson <ulf.hansson@linaro.org>,
	Jianxin Pan <jianxin.pan@amlogic.com>,
	Stephen Boyd <sboyd@kernel.org>,
	"linux-mmc@vger.kernel.org" <linux-mmc@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	yinxin_1989@aliyun.com, Anand Moon <linux.amoon@gmail.com>,
	Rob Herring <robh+dt@kernel.org>,
	"open list:ARM/Amlogic Meson..."
	<linux-amlogic@lists.infradead.org>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	lnykww@gmail.com
Subject: Re: [PATCH v6 2/2] mmc: host: meson-mx-sdhc: new driver for the Amlogic Meson SDHC host
Date: Tue, 12 May 2020 09:09:08 +0200	[thread overview]
Message-ID: <1j1rnpfx4b.fsf@starbuckisacylon.baylibre.com> (raw)
In-Reply-To: <CAFBinCAPGwb4YKJvUSyvzSC7hpVO0z-Ju_pBWx-06QzYXc0orw@mail.gmail.com>

On Sun 10 May 2020 at 22:52, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:

> Hi Jerome,
>
> On Tue, May 5, 2020 at 6:05 PM Jerome Brunet <jbrunet@baylibre.com> wrote:
> [...]
>> > 2. Keep the existing approach, with devm_clk_get(). I am fine with
>> > this as well, we can always switch to 1) later on.
>>
>> I have a problem with this approach.
>> The dt-bindings would include "#clock-cells = <1>" for a device that
>> does not actually provide and only needs it has a temporary work around.
>> Those bindings are supposed to be stable ...
> actually I don't see a problem here because this specific MMC
> controller has a clock controller built into it.

Clock controller is a bit exagerated. It's an MMC controller with a
composite input clock and a couple of gates. A fairly common setup.

Also the property does not indicate a "clock controller" (or any number
of clock hosted by the device) but the ability to provide clocks out of
the device.

This device does not actually provide clock externally. Your provider
is just meant to be used internally. It is a work around using DT for
something missing in CCF.

IHMO, it is not such a big deal but since the binding are supposed to be
stable, I'm just pointing out that it is not great.

> Rob also didn't see any problem with this when he reviewed the dt-bindings

Again the bindings would be fine if the component was actually providing
the clocks, AFAIU.

>
>> I have proposed 2 other short term solutions, let's see how it goes
> since I was also curious how this turns out I first implemented your
> suggestion to use a similar clk_hw registration style as
> dwmac-meson8b.
> That made the code easier to read - thank you for the suggestion!
>
> On top of that I switched from clk_hw_onecell_data to directly
> accessing "clk_hw.clk".
> Unfortunately the diffstat is not as great as I hoped, it saves 21
> lines (11 in the driver code, 6 in the soc.dtsi, 5 in the dt-bindings)
> Once devm_clk_hw_get_clk() is implemented 8 lines have to be added
> again for error checking.
> I attached the patch for the drivers/mmc/host/meson-mx-sdhc* parts if
> you are curious (it'll apply only on top of my github branch, not on
> this series).

Diffstat was not my concern, TBH

>
> Please let me know if you want me to submit an updated series where I
> directly access "clk_hw.clk"

I'd be happy if you did

> to get the "struct clk" or if I should
> keep clk_hw_onecell_data.
> If it's the former then I'll also add a TODO comment for the
> conversion to devm_clk_hw_get_clk() so it's easy to find.
>

Perfect !

>
> Regards,
> Martin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel