On 5 July 2012 07:20, Linus Walleij <linus.wall...@linaro.org> wrote:
> On Wed, Jun 20, 2012 at 3:04 PM, hongbo.zhang <hongbo.zh...@linaro.org>
> wrote:
>
> > /*
> > + * Thermal Sensor
> > + */
> > +
> > +#ifdef CONFIG_DB8500_THERMAL
>
> Please don't #ifdef the platform data like this, Documentation/CodingStyle
> dislikes #ifdefs unless needed and this just saves a very marginal piece of
> code in footprint, and will compile just as well even if the driver is not
> selected.
>
Yes I will remove #ifdef
>
> > +static struct resource db8500_thsens_resources[] = {
>
> > +#ifdef CONFIG_DB8500_CPUFREQ_COOLING
>
> Don't #ifdef this either.
>
> Then you will need to add device tree bindings for all this
> platform data sooner or later. Please check with Lee Jones if
> uncertain.
>
OK I will check device tree codes and check with him if any doubt.
>
> > @@ -607,6 +676,8 @@ static struct platform_device
> *snowball_platform_devs[] __initdata = {
> > &snowball_key_dev,
> > &snowball_sbnet_dev,
> > &ab8500_device,
> > + &u8500_thsens_device,
> > + &u8500_cpufreq_cooling_device,
> > };
>
> As you can see the above will break compilation when the drivers
> are not selected if you don't remove the #ifdefs.
>
This is a silly mistake I made :(
>
> > diff --git a/drivers/thermal/cpu_cooling.c
> b/drivers/thermal/cpu_cooling.c
> > index c40d9a0..a8aa10f 100644
> > --- a/drivers/thermal/cpu_cooling.c
> > +++ b/drivers/thermal/cpu_cooling.c
> > @@ -399,8 +399,7 @@ struct thermal_cooling_device
> *cpufreq_cooling_register(
> > /*Verify that all the entries of freq_clip_table are present*/
> > for (i = 0; i < tab_size; i++) {
> > clip_tab = ((struct freq_clip_table *)&tab_ptr[i]);
> > - if (!clip_tab->freq_clip_max || !clip_tab->mask_val
> > - || !clip_tab->temp_level) {
> > + if (!clip_tab->freq_clip_max || !clip_tab->mask_val) {
> > kfree(cpufreq_dev);
> > return ERR_PTR(-EINVAL);
> > }
>
> This looks like a generic patch to cpu_cooling, please break it out
> and send this
> separately to the thermal maintainer and discuss it already today.
>
Yes this should be a separate patch.
>
> > +#include <linux/compiler.h>
>
> Why is this include needed?
> (Just curious.)
>
This is due to some obsolete debuging codes, I forgot to remove this header
file.
>
> > +/* Bind callback functions for thermal zone */
> > +static int db8500_cdev_bind(struct thermal_zone_device *thermal,
> > + struct thermal_cooling_device *cdev)
> > +{
> > + struct db8500_thermal_zone *pzone;
> > + struct db8500_thsens_platform_data *ptrips;
> > + int i, j, ret;
> > +
> > + pzone = (struct db8500_thermal_zone *)thermal->devdata;
> > + ptrips = pzone->thsens_pdev->dev.platform_data;
> > +
> > + for (i = 0; i < ptrips->num_trips; i++)
> > + for (j = 0; j < COOLING_DEV_MAX; j++)
> > + if (ptrips->trip_points[i].cooling_dev_name[j]
> && cdev->type)
> > + if
> (strcmp(ptrips->trip_points[i].cooling_dev_name[j], cdev->type) == 0) {
> > + ret =
> thermal_zone_bind_cooling_device(thermal, i, cdev);
> > + if (ret)
> > + pr_err("Error binding
> cooling device.\n");
> > + else
> > + pr_info("Cooling device
> %s binded to trip point %d.\n", cdev->type, i);
> > + }
> > +
> > + schedule_work(&pzone->therm_work);
>
> So what happens if this work is scheduled and you suspend before
> it executes?
>
> I think you need a suspend/resume hook to clean that up?
> Probably also in remove().
>
Yes I will consider suspend/resume functions.
>
> > +static irqreturn_t prcmu_low_irq_handler(int irq, void *irq_data)
> > +{
> > + struct db8500_thermal_zone *pzone = irq_data;
> > + struct db8500_thsens_platform_data *ptrips;
> > + unsigned long next_low, next_high;
> > + int i;
> > +
> > + ptrips = pzone->thsens_pdev->dev.platform_data;
> > +
> > + for(i = 0; i < ptrips->num_trips; i++) {
> > + if (pzone->cur_high == ptrips->trip_points[i].temp)
> > + break;
> > + }
> > +
> > + if (i <= 1) {
> > + next_high = ptrips->trip_points[0].temp;
> > + next_low = PRCMU_DEFAULT_LOW_TEMP;
> > + } else {
> > + next_high = ptrips->trip_points[i-1].temp;
> > + next_low = ptrips->trip_points[i-2].temp;
> > + }
> > +
> > + (void) prcmu_stop_temp_sense();
> > + (void) prcmu_config_hotmon((u8)(next_low/1000),
> (u8)(next_high/1000));
> > + (void) prcmu_start_temp_sense(PRCMU_DEFAULT_MEASURE_TIME);
> > +
> > + pzone->cur_high = next_high;
> > + pzone->cur_low = next_low;
> > + pr_debug("PRCMU set max %ld, set min %ld\n", next_high,
> next_low);
> > +
> > + schedule_work(&pzone->therm_work);
> > +
> > + return IRQ_HANDLED;
> > +}
>
> This also triggers a work that can be inhibited by suspend/resume
> or remove.
>
> > + schedule_work(&pzone->therm_work);
>
> etc.
>
> > +static int __devinit db8500_thermal_probe(struct platform_device *pdev)
> > +{
> > + struct db8500_thermal_zone *pzone = NULL;
> > + struct db8500_thsens_platform_data *ptrips;
> > + int low_irq, high_irq, ret = 0;
> > + unsigned long dft_low, dft_high;
> > +
> > + pr_info("Function db8500_thermal_probe.\n");
> > +
> > + pzone = kzalloc(sizeof(struct db8500_thermal_zone), GFP_KERNEL);
>
> Use devm_kzalloc().
>
> > + if (!pzone)
> > + return ENOMEM;
> > +
> > + pzone->thsens_pdev = pdev;
> > +
> > + low_irq = platform_get_irq_byname(pdev, "IRQ_HOTMON_LOW");
> > + if (low_irq < 0) {
> > + pr_err("Get IRQ_HOTMON_LOW failed.\n");
> > + goto exit_irq;
> > + }
> > +
> > + ret = request_threaded_irq(low_irq, NULL, prcmu_low_irq_handler,
> > + IRQF_NO_SUSPEND, "dbx500_temp_low",
> pzone);
>
> Use devm_request_threaded_irq().
>
> > + if (ret < 0) {
> > + pr_err("Failed to allocate temp low irq.\n");
> > + goto exit_irq;
> > + }
> > +
> > + high_irq = platform_get_irq_byname(pdev, "IRQ_HOTMON_HIGH");
> > + if (high_irq < 0) {
> > + pr_err("Get IRQ_HOTMON_HIGH failed.\n");
> > + goto exit_irq;
> > + }
> > +
> > + ret = request_threaded_irq(high_irq, NULL,
> prcmu_high_irq_handler,
> > + IRQF_NO_SUSPEND, "dbx500_temp_high",
> pzone);
>
> Use devm_request_threaded_irq()
>
> > + if (ret < 0) {
> > + pr_err("Failed to allocate temp high irq.\n");
> > + goto exit_irq;
> > + }
>
> (...)
> > +exit_irq:
> > + if (pzone->low_irq > 0)
> > + free_irq(pzone->low_irq, pzone);
> > + if (pzone->low_irq > 0)
> > + free_irq(pzone->high_irq, pzone);
> > +
> > + kfree(pzone);
>
> None of this is needed if you use the devm_* functions, they
> are garbage collecting.
>
Good suggestion of using devm_* functions.
>
> > +static int __devexit db8500_thermal_remove(struct platform_device *pdev)
> > +{
> > + struct db8500_thermal_zone *pzone = th_zone;
> > +
> > + if (pzone && pzone->therm_dev)
> > + thermal_zone_device_unregister(pzone->therm_dev);
>
> And...
>
> > + if (pzone && pzone->low_irq)
> > + free_irq(pzone->low_irq, pzone);
> > + if (pzone && pzone->high_irq)
> > + free_irq(pzone->high_irq, pzone);
> > + if (pzone)
> > + kfree(pzone);
>
> None of this either.
>
> > diff --git a/include/linux/platform_data/db8500_thermal.h
> b/include/linux/platform_data/db8500_thermal.h
>
> Thanks for using the proper platform data dir!
>
Thanks for all of you comments.
>
> Yours,
> Linus Walleij
>
_______________________________________________
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
