Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1661472 > unrolled thread

Re: [PATCH 2/2] Input: pm8941-pwrkey: Introduce reboot mode support

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 2/2] Input: pm8941-pwrkey: Introduce reboot mode support Sebastian Reichel <sebastian.reichel@collabora.co.uk> - 2017-06-08 18:40 +0200
    Re: [PATCH 2/2] Input: pm8941-pwrkey: Introduce reboot mode support Bjorn Andersson <bjorn.andersson@linaro.org> - 2017-06-13 01:40 +0200
      Re: [PATCH 2/2] Input: pm8941-pwrkey: Introduce reboot mode support Sebastian Reichel <sebastian.reichel@collabora.co.uk> - 2017-06-15 18:30 +0200
        Re: [PATCH 2/2] Input: pm8941-pwrkey: Introduce reboot mode support Bjorn Andersson <bjorn.andersson@linaro.org> - 2017-06-15 20:40 +0200
          Re: [PATCH 2/2] Input: pm8941-pwrkey: Introduce reboot mode support Rob Herring <robh@kernel.org> - 2017-06-15 23:40 +0200

#1661472 — Re: [PATCH 2/2] Input: pm8941-pwrkey: Introduce reboot mode support

[Multipart message — attachments visible in raw view] — view raw

Hi,

On Mon, May 29, 2017 at 09:47:11PM -0700, Bjorn Andersson wrote:
> On Mon 29 May 19:53 PDT 2017, Dmitry Torokhov wrote:
> 
> > On Fri, May 26, 2017 at 11:51:30PM -0700, Bjorn Andersson wrote:
> > > In some Qualcomm platforms the magic for informing LK which mode to
> > > reboot into is stored in the PON_SOFT_RB_SPARE register. Register with
> > > the reboot mode helpers to expose this to the user.
> > 
> > Hmm, is the power key driver the best place to have this? WHy isn't this
> > a driver in its own right?
> > 
> 
> The functionality is part of the "PON" block in the Qualcomm PMICs,
> other functionality from this block relates to configuration and
> handling related to power-key and reset-key.
> 
> Several of these properties are intermingled, so I do believe it's best
> to handle them in a single driver; that said, it might no longer be
> correct to name the driver "pwrkey" or that it is a "misc input" driver.

I merged patch 1 and provided an immutable branch, so
that this could go through the input subsystem.

To me it doesn't look that intermingled, though. I think
the reboot and reboot-mode parts could go into their own
driver in drivers/power/reset.

-- Sebastian

[toc] | [next] | [standalone]

#1664295

On Thu 08 Jun 09:32 PDT 2017, Sebastian Reichel wrote:

> Hi,
> 
> On Mon, May 29, 2017 at 09:47:11PM -0700, Bjorn Andersson wrote:
> > On Mon 29 May 19:53 PDT 2017, Dmitry Torokhov wrote:
> > 
> > > On Fri, May 26, 2017 at 11:51:30PM -0700, Bjorn Andersson wrote:
> > > > In some Qualcomm platforms the magic for informing LK which mode to
> > > > reboot into is stored in the PON_SOFT_RB_SPARE register. Register with
> > > > the reboot mode helpers to expose this to the user.
> > > 
> > > Hmm, is the power key driver the best place to have this? WHy isn't this
> > > a driver in its own right?
> > > 
> > 
> > The functionality is part of the "PON" block in the Qualcomm PMICs,
> > other functionality from this block relates to configuration and
> > handling related to power-key and reset-key.
> > 
> > Several of these properties are intermingled, so I do believe it's best
> > to handle them in a single driver; that said, it might no longer be
> > correct to name the driver "pwrkey" or that it is a "misc input" driver.
> 
> I merged patch 1 and provided an immutable branch, so
> that this could go through the input subsystem.
> 

Thanks

> To me it doesn't look that intermingled, though. I think
> the reboot and reboot-mode parts could go into their own
> driver in drivers/power/reset.
> 

I did reach out to Rob regarding this and the single hardware block
should be described by a single node in DeviceTree.

As such if we split the non-input related handling into another driver
we would need to make the input driver create a subdevice during probe -
or create a new pon-driver with a new compatible that internally spawns
the pwrkey driver. Neither seems desirable to me...


