[Q] tps65185 EPD PMIC temperature interface - which subsystem

[Q] tps65185 EPD PMIC temperature interface - which subsystem

[Andreas Kemnade]

Hi,

I am going about to clean up stuff to further upstream support for my
ebook readers. One question arises about the temperature interface of
the EPD PMIC. Vendor code uses regulator_get_voltage in the EPDC
driver to read a temperature in celsius and provides temperature through
the regulator interface (besides sysfs/hwmon). That is ugly. But what
are the options, if a kernel consumer should be able to reference it via
devicetree phandle and read out from it? I see temperature sensors
both in the iio and the hwmon subsystem, but do not find a description
why these things are there. If I put it into the iio-subsystem
iio_channel_get() and friends can be used, if I understand things
correctly, there are no such functions in the hwmon subsystem, so I
would not be able to use it there. So the better choice is to put it
into the iio subsystem?

On the consumer side, the temperature, which is pratically the ambient
temperature, is used to choose the right waveform for the corresponding
temperature range. Here are some code snippets in the vendor kernel:

temperature = regulator_get_voltage(fb_data->tmst_regulator);
dev_dbg(fb_data->dev, "auto temperature reading = %d\n", temperature);

if (temperature != 0xFF) {
	fb_data->last_time_temp_auto_update = now;
	fb_data->temp_index = mxc_epdc_fb_get_temp_index(fb_data, temperature);
}

