linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Thierry Reding <thierry.reding@gmail.com>
To: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
Cc: Ryder Lee <ryder.lee@mediatek.com>,
	Rob Herring <robh+dt@kernel.org>,
	linux-pwm@vger.kernel.org, Weijie Gao <weijie.gao@mediatek.com>,
	Roy Luo <cheng-hao.luo@mediatek.com>,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-mediatek@lists.infradead.org,
	John Crispin <john@phrozen.org>
Subject: Re: [resend PATCH 1/3] pwm: mediatek: drop flag 'has_clks'
Date: Fri, 16 Nov 2018 11:22:44 +0100	[thread overview]
Message-ID: <20181116102244.GB14769@ulmo> (raw)
In-Reply-To: <20181116064748.6c7shn3am4wejrnj@pengutronix.de>

[-- Attachment #1: Type: text/plain, Size: 2089 bytes --]

On Fri, Nov 16, 2018 at 07:47:48AM +0100, Uwe Kleine-König wrote:
> On Wed, Nov 14, 2018 at 01:47:52PM +0100, Thierry Reding wrote:
> > On Tue, Nov 13, 2018 at 10:08:22AM +0800, Ryder Lee wrote:
> > > The flag 'has_clks' and related checks are superfluous as the CCF
> > > subsystem does this for you.
> > 
> > Both of these mechanisms aren't equivalent. While CCF can deal with
> > optional clocks, what the has_clks flag actually means is that the
> > device doesn't need a clock (or doesn't have a clock input) on the
> > devices where it is cleared.
> > 
> > So I'd actually be in favor of keeping the has_clks property because it
> > serves as an additional sanity check. For example if you run this driver
> > on an SoC that "has clocks" but if you don't list them in DT, then after
> > this patch the driver will happily continue without clocks, even though
> > it may break completely without those clocks. I've seen SoCs respond to
> > disabled clocks for a hardware block in different ways, in many cases an
> > access to any of the registers will completely hang the CPU. In other
> > cases it may just crash in some other way or give you some sort of
> > machine exception. None of those are good, and make the tiny bit of
> > additional code required to support the has_clks flag very attractive.
> > 
> > But that's just my opinion. If you prefer to throw away that safety
> > barrier, be my guest. But if you do, please move this functionality into
> > the clock framework first and then make the driver use it.
> 
> The usual policy is: If the things specified in the dt are
> wrong or incomplete, it's ok to fail however you like. So from a
> correctness POV I think the change is fine.

Erm... that's pretty much what I said. It doesn't necessarily mean that
it's the right thing to do, though. If we know that clocks are required
and we don't find them in DT, it's better to complain and let the user
know exactly what is wrong rather than just let it crash and have them
track down the bug without additional information.

Thierry

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

      reply	other threads:[~2018-11-16 10:22 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-11-13  2:08 Ryder Lee
2018-11-13  2:08 ` [resend PATCH 2/3] pwm: mediatek: add pwm support for MT7629 SoC Ryder Lee
2018-11-15 19:20   ` Sean Wang
2018-11-13  2:08 ` [resend PATCH 3/3] dt-bindings: pwm: update bindings " Ryder Lee
2018-11-13  9:55   ` Uwe Kleine-König
2018-11-13  9:52 ` [resend PATCH 1/3] pwm: mediatek: drop flag 'has_clks' Uwe Kleine-König
2018-11-13 18:00   ` Stephen Boyd
2018-11-14 12:47 ` Thierry Reding
2018-11-14 13:27   ` John Crispin
2018-11-15  2:16     ` Ryder Lee
2018-11-16  6:47   ` Uwe Kleine-König
2018-11-16 10:22     ` Thierry Reding [this message]

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=20181116102244.GB14769@ulmo \
    --to=thierry.reding@gmail.com \
    --cc=cheng-hao.luo@mediatek.com \
    --cc=devicetree@vger.kernel.org \
    --cc=john@phrozen.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mediatek@lists.infradead.org \
    --cc=linux-pwm@vger.kernel.org \
    --cc=robh+dt@kernel.org \
    --cc=ryder.lee@mediatek.com \
    --cc=u.kleine-koenig@pengutronix.de \
    --cc=weijie.gao@mediatek.com \
    --subject='Re: [resend PATCH 1/3] pwm: mediatek: drop flag '\''has_clks'\''' \
    /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

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).