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: Ulf Hansson <ulf.hansson@linaro.org>,
	Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Cc: DTML <devicetree@vger.kernel.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, 05 May 2020 18:05:53 +0200	[thread overview]
Message-ID: <1j1rnygye6.fsf@starbuckisacylon.baylibre.com> (raw)
In-Reply-To: <CAPDyKFq_USCNNps3s4+C_1hriycrxtRMKJvnPFcP59CZmLXbGw@mail.gmail.com>

On Tue 05 May 2020 at 10:17, Ulf Hansson <ulf.hansson@linaro.org> wrote:

> [...]
>
>> >> > +
>> >> > +     return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
>> >> > +                                        onecell_data);
>> >>
>> >> I think registering a provider for a module that does not provide clocks
>> >> to any other device is a bit overkill.
>> >>
>> >> I understand the matter is getting the per-user clk* pointer.
>> >> Since this is the module registering the clock, you can use clk_hw->clk
>> >> to get it.
>> >>
>> >> Once you have the clk* of the leaf clocks, you don't even need to keep
>> >> track of the clk_hw* since you are using devm_
>> >>
>> >> Afterward, we should propably discuss with Stephen if something should
>> >> be added in CCF to get a struct clk* from struct clk_hw*.
>> >>
>> >
>> > [...]
>> >
>> > Hmm.
>> >
>> > I am not sure the above is a good idea, at all. Unless, I am
>> > misunderstanding your point, which may be the case.
>> >
>> > I think above "shortcuts" could lead to abuse of the clock framework
>> > and its internal data structures. When going forward, this could make
>> > it unnecessary harder to maintain the clock framework.
>> >
>> > I know, it's not my responsibility, but from my experience with MMC
>> > and SDIO interfaces, is that those have been too easy abuse - since
>> > most of the data structures and interfaces have been exported. Now,
>> > it's hard to roll back that, if you see what I mean.
>>
>> Indeed, it worth clarifying this first.
>>
>> With clk_register deprecated in favor of clk_hw_register, we are likely
>> to see that case rise elsewhere.
>>
>
> So, according to the separate discussion [1], I think we can let
> Martin decide what option to implement at this point.
>
> 1. Implement the "clk_hw_get_clk()" approach. The preferred option,
> but requires wider changes of the clock subsystem as well.
>
> 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 ...

I have proposed 2 other short term solutions, let's see how it goes

>
> [...]
>
> Kind regards
> Uffe
>
> [1]
> https://www.spinics.net/lists/linux-clk/msg48373.html

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