static int mxc_epdc_fb_get_temp_index(struct mxc_epdc_fb_data *fb_data, int temp
)
{
        int i;
        int index = -1;

        if (fb_data->trt_entries == 0) {
                dev_err(fb_data->dev,
                        "No TRT exists...using default temp index\n");
                return DEFAULT_TEMP_INDEX;
        }

        /* Search temperature ranges for a match */
        for (i = 0; i < fb_data->trt_entries - 1; i++) {
                if ((temp >= fb_data->temp_range_bounds[i])
                        && (temp < fb_data->temp_range_bounds[i+1])) {
                        index = i;
                        break;
                }
        }

... and writing that index to some register in the EPDC.

As the consumer is not upstream (I have a basic drm-based variant also
in my clean-up queue), compatibilty to existing systems does not matter
that much. Also I see no drivers for similar chips upstream.

Regards,
Andreas

Re: [Q] tps65185 EPD PMIC temperature interface - which subsystem

[Guenter Roeck]

On Fri, Apr 30, 2021 at 11:24:04PM +0200, Andreas Kemnade wrote:
> Hi,
> 
> I am going about to clean up stuff to further upstream support for my
> ebook readers. One question arises about the temperature interface of
> the EPD PMIC. Vendor code uses regulator_get_voltage in the EPDC
> driver to read a temperature in celsius and provides temperature through
> the regulator interface (besides sysfs/hwmon). That is ugly. But what
> are the options, if a kernel consumer should be able to reference it via
> devicetree phandle and read out from it? I see temperature sensors
> both in the iio and the hwmon subsystem, but do not find a description
> why these things are there. If I put it into the iio-subsystem
> iio_channel_get() and friends can be used, if I understand things
> correctly, there are no such functions in the hwmon subsystem, so I
> would not be able to use it there. So the better choice is to put it
> into the iio subsystem?
> 

I am guessing a bit here since a lot of context is missing. Presumably
there is a regulator driver. That regulator driver could register itself
with the hwmon subsystem using [devm_]hwmon_device_register_with_info()
and tell it to register a thermal zone sensor. It should then be possible
to read the temperature of that sensor using thermal_zone_get_temp().

Guenter

> On the consumer side, the temperature, which is pratically the ambient
> temperature, is used to choose the right waveform for the corresponding
> temperature range. Here are some code snippets in the vendor kernel:
> 
> temperature = regulator_get_voltage(fb_data->tmst_regulator);
> dev_dbg(fb_data->dev, "auto temperature reading = %d\n", temperature);
> 
> if (temperature != 0xFF) {
> 	fb_data->last_time_temp_auto_update = now;
> 	fb_data->temp_index = mxc_epdc_fb_get_temp_index(fb_data, temperature);
> }
> 
> static int mxc_epdc_fb_get_temp_index(struct mxc_epdc_fb_data *fb_data, int temp
> )
> {
>         int i;
>         int index = -1;
> 
>         if (fb_data->trt_entries == 0) {
>                 dev_err(fb_data->dev,
>                         "No TRT exists...using default temp index\n");
>                 return DEFAULT_TEMP_INDEX;
>         }
> 
>         /* Search temperature ranges for a match */
>         for (i = 0; i < fb_data->trt_entries - 1; i++) {
>                 if ((temp >= fb_data->temp_range_bounds[i])
>                         && (temp < fb_data->temp_range_bounds[i+1])) {
>                         index = i;
>                         break;
>                 }
>         }
> 
> ... and writing that index to some register in the EPDC.
> 
> As the consumer is not upstream (I have a basic drm-based variant also
> in my clean-up queue), compatibilty to existing systems does not matter
> that much. Also I see no drivers for similar chips upstream.
> 
> Regards,
> Andreas

Re: [Q] tps65185 EPD PMIC temperature interface - which subsystem

[Andreas Kemnade]

On Fri, 30 Apr 2021 20:02:13 -0700
Guenter Roeck <linux@roeck-us.net> wrote:

> On Fri, Apr 30, 2021 at 11:24:04PM +0200, Andreas Kemnade wrote:
> > Hi,
> > 
> > I am going about to clean up stuff to further upstream support for my
> > ebook readers. One question arises about the temperature interface of
> > the EPD PMIC. Vendor code uses regulator_get_voltage in the EPDC
> > driver to read a temperature in celsius and provides temperature through
> > the regulator interface (besides sysfs/hwmon). That is ugly. But what
> > are the options, if a kernel consumer should be able to reference it via
> > devicetree phandle and read out from it? I see temperature sensors
> > both in the iio and the hwmon subsystem, but do not find a description
> > why these things are there. If I put it into the iio-subsystem
> > iio_channel_get() and friends can be used, if I understand things
> > correctly, there are no such functions in the hwmon subsystem, so I
> > would not be able to use it there. So the better choice is to put it
> > into the iio subsystem?
> >   
> 
> I am guessing a bit here since a lot of context is missing. Presumably
> there is a regulator driver. That regulator driver could register itself
> with the hwmon subsystem using [devm_]hwmon_device_register_with_info()
> and tell it to register a thermal zone sensor. It should then be possible
> to read the temperature of that sensor using thermal_zone_get_temp().
> 
Well, I try to give first some missing context. It is about temperature
compensation, not cooling vs. overheating protection. EPDs behave
different at different temperatures, so the driver needs a temperature
to compensate for it.
EPDs need also a bit more exotic voltages, so usually there is a
separate PMIC for them. Usually that PMIC can also deliver a
temperature. So drivers for that should consist of
- mfd (obvious)
- regulator (also obvious)
- something for providing the temperature (and that "something" is not
  that clear to me as there are several subsystems dealing with
  temperature)

 
And on EPD controller side I would like to be able to define (besides
other things) in the device tree
  epd-temperature = <&some_sensor>;

So your idea was to have that temperature sensor as a hwmon and
providing also a thermal_sensor. If I understand correcly that would
also require me to define a thermal zone where I can add the
thermal sensor and which I could then reference somewhere.
According to devicetree/bindings/thermal/thermal-zones.yaml defining
trip points is required. That does not make sense in this context So I
am wondering whether I am right there since it is not about
overheating but about compensation. And there is only a
thermal_zone_get_zone_by_name() but not a thermal_zone_get_sensor_by_name().
Maybe I am getting something wrong.
Vendor kernels in the wild additionally provide temperature by abusing
the regulator API which is IMHO not acceptable.

But if that thing would in to the iio subsystem, I would simply be able
to use iio_channel_get() to get the sensor from the device tree and
iio_channel_read() to read values from it. There is a iio_hwmon and no
hwmon_iio, so if someone wants a hwmon interface for it, it would not
block anything.

The main point about writing this mail now is that I do not want to
submit a driver, spin some polishing rounds, then somebody says:"Please
go to subsystem Y, not X"

Regards,
Andreas

Re: [Q] tps65185 EPD PMIC temperature interface - which subsystem

[Guenter Roeck]

On 5/1/21 1:21 AM, Andreas Kemnade wrote:
> On Fri, 30 Apr 2021 20:02:13 -0700
> Guenter Roeck <linux@roeck-us.net> wrote:
> 
>> On Fri, Apr 30, 2021 at 11:24:04PM +0200, Andreas Kemnade wrote:
>>> Hi,
>>>
>>> I am going about to clean up stuff to further upstream support for my
>>> ebook readers. One question arises about the temperature interface of
>>> the EPD PMIC. Vendor code uses regulator_get_voltage in the EPDC
>>> driver to read a temperature in celsius and provides temperature through
>>> the regulator interface (besides sysfs/hwmon). That is ugly. But what
>>> are the options, if a kernel consumer should be able to reference it via
>>> devicetree phandle and read out from it? I see temperature sensors
>>> both in the iio and the hwmon subsystem, but do not find a description
>>> why these things are there. If I put it into the iio-subsystem
>>> iio_channel_get() and friends can be used, if I understand things
>>> correctly, there are no such functions in the hwmon subsystem, so I
>>> would not be able to use it there. So the better choice is to put it
>>> into the iio subsystem?
>>>   
>>
>> I am guessing a bit here since a lot of context is missing. Presumably
>> there is a regulator driver. That regulator driver could register itself
>> with the hwmon subsystem using [devm_]hwmon_device_register_with_info()
>> and tell it to register a thermal zone sensor. It should then be possible
>> to read the temperature of that sensor using thermal_zone_get_temp().
>>
> Well, I try to give first some missing context. It is about temperature
> compensation, not cooling vs. overheating protection. EPDs behave
> different at different temperatures, so the driver needs a temperature
> to compensate for it.
> EPDs need also a bit more exotic voltages, so usually there is a
> separate PMIC for them. Usually that PMIC can also deliver a
> temperature. So drivers for that should consist of
> - mfd (obvious)

I would disagree. The presence of a thermal sensor does not make a chip
a multi-function device. Many Ethernet controllers have thermal sensors
nowadays. That doesn't make them multi-function devices.

> - regulator (also obvious)
> - something for providing the temperature (and that "something" is not
>   that clear to me as there are several subsystems dealing with
>   temperature)
> 
There are distinct use cases. iio is for industrial io, thermal is for
thermal management, and hwmon is to expose sensor data to userspace
for hardware monitoring. Normally one would pick the (or a) primary
use case and go from there.

For the tps65185, I could imagine using the thermal sensor for hardware
monitoring, and I can imagine its use for thermal control. I don't really
see a use case as industrial io.

>  
> And on EPD controller side I would like to be able to define (besides
> other things) in the device tree
>   epd-temperature = <&some_sensor>;
> 
> So your idea was to have that temperature sensor as a hwmon and
> providing also a thermal_sensor. If I understand correcly that would
> also require me to define a thermal zone where I can add the
> thermal sensor and which I could then reference somewhere.
> According to devicetree/bindings/thermal/thermal-zones.yaml defining
> trip points is required. That does not make sense in this context So I
> am wondering whether I am right there since it is not about
> overheating but about compensation. And there is only a
> thermal_zone_get_zone_by_name() but not a thermal_zone_get_sensor_by_name().
> Maybe I am getting something wrong.

drivers/mmc/host/sdhci-omap.c seems to do something similar, and doesn't
have trouble using a thermal zone for it.

> Vendor kernels in the wild additionally provide temperature by abusing
> the regulator API which is IMHO not acceptable.
> 
> But if that thing would in to the iio subsystem, I would simply be able
> to use iio_channel_get() to get the sensor from the device tree and
> iio_channel_read() to read values from it. There is a iio_hwmon and no
> hwmon_iio, so if someone wants a hwmon interface for it, it would not
> block anything.
> 
> The main point about writing this mail now is that I do not want to
> submit a driver, spin some polishing rounds, then somebody says:"Please
> go to subsystem Y, not X"
> 

You seem to be set in going along the mfd/iio path, though, which is fine
with me. It is not me you'll have to convince, after all.

Good luck!

Thanks,
Guenter

Re: [Q] tps65185 EPD PMIC temperature interface - which subsystem

[Andreas Kemnade]

On Sat, 1 May 2021 12:05:15 -0700
Guenter Roeck <linux@roeck-us.net> wrote:

[...]
> > Well, I try to give first some missing context. It is about temperature
> > compensation, not cooling vs. overheating protection. EPDs behave
> > different at different temperatures, so the driver needs a temperature
> > to compensate for it.
> > EPDs need also a bit more exotic voltages, so usually there is a
> > separate PMIC for them. Usually that PMIC can also deliver a
> > temperature. So drivers for that should consist of
> > - mfd (obvious)  
> 
> I would disagree. The presence of a thermal sensor does not make a chip
> a multi-function device. Many Ethernet controllers have thermal sensors
> nowadays. That doesn't make them multi-function devices.
> 
well, there is one difference here, the ethernet controllers have
probably thermal sensors for monitoring themself. Here is thermal
sensor is not intended to monitor the EPD PMIC itself. But yes, it
makes life easier if I do not need to have a mfd.

> > - regulator (also obvious)
> > - something for providing the temperature (and that "something" is not
> >   that clear to me as there are several subsystems dealing with
> >   temperature)
> >   
> There are distinct use cases. iio is for industrial io, thermal is for
> thermal management, and hwmon is to expose sensor data to userspace
> for hardware monitoring. Normally one would pick the (or a) primary
> use case and go from there.
> 
> For the tps65185, I could imagine using the thermal sensor for hardware
> monitoring, and I can imagine its use for thermal control. I don't really
> see a use case as industrial io.
> 
[...]

Industrial io would be if the main intention is to measure room
temperature.

> drivers/mmc/host/sdhci-omap.c seems to do something similar, and doesn't
> have trouble using a thermal zone for it.
> 
yes, that is similar.

> > Vendor kernels in the wild additionally provide temperature by abusing
> > the regulator API which is IMHO not acceptable.
> > 
> > But if that thing would in to the iio subsystem, I would simply be able
> > to use iio_channel_get() to get the sensor from the device tree and
> > iio_channel_read() to read values from it. There is a iio_hwmon and no
> > hwmon_iio, so if someone wants a hwmon interface for it, it would not
> > block anything.
> > 
> > The main point about writing this mail now is that I do not want to
> > submit a driver, spin some polishing rounds, then somebody says:"Please
> > go to subsystem Y, not X"
> >   
> 
> You seem to be set in going along the mfd/iio path, though, which is fine
> with me. It is not me you'll have to convince, after all.
> 
I am not that fixed anymore, you opened my eyes for alternatives.

Thanks a lot for your explanations and examples, that really has helped
me to understand things better.

Regards,
Andreas