linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
To: Sean Anderson <sean.anderson@seco.com>,
	Michal Simek <michal.simek@xilinx.com>
Cc: linux-pwm@vger.kernel.org, devicetree@vger.kernel.org,
	Rob Herring <robh+dt@kernel.org>,
	Thierry Reding <thierry.reding@gmail.com>,
	Alvaro Gamez <alvaro.gamez@hazent.com>,
	linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	Daniel Lezcano <daniel.lezcano@linaro.org>,
	Lee Jones <lee.jones@linaro.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	kernel@pengutronix.de
Subject: Re: [PATCH v3 2/2] clocksource: Add support for Xilinx AXI Timer
Date: Tue, 25 May 2021 08:11:31 +0200	[thread overview]
Message-ID: <20210525061131.omrbcdewf4z75ib7@pengutronix.de> (raw)
In-Reply-To: <2296d4e5-717a-0470-d487-e0924cf6c076@xilinx.com>


[-- Attachment #1.1: Type: text/plain, Size: 4073 bytes --]

Hello Sean, hello Michal,

On Mon, May 24, 2021 at 09:00:51AM +0200, Michal Simek wrote:
> On 5/20/21 10:13 PM, Sean Anderson wrote:
> > On 5/19/21 3:24 AM, Michal Simek wrote:
> >> On 5/18/21 12:15 AM, Sean Anderson wrote:
> >>> This could be deprecated, but cannot be removed since existing device
> >>> trees (e.g. qemu) have neither clocks nor clock-frequency properties.
> >>
> >> Rob: Do we have any obligation to keep properties for other projects?

If a binding is in the wild and used to be documented, it has to stay.

> >>>> 4. Make driver as module
> >>>> 5. Do whatever changes you want before adding pwm support
> >>>> 6. Extend DT binding doc for PWM support
> >>>> 7. Add PWM support
> >>>
> >>> Frankly, I am inclined to just leave the microblaze timer as-is. The PWM
> >>> driver is completely independent. I have already put too much effort into
> >>> this driver, and I don't have the energy to continue working on the
> >>> microblaze timer.
> >>
> >> I understand. I am actually using axi timer as pwm driver in one of my
> >> project but never had time to upstream it because of couple of steps above.
> >> We need to do it right based on steps listed above. If this is too much
> >> work it will have to wait. I will NACK all attempts to add separate
> >> driver for IP which we already support in the tree.
> > 
> > 1. Many timers have separate clocksource and PWM drivers. E.g. samsung,
> >    renesas TPU, etc. It is completely reasonable to keep separate
> >    drivers for these purposes. There is no Linux requirement that each
> >    device have only one driver, especially if it has multiple functions
> >    or ways to be configured.
> 
> It doesn't mean that it was done properly and correctly. Code
> duplication is bad all the time.

IMHO it's not so much about code duplication. Yes, code duplication is
bad and should be prevented if possible. But it's more important to not
introduce surprises. So I think it should be obvious from reading the
device tree source which timer is used to provide the PWM. I don't care
much if this is from an extra property (like xilinx,provide-pwm),
overriding the compatible or some other explicit mechanism. IIUC in this
suggested patch the selection is implicit and so this isn't so nice.

> > 2. If you want to do work on a driver, I'm all for it. However, if you
> >    have not yet submitted that work to the list, you should not gate
> >    other work behind it. Saying that X feature must be gated behind Y
> >    *even if X works completely independently of Y* is just stifling
> >    development.
> 
> I gave you guidance how I think this should be done. I am not gating you
> from this work. Your patch is not working on Microblaze arch which is
> what I maintain. And I don't want to go the route that we will have two
> drivers for the same IP without integration. We were there in past and
> it is just pain.
> I am expecting that PWM guys will guide how this should be done
> properly. I haven't heard any guidance on this yet.
> Thierry/Uwe: Any comment?

Not sure I can and want to provide guidance here. This is not Perl, but
still TIMTOWTDI. If it was me who cared here, I'd look into the
auxiliary bus (Documentation/driver-api/auxiliary_bus.rst) to check if
it can help to solve this problem.
 
> > 3. There is a clear desire for a PWM driver for this device. You, I, and
> >    Alvaro have all written separate drivers for this device because we
> >    want to use it as a PWM. By preventing merging this driver, you are
> >    encouraging duplicate effort by the next person who wants to use this
> >    device as a PWM, and sees that there is no driver in the tree.
> 
> We should do it cleanly that it will be easy to maintain which is not by
> creating two separate drivers or by switching to completely new driver.

+1

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

  parent reply	other threads:[~2021-05-25  6:13 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-11 19:12 [PATCH v3 1/2] dt-bindings: pwm: Add Xilinx AXI Timer Sean Anderson
2021-05-11 19:12 ` [PATCH v3 2/2] clocksource: Add support for " Sean Anderson
2021-05-11 19:19   ` Sean Anderson
2021-05-12  8:31   ` kernel test robot
2021-05-14  8:59   ` Michal Simek
2021-05-14 14:40     ` Sean Anderson
2021-05-17  7:54       ` Michal Simek
2021-05-17 22:15         ` Sean Anderson
2021-05-19  7:24           ` Michal Simek
2021-05-20 20:13             ` Sean Anderson
2021-05-24  7:00               ` Michal Simek
2021-05-24 18:34                 ` Sean Anderson
2021-05-25  6:11                 ` Uwe Kleine-König [this message]
2021-05-25 14:30                   ` Sean Anderson
2021-06-16 12:12                   ` Michal Simek
2021-06-18 21:24                     ` Sean Anderson
2021-06-24 16:25                       ` Michal Simek
2021-05-12 18:35 ` [PATCH v3 1/2] dt-bindings: pwm: Add " Rob Herring
2021-05-13  2:16 ` Rob Herring
2021-05-13 14:33   ` Sean Anderson
2021-05-13 15:28     ` Sean Anderson
2021-05-13 20:43       ` Rob Herring
2021-05-13 21:01         ` Sean Anderson
2021-05-14  8:50         ` Michal Simek
2021-05-14 17:13           ` Sean Anderson
2021-05-17  8:28             ` Michal Simek
2021-05-17 14:40               ` Sean Anderson
2021-05-17 14:49                 ` Michal Simek

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=20210525061131.omrbcdewf4z75ib7@pengutronix.de \
    --to=u.kleine-koenig@pengutronix.de \
    --cc=alvaro.gamez@hazent.com \
    --cc=daniel.lezcano@linaro.org \
    --cc=devicetree@vger.kernel.org \
    --cc=kernel@pengutronix.de \
    --cc=lee.jones@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pwm@vger.kernel.org \
    --cc=michal.simek@xilinx.com \
    --cc=robh+dt@kernel.org \
    --cc=sean.anderson@seco.com \
    --cc=tglx@linutronix.de \
    --cc=thierry.reding@gmail.com \
    /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).