Re: [PATCH v2 2/5] input: pmic8xxx-pwrkey: Add support for pm8018 pwrkey

[Neil Armstrong]

On 06/25/2016 05:53 PM, Dmitry Torokhov wrote:
> On Sat, Jun 25, 2016 at 10:34:04AM +0200, Neil Armstrong wrote:
>> On 06/25/2016 12:07 AM, Dmitry Torokhov wrote:
>>> On Fri, Jun 24, 2016 at 11:18:04AM +0200, Neil Armstrong wrote:
 In order to support pwrkey for Qualcomm MDM9615 SoC, add support
 for the pm8018 pwrkey in pmic8xxx-pwrkey.

 Reviewed-by: Bjorn Andersson 
 Signed-off-by: Neil Armstrong 
>>>
>>> NAK.
>>
>> Hi Dmitry,
>>
>> Actually, the new compatible string make sense, because the driver is 
>> compatible with the
>> "pm8018" pwrkey but from a system point of view, it's not a pm8921 pwrkey, 
>> hence the new
>> compatible string.
> 
> A lot of systems note this fact in DTS, but not require driver changes,
> by specifying several compatible strings:
> 
>   compatible = "nvidia,tegra114-sdhci", "nvidia,tegra30-sdhci";
>   compatible = "fsl,imx6q-i2c", "fsl,imx21-i2c";
>   compatible = "rockchip,rk3036-timer", "rockchip,rk3288-timer";

Sure, your point is valid.
But here, the situation is quite different, the question is about confidence.
>From the system point of view, I'm 100% sure there is a pm8010-pwrkey variant 
>here, but
I`m not convinced at all how it is similar from the 8921 version.
>From the software point of view, I'm 80% sure the *actual* driver in it 
>current form
somehow works for the pm8018-pwrkey, not more.
If somehow the driver is updated to support a 8921 feature that is not 
supported by the
8018 version, it will rely on the compatible string to make this a smart move.
Since I do not have the pm8018 datasheet and the 8921 either, I cannot statue 
on this,
So the smartest move from my side is to actually have a different compatible 
string
to avoid future blocking situations.

>>
>> Rob Herring was very clear with me with this policy, and it will simplify 
>> further driver
> 
> Could I get a pointer to this discussion so I can educate myself better
> about DT policies?

I had quite a lot of comments on the OXNAS support push (started here 
https://lkml.org/lkml/2016/3/3/495) were
the policy was to narrow the new compatible strings to a SoC specific naming.
For the qcom driver, the strings the already compliant and why not continue 
with the pm8018 ?

>> architecture change since it will not imply devicetree changes anymore.
> 
> Would we need the driver changes? What are the differences in power key
> functionality between 8018 and 8921?

You raise the biggest question, I do not know, so why should we say the 
pm8018-pwrkey /is/ compatible with pm8921-pwronly only by looking existing 
driver ?

>>
>> My point of view is that the devicetree describes the hardware and need to 
>> have SoC specific
>> compatible string since it describes the actual silicon, and drivers must 
>> make sure to handle
>> all the SoC or family variants using the compatible string and the match 
>> data.
> 
> No, the compatible string means that the hardware is *compatible* with
> something. It does not mean that we need to adjust driver every time a
> company pumps out a new package including said hardware.

It was something that I questionned myself about, but it seems the maintainers 
agrees quite easily to accept these compatible adding patches
like the USB Ids or PCI ids patches.

Regards,
Neil

> Thanks.
> 

Re: [PATCH v2 2/5] input: pmic8xxx-pwrkey: Add support for pm8018 pwrkey

[Neil Armstrong]

On 06/25/2016 05:53 PM, Dmitry Torokhov wrote:
> On Sat, Jun 25, 2016 at 10:34:04AM +0200, Neil Armstrong wrote:
>> On 06/25/2016 12:07 AM, Dmitry Torokhov wrote:
>>> On Fri, Jun 24, 2016 at 11:18:04AM +0200, Neil Armstrong wrote:
 In order to support pwrkey for Qualcomm MDM9615 SoC, add support
 for the pm8018 pwrkey in pmic8xxx-pwrkey.

 Reviewed-by: Bjorn Andersson 
 Signed-off-by: Neil Armstrong 
>>>
>>> NAK.
>>
>> Hi Dmitry,
>>
>> Actually, the new compatible string make sense, because the driver is 
>> compatible with the
>> "pm8018" pwrkey but from a system point of view, it's not a pm8921 pwrkey, 
>> hence the new
>> compatible string.
> 
> A lot of systems note this fact in DTS, but not require driver changes,
> by specifying several compatible strings:
> 
>   compatible = "nvidia,tegra114-sdhci", "nvidia,tegra30-sdhci";
>   compatible = "fsl,imx6q-i2c", "fsl,imx21-i2c";
>   compatible = "rockchip,rk3036-timer", "rockchip,rk3288-timer";

Sure, your point is valid.
But here, the situation is quite different, the question is about confidence.
>From the system point of view, I'm 100% sure there is a pm8010-pwrkey variant 
>here, but
I`m not convinced at all how it is similar from the 8921 version.
>From the software point of view, I'm 80% sure the *actual* driver in it 
>current form
somehow works for the pm8018-pwrkey, not more.
If somehow the driver is updated to support a 8921 feature that is not 
supported by the
8018 version, it will rely on the compatible string to make this a smart move.
Since I do not have the pm8018 datasheet and the 8921 either, I cannot statue 
on this,
So the smartest move from my side is to actually have a different compatible 
string
to avoid future blocking situations.

