Re: [RFC PATCH 4/6] OMAP4: Temperature sensor device support

["J, KEERTHY" <j-keerthy@ti.com>]

On Thu, Aug 11, 2011 at 7:35 PM, Felipe Balbi <balbi@ti.com> wrote:
> Hi,
>
> On Thu, Aug 11, 2011 at 04:31:50PM +0530, J, KEERTHY wrote:
>> On Thu, Aug 11, 2011 at 4:00 PM, Felipe Balbi <balbi@ti.com> wrote:
>> > Hi,
>> >
>> > On Thu, Aug 11, 2011 at 08:10:07AM +0530, J, KEERTHY wrote:
>> >> >> diff --git a/arch/arm/plat-omap/Kconfig b/arch/arm/plat-omap/Kconfig
>> >> >> index 6e6735f..8fd8e80 100644
>> >> >> --- a/arch/arm/plat-omap/Kconfig
>> >> >> +++ b/arch/arm/plat-omap/Kconfig
>> >> >> @@ -115,6 +115,18 @@ config OMAP_MCBSP
>> >> >>         Say Y here if you want support for the OMAP Multichannel
>> >> >>         Buffered Serial Port.
>> >> >>
>> >> >> +config OMAP_TEMP_SENSOR
>> >> >> +     bool "OMAP Temp Sensor Support"
>> >> >> +     depends on ARCH_OMAP
>> >> >> +     default n
>> >> >> +     help
>> >> >> +       Say Y here if you want support for the temp sensor
>> >> >> +       on OMAP4460.
>> >> >> +
>> >> >> +       This provides the temperature of the MPU
>> >> >> +       subsystem. Only one instance of on die temperature
>> >> >> +       sensor is present.
>> >> >
>> >> > if there's only one instance, why do you use
>> >> > omap_hwmod_for_each_by_class() ??
>> >>
>> >> In case of OMAP5 there are multiple instances. Hence using
>> >> omap_hwmod_for_each_by_class().
>> >
>> > that's not a reality yet, so why don't you leave it for when OMAP5 is
>> > around ?
>>
>> Keeping it generic so that we need not change again. We are pretty
>> close to reality i guess. Why not keep it generic? Any specific reason
>> for not keeping this loop?
>
> Other than the loop being completely unnecessary on the only OMAP
> version you're supporting ? no... not really.
>
>> >> >> diff --git a/arch/arm/plat-omap/include/plat/temperature_sensor.h b/arch/arm/plat-omap/include/plat/temperature_sensor.h
>> >> >> new file mode 100644
>> >> >> index 0000000..692ebdc
>> >> >> --- /dev/null
>> >> >> +++ b/arch/arm/plat-omap/include/plat/temperature_sensor.h
>> >> >> @@ -0,0 +1,87 @@
>> >> >> +/*
>> >> >> + * OMAP Temperature sensor header file
>> >> >> + *
>> >> >> + * Copyright (C) 2011 Texas Instruments Incorporated - http://www.ti.com/
>> >> >> + * Author: J Keerthy <j-keerthy@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 __ARCH_ARM_PLAT_OMAP_INCLUDE_PLAT_TEMPERATURE_SENSOR_H
>> >> >> +#define __ARCH_ARM_PLAT_OMAP_INCLUDE_PLAT_TEMPERATURE_SENSOR_H
>> >> >> +
>> >> >> +/* Offsets from the base of temperature sensor registers */
>> >> >> +
>> >> >> +#define OMAP4460_TEMP_SENSOR_CTRL_OFFSET     0x00
>> >> >> +#define OMAP4460_BGAP_CTRL_OFFSET            0x4c
>> >> >> +#define OMAP4460_BGAP_COUNTER_OFFSET         0x50
>> >> >> +#define OMAP4460_BGAP_THRESHOLD_OFFSET               0x54
>> >> >> +#define OMAP4460_BGAP_TSHUT_OFFSET           0x58
>> >> >> +#define OMAP4460_BGAP_STATUS_OFFSET          0x5c
>> >> >> +#define OMAP4460_FUSE_OPP_BGAP                       -0xcc
>> >> >> +
>> >> >> +struct omap_temp_sensor_registers {
>> >> >> +     u32     temp_sensor_ctrl;
>> >> >> +     u32     bgap_tempsoff_mask;
>> >> >> +     u32     bgap_soc_mask;
>> >> >> +     u32     bgap_eocz_mask;
>> >> >> +     u32     bgap_dtemp_mask;
>> >> >> +
>> >> >> +     u32     bgap_mask_ctrl;
>> >> >> +     u32     mask_hot_mask;
>> >> >> +     u32     mask_cold_mask;
>> >> >> +
>> >> >> +     u32     bgap_mode_ctrl;
>> >> >> +     u32     mode_ctrl_mask;
>> >> >> +
>> >> >> +     u32     bgap_counter;
>> >> >> +     u32     counter_mask;
>> >> >> +
>> >> >> +     u32     bgap_threshold;
>> >> >> +     u32     threshold_thot_mask;
>> >> >> +     u32     threshold_tcold_mask;
>> >> >> +
>> >> >> +     u32     thsut_threshold;
>> >> >> +     u32     tshut_hot_mask;
>> >> >> +     u32     tshut_cold_mask;
>> >> >> +
>> >> >> +     u32     bgap_status;
>> >> >> +     u32     status_clean_stop_mask;
>> >> >> +     u32     status_bgap_alert_mask;
>> >> >> +     u32     status_hot_mask;
>> >> >> +     u32     status_cold_mask;
>> >> >> +
>> >> >> +     u32     bgap_efuse;
>> >> >> +};
>> >> >
>> >> > I find it unnecessary to pass the register map to driver using
>> >> > platform_data.
>> >>
>> >> With multiple instances the register map to individual instances will change.
>> >> So passing it via platform_data.
>> >
>> > what will change is the base address, the offsets should remain the
>> > same.
>>
>> The base address offsets and even bit fields seem to be differing across
>> different OMAP versions.
>
> then a comment making that clear is necessary. But as of today, you
> support only one OMAP version, so I'm sure it's worth the trouble for a
> first version of the driver.

I will add a comment.

>
> --
> balbi
>



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