Re: [PATCH 2/3] thermal: Add Mediatek thermal controller support

Sascha Hauer
2015-08-06T08:10:32Z

On Thu, Aug 06, 2015 at 02:02:30AM +0800, Daniel Kurtz wrote:
>
> > +
> > +#define THERMAL_NAME "mtk-thermal"
> > +
> > +struct mtk_thermal;
> > +
> > +struct mtk_thermal_bank {
> > + struct mtk_thermal *mt;
> > + struct thermal_zone_device *tz;
>
> A better name for this field is "tzd" - that is what is used in
> drivers/thermal/of-thermal.c.
>
> > + int id;
>
> nit: we don't need both *mt and id, since "id" here is always just the
> index into the mtk_thermal->banks[] array, given either mt or id and
> our bank pointer, we can derive the other.
>
> I think something like this would work:
>
> static inline struct mtk_thermal *to_mtk_thermal(struct mtk_thermal_bank* bank)
> {
> struct mtk_thermal_bank *banks = bank - bank->id;
> return container_of(banks, struct mtk_thermal_bank, banks);
> }
>
> or:
>
> static inline int mtk_thermal_bank_id(struct mtk_thermal_bank* bank)
> {
> return bank - bank->mt->banks;
> }

Yes, probably both work. I don't think though that any of these makes
the code clearer.

> > +static void mtk_thermal_init_bank(struct mtk_thermal *mt, int num,
> > + u32 apmixed_phys_base, u32 auxadc_phys_base)
> > +{
> > + struct mtk_thermal_bank *bank = &mt->banks[num];
> > + const struct mtk_thermal_bank_cfg *cfg = &bank_data[num];
> > + int i;
> > +
> > + bank->id = num;
> > + bank->mt = mt;
> > +
> > + mtk_thermal_get_bank(bank);
> > +
> > + /* bus clock 66M counting unit is 12 * 15.15ns * 256 = 46.540us */
> > + writel(TEMP_MONCTL1_PERIOD_UNIT(12), mt->thermal_base + TEMP_MONCTL1);
> > +
>
> The only things that look zone specific here are the following:
>
> mtk_thermal_get_bank(bank);
> for (i = 0; i < cfg->num_sensors; i++)
> writel(sensor_mux_values[cfg->sensors[i]], mt->thermal_base +
> sensing_points[i].adcpnp);
> writel((1 << cfg->num_sensors) - 1, mt->thermal_base + TEMP_MONCTL0);
> mtk_thermal_put_bank(bank);
>
> The rest is just rewriting the same constants to the same fixed
> register addresses.
>
> But maybe that get/put zone thing does some magic shadow register selecting?

Yes. The comment above mtk_thermal_get_bank() explains that.

> > +
> > + ret = clk_prepare_enable(mt->clk_peri_therm);
> > + if (ret) {
> > + dev_err(&pdev->dev, "Can't enable peri clk: %d\n", ret);
> > + goto err_disable_clk_auxadc;
> > + }
> > +
> > + /*
> > + * These calibration values should finally be provided by the
> > + * firmware or fuses. For now use default values.
> > + */
> > + mt->calib_a = -123;
> > + mt->calib_b = 465124;
> > +
> > + for (i = 0; i < MT8173_NUM_BANKS; i++)
> > + mtk_thermal_init_bank(mt, i, apmixed_phys_base, auxadc_phys_base);
> > +
> > + platform_set_drvdata(pdev, mt);
> > +
> > + for (i = 0; i < MT8173_NUM_BANKS; i++) {
> > + struct mtk_thermal_bank *bank = &mt->banks[i];
> > +
> > + bank->tz = thermal_zone_of_sensor_register(&pdev->dev, i, bank,
> > + &mtk_thermal_ops);
>
> Hmm.. the 'i' passed here to thermal_zone_of_sensor_register is the
> id that a .dts thermal-zone will use to select one of the sensors
> provided by our "mediatek,mt8173-thermal" compatible thermal node.
> So, I think it is really ABI and should be some label defined in a
> header file as part of the of binding for the sensor node. Each label
> would then be encoded somehow in mtk_thermal_bank_cfg[] (or perhaps
> used as the index when initializing the array, like you did for scpsys
> in

Makes sense. I'll add that for the next version.

Thanks for reviewing,

Sascha

--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |