All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Andrew F. Davis" <afd@ti.com>
To: Mark Brown <broonie@kernel.org>
Cc: Rob Herring <robh+dt@kernel.org>, Pawel Moll <pawel.moll@arm.com>,
	Mark Rutland <mark.rutland@arm.com>,
	Ian Campbell <ijc+devicetree@hellion.org.uk>,
	Kumar Gala <galak@codeaurora.org>,
	Lee Jones <lee.jones@linaro.org>,
	Alexandre Courbot <gnurou@gmail.com>,
	Grygorii Strashko <grygorii.strashko@ti.com>,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 4/5] regulator: tps65912: Add regulator driver for the TPS65912 PMIC
Date: Fri, 23 Oct 2015 19:11:56 -0500	[thread overview]
Message-ID: <562ACCCC.503@ti.com> (raw)
In-Reply-To: <20151023231822.GQ29919@sirena.org.uk>

On 10/23/2015 06:18 PM, Mark Brown wrote:
> On Fri, Oct 23, 2015 at 07:46:39AM -0500, Andrew F. Davis wrote:
>
>> I know just because other drivers do it doesn't mean it's a good idea,
>> but this is not new for MFDs and it is done in other regulators as well
>> (mt6397, tps659038, qcom,spmi, etc..).
>
> mt6397 doesn't do this, it doesn't have a compatible string at all (it's
> doing what I'm recommending that you do).  The SPMI devices are
> standalone devices, their parent device is actually functioning as a bus
> controller here (it's really a microcontroller inside the SoC).  The
> Palmas is part of how we realised this was a problem.
>

mt6397: Documentation/devicetree/bindings/mfd/mt6397.txt
Doing exactly what I'm doing,

pmic {
	compatible = "mediatek,mt6397";

	codec: mt6397codec {
		compatible = "mediatek,mt6397-codec";
	};

	regulators {
		compatible = "mediatek,mt6397-regulator";

		buck_vpca15 {
			....

The Palmas is a great example of why this is a good idea, there are
so many spins on this common base, and look how we can re-use sub-nodes:

tps659038: tps659038@58 {
	compatible = "ti,tps659038";
	reg = <0x58>;
	...

	tps659038_pmic {
		compatible = "ti,tps659038-pmic";
		...
	};

	tps659038_rtc: tps659038_rtc {
		compatible = "ti,palmas-rtc";
		...
	};

	tps659038_pwr_button: tps659038_pwr_button {
		compatible = "ti,palmas-pwrbutton";
		...
	};

	tps659038_gpio: tps659038_gpio {
		compatible = "ti,palmas-gpio";
		...
	};
};

(from am57xx-beagle-x15.dts)

looks like only the "ti,tps659038-pmic" node needed re-made without
re-making the whole driver.

>>> It seems like this is describing how Linux
>>> loads drivers not how the hardware is constructed but DT should describe
>>> the hardware.
>
>> While I agree to a point, if we follow this to its logical conclusion we
>> would end up with one compatible binding per SoC and be basically back to
>> board files. We need some granularity, just finding out where is the issue,
>
> The fact that the SoC DT is not distinct from the board DT is actually
> one of the problems with the way we're using DT at the minute, it means
> that DTBs are much less stable than they should be since we can enhance
> support for SoCs but DTBs need regenerating to take advantage of it.  It
> would be much better if the boards just referenced the SoC they use and
> pulled in a separate definition of the SoC (DT overlays will make it
> much more tractable to implement that if someone has time...).
>

I figured this can already be done by keeping the SoC stuff in dtsi files?
Anyway DT seems to have a lot of use issues with how it is being used, but
I'm probably not a person with enough free time for fixing that.. :|

>> I would say that as these devices belong to different subsystems and are
>> almost completely independent there should be no problem with having their
>> own compatible matched hardware sub-node.
>
> All it's adding is more typing for users.
>

Well I have to match the sub-devices on something, it's ether the node name
or the compatible string, so they might have to get used to typing :)

WARNING: multiple messages have this Message-ID (diff)
From: "Andrew F. Davis" <afd@ti.com>
To: Mark Brown <broonie@kernel.org>
Cc: Rob Herring <robh+dt@kernel.org>, Pawel Moll <pawel.moll@arm.com>,
	Mark Rutland <mark.rutland@arm.com>,
	Ian Campbell <ijc+devicetree@hellion.org.uk>,
	Kumar Gala <galak@codeaurora.org>,
	Lee Jones <lee.jones@linaro.org>,
	Alexandre Courbot <gnurou@gmail.com>,
	Grygorii Strashko <grygorii.strashko@ti.com>,
	<devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v4 4/5] regulator: tps65912: Add regulator driver for the TPS65912 PMIC
Date: Fri, 23 Oct 2015 19:11:56 -0500	[thread overview]
Message-ID: <562ACCCC.503@ti.com> (raw)
In-Reply-To: <20151023231822.GQ29919@sirena.org.uk>

On 10/23/2015 06:18 PM, Mark Brown wrote:
> On Fri, Oct 23, 2015 at 07:46:39AM -0500, Andrew F. Davis wrote:
>
>> I know just because other drivers do it doesn't mean it's a good idea,
>> but this is not new for MFDs and it is done in other regulators as well
>> (mt6397, tps659038, qcom,spmi, etc..).
>
> mt6397 doesn't do this, it doesn't have a compatible string at all (it's
> doing what I'm recommending that you do).  The SPMI devices are
> standalone devices, their parent device is actually functioning as a bus
> controller here (it's really a microcontroller inside the SoC).  The
> Palmas is part of how we realised this was a problem.
>

mt6397: Documentation/devicetree/bindings/mfd/mt6397.txt
Doing exactly what I'm doing,

pmic {
	compatible = "mediatek,mt6397";

	codec: mt6397codec {
		compatible = "mediatek,mt6397-codec";
	};

	regulators {
		compatible = "mediatek,mt6397-regulator";

		buck_vpca15 {
			....

The Palmas is a great example of why this is a good idea, there are
so many spins on this common base, and look how we can re-use sub-nodes:

tps659038: tps659038@58 {
	compatible = "ti,tps659038";
	reg = <0x58>;
	...

	tps659038_pmic {
		compatible = "ti,tps659038-pmic";
		...
	};

	tps659038_rtc: tps659038_rtc {
		compatible = "ti,palmas-rtc";
		...
	};

	tps659038_pwr_button: tps659038_pwr_button {
		compatible = "ti,palmas-pwrbutton";
		...
	};

	tps659038_gpio: tps659038_gpio {
		compatible = "ti,palmas-gpio";
		...
	};
};

(from am57xx-beagle-x15.dts)

looks like only the "ti,tps659038-pmic" node needed re-made without
re-making the whole driver.

>>> It seems like this is describing how Linux
>>> loads drivers not how the hardware is constructed but DT should describe
>>> the hardware.
>
>> While I agree to a point, if we follow this to its logical conclusion we
>> would end up with one compatible binding per SoC and be basically back to
>> board files. We need some granularity, just finding out where is the issue,
>
> The fact that the SoC DT is not distinct from the board DT is actually
> one of the problems with the way we're using DT at the minute, it means
> that DTBs are much less stable than they should be since we can enhance
> support for SoCs but DTBs need regenerating to take advantage of it.  It
> would be much better if the boards just referenced the SoC they use and
> pulled in a separate definition of the SoC (DT overlays will make it
> much more tractable to implement that if someone has time...).
>

I figured this can already be done by keeping the SoC stuff in dtsi files?
Anyway DT seems to have a lot of use issues with how it is being used, but
I'm probably not a person with enough free time for fixing that.. :|

>> I would say that as these devices belong to different subsystems and are
>> almost completely independent there should be no problem with having their
>> own compatible matched hardware sub-node.
>
> All it's adding is more typing for users.
>

Well I have to match the sub-devices on something, it's ether the node name
or the compatible string, so they might have to get used to typing :)


  reply	other threads:[~2015-10-24  0:11 UTC|newest]

Thread overview: 75+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-10-01 20:37 [PATCH v4 0/5] mfd: tps65912: Driver rewrite with DT support Andrew F. Davis
2015-10-01 20:37 ` Andrew F. Davis
2015-10-01 20:37 ` [PATCH v4 1/5] Documentation: tps65912: Add DT bindings for the TPS65912 PMIC Andrew F. Davis
2015-10-01 20:37   ` Andrew F. Davis
2015-10-01 20:37 ` [PATCH v4 2/5] mfd: tps65912: Remove old driver in preparation for new driver Andrew F. Davis
2015-10-01 20:37   ` Andrew F. Davis
2015-10-05  9:28   ` Lee Jones
2015-10-05  9:29     ` Lee Jones
2015-10-05  9:29       ` Lee Jones
2015-10-05 16:01       ` Andrew F. Davis
2015-10-05 16:01         ` Andrew F. Davis
2015-10-01 20:37 ` [PATCH v4 3/5] mfd: tps65912: Add driver for the TPS65912 PMIC Andrew F. Davis
2015-10-01 20:37   ` Andrew F. Davis
2015-10-01 20:57   ` kbuild test robot
2015-10-01 20:57     ` kbuild test robot
     [not found]   ` <1443731874-21362-4-git-send-email-afd-l0cyMroinI0@public.gmane.org>
2015-10-01 20:51     ` kbuild test robot
2015-10-01 20:51       ` kbuild test robot
     [not found]       ` <20151002095859.GN12635@sirena.org.uk>
2015-10-02 13:32         ` [lkp] " Fengguang Wu
2015-10-02 13:47           ` Mark Brown
2015-10-01 20:57     ` kbuild test robot
2015-10-01 20:57       ` kbuild test robot
2015-10-01 23:49   ` Andrew F. Davis
2015-10-01 23:49     ` Andrew F. Davis
2015-10-05  9:24   ` Lee Jones
2015-10-05  9:27     ` Lee Jones
2015-10-12 15:06       ` Andrew F. Davis
2015-10-12 15:06         ` Andrew F. Davis
     [not found]         ` <561BCC8A.3090402-l0cyMroinI0@public.gmane.org>
2015-10-13  7:34           ` Lee Jones
2015-10-13  7:34             ` Lee Jones
2015-10-01 20:37 ` [PATCH v4 4/5] regulator: tps65912: Add regulator " Andrew F. Davis
2015-10-01 20:37   ` Andrew F. Davis
2015-10-02 19:21   ` Grygorii Strashko
2015-10-02 19:21     ` Grygorii Strashko
2015-10-22 16:47   ` Mark Brown
     [not found]     ` <20151022164724.GZ8232-GFdadSzt00ze9xe1eoZjHA@public.gmane.org>
2015-10-23 12:46       ` Andrew F. Davis
2015-10-23 12:46         ` Andrew F. Davis
2015-10-23 23:18         ` Mark Brown
2015-10-24  0:11           ` Andrew F. Davis [this message]
2015-10-24  0:11             ` Andrew F. Davis
     [not found]             ` <562ACCCC.503-l0cyMroinI0@public.gmane.org>
2015-10-24 22:14               ` Mark Brown
2015-10-24 22:14                 ` Mark Brown
     [not found]                 ` <20151024221457.GS29919-GFdadSzt00ze9xe1eoZjHA@public.gmane.org>
2015-10-25 20:45                   ` Andrew F. Davis
2015-10-25 20:45                     ` Andrew F. Davis
     [not found]                     ` <562D3F77.5040205-l0cyMroinI0@public.gmane.org>
2015-10-26  0:43                       ` Mark Brown
2015-10-26  0:43                         ` Mark Brown
2015-10-26 15:47                         ` Andrew F. Davis
2015-10-26 15:47                           ` Andrew F. Davis
     [not found]                           ` <562E4B1D.4060205-l0cyMroinI0@public.gmane.org>
2015-10-27  0:16                             ` Mark Brown
2015-10-27  0:16                               ` Mark Brown
     [not found]                               ` <20151027001608.GJ28319-GFdadSzt00ze9xe1eoZjHA@public.gmane.org>
2015-10-27 14:23                                 ` Andrew F. Davis
2015-10-27 14:23                                   ` Andrew F. Davis
2015-11-04 15:35     ` Andrew F. Davis
2015-11-04 15:35       ` Andrew F. Davis
2015-11-05 10:14       ` Mark Brown
2015-11-05 18:04         ` Andrew F. Davis
2015-11-05 18:04           ` Andrew F. Davis
2015-11-06 10:43           ` Mark Brown
2015-11-06 18:10             ` Andrew F. Davis
2015-11-06 18:10               ` Andrew F. Davis
2015-11-06 21:16               ` Mark Brown
     [not found]                 ` <20151106211651.GJ18409-GFdadSzt00ze9xe1eoZjHA@public.gmane.org>
2015-11-09 17:41                   ` Andrew F. Davis
2015-11-09 17:41                     ` Andrew F. Davis
2015-11-10  9:57                     ` Mark Brown
2015-11-10 16:47                       ` Andrew F. Davis
2015-11-10 16:47                         ` Andrew F. Davis
2015-11-10 17:04                         ` Mark Brown
2015-11-10 17:52                           ` Andrew F. Davis
2015-11-10 17:52                             ` Andrew F. Davis
2015-11-10 18:44                             ` Mark Brown
2015-11-10 19:40                               ` Andrew F. Davis
2015-11-10 19:40                                 ` Andrew F. Davis
     [not found]                                 ` <56424836.7000608-l0cyMroinI0@public.gmane.org>
2015-11-16 18:23                                   ` Mark Brown
2015-11-16 18:23                                     ` Mark Brown
2015-10-01 20:37 ` [PATCH v4 5/5] gpio: tps65912: Add GPIO " Andrew F. Davis
2015-10-01 20:37   ` Andrew F. Davis

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=562ACCCC.503@ti.com \
    --to=afd@ti.com \
    --cc=broonie@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=galak@codeaurora.org \
    --cc=gnurou@gmail.com \
    --cc=grygorii.strashko@ti.com \
    --cc=ijc+devicetree@hellion.org.uk \
    --cc=lee.jones@linaro.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=pawel.moll@arm.com \
    --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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.