Re: Re: [PATCH v3 1/4] thermal: rockchip: add driver for thermal

[edubezval@xxxxxxxxx]

Hello Zhao,

On Thu, Aug 28, 2014 at 9:54 PM, èäå <zyf@xxxxxxxxxxxxxx> wrote:
> Hi Heikoï
>
>    The TS-ADC on RK3288 has two component, a tsadc and a tsadc controller.
> The tsadc controller is similar like the thermal manager unit on other SOCs.
> We followed the naming of 3066, but not named as the Thermal Manager.
>
>    Moreover,there is only one set of apb registers to access the tsadc
> controller,and the tsadc is controlled by the tsadc controller,could not
> access directly. If we write a general tsadc driver by accessed tsadc
> controller registers, and it hardly to write a driver for the tsadc
> controller.

As suggested by Arnd, you can use the generic driver as interface
between thermal framework and IIO layer. The driver you are going to
write to your ADC is going to be in the IIO subsystem. The only
difference is that the generic driver would work for any ADC driver in
the IIO subsystem.
https://lkml.org/lkml/2014/2/5/810

In fact, there is already a generic driver. We just need to get it up
to date.  I see some testing has been already done, and results sound
promising.

>
>    So, I do not agree to write a generic adc driver for the rk3288-tsadc.
>
> ________________________________
> Yifeng Zhao
>
> åääï HeikoStÃbner
> åéæéï 2014-08-29 00:11
> æääï Eduardo Valentin
> æéï Arnd Bergmann; Caesar Wang; rui.zhang; grant.likely; robh+dt;
> linux-kernel; linux-pm; linux-arm-kernel; devicetree; linux-doc; huangtao;
> cf; dianders; dtor; zyw; addy.ke; dmitry.torokhov; zhaoyifeng; linux-iio;
> Jonathan Cameron
> äéï Re: [PATCH v3 1/4] thermal: rockchip: add driver for thermal
>
> Am Donnerstag, 28. August 2014, 10:37:35 schrieb Eduardo Valentin:
>> Ceasar and Arnd,
>>
>> On Thu, Aug 28, 2014 at 10:48:23AM +0200, Arnd Bergmann wrote:
>> > On Thursday 28 August 2014 08:59:19 Caesar Wang wrote:
>> > > Thermal is TS-ADC Controller module supports user-defined mode and
>> > > automatic mode.
>> > >
>> > > User-defined mode refers,TSADC all the control signals entirely by
>> > > software
>> > > writing to register for direct control.
>> > >
>> > > Automaic mode refers to the module automatically poll TSADC output,and
>> > > the results Were checked.
>> > >
>> > > If you find that the temperature High in a period of time, an
>> > > interrupt
>> > > is generated to the processor down-measures taken;if the temperature
>> > > over a period of time High, the resulting TSHUT gave CRU module,let it
>> > > reset the entire chip, or via GPIO give PMIC.
>> > >
>> > > Signed-off-by: zhaoyifeng <zyf@xxxxxxxxxxxxxx>
>> > > Signed-off-by: Caesar Wang <caesar.wang@xxxxxxxxxxxxxx>
>> >
>> > Hi Caesar,
>> >
>> > After looking at the driver (last time I only received the patch for
>> > the binding), I have a more general comment:
>> >
>> > This looks like a general-purpose ADC device, not an IP block that is
>> > specific to thermal management. The binding looks ok for that purpose
>> > but should probably be moved into
>> > Documentation/devicetree/bindings/iio/adc/ as a minor change.
>>
>> I agree with Arnd's point here. It makes sense to me to have this driver
>> under the IIO umbrella.
>
> interesting suggestion :-)
>
> I've just taken another look at the registers of the ts-adc on the rk3066
> which is completely different from the rk3288 one. Interestingly the rk3066
> one
> is "just" another saradc IP, so for this one it would really make sense.
>
>
>> > On the driver side, I believe the correct way to deal with this setup
>> > is to split your driver into a generic drivers/iio/adc/rockchips-tsadc.c
>> > file, and a smaller thermal driver that uses the iio in-kernel
>> > interfaces,
>> > ideally one that is independent of the underlying hardware and can
>> > work on any ADC implementation.
>>
>> Agreed. If you can write such interface and make your driver to work in
>> such way, that would be great.
>
> But I currently don't see how you would model the temperature handling parts
> from a generic thermal driver to a generic adc driver for the rk3288-tsadc.
>
> I guess the general temperature irq handling would use iio-triggers? But how
> does the target temperature get into the TSADC_COMP1_INT register.
>
> Also when getting the temperature, Caesar's driver compares it to its trip
> points and sets the next trip point depending on the current temperature
> (passive <-> critical) in rockchip_get_temp.
>
> Maybe there is some completely easy way for this, but currently I don't see
> it.
>
>
> Heiko
>
>



-- 
Eduardo Bezerra Valentin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/