[RFC PATCH 09/11] ARM: OMAP4+: thermal: introduce bandgap temperature sensor
Cousson, Benoit
b-cousson at ti.com
Tue May 29 09:14:48 EDT 2012
On 5/28/2012 1:16 PM, Eduardo Valentin wrote:
> Hello again,
>
> On Fri, May 25, 2012 at 05:49:44PM +0200, Cousson Benoit wrote:
>> On 5/25/2012 10:25 AM, Eduardo Valentin wrote:
>
> <big cut>
>
>>> +
>>> +static const struct omap_bandgap_data omap4460_data = {
>>> + .has_talert = true,
>>> + .has_tshut = true,
>>> + .fclock_name = "bandgap_ts_fclk",
>>> + .div_ck_name = "div_ts_ck",
>>
>> None of these clock data should be there ideally. You should ensure
>> that the proper device alias will be there using clkdev entries.
>
> In fact, it is a shame that it would be needed to have this entries there :-(
>
>>
>> Except that with DT, it will not work without the clock DT binding :-(
>>
>> I think Rob posted a latest update based on CCF... but for the
>> moment we are stuck :-(
>
> OK. But would it work for BG as well as it seams to be a special case?
>
>>
>>> + .conv_table = omap4460_adc_to_temp,
>>> + .sensors = {
>>> + {
>>> + .registers =&omap4460_mpu_temp_sensor_registers,
>>> + .ts_data =&omap4460_mpu_temp_sensor_data,
>>> + .domain = "cpu",
>>> + },
>>> + },
>>> + .sensor_count = 1,
>>> +};
>>> +
>>> +static const struct omap_bandgap_data omap5430_data = {
>>> + .has_talert = true,
>>> + .has_tshut = true,
>>> + .fclock_name = "ts_clk_div_ck",
>>> + .div_ck_name = "ts_clk_div_ck",
>>> + .conv_table = omap5430_adc_to_temp,
>>> + .sensors = {
>>> + {
>>> + .registers =&omap5430_mpu_temp_sensor_registers,
>>> + .ts_data =&omap5430_mpu_temp_sensor_data,
>>> + .domain = "cpu",
>>> + },
>>> + {
>>> + .registers =&omap5430_gpu_temp_sensor_registers,
>>> + .ts_data =&omap5430_gpu_temp_sensor_data,
>>> + .domain = "gpu",
>>> + },
>>> + {
>>> + .registers =&omap5430_core_temp_sensor_registers,
>>> + .ts_data =&omap5430_core_temp_sensor_data,
>>> + .domain = "core",
>>> + },
>>> + },
>>> + .sensor_count = 3,
>>
>> It can probably be replaced by a sizeof.
>
> ARRAY_SIZE prob, will check.
Ah, yes, that's one... I was looking for it for forgot the name :-)
>>> +};
>>> +
>>> +static const struct of_device_id of_omap_bandgap_match[] = {
>>> + /*
>>> + * TODO: Add support to 4430
>>> + * { .compatible = "ti,omap4430-bandgap", .data = , },
>>> + */
>>> + {
>>> + .compatible = "ti,omap4460-bandgap",
>>> + .data = (void *)&omap4460_data,
>>
>> No need to cast toward a void *.
>
> In this case, there is a need, because the omap4460_data is const but the .data field isn't. So I need to force it.
>
>>
>>> + },
>>> + {
>>> + .compatible = "ti,omap5430-bandgap",
>>> + .data = (void *)&omap5430_data,
>>> + },
>>> + /* Sentinel */
>>> + { },
>>> +};
>>> +
>>> +static struct omap_bandgap *omap_bandgap_build(struct platform_device *pdev)
>>> +{
>>> + struct device_node *node = pdev->dev.of_node;
>>> + const struct of_device_id *of_id;
>>> + struct omap_bandgap *bg_ptr;
>>
>> bg_ptr is not a super name.
>
> Got a better name? Just don't want a long one to avoid code bending at 80th column...
My concern was mainly with the _ptr, it looks like a Hungarian notation.
:-)...
Maybe bg only? Or omap_bg?
>
>>
>>> + u32 prop;
>>> +
>>> + /* just for the sake */
>>> + if (!node) {
>>> + dev_err(&pdev->dev, "no platform information available\n");
>>> + return ERR_PTR(-EINVAL);
>>> + }
>>
>> Not needed, just do the of_match_device here directly.
>
> Indeed...
>
>>
>>> +
>>> + bg_ptr = devm_kzalloc(&pdev->dev, sizeof(struct omap_bandgap),
>>> + GFP_KERNEL);
>>> + if (!bg_ptr) {
>>> + dev_err(&pdev->dev, "Unable to allocate mem for driver ref\n");
>>> + return ERR_PTR(-ENOMEM);
>>> + }
>>> +
>>> + of_id = of_match_device(of_omap_bandgap_match,&pdev->dev);
>>> + if (of_id)
>>> + bg_ptr->pdata = of_id->data;
>>
>> Nit: This is not really pdata anymore, so you should maybe remove
>> the "p" to avoid confusion.
>
> OK...
>
>>
>>> +
>>> + if (bg_ptr->pdata->has_tshut) {
>>> + if (of_property_read_u32(node, "ti,tshut-gpio",&prop)< 0) {
>>> + dev_err(&pdev->dev, "missing tshut gpio in device tree\n");
>>> + return ERR_PTR(-EINVAL);
>>> + }
>>> + bg_ptr->tshut_gpio = prop;
>>> + if (!gpio_is_valid(bg_ptr->tshut_gpio)) {
>>> + dev_err(&pdev->dev, "invalid gpio for tshut (%d)\n",
>>> + bg_ptr->tshut_gpio);
>>> + return ERR_PTR(-EINVAL);
>>> + }
>>> + }
>>> +
>>> + return bg_ptr;
>>> +}
>>> +
>>> +static
>>> +int __devinit omap_bandgap_probe(struct platform_device *pdev)
>>> +{
>>> + struct device *cdev = pdev->dev.parent;
>>> + struct omap_bandgap *bg_ptr;
>>> + int clk_rate, ret = 0, i;
>>> +
>>> + if (!cdev) {
>>> + dev_err(&pdev->dev, "no omap control ref in our parent\n");
>>> + return -EINVAL;
>>> + }
>>> +
>>> + bg_ptr = omap_bandgap_build(pdev);
>>> + if (IS_ERR_OR_NULL(bg_ptr)) {
>>> + dev_err(&pdev->dev, "failed to fetch platform data\n");
>>> + return PTR_ERR(bg_ptr);
>>> + }
>>> +
>>> + if (bg_ptr->pdata->has_talert) {
>>
>> Nit2: Yeah, in fact instead of pdata, "conf" or "settings" seems to
>> be more representative of what this structure really contain.
>
> conf looks good to me.
>
>>
>>> + ret = omap_bandgap_talert_init(bg_ptr, pdev);
>>> + if (ret) {
>>> + dev_err(&pdev->dev, "failed to initialize Talert IRQ\n");
>>> + return ret;
>>> + }
>>> + }
>>> +
>>> + if (bg_ptr->pdata->has_tshut) {
>>> + ret = omap_bandgap_tshut_init(bg_ptr, pdev);
>>> + if (ret) {
>>> + dev_err(&pdev->dev,
>>> + "failed to initialize system tshut IRQ\n");
>>> + goto free_talert;
>>> + }
>>> + }
>>> +
>>> + bg_ptr->fclock = clk_get(NULL, bg_ptr->pdata->fclock_name);
>>
>> That's not good to get a clock without using the local dev alias.
>> But because of lack of clock DT binding yet, I'm not sure we have
>> the choice.
>
> In fact I didn't touch the clk data on purpose and left the clock handling
> as is. On my side I didn't know how the clock struct would look like with DT,
> so, I didn't mess with it.
>
> Do you have a reference to check the work in progress for clock DT ?
Rob sent a pull request, it seems that now it is up to Mike T. :-)
http://lists-archives.com/linux-kernel/27640907-dt-clk-binding-support.html
>
>>
>>> + ret = IS_ERR_OR_NULL(bg_ptr->fclock);
>>> + if (ret) {
>>> + dev_err(&pdev->dev, "failed to request fclock reference\n");
>>> + goto free_irqs;
>>> + }
>>> +
>>> + bg_ptr->div_clk = clk_get(NULL, bg_ptr->pdata->div_ck_name);
>>> + ret = IS_ERR_OR_NULL(bg_ptr->div_clk);
>>> + if (ret) {
>>> + dev_err(&pdev->dev,
>>> + "failed to request div_ts_ck clock ref\n");
>>> + goto free_irqs;
>>> + }
>>> +
>>> + bg_ptr->conv_table = bg_ptr->pdata->conv_table;
>>> + for (i = 0; i< bg_ptr->pdata->sensor_count; i++) {
>>> + struct temp_sensor_registers *tsr;
>>> + u32 val;
>>> +
>>> + tsr = bg_ptr->pdata->sensors[i].registers;
>>> + /*
>>> + * check if the efuse has a non-zero value if not
>>> + * it is an untrimmed sample and the temperatures
>>> + * may not be accurate
>>> + */
>>> + ret = omap_control_readl(cdev, tsr->bgap_efuse,&val);
>>> + if (ret || !val)
>>> + dev_info(&pdev->dev,
>>> + "Non-trimmed BGAP, Temp not accurate\n");
>>> + }
>>> +
>>> + clk_rate = clk_round_rate(bg_ptr->div_clk,
>>> + bg_ptr->pdata->sensors[0].ts_data->max_freq);
>>> + if (clk_rate< bg_ptr->pdata->sensors[0].ts_data->min_freq ||
>>> + clk_rate == 0xffffffff) {
>>> + ret = -ENODEV;
>>> + goto put_clks;
>>> + }
>>> +
>>> + ret = clk_set_rate(bg_ptr->div_clk, clk_rate);
>>> + if (ret) {
>>> + dev_err(&pdev->dev, "Cannot set clock rate\n");
>>> + goto put_clks;
>>> + }
>>> +
>>> + bg_ptr->clk_rate = clk_rate;
>>> + clk_enable(bg_ptr->fclock);
>>> +
>>> + mutex_init(&bg_ptr->bg_mutex);
>>> + bg_ptr->dev =&pdev->dev;
>>> + platform_set_drvdata(pdev, bg_ptr);
>>> +
>>> + /* 1 clk cycle */
>>
>> What does that mean exactly?
>
> That's indeed a good question. I guess by default we configure the bandgap to
> wait only 1 cycle before it goes to the next read round, if in continuous mode.
OK, so some more explanation in that comment are maybe required.
Thanks,
Benoit
>
> That should get overwritten when we have, for instance, some policy initialized
> and changing the update rate based on the temperature level that the sensor is.
>
> These decisions would go under omapXX-thermal.c, BTW. Check my reply on your
> suggestion for data structure split.
>
>>
>> Regards,
>> Benoit
>>
>>> + for (i = 0; i< bg_ptr->pdata->sensor_count; i++)
>>> + configure_temp_sensor_counter(bg_ptr, i, 1);
>>> +
>>> + for (i = 0; i< bg_ptr->pdata->sensor_count; i++) {
>>> + struct temp_sensor_data *ts_data;
>>> +
>>> + ts_data = bg_ptr->pdata->sensors[i].ts_data;
>>> +
>>> + temp_sensor_init_talert_thresholds(bg_ptr, i,
>>> + ts_data->t_hot,
>>> + ts_data->t_cold);
>>> + temp_sensor_configure_tshut_hot(bg_ptr, i,
>>> + ts_data->tshut_hot);
>>> + temp_sensor_configure_tshut_cold(bg_ptr, i,
>>> + ts_data->tshut_cold);
>>> + }
>>> +
>>> + enable_continuous_mode(bg_ptr);
>>> +
>>> + /* Set .250 seconds time as default counter */
>>> + for (i = 0; i< bg_ptr->pdata->sensor_count; i++)
>>> + configure_temp_sensor_counter(bg_ptr, i,
>>> + bg_ptr->clk_rate / 4);
>>> +
>>> + /* Every thing is good? Then expose the sensors */
>>> + for (i = 0; i< bg_ptr->pdata->sensor_count; i++) {
>>> + char *domain;
>>> +
>>> + domain = bg_ptr->pdata->sensors[i].domain;
>>> + if (bg_ptr->pdata->expose_sensor)
>>> + bg_ptr->pdata->expose_sensor(bg_ptr, i, domain);
>>> + }
>>> +
>>> + return 0;
>>> +
>>> +put_clks:
>>> + clk_disable(bg_ptr->fclock);
>>> + clk_put(bg_ptr->fclock);
>>> + clk_put(bg_ptr->div_clk);
>>> +free_irqs:
>>> + free_irq(gpio_to_irq(bg_ptr->tshut_gpio), NULL);
>>> + gpio_free(bg_ptr->tshut_gpio);
>>> +free_talert:
>>> + free_irq(bg_ptr->irq, bg_ptr);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static
>>> +int __devexit omap_bandgap_remove(struct platform_device *pdev)
>>> +{
>>> + struct omap_bandgap *bg_ptr = platform_get_drvdata(pdev);
>>> +
>>> + clk_disable(bg_ptr->fclock);
>>> + clk_put(bg_ptr->fclock);
>>> + clk_put(bg_ptr->div_clk);
>>> + free_irq(bg_ptr->irq, bg_ptr);
>>> + free_irq(gpio_to_irq(bg_ptr->tshut_gpio), NULL);
>>> + gpio_free(bg_ptr->tshut_gpio);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +#ifdef CONFIG_PM
>>> +static int omap_bandgap_save_ctxt(struct omap_bandgap *bg_ptr)
>>> +{
>>> + struct device *cdev = bg_ptr->dev->parent;
>>> + int err = 0;
>>> + int i;
>>> +
>>> + for (i = 0; i< bg_ptr->pdata->sensor_count; i++) {
>>> + struct temp_sensor_registers *tsr;
>>> + struct temp_sensor_regval *rval;
>>> +
>>> + rval = bg_ptr->pdata->sensors[i].regval;
>>> + tsr = bg_ptr->pdata->sensors[i].registers;
>>> +
>>> + err = omap_control_readl(cdev, tsr->bgap_mode_ctrl,
>>> + &rval->bg_mode_ctrl);
>>> + err |= omap_control_readl(cdev, tsr->bgap_mask_ctrl,
>>> + &rval->bg_ctrl);
>>> + err |= omap_control_readl(cdev, tsr->bgap_counter,
>>> + &rval->bg_counter);
>>> + err |= omap_control_readl(cdev, tsr->bgap_threshold,
>>> + &rval->bg_threshold);
>>> + err |= omap_control_readl(cdev, tsr->tshut_threshold,
>>> + &rval->tshut_threshold);
>>> +
>>> + if (err)
>>> + dev_err(bg_ptr->dev, "could not save sensor %d\n", i);
>>> + }
>>> +
>>> + return err ? -EIO : 0;
>>> +}
>>> +
>>> +static int
>>> +omap_bandgap_force_single_read(struct omap_bandgap *bg_ptr, int id)
>>> +{
>>> + struct device *cdev = bg_ptr->dev->parent;
>>> + struct temp_sensor_registers *tsr;
>>> + u32 temp = 0, counter = 1000;
>>> + int err;
>>> +
>>> + tsr = bg_ptr->pdata->sensors[id].registers;
>>> + /* Select single conversion mode */
>>> + err = omap_control_readl(cdev, tsr->bgap_mode_ctrl,&temp);
>>> + temp&= ~(1<< __ffs(tsr->mode_ctrl_mask));
>>> + omap_control_writel(cdev, temp, tsr->bgap_mode_ctrl);
>>> +
>>> + /* Start of Conversion = 1 */
>>> + err |= omap_control_readl(cdev, tsr->temp_sensor_ctrl,&temp);
>>> + temp |= 1<< __ffs(tsr->bgap_soc_mask);
>>> + omap_control_writel(cdev, temp, tsr->temp_sensor_ctrl);
>>> + /* Wait until DTEMP is updated */
>>> + err |= omap_control_readl(cdev, tsr->temp_sensor_ctrl,&temp);
>>> + temp&= (tsr->bgap_dtemp_mask);
>>> + while ((temp == 0)&& --counter) {
>>> + err |= omap_control_readl(cdev, tsr->temp_sensor_ctrl,&temp);
>>> + temp&= (tsr->bgap_dtemp_mask);
>>> + }
>>> + /* Start of Conversion = 0 */
>>> + err |= omap_control_readl(cdev, tsr->temp_sensor_ctrl,&temp);
>>> + temp&= ~(1<< __ffs(tsr->bgap_soc_mask));
>>> + err |= omap_control_writel(cdev, temp, tsr->temp_sensor_ctrl);
>>> +
>>> + return err ? -EIO : 0;
>>> +}
>>> +
>>> +static int omap_bandgap_restore_ctxt(struct omap_bandgap *bg_ptr)
>>> +{
>>> + struct device *cdev = bg_ptr->dev->parent;
>>> + int i, err = 0;
>>> + u32 temp = 0;
>>> +
>>> + for (i = 0; i< bg_ptr->pdata->sensor_count; i++) {
>>> + struct temp_sensor_registers *tsr;
>>> + struct temp_sensor_regval *rval;
>>> + u32 val;
>>> +
>>> + rval = bg_ptr->pdata->sensors[i].regval;
>>> + tsr = bg_ptr->pdata->sensors[i].registers;
>>> +
>>> + err = omap_control_readl(cdev, tsr->bgap_counter,&val);
>>> + if (val == 0) {
>>> + err |= omap_control_writel(cdev, rval->bg_threshold,
>>> + tsr->bgap_threshold);
>>> + err |= omap_control_writel(cdev, rval->tshut_threshold,
>>> + tsr->tshut_threshold);
>>> + /* Force immediate temperature measurement and update
>>> + * of the DTEMP field
>>> + */
>>> + omap_bandgap_force_single_read(bg_ptr, i);
>>> + err |= omap_control_writel(cdev, rval->bg_counter,
>>> + tsr->bgap_counter);
>>> + err |= omap_control_writel(cdev, rval->bg_mode_ctrl,
>>> + tsr->bgap_mode_ctrl);
>>> + err |= omap_control_writel(cdev, rval->bg_ctrl,
>>> + tsr->bgap_mask_ctrl);
>>> + } else {
>>> + err |= omap_control_readl(cdev, tsr->temp_sensor_ctrl,
>>> + &temp);
>>> + temp&= (tsr->bgap_dtemp_mask);
>>> + if (temp == 0) {
>>> + omap_bandgap_force_single_read(bg_ptr, i);
>>> + err |= omap_control_readl(cdev,
>>> + tsr->bgap_mask_ctrl,
>>> + &temp);
>>> + temp |= 1<< __ffs(tsr->mode_ctrl_mask);
>>> + err |= omap_control_writel(cdev, temp,
>>> + tsr->bgap_mask_ctrl);
>>> + }
>>> + }
>>> + if (err)
>>> + dev_err(bg_ptr->dev, "could not save sensor %d\n", i);
>>> + }
>>> +
>>> + return err ? -EIO : 0;
>>> +}
>>> +
>>> +static int omap_bandgap_suspend(struct device *dev)
>>> +{
>>> + struct omap_bandgap *bg_ptr = dev_get_drvdata(dev);
>>> + int err;
>>> +
>>> + err = omap_bandgap_save_ctxt(bg_ptr);
>>> + clk_disable(bg_ptr->fclock);
>>> +
>>> + return err;
>>> +}
>>> +
>>> +static int omap_bandgap_resume(struct device *dev)
>>> +{
>>> + struct omap_bandgap *bg_ptr = dev_get_drvdata(dev);
>>> +
>>> + clk_enable(bg_ptr->fclock);
>>> +
>>> + return omap_bandgap_restore_ctxt(bg_ptr);
>>> +}
>>> +static const struct dev_pm_ops omap_bandgap_dev_pm_ops = {
>>> + SET_SYSTEM_SLEEP_PM_OPS(omap_bandgap_suspend,
>>> + omap_bandgap_resume)
>>> +};
>>> +
>>> +#define DEV_PM_OPS (&omap_bandgap_dev_pm_ops)
>>> +#else
>>> +#define DEV_PM_OPS NULL
>>> +#endif
>>> +
>>> +static struct platform_driver omap_bandgap_sensor_driver = {
>>> + .probe = omap_bandgap_probe,
>>> + .remove = omap_bandgap_remove,
>>> + .driver = {
>>> + .name = "omap-bandgap",
>>> + .pm = DEV_PM_OPS,
>>> + .of_match_table = of_omap_bandgap_match,
>>> + },
>>> +};
>>> +
>>> +module_platform_driver(omap_bandgap_sensor_driver);
>>> +early_platform_init("early_omap_temperature",&omap_bandgap_sensor_driver);
>>> +
>>> +MODULE_DESCRIPTION("OMAP4+ bandgap temperature sensor driver");
>>> +MODULE_LICENSE("GPL v2");
>>> +MODULE_ALIAS("platform:omap-bandgap");
>>> +MODULE_AUTHOR("Texas Instrument Inc.");
>>> diff --git a/drivers/thermal/omap-bandgap.h b/drivers/thermal/omap-bandgap.h
>>> new file mode 100644
>>> index 0000000..12e0d6b
>>> --- /dev/null
>>> +++ b/drivers/thermal/omap-bandgap.h
>>> @@ -0,0 +1,63 @@
>>> +/*
>>> + * OMAP4 Bandgap temperature sensor driver
>>> + *
>>> + * Copyright (C) 2011 Texas Instruments Incorporated - http://www.ti.com/
>>> + * Contact:
>>> + * Eduardo Valentin<eduardo.valentin at ti.com>
>>> + *
>>> + * This program is free software; you can redistribute it and/or
>>> + * modify it under the terms of the GNU General Public License
>>> + * version 2 as published by the Free Software Foundation.
>>> + *
>>> + * This program is distributed in the hope that it will be useful, but
>>> + * WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>>> + * General Public License for more details.
>>> + *
>>> + * You should have received a copy of the GNU General Public License
>>> + * along with this program; if not, write to the Free Software
>>> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
>>> + * 02110-1301 USA
>>> + *
>>> + */
>>> +#ifndef __OMAP_BANDGAP_H
>>> +#define __OMAP_BANDGAP_H
>>> +
>>> +struct omap_bandgap_data;
>>> +
>>> +/**
>>> + * struct omap_bandgap - bandgap device structure
>>> + * @dev: device pointer
>>> + * @pdata: platform data with sensor data
>>> + * @fclock: pointer to functional clock of temperature sensor
>>> + * @div_clk: pointer to parent clock of temperature sensor fclk
>>> + * @conv_table: Pointer to adc to temperature conversion table
>>> + * @bg_mutex: Mutex for sysfs, irq and PM
>>> + * @irq: MPU Irq number for thermal alert
>>> + * @tshut_gpio: GPIO where Tshut signal is routed
>>> + * @clk_rate: Holds current clock rate
>>> + */
>>> +struct omap_bandgap {
>>> + struct device *dev;
>>> + const struct omap_bandgap_data *pdata;
>>> + struct clk *fclock;
>>> + struct clk *div_clk;
>>> + const int *conv_table;
>>> + struct mutex bg_mutex; /* Mutex for irq and PM */
>>> + int irq;
>>> + int tshut_gpio;
>>> + u32 clk_rate;
>>> +};
>>> +
>>> +int omap_bandgap_read_thot(struct omap_bandgap *bg_ptr, int id, int *thot);
>>> +int omap_bandgap_write_thot(struct omap_bandgap *bg_ptr, int id, int val);
>>> +int omap_bandgap_read_tcold(struct omap_bandgap *bg_ptr, int id, int *tcold);
>>> +int omap_bandgap_write_tcold(struct omap_bandgap *bg_ptr, int id, int val);
>>> +int omap_bandgap_read_update_interval(struct omap_bandgap *bg_ptr, int id,
>>> + int *interval);
>>> +int omap_bandgap_write_update_interval(struct omap_bandgap *bg_ptr, int id,
>>> + u32 interval);
>>> +int omap_bandgap_read_temperature(struct omap_bandgap *bg_ptr, int id,
>>> + int *temperature);
>>> +
>>> +#endif
>>
>
> ---
> Eduardo
More information about the linux-arm-kernel
mailing list