sun4i-ts hwmon broken with linux 4.10

[Michael Weiser]

Hi,

I'm running statically linked (i.e. no loadable modules) vanilla kernels
on Cubieboard2's. With 4.9 and earlier the hwmon interface of the
sun4i-ts driver works flawlessly and reports something like:

# sensors
sun4i_ts-isa-0000
Adapter: ISA adapter
SoC temperature: +44.4°C

Conversely, /sys has a class/hwmon/hwmon0 node containing:

# cat /sys/class/hwmon/hwmon0/name
sun4i_ts

The driver is detected automatically at boot time.

With 4.10 the sensor isn't found any more and /sys/class/hwmon is
completely empty:

# sensors
No sensors found!
Make sure you loaded all the kernel drivers you need.
Try sensors-detect to find out which these are.
# find /sys/class/hwmon/
/sys/class/hwmon/

From the commit logs I noticed that there's been some cleanup in the
hwmon core.

Is 4.10 broken in terms of hwmon for sunxi or am I doing something
wrong?
--
Thanks,
Michael

[Michael Weiser]

Okay, shame on me,

On Sun, Feb 26, 2017 at 03:23:48PM +0100, Michael Weiser wrote:

> With 4.10 the sensor isn't found any more and /sys/class/hwmon is
> completely empty:

Turns out with CONFIG_MFD_SUN4I_GPADC enabled sun4i-ts isn't found any
more. If I leave sun4i-gpadc out of the kernel, all is well again.

Is sun4i-gpadc supposed to replace the hwmon part of sun4i-ts? If so:
How is it supposed to work, because even with sun4i-ts disabled and
iio_hwmon activated I don't get any sensors.
--
Thanks,
Michael

[Michael Weiser]

Hi Quentin,

On Wed, Mar 08, 2017 at 08:43:12AM +0100, Quentin Schulz wrote:

> First of all, you should have sent this mail to the linux-arm-kernel
[...]
> CONFIG_MFD_SUN4I_GPADC enabled seems to be the culprit. Then, Cc the
> guy(s) who wrote/edited the driver. In that case, myself.

I can only ask for understanding that the way of communication in the
Linux community (massive cross-posting, need to mail individuals
directly to get noticed) is exactly the opposite of all other projects I
interact with and would get me admonished just as harshly there. It's
hard to keep track of per-project Netiquette as an infrequent requestor
or contributor. I'll certainly try to do better.

> > If so:
> > How is it supposed to work, because even with sun4i-ts disabled and
> > iio_hwmon activated I don't get any sensors.

> It'll not work because these two drivers are drivers for the same
> component. The mfd driver is probing other drivers (such as iio_hwmon
> for example, but also the IIO driver in charge of registering the sensor
> in Linux and returning values). However, the IIO driver is not merged yet.

I figured out as much by looking more closely at the source and
researching your patch series' and discussions yesterday. So it's very
unfortunate that https://patchwork.kernel.org/patch/9472445/ didn't make
it into 4.10 or this whole discussion could have been avoided.

BTW: Out of curiosity I applied
https://patchwork.kernel.org/patch/9472451/ to yesterday's mainline
master and activated it. All the rest is already there, it seems. So I
now have:

# CONFIG_INPUT_TOUCHSCREEN is not set
CONFIG_SENSORS_IIO_HWMON=y
CONFIG_MFD_SUN4I_GPADC=y
CONFIG_SUN4I_GPADC=y

Still no sensor reading though. I guess, I'll wait for 4.12 and see.
--
Thanks for getting back to me,
Micha

[Quentin Schulz]

Hi Michael,

On 08/03/2017 09:31, Michael Weiser wrote:
> Hi Quentin,
>
> On Wed, Mar 08, 2017 at 08:43:12AM +0100, Quentin Schulz wrote:
>
>> First of all, you should have sent this mail to the linux-arm-kernel
> [...]
>> CONFIG_MFD_SUN4I_GPADC enabled seems to be the culprit. Then, Cc the
>> guy(s) who wrote/edited the driver. In that case, myself.
>
> I can only ask for understanding that the way of communication in the
> Linux community (massive cross-posting, need to mail individuals
> directly to get noticed)

We receive a ton of mail through the different mailing lists. The
mailing list is seen more as an archive. Most of the time, the
maintainers or developers contributing to the drivers will be the one
having the answer to your question, but sometimes other people might
have a different opinion or participate in the thread, that's also why
we use the mailing list.

So, you Cc the mailing list so we have an archive of the mail and people
which are not devs of the drivers or maintainers can answer to your
question. You Cc the maintainers/devs so we know we should be able to
answer to your question and it does not hide in the huge pile of unread
mails from the mailing list.

> is exactly the opposite of all other projects I
> interact with and would get me admonished just as harshly there.

It wasn't meant to be harsh, sorry about that.

> It's
> hard to keep track of per-project Netiquette as an infrequent requestor
> or contributor. I'll certainly try to do better.
>
>>> If so:
>>> How is it supposed to work, because even with sun4i-ts disabled and
>>> iio_hwmon activated I don't get any sensors.
>> It'll not work because these two drivers are drivers for the same
>> component. The mfd driver is probing other drivers (such as iio_hwmon
>> for example, but also the IIO driver in charge of registering the sensor
>> in Linux and returning values). However, the IIO driver is not merged yet.
>
> I figured out as much by looking more closely at the source and
> researching your patch series' and discussions yesterday. So it's very
> unfortunate that https://patchwork.kernel.org/patch/9472445/ didn't make
> it into 4.10 or this whole discussion could have been avoided.
>

Indeed.

> BTW: Out of curiosity I applied
> https://patchwork.kernel.org/patch/9472451/ to yesterday's mainline
> master and activated it. All the rest is already there, it seems. So I
> now have:
>
> # CONFIG_INPUT_TOUCHSCREEN is not set
> CONFIG_SENSORS_IIO_HWMON=y
> CONFIG_MFD_SUN4I_GPADC=y
> CONFIG_SUN4I_GPADC=y
>
> Still no sensor reading though. I guess, I'll wait for 4.12 and see.
>

Hum, no. Let's get that fixed (if it's a bug) before reaching 4.12 :)

Have you applied this[1] patch as well or enabled CONFIG_OF_THERMAL?

[1] https://patchwork.kernel.org/patch/9472443/

Quentin

--
Quentin Schulz, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[Michael Weiser]

Hi Quentin,

On Wed, Mar 08, 2017 at 10:35:54AM +0100, Quentin Schulz wrote:

> > Still no sensor reading though. I guess, I'll wait for 4.12 and see.
> Hum, no. Let's get that fixed (if it's a bug) before reaching 4.12 :)

> Have you applied this[1] patch as well or enabled CONFIG_OF_THERMAL?

> [1] https://patchwork.kernel.org/patch/9472443/

Well, the patch is in but the setting didn't make it from the defconfig
to my config. My bad. Now it's sensoring as expected:

# sensors
iio_hwmon-isa-0000
Adapter: ISA adapter
temp1: +28.4°C

Thanks for your help!

On a general note: Am I right in my perception that there's currently a
trend in the Linux kernel to have things abstracted to such a degree
and spread across so large a number of independent subsystems that it's
not possible any more and doesn't make sense to have settings depend on
each other in a manner that a single switch can enable a whole function?

iio_hwmon-isa-0000 = (SENSORS_IIO_HWMON | MFD_SUN4I_GPADC | SUN4I_GPADC | THERMAL_OF)

This is the second instance where I had to chase some quite non-obvious
dependencies with no aid from Kconfig to get something working on my
Cubieboard. (I think the last one had to do with pinctrl and gpio - not
sure.)

In the current case: Maybe the help text of SUN4I_GPADC could be
adjusted to state clearly that THERMAL_OF is required for the sensor to
work?

"For the thermal sensor to work, CONFIG_THERMAL_OF needs to be enabled as
well. The thermal sensor does slow down ADC readings though. This can be
avoided by not enabling CONFIG_THERMAL_OF. However, the thermal sensor
should be enabled by default since the SoC temperature is usually more
critical than ADC readings."
--
Thanks for your patience,
Michael

[Quentin Schulz]

Hi Michael,

On 08/03/2017 11:33, Michael Weiser wrote:
> Hi Quentin,
>
> On Wed, Mar 08, 2017 at 10:35:54AM +0100, Quentin Schulz wrote:
>
>>> Still no sensor reading though. I guess, I'll wait for 4.12 and see.
>> Hum, no. Let's get that fixed (if it's a bug) before reaching 4.12 :)
>
>> Have you applied this[1] patch as well or enabled CONFIG_OF_THERMAL?
>
>> [1] https://patchwork.kernel.org/patch/9472443/
>
> Well, the patch is in but the setting didn't make it from the defconfig
> to my config. My bad. Now it's sensoring as expected:
>
> # sensors
> iio_hwmon-isa-0000
> Adapter: ISA adapter
> temp1: +28.4°C
>
> Thanks for your help!
>

Cool!

