RE: [PATCH 07/13] Thermal: Update binding logic based on platform data

[Zhang Rui <rui.zhang@intel.com>]

On 一, 2012-08-13 at 09:41 -0600, R, Durgadoss wrote:
> Hi Rui,
> 
> > -----Original Message-----
> > From: Zhang, Rui
> > Sent: Monday, August 13, 2012 12:11 PM
> > To: R, Durgadoss
> > Cc: lenb@kernel.org; rjw@sisk.pl; linux-acpi@vger.kernel.org; linux-
> > pm@vger.kernel.org; eduardo.valentin@ti.com; amit.kachhap@linaro.org;
> > wni@nvidia.com
> > Subject: Re: [PATCH 07/13] Thermal: Update binding logic based on platform data
> > 
> > On 四, 2012-08-09 at 18:15 +0530, Durgadoss R wrote:
> > > This patch updates the binding logic in thermal_sys.c
> > > It uses the platform layer data to bind a thermal zone
> > > to a cdev for a particular trip point.
> > >
> > >  * If we do not have platform data and do not have
> > >    .bind defined, do not bind.
> > >  * If we do not have platform data but .bind is
> > >    defined, then use tz->ops->bind.
> > >  * If we have platform data, use it to create binding.
> > >
> > > The same logic sequence is followed for unbind also.
> > >
> > > Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> > > ---
> > >  drivers/thermal/thermal_sys.c |  170
> > ++++++++++++++++++++++++++++++++++-------
> > >  1 file changed, 144 insertions(+), 26 deletions(-)
> > >
> > > diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> > > index 195e529..748b12f 100644
> > > --- a/drivers/thermal/thermal_sys.c
> > > +++ b/drivers/thermal/thermal_sys.c
> > > @@ -158,6 +158,107 @@ static void retrieve_zone_params(struct
> > thermal_zone_device *tz)
> > >  	}
> > >  }
> > >
> > > +static void print_bind_err_msg(struct thermal_zone_device *tz,
> > > +			struct thermal_cooling_device *cdev, int ret)
> > > +{
> > > +	dev_err(&tz->device, "binding zone %s with cdev %s failed:%d\n",
> > > +				tz->type, cdev->type, ret);
> > > +}
> > > +
> > > +static void __bind(struct thermal_zone_device *tz, int mask,
> > > +			struct thermal_cooling_device *cdev)
> > > +{
> > > +	int i, ret;
> > > +
> > > +	for (i = 0; i < tz->trips; i++) {
> > > +		if (mask & (1 << i)) {
> > > +			ret = thermal_zone_bind_cooling_device(tz, i, cdev,
> > > +					THERMAL_NO_LIMIT,
> > THERMAL_NO_LIMIT);
> > > +			if (ret)
> > > +				print_bind_err_msg(tz, cdev, ret);
> > > +		}
> > > +	}
> > > +}
> > > +
> > > +static void __unbind(struct thermal_zone_device *tz, int mask,
> > > +			struct thermal_cooling_device *cdev)
> > > +{
> > > +	int i;
> > > +
> > > +	for (i = 0; i < tz->trips; i++)
> > > +		if (mask & (1 << i))
> > > +			thermal_zone_unbind_cooling_device(tz, i, cdev);
> > > +}
> > > +
> > > +static void update_bind_info(struct thermal_cooling_device *cdev)
> > > +{
> > > +	int i, ret;
> > > +	struct thermal_zone_params *tzp;
> > > +	struct thermal_zone_device *pos = NULL;
> > > +
> > > +	mutex_lock(&thermal_list_lock);
> > > +
> > > +	list_for_each_entry(pos, &thermal_tz_list, node) {
> > > +		if (!pos->tzp && !pos->ops->bind)
> > > +			continue;
> > > +
> > > +		if (!pos->tzp && pos->ops->bind) {
> > > +			ret = pos->ops->bind(pos, cdev);
> > > +			if (ret)
> > > +				print_bind_err_msg(pos, cdev, ret);
> > > +		}
> > > +
> > > +		tzp = pos->tzp;
> > > +		for (i = 0; i < tzp->num_cdevs; i++) {
> > > +			if (!strcmp(tzp->cdevs_name[i], cdev->type)) {
> > > +				__bind(pos, tzp->trip_mask[i], cdev);
> > > +				break;
> > > +			}
> > > +		}
> > > +	}
> > > +	mutex_unlock(&thermal_list_lock);
> > > +}
> > 
> > I still do not understand why we need this kind of bind.
> > Say, the platform thermal driver knows the platform data, i.e. it knows
> > which cooling devices should be bound to which trip points.
> > why we can not move this kind of logic to the .bind() callback, offered
> > by the platform thermal driver?
> > say, in .bind() callback,
> > the platform thermal driver has the pointer of the platform data, right?
> > the .cdev parameter can be used to find the cooling device name,
> > and we can make the comparison there. instead of introducing new binding
> > functions in the generic thermal layer.
> 
> For once, I got little confused between the generic platform thermal sensor
> drivers (the chip drivers) and the platform level driver (not specific for chip,
> but for a platform). So, yes we can put this in the platform level driver.
> 
Hmm,
I'm not clear about the difference between these two drivers.
what is supposed to be done in the platform thermal sensor drivers and
what is supposed to be done in the platform level driver?

At least for now, all the thermal drivers are both thermal sensor driver
and platform level driver, right?

thanks,
rui
> On the other hand, I believe we will have more and more platform thermal
> drivers, as new devices arrive. And in each of the drivers, we are going to
> do the 'looping, finding cdev and then binding' part. I was wondering wouldn't
> it be better to move the common code to the framework, so that the same
> code does not get duplicated, over several places.
> 
> 
> 
> So, please give a second thought on this and let me know your opinion.

> Thanks,
> Durga


--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html