Re: [PATCH] thermal/drivers/of: Add a get_temp_id callback function

[Andrey Smirnov]

On Tue, May 28, 2019 at 8:05 PM Eduardo Valentin <edubezval@xxxxxxxxx> wrote:
>
> On Thu, May 23, 2019 at 07:48:56PM -0700, Andrey Smirnov wrote:
> > On Mon, Apr 29, 2019 at 9:51 AM Daniel Lezcano
> > <daniel.lezcano@xxxxxxxxxx> wrote:
> > >
> > > On 24/04/2019 01:08, Daniel Lezcano wrote:
> > > > On 23/04/2019 17:44, Eduardo Valentin wrote:
> > > >> Hello,
> > > >>
> > > >> On Tue, Apr 16, 2019 at 07:22:03PM +0200, Daniel Lezcano wrote:
> > > >>> Currently when we register a sensor, we specify the sensor id and a data
> > > >>> pointer to be passed when the get_temp function is called. However the
> > > >>> sensor_id is not passed to the get_temp callback forcing the driver to
> > > >>> do extra allocation and adding back pointer to find out from the sensor
> > > >>> information the driver data and then back to the sensor id.
> > > >>>
> > > >>> Add a new callback get_temp_id() which will be called if set. It will
> > > >>> call the get_temp_id() with the sensor id.
> > > >>>
> > > >>> That will be more consistent with the registering function.
> > > >>
> > > >> I still do not understand why we need to have a get_id callback.
> > > >> The use cases I have seen so far, which I have been intentionally rejecting, are
> > > >> mainly solvable by creating other compatible entries. And really, if you
> > > >> have, say a bandgap, chip that supports multiple sensors, but on
> > > >> SoC version A it has 5 sensors, and on SoC version B it has only 4,
> > > >> or on SoC version C, it has 5 but they are either logially located
> > > >> in different places (gpu vs iva regions), these are all cases in which
> > > >> you want a different compatible!
> > > >>
> > > >> Do you mind sharing why you need a get sensor id callback?
> > > >
> > > > It is not a get sensor id callback, it is a get_temp callback which pass
> > > > the sensor id.
> > > >
> > > > See in the different drivers, it is a common pattern there is a
> > > > structure for the driver, then a structure for the sensor. When the
> > > > get_temp is called, the callback needs info from the sensor structure
> > > > and from the driver structure, so a back pointer to the driver structure
> > > > is added in the sensor structure.
> > >
>
> Do you mind sending a patch showing how one could convert an existing
> driver to use this new API?
>

There's already a patch doing that for qoriq driver:
https://lore.kernel.org/lkml/20190404080647.8173-2-daniel.lezcano@xxxxxxxxxx/

Here's what can be done with Armada sensor driver:
https://gist.github.com/ndreys/d733228756a17e1dcb31ec8f9a0e9115
(please note that I don't have Armada HW, so I haven't tested those
changes, just made them as an example)

> > > Hi Eduardo,
> > >
> > > does the explanation clarifies the purpose of this change?
> > >
> >
> > Eduardo, did you ever have a chance to revisit this thread? I would
> > really like to make some progress on this one to unblock my i.MX8MQ
> > hwmon series.
>
> The problem I have with this patch is that it is an API which resides
> only in of-thermal. Growing APIs on DT only diverges of-thermal from
> thermal core and platform drivers.
>

I don't have a horse in this race and would be fine if this API isn't
accepted, so this is up for you to decide. My main goal is to get my
i.MX8MQ hwmon patches moving and accepted.

> Besides, this patch needs to document the API in Documention/

I'd like to get more of a commitment on this before I'd start
investing time into improving this patch any further. If this patch is
a go, then I'd be happy to add that documentation.

Thanks,
Andrey Smirnov