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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxx> 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@xxxxxx>
> >> >> + *
> >> >> + * 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.

-- 
balbi

Attachment: signature.asc
Description: Digital signature


[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux