[U-Boot] [PATCH 6/7 v5] TMU: Add TMU support in dtt command

[Simon Glass]

Hi Akshay,

On Thu, Jan 24, 2013 at 4:02 AM, Akshay Saraswat <akshay.s at samsung.com>wrote:

>  Hi Simon,
>
>
>
> If we create two versions of dtt_get_temp(), we have to declare
> unnecessarily few configs and their values like CONFIG_DTT_SENSORS and
> CONFIG_SYS_DTT_BUS_NUM when we can easily ignore them. That's why I thought
> of including TMU to dtt this way. I dont think we need both TMU and I2C bus
> sensors together anywhere. Please suggest which way is better.
>

(Sorry I am on holiday for a week so not very responsive. I get back next
Thurs)

The thing is that these two implementations share no code at all. I think
the way you have done it is good, I was just wondering whether it would be
better to have do_dtt_i2c() and do_dtt_tmu() as separate function. But I
suppose that is really not any better. The alternative would be to share
the same  do_dtt() function, with it avoiding the i2c code if
CONFIG_SYS_DTT_BUS_NUM is defined, and using a single sensor number 0 if
CONFIG_DTT_SENSORS is not defined. That could use much of the same code
(perhaps with a weak function for dtt_init_one(). But that is probably
trying too hard - if support for multiple TMU sensors is required in the
future we can worry about it then.

Sorry for the trouble.

Regards,
Simon

>
>
> Regards,
>
> Akshay
>
>
>
> ------- *Original Message* -------
>
> *Sender* : Simon Glass<sjg at chromium.org>
>
> *Date* : Jan 23, 2013 05:51 (GMT+09:00)
>
> *Title* : Re: [PATCH 6/7 v5] TMU: Add TMU support in dtt command
>
>
> Hi Akshay,
>
>
> On Mon, Jan 21, 2013 at 3:11 AM, Akshay Saraswat **wrote:
> > Add generic TMU support alongwith i2c sensors in dtt command
> > to enable temperature reading in cases where TMU is present
> > instead of i2c sensors.
> >
> > Signed-off-by: Akshay Saraswat **
> > ---
> > Changes since v4:
> >         - Removed tmu command and added to dtt.
> >
> >  common/cmd_dtt.c |   19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> >
> > diff --git a/common/cmd_dtt.c b/common/cmd_dtt.c
> > index cd94423..715f4ba 100644
> > --- a/common/cmd_dtt.c
> > +++ b/common/cmd_dtt.c
> > @@ -28,6 +28,20 @@
> >  #include **
> >  #include **
> >
> > +#if defined CONFIG_TMU_CMD_DTT
> > +#include **
> > +
> > +void dtt_get_temp(void)
> > +{
> > +       int cur_temp;
> > +
> > +       if (tmu_monitor(&cur_temp) == TMU_STATUS_INIT)
> > +               printf("TMU is in unknown state, temperature is invalid
> \n");
>
> puts()
>
> Should return an error result here so that do_dtt() returns 1.
>
> > +       else
> > +               printf("Current temperature: %u degrees Celsius \n",
> cur_temp);
> > +}
> > +
> > +#else
> >  static unsigned long sensor_initialized;
> >
> >  static void _initialize_dtt(void)
> > @@ -59,9 +73,13 @@ void dtt_init(void)
> >         /* switch back to original I2C bus */
> >         I2C_SET_BUS(old_bus);
> >  }
> > +#endif
> >
> >  int do_dtt (cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
> >  {
> > +#if defined CONFIG_TMU_CMD_DTT
> > +       dtt_get_temp();
> > +#else
>
> How about creating two versions of the dtt_get_temp() function: one
> with your code and one with the old code? Then you don't have an
> #ifdef here.
>
>
>
> >         int i;
> >         unsigned char sensors[] = CONFIG_DTT_SENSORS;
> >         int old_bus;
> > @@ -83,6 +101,7 @@ int do_dtt (cmd_tbl_t * cmdtp, int flag, int argc,
> char * const argv[])
> >
> >         /* switch back to original I2C bus */
> >         I2C_SET_BUS(old_bus);
> > +#endif
> >
> >         return 0;
> >  }      /* do_dtt() */
> > --
> > 1.7.9.5
> >
>
> Regards,
> Simon
>
>
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: image/gif
Size: 14036 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130126/da5ca19f/attachment.gif>