> On a general note: Am I right in my perception that there's currently a
> trend in the Linux kernel to have things abstracted to such a degree
> and spread across so large a number of independent subsystems that it's
> not possible any more and doesn't make sense to have settings depend on
> each other in a manner that a single switch can enable a whole function?
>
> iio_hwmon-isa-0000 = (SENSORS_IIO_HWMON | MFD_SUN4I_GPADC | SUN4I_GPADC | THERMAL_OF)
>

Not sure I get what you mean, correct me if I don't.

Hardware components tend to be more and more complex with a lot of
different "sub-"components. For example, the AXP20X PMICs have ADCs,
GPIOs, handle different power supplies, handle power buttons, handle
battery fuel gauge, etc... You need to separate all that in different
drivers so you can take the most from each impacted subsystem and have
multiple simple drivers rather than one unmaintainable gigantic driver
doing everything.

The point to have also multiple simple drivers is that some components
have parts or "sub-"components that are almost the same across different
series of components or brands. This allows us to reuse the maximum code
we can, or adapt existing code to add support for a new component. If we
had to write a big driver for it, it would be just tedious and totally
unmaintainable.

This adds a complexity layer on the Kconfig though, as for example,
within the same driver some parts cannot be used for different
components. The driver is still the same, the Kconfig option is still
the same but depending on the hardware, you'll need to add such or such
additional Kconfig option to take the most of your driver.

If we had to say for this driver to work on this SoC or board, you need
to enable such or such options... that would be a huge list and
something to maintain.

> This is the second instance where I had to chase some quite non-obvious
> dependencies with no aid from Kconfig to get something working on my
> Cubieboard. (I think the last one had to do with pinctrl and gpio - not
> sure.)
>
> In the current case: Maybe the help text of SUN4I_GPADC could be
> adjusted to state clearly that THERMAL_OF is required for the sensor to
> work?
>

The patch enables CONFIG_THERMAL_OF by default for all sunxi platforms.
The Kconfig states that you can disable it if you want to get rid of the
thermal sensor. That should be enough IMHO.

> "For the thermal sensor to work, CONFIG_THERMAL_OF needs to be enabled as
> well. The thermal sensor does slow down ADC readings though. This can be
> avoided by not enabling CONFIG_THERMAL_OF. However, the thermal sensor
> should be enabled by default since the SoC temperature is usually more
> critical than ADC readings."
>

[Michael Weiser]

Hi Quentin,

On Wed, Mar 08, 2017 at 02:14:46PM +0100, Quentin Schulz wrote:

> > iio_hwmon-isa-0000 = (SENSORS_IIO_HWMON | MFD_SUN4I_GPADC |
> > SUN4I_GPADC | THERMAL_OF)

> Not sure I get what you mean, correct me if I don't.

I understand your points and I was just asking out of curiosity. It's a
meta-discussion anyway - let's table it. :)

> > In the current case: Maybe the help text of SUN4I_GPADC could be
> > adjusted to state clearly that THERMAL_OF is required for the sensor to
> > work?
> The patch enables CONFIG_THERMAL_OF by default for all sunxi platforms.
> The Kconfig states that you can disable it if you want to get rid of the
> thermal sensor. That should be enough IMHO.

Not everyone is working off the defconfig though. My custom config
started out with sunxi_defconfig but now I just 'make oldconfig' it from
the one kernel version to the next. I decide on new options based on the
help text. In this case THERMAL_OF was not enabled because it wasn't
necessary and not in the defconfig before. For such cases a clear(er)
pointer to the necessity of THERMAL_OF in the help text of SUN4I_GPADC
might be helpful.

Anyway, thanks again for all your help and patience. Looking forward to
4.12 now. ;)
--
Michael

[Timo S.]

Hi Michael,

Am Mittwoch, 8. März 2017 15:34:03 UTC+1 schrieb Michael Weiser:

Not everyone is working off the defconfig though. My custom config
started out with sunxi_defconfig but now I just 'make oldconfig' it from
the one kernel version to the next. I decide on new options based on the
help text. In this case THERMAL_OF was not enabled because it wasn't
necessary and not in the defconfig before. For such cases a clear(er)
pointer to the necessity of THERMAL_OF in the help text of SUN4I_GPADC
might be helpful.

I use a similar approach, but I always check the diff of sunxi_defconfig when moving to a new major release. There have been quite a few changes in the past where new options were required to make previously working things working again or where options have been renamed, etc. That's why I got accustomed to looking at the changes in sunxi_defconfig first, before I try to generate my own config for a new major release. And while that really makes things easier, it's of course no guarantee that something might still have slipped through and needs to be added/fixed in the next relase.

Regards,

Timo