diff mbox series

[v3,5/8] qcom: pmic_glink: enable altmode for SM8550

Message ID 20230601-topic-sm8550-upstream-type-c-v3-5-22c9973012b6@linaro.org (mailing list archive)
State Accepted
Commit 7daada8630eb49cd841c0fd022fadd52da100bce
Headers show
Series arm64: qcom: add Type-C Altmode support | expand

Commit Message

Neil Armstrong June 13, 2023, 7:55 a.m. UTC
Altmode is also supported for SM8550, allow it.

Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
 drivers/soc/qcom/pmic_glink.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

Comments

Konrad Dybcio June 13, 2023, 11:23 a.m. UTC | #1
On 13.06.2023 09:55, Neil Armstrong wrote:
> Altmode is also supported for SM8550, allow it.
> 
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad
>  drivers/soc/qcom/pmic_glink.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/soc/qcom/pmic_glink.c b/drivers/soc/qcom/pmic_glink.c
> index c87056769ebd..8af06bdc6f5a 100644
> --- a/drivers/soc/qcom/pmic_glink.c
> +++ b/drivers/soc/qcom/pmic_glink.c
> @@ -342,13 +342,9 @@ static const unsigned long pmic_glink_sm8450_client_mask = BIT(PMIC_GLINK_CLIENT
>  							   BIT(PMIC_GLINK_CLIENT_ALTMODE) |
>  							   BIT(PMIC_GLINK_CLIENT_UCSI);
>  
> -/* Do not handle altmode for now on those platforms */
> -static const unsigned long pmic_glink_sm8550_client_mask = BIT(PMIC_GLINK_CLIENT_BATT) |
> -							   BIT(PMIC_GLINK_CLIENT_UCSI);
> -
>  static const struct of_device_id pmic_glink_of_match[] = {
>  	{ .compatible = "qcom,sm8450-pmic-glink", .data = &pmic_glink_sm8450_client_mask },
> -	{ .compatible = "qcom,sm8550-pmic-glink", .data = &pmic_glink_sm8550_client_mask },
> +	{ .compatible = "qcom,sm8550-pmic-glink", .data = &pmic_glink_sm8450_client_mask },
>  	{ .compatible = "qcom,pmic-glink" },
>  	{}
>  };
>
Krzysztof Kozlowski June 13, 2023, 11:54 a.m. UTC | #2
On 13/06/2023 09:55, Neil Armstrong wrote:
> Altmode is also supported for SM8550, allow it.
> 
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> ---
>  drivers/soc/qcom/pmic_glink.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/soc/qcom/pmic_glink.c b/drivers/soc/qcom/pmic_glink.c
> index c87056769ebd..8af06bdc6f5a 100644
> --- a/drivers/soc/qcom/pmic_glink.c
> +++ b/drivers/soc/qcom/pmic_glink.c
> @@ -342,13 +342,9 @@ static const unsigned long pmic_glink_sm8450_client_mask = BIT(PMIC_GLINK_CLIENT
>  							   BIT(PMIC_GLINK_CLIENT_ALTMODE) |
>  							   BIT(PMIC_GLINK_CLIENT_UCSI);
>  
> -/* Do not handle altmode for now on those platforms */
> -static const unsigned long pmic_glink_sm8550_client_mask = BIT(PMIC_GLINK_CLIENT_BATT) |
> -							   BIT(PMIC_GLINK_CLIENT_UCSI);
> -
>  static const struct of_device_id pmic_glink_of_match[] = {
>  	{ .compatible = "qcom,sm8450-pmic-glink", .data = &pmic_glink_sm8450_client_mask },

Orientation and maybe all of the USB-related properties do not look like
sm8450 specific, but PM8350B. That's where CC_OUT pin is. I don't think
we represented this correctly, but rather copy-pasted downstream solution...

Best regards,
Krzysztof
Neil Armstrong June 13, 2023, 1:43 p.m. UTC | #3
On 13/06/2023 13:54, Krzysztof Kozlowski wrote:
> On 13/06/2023 09:55, Neil Armstrong wrote:
>> Altmode is also supported for SM8550, allow it.
>>
>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>> ---
>>   drivers/soc/qcom/pmic_glink.c | 6 +-----
>>   1 file changed, 1 insertion(+), 5 deletions(-)
>>
>> diff --git a/drivers/soc/qcom/pmic_glink.c b/drivers/soc/qcom/pmic_glink.c
>> index c87056769ebd..8af06bdc6f5a 100644
>> --- a/drivers/soc/qcom/pmic_glink.c
>> +++ b/drivers/soc/qcom/pmic_glink.c
>> @@ -342,13 +342,9 @@ static const unsigned long pmic_glink_sm8450_client_mask = BIT(PMIC_GLINK_CLIENT
>>   							   BIT(PMIC_GLINK_CLIENT_ALTMODE) |
>>   							   BIT(PMIC_GLINK_CLIENT_UCSI);
>>   
>> -/* Do not handle altmode for now on those platforms */
>> -static const unsigned long pmic_glink_sm8550_client_mask = BIT(PMIC_GLINK_CLIENT_BATT) |
>> -							   BIT(PMIC_GLINK_CLIENT_UCSI);
>> -
>>   static const struct of_device_id pmic_glink_of_match[] = {
>>   	{ .compatible = "qcom,sm8450-pmic-glink", .data = &pmic_glink_sm8450_client_mask },
> 
> Orientation and maybe all of the USB-related properties do not look like
> sm8450 specific, but PM8350B. That's where CC_OUT pin is. I don't think
> we represented this correctly, but rather copy-pasted downstream solution...

We do not interact directly with PM8350B or PM8550B but with pmic_glink who
does the work work for use, and this is platform specific.

Neil

> 
> Best regards,
> Krzysztof
>
Konrad Dybcio June 13, 2023, 1:58 p.m. UTC | #4
On 13.06.2023 15:43, Neil Armstrong wrote:
> On 13/06/2023 13:54, Krzysztof Kozlowski wrote:
>> On 13/06/2023 09:55, Neil Armstrong wrote:
>>> Altmode is also supported for SM8550, allow it.
>>>
>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>>> ---
>>>   drivers/soc/qcom/pmic_glink.c | 6 +-----
>>>   1 file changed, 1 insertion(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/soc/qcom/pmic_glink.c b/drivers/soc/qcom/pmic_glink.c
>>> index c87056769ebd..8af06bdc6f5a 100644
>>> --- a/drivers/soc/qcom/pmic_glink.c
>>> +++ b/drivers/soc/qcom/pmic_glink.c
>>> @@ -342,13 +342,9 @@ static const unsigned long pmic_glink_sm8450_client_mask = BIT(PMIC_GLINK_CLIENT
>>>                                  BIT(PMIC_GLINK_CLIENT_ALTMODE) |
>>>                                  BIT(PMIC_GLINK_CLIENT_UCSI);
>>>   -/* Do not handle altmode for now on those platforms */
>>> -static const unsigned long pmic_glink_sm8550_client_mask = BIT(PMIC_GLINK_CLIENT_BATT) |
>>> -                               BIT(PMIC_GLINK_CLIENT_UCSI);
>>> -
>>>   static const struct of_device_id pmic_glink_of_match[] = {
>>>       { .compatible = "qcom,sm8450-pmic-glink", .data = &pmic_glink_sm8450_client_mask },
>>
>> Orientation and maybe all of the USB-related properties do not look like
>> sm8450 specific, but PM8350B. That's where CC_OUT pin is. I don't think
>> we represented this correctly, but rather copy-pasted downstream solution...
> 
> We do not interact directly with PM8350B or PM8550B but with pmic_glink who
> does the work work for use, and this is platform specific.
Yep, pmic_glink is totally a firmware construct and not all platforms with
pm8350b or so implement it.

Konrad
> 
> Neil
> 
>>
>> Best regards,
>> Krzysztof
>>
>
Krzysztof Kozlowski June 13, 2023, 6:37 p.m. UTC | #5
On 13/06/2023 15:58, Konrad Dybcio wrote:
> 
> 
> On 13.06.2023 15:43, Neil Armstrong wrote:
>> On 13/06/2023 13:54, Krzysztof Kozlowski wrote:
>>> On 13/06/2023 09:55, Neil Armstrong wrote:
>>>> Altmode is also supported for SM8550, allow it.
>>>>
>>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>>>> ---
>>>>   drivers/soc/qcom/pmic_glink.c | 6 +-----
>>>>   1 file changed, 1 insertion(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/soc/qcom/pmic_glink.c b/drivers/soc/qcom/pmic_glink.c
>>>> index c87056769ebd..8af06bdc6f5a 100644
>>>> --- a/drivers/soc/qcom/pmic_glink.c
>>>> +++ b/drivers/soc/qcom/pmic_glink.c
>>>> @@ -342,13 +342,9 @@ static const unsigned long pmic_glink_sm8450_client_mask = BIT(PMIC_GLINK_CLIENT
>>>>                                  BIT(PMIC_GLINK_CLIENT_ALTMODE) |
>>>>                                  BIT(PMIC_GLINK_CLIENT_UCSI);
>>>>   -/* Do not handle altmode for now on those platforms */
>>>> -static const unsigned long pmic_glink_sm8550_client_mask = BIT(PMIC_GLINK_CLIENT_BATT) |
>>>> -                               BIT(PMIC_GLINK_CLIENT_UCSI);
>>>> -
>>>>   static const struct of_device_id pmic_glink_of_match[] = {
>>>>       { .compatible = "qcom,sm8450-pmic-glink", .data = &pmic_glink_sm8450_client_mask },
>>>
>>> Orientation and maybe all of the USB-related properties do not look like
>>> sm8450 specific, but PM8350B. That's where CC_OUT pin is. I don't think
>>> we represented this correctly, but rather copy-pasted downstream solution...
>>
>> We do not interact directly with PM8350B or PM8550B but with pmic_glink who
>> does the work work for use, and this is platform specific.
> Yep, pmic_glink is totally a firmware construct and not all platforms with
> pm8350b or so implement it.

The drivers interact like this, but there is no pmic glink on schematics
but PM8350B and PM8550 and DT represents here the hardware, thus the
schematics, not your chosen way of interface. This looks like a copy of
downstream choice which is in most cases just wrong choice.

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/drivers/soc/qcom/pmic_glink.c b/drivers/soc/qcom/pmic_glink.c
index c87056769ebd..8af06bdc6f5a 100644
--- a/drivers/soc/qcom/pmic_glink.c
+++ b/drivers/soc/qcom/pmic_glink.c
@@ -342,13 +342,9 @@  static const unsigned long pmic_glink_sm8450_client_mask = BIT(PMIC_GLINK_CLIENT
 							   BIT(PMIC_GLINK_CLIENT_ALTMODE) |
 							   BIT(PMIC_GLINK_CLIENT_UCSI);
 
-/* Do not handle altmode for now on those platforms */
-static const unsigned long pmic_glink_sm8550_client_mask = BIT(PMIC_GLINK_CLIENT_BATT) |
-							   BIT(PMIC_GLINK_CLIENT_UCSI);
-
 static const struct of_device_id pmic_glink_of_match[] = {
 	{ .compatible = "qcom,sm8450-pmic-glink", .data = &pmic_glink_sm8450_client_mask },
-	{ .compatible = "qcom,sm8550-pmic-glink", .data = &pmic_glink_sm8550_client_mask },
+	{ .compatible = "qcom,sm8550-pmic-glink", .data = &pmic_glink_sm8450_client_mask },
 	{ .compatible = "qcom,pmic-glink" },
 	{}
 };