Re: [PATCH 3/6] OMAP4460: Temperature sensor data

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

On Sat, Sep 24, 2011 at 1:18 PM, Paul Walmsley <paul@pwsan.com> wrote:
> On Fri, 23 Sep 2011, J, KEERTHY wrote:
>
>> On Fri, Sep 23, 2011 at 11:33 AM, Paul Walmsley <paul@pwsan.com> wrote:
>> >
>> > On Thu, 22 Sep 2011, Keerthy wrote:
>> >
>> >> @@ -0,0 +1,175 @@
>> >> +/*
>> >> + * OMAP system control module 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
>> >
>> > You're also missing important #includes here for things like mutexes
>> > and kernel types that you use later on in the file.
>>
>> Those header files are included in c files.
>
> And how does that affect my comment?
>
>> >> +#define OMAP_ADC_START_VALUE    530
>> >> +#define OMAP_ADC_END_VALUE      923
>> >
>> > Are these OMAP4460, OMAP4xxx, or OMAP2+ specific?
>>
>> OMAP4460. I will pass even these values through pdata
>> since they differ from platform to platform.
>
> So then the macro names need to include "OMAP4460" or whatever SoC
> they are first valid for.
>
>> >> +
>> >> +/**
>> >> + * struct omap4460plus_scm_dev_attr - device attributes for scm
>> >
>> > There are loads of references to 'omap4460plus' when it seems to me that
>> > much of this driver should also apply to OMAP4430 also.  Shouldn't this
>> > driver be named something like 'omap4430plus_scm' or even
>> > better 'omap4_scm' ?
>>
>> This is used by hwmod. Hence keeping it in the header file.
>
> Did you even read my comment before responding?

Sorry about this. OMAP4430 and OMAP4460 temperature sensors are different.
The register layout and the functionalities differ. The OMAP4430 temperature
sensor is not accurate. The SCM driver can be generic but the temperature
sensor driver should be OMAP4460 onwards. Please let me know if this is fine?

>
>
> - Paul



-- 
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