[4/7] iio: adc: qcom-spmi-adc5: Add missing VCOIN/AMUX_THM3/GPIO# channels

Commit Message

Marijn Suijten May 11, 2022, 10:06 p.m. UTC

These channels are specified in downstream kernels [1] and actively used
by ie. the Sony Seine platform on the SM6125 SoC.

[1]: https://source.codeaurora.org/quic/la/kernel/msm-4.14/tree/drivers/iio/adc/qcom-spmi-adc5.c?h=LA.UM.7.11.r1-05200-NICOBAR.0#n688

Signed-off-by: Marijn Suijten <[e-mail]>
Reviewed-by: AngeloGioacchino Del Regno <[e-mail]>
---
 drivers/iio/adc/qcom-spmi-adc5.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Jonathan Cameron May 14, 2022, 4:13 p.m. UTC | #1

On Thu, 12 May 2022 00:06:10 +0200
Marijn Suijten <[e-mail]> wrote:

> These channels are specified in downstream kernels [1] and actively used
> by ie. the Sony Seine platform on the SM6125 SoC.

Looking at the links, some of them are on that platform but not all.
Better to make that explicit in this description.

> 
> [1]: https://source.codeaurora.org/quic/la/kernel/msm-4.14/tree/drivers/iio/adc/qcom-spmi-adc5.c?h=LA.UM.7.11.r1-05200-NICOBAR.0#n688
> 
> Signed-off-by: Marijn Suijten <[e-mail]>
> Reviewed-by: AngeloGioacchino Del Regno <[e-mail]>

I'm not keen on patches with no context being
sent to mailing lists. Please cc all lists (and preferably individuals)
on at least the cover letter so we can see overall discussion.

If nothing else, I've no idea if intent is that the patches go through different
trees or all need to merge via one route.

Patch itself looks fine,

Jonathan


> ---
>  drivers/iio/adc/qcom-spmi-adc5.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/iio/adc/qcom-spmi-adc5.c b/drivers/iio/adc/qcom-spmi-adc5.c
> index 87438d1e5c0b..69c7fd44d34c 100644
> --- a/drivers/iio/adc/qcom-spmi-adc5.c
> +++ b/drivers/iio/adc/qcom-spmi-adc5.c
> @@ -526,6 +526,8 @@ static const struct adc5_channels adc5_chans_pmic[ADC5_MAX_CHANNEL] = {
>  					SCALE_HW_CALIB_DEFAULT)
>  	[ADC5_VBAT_SNS]		= ADC5_CHAN_VOLT("vbat_sns", 1,
>  					SCALE_HW_CALIB_DEFAULT)
> +	[ADC5_VCOIN]		= ADC5_CHAN_VOLT("vcoin", 1,
> +					SCALE_HW_CALIB_DEFAULT)
>  	[ADC5_DIE_TEMP]		= ADC5_CHAN_TEMP("die_temp", 0,
>  					SCALE_HW_CALIB_PMIC_THERM)
>  	[ADC5_USB_IN_I]		= ADC5_CHAN_VOLT("usb_in_i_uv", 0,
> @@ -549,6 +551,16 @@ static const struct adc5_channels adc5_chans_pmic[ADC5_MAX_CHANNEL] = {
>  					SCALE_HW_CALIB_THERM_100K_PULLUP)
>  	[ADC5_AMUX_THM2]	= ADC5_CHAN_TEMP("amux_thm2", 0,
>  					SCALE_HW_CALIB_PM5_SMB_TEMP)
> +	[ADC5_AMUX_THM3]	= ADC5_CHAN_TEMP("amux_thm3", 0,
> +					SCALE_HW_CALIB_PM5_SMB_TEMP)
> +	[ADC5_GPIO1_100K_PU]	= ADC5_CHAN_TEMP("gpio1_100k_pu", 0,
> +					SCALE_HW_CALIB_THERM_100K_PULLUP)
> +	[ADC5_GPIO2_100K_PU]	= ADC5_CHAN_TEMP("gpio2_100k_pu", 0,
> +					SCALE_HW_CALIB_THERM_100K_PULLUP)
> +	[ADC5_GPIO3_100K_PU]	= ADC5_CHAN_TEMP("gpio3_100k_pu", 0,
> +					SCALE_HW_CALIB_THERM_100K_PULLUP)
> +	[ADC5_GPIO4_100K_PU]	= ADC5_CHAN_TEMP("gpio4_100k_pu", 0,
> +					SCALE_HW_CALIB_THERM_100K_PULLUP)
>  };
>  
>  static const struct adc5_channels adc7_chans_pmic[ADC5_MAX_CHANNEL] = {

Marijn Suijten May 15, 2022, 3:30 p.m. UTC | #2

On 2022-05-14 17:13:12, Jonathan Cameron wrote:
> On Thu, 12 May 2022 00:06:10 +0200
> Marijn Suijten <[e-mail]> wrote:
> 
> > These channels are specified in downstream kernels [1] and actively used
> > by ie. the Sony Seine platform on the SM6125 SoC.
> 
> Looking at the links, some of them are on that platform but not all.
> Better to make that explicit in this description.

This has already been queued up for v2.  Adding these seemed easy at the
time but they are in fact not used, and I ended up sending the wrong
patch.

Just so that we're on the same page: only ADC5_AMUX_THM3 and
ADC5_GPIO2_100K_PU are unused by my platform.  It seems the first should
be dropped, but the latter can probably stay in the patch with an
explicit mention.  If you think both should stay, there are a bunch more
channels defined in the downstream kernel as per [1] and I'm not sure if
all should be added for completeness.

> > 
> > [1]: https://source.codeaurora.org/quic/la/kernel/msm-4.14/tree/drivers/iio/adc/qcom-spmi-adc5.c?h=LA.UM.7.11.r1-05200-NICOBAR.0#n688
> > 
> > Signed-off-by: Marijn Suijten <[e-mail]>
> > Reviewed-by: AngeloGioacchino Del Regno <[e-mail]>
> 
> I'm not keen on patches with no context being
> sent to mailing lists. Please cc all lists (and preferably individuals)
> on at least the cover letter so we can see overall discussion.

That can be attributed to the terrible workflow for sending
patch-series.  Somehow only `git send-email` supports --cc-cmd yet I'd
expect it on `git format-patch` for auditing and possibly copying to the
cover letter, if `git format-patch --cover-letter` couldn't do this from
the beginning.
At the same time `git send-email` has --[to/cc]-cover options to
propagate email addresses from the cover letter to all the individual
patch-replies, but not the inverse :(

In the end this leaves me manually running get_maintainer.pl over the
entire formatted patch-series, and manually copy-pasting + editing the
addresses into the cover letter... Which is easy to forget and is no
different here.

My apologies for (yet again) accidentally not sending at least the cover
letter to everyone.  That's a gross oversight, and I'm probably - no, I
must - be doing something wrong.  Suggestions and/or documentation
references are welcome.

> If nothing else, I've no idea if intent is that the patches go through different
> trees or all need to merge via one route.

I have no idea either, and have not yet had an answer to a similar
question on a different list.  Usually it seems the maintainers work out
amongst themselves who picks what patch, putting them on hold where
necessary to preseve ordering.  If not, should the sender split patches
across multiple series, either holding off sending part of it or linking
to a dependent series?

In this particular case DT has to wait for these driver patches to land,
otherwise they may define channels that do not exist and unnecessarily
fail probe.

> Patch itself looks fine,

Thanks.

Looking forward to your suggestions and answers,

- Marijn

> [..]

Jonathan Cameron May 15, 2022, 4:57 p.m. UTC | #3

On Sun, 15 May 2022 17:30:04 +0200
Marijn Suijten <[e-mail]> wrote:

> On 2022-05-14 17:13:12, Jonathan Cameron wrote:
> > On Thu, 12 May 2022 00:06:10 +0200
> > Marijn Suijten <[e-mail]> wrote:
> >   
> > > These channels are specified in downstream kernels [1] and actively used
> > > by ie. the Sony Seine platform on the SM6125 SoC.  
> > 
> > Looking at the links, some of them are on that platform but not all.
> > Better to make that explicit in this description.  
> 
> This has already been queued up for v2.  Adding these seemed easy at the
> time but they are in fact not used, and I ended up sending the wrong
> patch.
> 
> Just so that we're on the same page: only ADC5_AMUX_THM3 and
> ADC5_GPIO2_100K_PU are unused by my platform.  It seems the first should
> be dropped, but the latter can probably stay in the patch with an
> explicit mention.  If you think both should stay, there are a bunch more
> channels defined in the downstream kernel as per [1] and I'm not sure if
> all should be added for completeness.

Probably better to add them with a comment for platforms on which they
apply (either in commit log or alongside the definitions in the code).

Longer term we should think about whether the code can be adjusted
to not need an explicit definition for these multi purpose channels
though letting the dt itself describe them (under constraints of the
hardware platform).  Not worth doing before this patch though.

> 
> > > 
> > > [1]: https://source.codeaurora.org/quic/la/kernel/msm-4.14/tree/drivers/iio/adc/qcom-spmi-adc5.c?h=LA.UM.7.11.r1-05200-NICOBAR.0#n688
> > > 
> > > Signed-off-by: Marijn Suijten <[e-mail]>
> > > Reviewed-by: AngeloGioacchino Del Regno <[e-mail]>  
> > 
> > I'm not keen on patches with no context being
> > sent to mailing lists. Please cc all lists (and preferably individuals)
> > on at least the cover letter so we can see overall discussion.  
> 
> That can be attributed to the terrible workflow for sending
> patch-series.  Somehow only `git send-email` supports --cc-cmd yet I'd
> expect it on `git format-patch` for auditing and possibly copying to the
> cover letter, if `git format-patch --cover-letter` couldn't do this from
> the beginning.

It would definitely be nice as an option.  Can't do it every time because
on tree wide change the cc list can become so large the mailing lists
reject it.

> At the same time `git send-email` has --[to/cc]-cover options to
> propagate email addresses from the cover letter to all the individual
> patch-replies, but not the inverse :(
> 
> In the end this leaves me manually running get_maintainer.pl over the
> entire formatted patch-series, and manually copy-pasting + editing the
> addresses into the cover letter... Which is easy to forget and is no
> different here.
> 
> My apologies for (yet again) accidentally not sending at least the cover
> letter to everyone.  That's a gross oversight, and I'm probably - no, I
> must - be doing something wrong.  Suggestions and/or documentation
> references are welcome.

Andy Shevchenko has some scripts to try and help with this:
https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh

I've not started using them myself (not gotten around to it yet!) but
he's pointed to them when I've missed people from cover letter cc lists
in the past.

> 
> > If nothing else, I've no idea if intent is that the patches go through different
> > trees or all need to merge via one route.  
> 
> I have no idea either, and have not yet had an answer to a similar
> question on a different list.  Usually it seems the maintainers work out
> amongst themselves who picks what patch, putting them on hold where
> necessary to preseve ordering.  If not, should the sender split patches
> across multiple series, either holding off sending part of it or linking
> to a dependent series?

In this case I think I can pick this patch up directly into the IIO tree
once everyone else is happy. As you note dts patches normally wait on
knowing the necessary support is heading in.  If you have a view on what
makes sense as the submitter it's good to stick it in the cover letter, but
in this case sounds like you don't. :)

Given we are near the end of this cycle, we are probably looking at next
cycle anyway now, so plenty of time to figure it out!

> 
> In this particular case DT has to wait for these driver patches to land,
> otherwise they may define channels that do not exist and unnecessarily
> fail probe.
> 
> > Patch itself looks fine,  
> 
> Thanks.
> 
> Looking forward to your suggestions and answers,
> 
> - Marijn
> 
> > [..]

Jonathan Cameron May 15, 2022, 4:58 p.m. UTC | #4

On Sun, 15 May 2022 17:57:14 +0100
Jonathan Cameron <[e-mail]> wrote:

> On Sun, 15 May 2022 17:30:04 +0200
> Marijn Suijten <[e-mail]> wrote:
> 
> > On 2022-05-14 17:13:12, Jonathan Cameron wrote:  
> > > On Thu, 12 May 2022 00:06:10 +0200
> > > Marijn Suijten <[e-mail]> wrote:
> > >     
> > > > These channels are specified in downstream kernels [1] and actively used
> > > > by ie. the Sony Seine platform on the SM6125 SoC.    
> > > 
> > > Looking at the links, some of them are on that platform but not all.
> > > Better to make that explicit in this description.    
> > 
> > This has already been queued up for v2.  Adding these seemed easy at the
> > time but they are in fact not used, and I ended up sending the wrong
> > patch.
> > 
> > Just so that we're on the same page: only ADC5_AMUX_THM3 and
> > ADC5_GPIO2_100K_PU are unused by my platform.  It seems the first should
> > be dropped, but the latter can probably stay in the patch with an
> > explicit mention.  If you think both should stay, there are a bunch more
> > channels defined in the downstream kernel as per [1] and I'm not sure if
> > all should be added for completeness.  
> 
> Probably better to add them with a comment for platforms on which they
> apply (either in commit log or alongside the definitions in the code).
By 'them' I mean add the ones used on your platform only.  Let others
add theirs when / if boards using them are upstreamed.

(realised I'd been very unclear just after hitting send!)


> 
> Longer term we should think about whether the code can be adjusted
> to not need an explicit definition for these multi purpose channels
> though letting the dt itself describe them (under constraints of the
> hardware platform).  Not worth doing before this patch though.
> 
> >   
> > > > 
> > > > [1]: https://source.codeaurora.org/quic/la/kernel/msm-4.14/tree/drivers/iio/adc/qcom-spmi-adc5.c?h=LA.UM.7.11.r1-05200-NICOBAR.0#n688
> > > > 
> > > > Signed-off-by: Marijn Suijten <[e-mail]>
> > > > Reviewed-by: AngeloGioacchino Del Regno <[e-mail]>    
> > > 
> > > I'm not keen on patches with no context being
> > > sent to mailing lists. Please cc all lists (and preferably individuals)
> > > on at least the cover letter so we can see overall discussion.    
> > 
> > That can be attributed to the terrible workflow for sending
> > patch-series.  Somehow only `git send-email` supports --cc-cmd yet I'd
> > expect it on `git format-patch` for auditing and possibly copying to the
> > cover letter, if `git format-patch --cover-letter` couldn't do this from
> > the beginning.  
> 
> It would definitely be nice as an option.  Can't do it every time because
> on tree wide change the cc list can become so large the mailing lists
> reject it.
> 
> > At the same time `git send-email` has --[to/cc]-cover options to
> > propagate email addresses from the cover letter to all the individual
> > patch-replies, but not the inverse :(
> > 
> > In the end this leaves me manually running get_maintainer.pl over the
> > entire formatted patch-series, and manually copy-pasting + editing the
> > addresses into the cover letter... Which is easy to forget and is no
> > different here.
> > 
> > My apologies for (yet again) accidentally not sending at least the cover
> > letter to everyone.  That's a gross oversight, and I'm probably - no, I
> > must - be doing something wrong.  Suggestions and/or documentation
> > references are welcome.  
> 
> Andy Shevchenko has some scripts to try and help with this:
> https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh
> 
> I've not started using them myself (not gotten around to it yet!) but
> he's pointed to them when I've missed people from cover letter cc lists
> in the past.
> 
> >   
> > > If nothing else, I've no idea if intent is that the patches go through different
> > > trees or all need to merge via one route.    
> > 
> > I have no idea either, and have not yet had an answer to a similar
> > question on a different list.  Usually it seems the maintainers work out
> > amongst themselves who picks what patch, putting them on hold where
> > necessary to preseve ordering.  If not, should the sender split patches
> > across multiple series, either holding off sending part of it or linking
> > to a dependent series?  
> 
> In this case I think I can pick this patch up directly into the IIO tree
> once everyone else is happy. As you note dts patches normally wait on
> knowing the necessary support is heading in.  If you have a view on what
> makes sense as the submitter it's good to stick it in the cover letter, but
> in this case sounds like you don't. :)
> 
> Given we are near the end of this cycle, we are probably looking at next
> cycle anyway now, so plenty of time to figure it out!
> 
> > 
> > In this particular case DT has to wait for these driver patches to land,
> > otherwise they may define channels that do not exist and unnecessarily
> > fail probe.
> >   
> > > Patch itself looks fine,    
> > 
> > Thanks.
> > 
> > Looking forward to your suggestions and answers,
> > 
> > - Marijn
> >   
> > > [..]    
>

Patch

diff --git a/drivers/iio/adc/qcom-spmi-adc5.c b/drivers/iio/adc/qcom-spmi-adc5.c
index 87438d1e5c0b..69c7fd44d34c 100644
--- a/drivers/iio/adc/qcom-spmi-adc5.c
+++ b/drivers/iio/adc/qcom-spmi-adc5.c
@@ -526,6 +526,8 @@  static const struct adc5_channels adc5_chans_pmic[ADC5_MAX_CHANNEL] = {
 					SCALE_HW_CALIB_DEFAULT)
 	[ADC5_VBAT_SNS]		= ADC5_CHAN_VOLT("vbat_sns", 1,
 					SCALE_HW_CALIB_DEFAULT)
+	[ADC5_VCOIN]		= ADC5_CHAN_VOLT("vcoin", 1,
+					SCALE_HW_CALIB_DEFAULT)
 	[ADC5_DIE_TEMP]		= ADC5_CHAN_TEMP("die_temp", 0,
 					SCALE_HW_CALIB_PMIC_THERM)
 	[ADC5_USB_IN_I]		= ADC5_CHAN_VOLT("usb_in_i_uv", 0,
@@ -549,6 +551,16 @@  static const struct adc5_channels adc5_chans_pmic[ADC5_MAX_CHANNEL] = {
 					SCALE_HW_CALIB_THERM_100K_PULLUP)
 	[ADC5_AMUX_THM2]	= ADC5_CHAN_TEMP("amux_thm2", 0,
 					SCALE_HW_CALIB_PM5_SMB_TEMP)
+	[ADC5_AMUX_THM3]	= ADC5_CHAN_TEMP("amux_thm3", 0,
+					SCALE_HW_CALIB_PM5_SMB_TEMP)
+	[ADC5_GPIO1_100K_PU]	= ADC5_CHAN_TEMP("gpio1_100k_pu", 0,
+					SCALE_HW_CALIB_THERM_100K_PULLUP)
+	[ADC5_GPIO2_100K_PU]	= ADC5_CHAN_TEMP("gpio2_100k_pu", 0,
+					SCALE_HW_CALIB_THERM_100K_PULLUP)
+	[ADC5_GPIO3_100K_PU]	= ADC5_CHAN_TEMP("gpio3_100k_pu", 0,
+					SCALE_HW_CALIB_THERM_100K_PULLUP)
+	[ADC5_GPIO4_100K_PU]	= ADC5_CHAN_TEMP("gpio4_100k_pu", 0,
+					SCALE_HW_CALIB_THERM_100K_PULLUP)
 };
 
 static const struct adc5_channels adc7_chans_pmic[ADC5_MAX_CHANNEL] = {