>>
>> Rob Herring was very clear with me with this policy, and it will simplify 
>> further driver
> 
> Could I get a pointer to this discussion so I can educate myself better
> about DT policies?

I had quite a lot of comments on the OXNAS support push (started here 
https://lkml.org/lkml/2016/3/3/495) were
the policy was to narrow the new compatible strings to a SoC specific naming.
For the qcom driver, the strings the already compliant and why not continue 
with the pm8018 ?

>> architecture change since it will not imply devicetree changes anymore.
> 
> Would we need the driver changes? What are the differences in power key
> functionality between 8018 and 8921?

You raise the biggest question, I do not know, so why should we say the 
pm8018-pwrkey /is/ compatible with pm8921-pwronly only by looking existing 
driver ?

>>
>> My point of view is that the devicetree describes the hardware and need to 
>> have SoC specific
>> compatible string since it describes the actual silicon, and drivers must 
>> make sure to handle
>> all the SoC or family variants using the compatible string and the match 
>> data.
> 
> No, the compatible string means that the hardware is *compatible* with
> something. It does not mean that we need to adjust driver every time a
> company pumps out a new package including said hardware.

It was something that I questionned myself about, but it seems the maintainers 
agrees quite easily to accept these compatible adding patches
like the USB Ids or PCI ids patches.

Regards,
Neil

> Thanks.
> 

Re: [PATCH v2 2/5] input: pmic8xxx-pwrkey: Add support for pm8018 pwrkey

[Dmitry Torokhov]

On Sat, Jun 25, 2016 at 10:34:04AM +0200, Neil Armstrong wrote:
> On 06/25/2016 12:07 AM, Dmitry Torokhov wrote:
> > On Fri, Jun 24, 2016 at 11:18:04AM +0200, Neil Armstrong wrote:
> >> In order to support pwrkey for Qualcomm MDM9615 SoC, add support
> >> for the pm8018 pwrkey in pmic8xxx-pwrkey.
> >>
> >> Reviewed-by: Bjorn Andersson 
> >> Signed-off-by: Neil Armstrong 
> > 
> > NAK.
> 
> Hi Dmitry,
> 
> Actually, the new compatible string make sense, because the driver is 
> compatible with the
> "pm8018" pwrkey but from a system point of view, it's not a pm8921 pwrkey, 
> hence the new
> compatible string.

A lot of systems note this fact in DTS, but not require driver changes,
by specifying several compatible strings:

compatible = "nvidia,tegra114-sdhci", "nvidia,tegra30-sdhci";
compatible = "fsl,imx6q-i2c", "fsl,imx21-i2c";
compatible = "rockchip,rk3036-timer", "rockchip,rk3288-timer";

> 
> Rob Herring was very clear with me with this policy, and it will simplify 
> further driver

Could I get a pointer to this discussion so I can educate myself better
about DT policies?

> architecture change since it will not imply devicetree changes anymore.

Would we need the driver changes? What are the differences in power key
functionality between 8018 and 8921?

> 
> My point of view is that the devicetree describes the hardware and need to 
> have SoC specific
> compatible string since it describes the actual silicon, and drivers must 
> make sure to handle
> all the SoC or family variants using the compatible string and the match data.

No, the compatible string means that the hardware is *compatible* with
something. It does not mean that we need to adjust driver every time a
company pumps out a new package including said hardware.

Thanks.

-- 
Dmitry

Re: [PATCH v2 2/5] input: pmic8xxx-pwrkey: Add support for pm8018 pwrkey

[Dmitry Torokhov]

On Sat, Jun 25, 2016 at 10:34:04AM +0200, Neil Armstrong wrote:
> On 06/25/2016 12:07 AM, Dmitry Torokhov wrote:
> > On Fri, Jun 24, 2016 at 11:18:04AM +0200, Neil Armstrong wrote:
> >> In order to support pwrkey for Qualcomm MDM9615 SoC, add support
> >> for the pm8018 pwrkey in pmic8xxx-pwrkey.
> >>
> >> Reviewed-by: Bjorn Andersson 
> >> Signed-off-by: Neil Armstrong 
> > 
> > NAK.
> 
> Hi Dmitry,
> 
> Actually, the new compatible string make sense, because the driver is 
> compatible with the
> "pm8018" pwrkey but from a system point of view, it's not a pm8921 pwrkey, 
> hence the new
> compatible string.

A lot of systems note this fact in DTS, but not require driver changes,
by specifying several compatible strings:

compatible = "nvidia,tegra114-sdhci", "nvidia,tegra30-sdhci";
compatible = "fsl,imx6q-i2c", "fsl,imx21-i2c";
compatible = "rockchip,rk3036-timer", "rockchip,rk3288-timer";

> 
> Rob Herring was very clear with me with this policy, and it will simplify 
> further driver

Could I get a pointer to this discussion so I can educate myself better
about DT policies?

> architecture change since it will not imply devicetree changes anymore.

Would we need the driver changes? What are the differences in power key
functionality between 8018 and 8921?

> 
> My point of view is that the devicetree describes the hardware and need to 
> have SoC specific
> compatible string since it describes the actual silicon, and drivers must 
> make sure to handle
> all the SoC or family variants using the compatible string and the match data.

No, the compatible string means that the hardware is *compatible* with
something. It does not mean that we need to adjust driver every time a
company pumps out a new package including said hardware.

Thanks.

-- 
Dmitry

Re: [PATCH v2 2/5] input: pmic8xxx-pwrkey: Add support for pm8018 pwrkey

[Neil Armstrong]

On 06/25/2016 12:07 AM, Dmitry Torokhov wrote:
> On Fri, Jun 24, 2016 at 11:18:04AM +0200, Neil Armstrong wrote:
>> In order to support pwrkey for Qualcomm MDM9615 SoC, add support
>> for the pm8018 pwrkey in pmic8xxx-pwrkey.
>>
>> Reviewed-by: Bjorn Andersson 
>> Signed-off-by: Neil Armstrong 
> 
> NAK.

Hi Dmitry,

Actually, the new compatible string make sense, because the driver is 
compatible with the
"pm8018" pwrkey but from a system point of view, it's not a pm8921 pwrkey, 
hence the new
compatible string.

Rob Herring was very clear with me with this policy, and it will simplify 
further driver
architecture change since it will not imply devicetree changes anymore.

My point of view is that the devicetree describes the hardware and need to have 
SoC specific
compatible string since it describes the actual silicon, and drivers must make 
sure to handle
all the SoC or family variants using the compatible string and the match data.

Neil
> 
>> ---
>>  Documentation/devicetree/bindings/input/qcom,pm8xxx-pwrkey.txt | 1 +
>>  drivers/input/misc/pmic8xxx-pwrkey.c   | 1 +
>>  2 files changed, 2 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/input/qcom,pm8xxx-pwrkey.txt 
>> b/Documentation/devicetree/bindings/input/qcom,pm8xxx-pwrkey.txt
>> index 588536c..9e530e2 100644
>> --- a/Documentation/devicetree/bindings/input/qcom,pm8xxx-pwrkey.txt
>> +++ b/Documentation/devicetree/bindings/input/qcom,pm8xxx-pwrkey.txt
>> @@ -8,6 +8,7 @@ PROPERTIES
>>  Definition: must be one of:
>>  "qcom,pm8058-pwrkey"
>>  "qcom,pm8921-pwrkey"
>> +"qcom,pm8018-pwrkey"
>>  
>>  - reg:
>>  Usage: required
>> diff --git a/drivers/input/misc/pmic8xxx-pwrkey.c 
>> b/drivers/input/misc/pmic8xxx-pwrkey.c
>> index 67aab86..f6f9b87 100644
>> --- a/drivers/input/misc/pmic8xxx-pwrkey.c
>> +++ b/drivers/input/misc/pmic8xxx-pwrkey.c
>> @@ -446,6 +446,7 @@ static int pmic8xxx_pwrkey_remove(struct platform_device 
>> *pdev)
>>  }
>>  
>>  static const struct of_device_id pm8xxx_pwr_key_id_table[] = {
>> +{ .compatible = "qcom,pm8018-pwrkey", .data = _pwrkey_shutdown },
>>  { .compatible = "qcom,pm8058-pwrkey", .data = _pwrkey_shutdown },
>>  { .compatible = "qcom,pm8921-pwrkey", .data = _pwrkey_shutdown },
>>  { }
>> -- 
>> 1.9.1
>>
> 

Re: [PATCH v2 2/5] input: pmic8xxx-pwrkey: Add support for pm8018 pwrkey

[Neil Armstrong]

On 06/25/2016 12:07 AM, Dmitry Torokhov wrote:
> On Fri, Jun 24, 2016 at 11:18:04AM +0200, Neil Armstrong wrote:
>> In order to support pwrkey for Qualcomm MDM9615 SoC, add support
>> for the pm8018 pwrkey in pmic8xxx-pwrkey.
>>
>> Reviewed-by: Bjorn Andersson 
>> Signed-off-by: Neil Armstrong 
> 
> NAK.

Hi Dmitry,

Actually, the new compatible string make sense, because the driver is 
compatible with the
"pm8018" pwrkey but from a system point of view, it's not a pm8921 pwrkey, 
hence the new
compatible string.

Rob Herring was very clear with me with this policy, and it will simplify 
further driver
architecture change since it will not imply devicetree changes anymore.

My point of view is that the devicetree describes the hardware and need to have 
SoC specific
compatible string since it describes the actual silicon, and drivers must make 
sure to handle
all the SoC or family variants using the compatible string and the match data.

Neil
> 
>> ---
>>  Documentation/devicetree/bindings/input/qcom,pm8xxx-pwrkey.txt | 1 +
>>  drivers/input/misc/pmic8xxx-pwrkey.c   | 1 +
>>  2 files changed, 2 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/input/qcom,pm8xxx-pwrkey.txt 
>> b/Documentation/devicetree/bindings/input/qcom,pm8xxx-pwrkey.txt
>> index 588536c..9e530e2 100644
>> --- a/Documentation/devicetree/bindings/input/qcom,pm8xxx-pwrkey.txt
>> +++ b/Documentation/devicetree/bindings/input/qcom,pm8xxx-pwrkey.txt
>> @@ -8,6 +8,7 @@ PROPERTIES
>>  Definition: must be one of:
>>  "qcom,pm8058-pwrkey"
>>  "qcom,pm8921-pwrkey"
>> +"qcom,pm8018-pwrkey"
>>  
>>  - reg:
>>  Usage: required
>> diff --git a/drivers/input/misc/pmic8xxx-pwrkey.c 
>> b/drivers/input/misc/pmic8xxx-pwrkey.c
>> index 67aab86..f6f9b87 100644
>> --- a/drivers/input/misc/pmic8xxx-pwrkey.c
>> +++ b/drivers/input/misc/pmic8xxx-pwrkey.c
>> @@ -446,6 +446,7 @@ static int pmic8xxx_pwrkey_remove(struct platform_device 
>> *pdev)
>>  }
>>  
>>  static const struct of_device_id pm8xxx_pwr_key_id_table[] = {
>> +{ .compatible = "qcom,pm8018-pwrkey", .data = _pwrkey_shutdown },
>>  { .compatible = "qcom,pm8058-pwrkey", .data = _pwrkey_shutdown },
>>  { .compatible = "qcom,pm8921-pwrkey", .data = _pwrkey_shutdown },
>>  { }
>> -- 
>> 1.9.1
>>
> 

Re: [PATCH v2 2/5] input: pmic8xxx-pwrkey: Add support for pm8018 pwrkey

[Dmitry Torokhov]

On Fri, Jun 24, 2016 at 11:18:04AM +0200, Neil Armstrong wrote:
> In order to support pwrkey for Qualcomm MDM9615 SoC, add support
> for the pm8018 pwrkey in pmic8xxx-pwrkey.
> 
> Reviewed-by: Bjorn Andersson 
> Signed-off-by: Neil Armstrong 

NAK.

> ---
>  Documentation/devicetree/bindings/input/qcom,pm8xxx-pwrkey.txt | 1 +
>  drivers/input/misc/pmic8xxx-pwrkey.c   | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/input/qcom,pm8xxx-pwrkey.txt 
> b/Documentation/devicetree/bindings/input/qcom,pm8xxx-pwrkey.txt
> index 588536c..9e530e2 100644
> --- a/Documentation/devicetree/bindings/input/qcom,pm8xxx-pwrkey.txt
> +++ b/Documentation/devicetree/bindings/input/qcom,pm8xxx-pwrkey.txt
> @@ -8,6 +8,7 @@ PROPERTIES
>   Definition: must be one of:
>   "qcom,pm8058-pwrkey"
>   "qcom,pm8921-pwrkey"
> + "qcom,pm8018-pwrkey"
>  
>  - reg:
>   Usage: required
> diff --git a/drivers/input/misc/pmic8xxx-pwrkey.c 
> b/drivers/input/misc/pmic8xxx-pwrkey.c
> index 67aab86..f6f9b87 100644
> --- a/drivers/input/misc/pmic8xxx-pwrkey.c
> +++ b/drivers/input/misc/pmic8xxx-pwrkey.c
> @@ -446,6 +446,7 @@ static int pmic8xxx_pwrkey_remove(struct platform_device 
> *pdev)
>  }
>  
>  static const struct of_device_id pm8xxx_pwr_key_id_table[] = {
> + { .compatible = "qcom,pm8018-pwrkey", .data = _pwrkey_shutdown },
>   { .compatible = "qcom,pm8058-pwrkey", .data = _pwrkey_shutdown },
>   { .compatible = "qcom,pm8921-pwrkey", .data = _pwrkey_shutdown },
>   { }
> -- 
> 1.9.1
> 

-- 
Dmitry

Re: [PATCH v2 2/5] input: pmic8xxx-pwrkey: Add support for pm8018 pwrkey

[Dmitry Torokhov]

On Fri, Jun 24, 2016 at 11:18:04AM +0200, Neil Armstrong wrote:
> In order to support pwrkey for Qualcomm MDM9615 SoC, add support
> for the pm8018 pwrkey in pmic8xxx-pwrkey.
> 
> Reviewed-by: Bjorn Andersson 
> Signed-off-by: Neil Armstrong 

NAK.

> ---
>  Documentation/devicetree/bindings/input/qcom,pm8xxx-pwrkey.txt | 1 +
>  drivers/input/misc/pmic8xxx-pwrkey.c   | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/input/qcom,pm8xxx-pwrkey.txt 
> b/Documentation/devicetree/bindings/input/qcom,pm8xxx-pwrkey.txt
> index 588536c..9e530e2 100644
> --- a/Documentation/devicetree/bindings/input/qcom,pm8xxx-pwrkey.txt
> +++ b/Documentation/devicetree/bindings/input/qcom,pm8xxx-pwrkey.txt
> @@ -8,6 +8,7 @@ PROPERTIES
>   Definition: must be one of:
>   "qcom,pm8058-pwrkey"
>   "qcom,pm8921-pwrkey"
> + "qcom,pm8018-pwrkey"
>  
>  - reg:
>   Usage: required
> diff --git a/drivers/input/misc/pmic8xxx-pwrkey.c 
> b/drivers/input/misc/pmic8xxx-pwrkey.c
> index 67aab86..f6f9b87 100644
> --- a/drivers/input/misc/pmic8xxx-pwrkey.c
> +++ b/drivers/input/misc/pmic8xxx-pwrkey.c
> @@ -446,6 +446,7 @@ static int pmic8xxx_pwrkey_remove(struct platform_device 
> *pdev)
>  }
>  
>  static const struct of_device_id pm8xxx_pwr_key_id_table[] = {
> + { .compatible = "qcom,pm8018-pwrkey", .data = _pwrkey_shutdown },
>   { .compatible = "qcom,pm8058-pwrkey", .data = _pwrkey_shutdown },
>   { .compatible = "qcom,pm8921-pwrkey", .data = _pwrkey_shutdown },
>   { }
> -- 
> 1.9.1
> 

-- 
Dmitry