The features of the PON block not yet shown on LKML are status registers
to indicate the reason for powering up the PMIC and a watchdog (which I
don't believe is used or exposed today).

Regards,
Bjorn

[toc] | [prev] | [next] | [standalone]

#1666854

[Multipart message — attachments visible in raw view] — view raw

Hi,

On Mon, Jun 12, 2017 at 04:32:03PM -0700, Bjorn Andersson wrote:
> On Thu 08 Jun 09:32 PDT 2017, Sebastian Reichel wrote:
> 
> > Hi,
> > 
> > On Mon, May 29, 2017 at 09:47:11PM -0700, Bjorn Andersson wrote:
> > > On Mon 29 May 19:53 PDT 2017, Dmitry Torokhov wrote:
> > > 
> > > > On Fri, May 26, 2017 at 11:51:30PM -0700, Bjorn Andersson wrote:
> > > > > In some Qualcomm platforms the magic for informing LK which mode to
> > > > > reboot into is stored in the PON_SOFT_RB_SPARE register. Register with
> > > > > the reboot mode helpers to expose this to the user.
> > > > 
> > > > Hmm, is the power key driver the best place to have this? WHy isn't this
> > > > a driver in its own right?
> > > > 
> > > 
> > > The functionality is part of the "PON" block in the Qualcomm PMICs,
> > > other functionality from this block relates to configuration and
> > > handling related to power-key and reset-key.
> > > 
> > > Several of these properties are intermingled, so I do believe it's best
> > > to handle them in a single driver; that said, it might no longer be
> > > correct to name the driver "pwrkey" or that it is a "misc input" driver.
> > 
> > I merged patch 1 and provided an immutable branch, so
> > that this could go through the input subsystem.
> > 
> 
> Thanks
> 
> > To me it doesn't look that intermingled, though. I think
> > the reboot and reboot-mode parts could go into their own
> > driver in drivers/power/reset.
> > 
> 
> I did reach out to Rob regarding this and the single hardware block
> should be described by a single node in DeviceTree.
> 
> As such if we split the non-input related handling into another driver
> we would need to make the input driver create a subdevice during probe -
> or create a new pon-driver with a new compatible that internally spawns
> the pwrkey driver. Neither seems desirable to me...

The pon-driver would have been the proper solution, but with the
binding already being defined that's no longer a nice option :(

> The features of the PON block not yet shown on LKML are status registers
> to indicate the reason for powering up the PMIC and a watchdog (which I
> don't believe is used or exposed today).

So we have a block, which has watchdog, powerdown, reboot, boot-reason,
reboot-mode and power key. To me that does not look like it should be
one driver.

-- Sebastian

[toc] | [prev] | [next] | [standalone]

#1667120

On Thu 15 Jun 09:26 PDT 2017, Sebastian Reichel wrote:

> Hi,
> 
> On Mon, Jun 12, 2017 at 04:32:03PM -0700, Bjorn Andersson wrote:
[..]
> > As such if we split the non-input related handling into another driver
> > we would need to make the input driver create a subdevice during probe -
> > or create a new pon-driver with a new compatible that internally spawns
> > the pwrkey driver. Neither seems desirable to me...
> 
> The pon-driver would have been the proper solution, but with the
> binding already being defined that's no longer a nice option :(
> 

We have a binding for the "qcom,pm8941-pwrkey", but as long as we
maintain the compatibility in the input driver with this we could come
up with a new binding for the "pon" block.

> > The features of the PON block not yet shown on LKML are status registers
> > to indicate the reason for powering up the PMIC and a watchdog (which I
> > don't believe is used or exposed today).
> 
> So we have a block, which has watchdog, powerdown, reboot, boot-reason,
> reboot-mode and power key. To me that does not look like it should be
> one driver.
> 

Unfortunately I do agree with this.

It would make sense to describe the pon in a single DT-node and have a
pon-driver spawning off individual driver for each functionality. That
way we get a clean representation in DT and we get clean implementation
of each component...

Regards,
Bjorn

[toc] | [prev] | [next] | [standalone]

#1667286

On Thu, Jun 15, 2017 at 1:33 PM, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
> On Thu 15 Jun 09:26 PDT 2017, Sebastian Reichel wrote:
>
>> Hi,
>>
>> On Mon, Jun 12, 2017 at 04:32:03PM -0700, Bjorn Andersson wrote:
> [..]
>> > As such if we split the non-input related handling into another driver
>> > we would need to make the input driver create a subdevice during probe -
>> > or create a new pon-driver with a new compatible that internally spawns
>> > the pwrkey driver. Neither seems desirable to me...
>>
>> The pon-driver would have been the proper solution, but with the
>> binding already being defined that's no longer a nice option :(
>>
>
> We have a binding for the "qcom,pm8941-pwrkey", but as long as we
> maintain the compatibility in the input driver with this we could come
> up with a new binding for the "pon" block.

Yes. My only objection was having the pon node with child devices
purely because that is how Linux splits the driver.

>> > The features of the PON block not yet shown on LKML are status registers
>> > to indicate the reason for powering up the PMIC and a watchdog (which I
>> > don't believe is used or exposed today).
>>
>> So we have a block, which has watchdog, powerdown, reboot, boot-reason,
>> reboot-mode and power key. To me that does not look like it should be
>> one driver.
>>
>
> Unfortunately I do agree with this.

As do I. But that is a separate decision from DT bindings.

> It would make sense to describe the pon in a single DT-node and have a
> pon-driver spawning off individual driver for each functionality. That
> way we get a clean representation in DT and we get clean implementation
> of each component...

My objection here is we should not just create child nodes to align
with current Linux driver needs. That said, child nodes do sometimes
make sense. If, for example, the key function was not fixed and you
needed to configure what key function is used, then probably a child
node makes sense. It's always possible to add child nodes later
without breaking compatibility. It's hard to remove them.

Rob

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web