Re: [PATCH v2 2/6] thermal: amlogic: Add thermal driver to support G12 SoCs

[Martin Blumenstingl <martin.blumenstingl@googlemail.com>]

Hi Guillaume,

On Mon, Aug 5, 2019 at 2:48 PM guillaume La Roque <glaroque@baylibre.com> wrote:
>
> Hi Martin,
>
> again thanks for your review.
you're welcome - thank you for working on the driver :-)

[...]
> > The IP block has more functionality, which may be added to this driver
> > in the future:
> > - reading up to 16 stored temperature samples
>
> it's not working, you can verify it if you check the regmap define in the driver. in fact temp is only write in one register, it's confirmed by amlogic.
I missed that - so please skip this part

[...]
> >> +config AMLOGIC_THERMAL
> > we typically use "MESON" in the Kconfig symbols:
> > $ grep -c AMLOGIC .config
> > 1
> > $ grep -c MESON .config
> > 33
> >
> > I also wonder if we should add G12 or G12A so we don't conflict with
> > upcoming thermal sensors with a different design (assuming that this
> > will be a thing).
> > for example we already have three different USB2 PHY drivers
> >
> > [...]
>
> i check with Neil and for new family it's better to use Amlogic instead of meson.
can you please share the considerations behind this decision?
if new drivers should use AMLOGIC_* Kconfig symbols instead of MESON_*
then we all should know about it

> i don't add G12 because we already know it's same sensors for SM1 SoC family [0].
my idea behind this was to avoid conflicts in the future
in case of the thermal driver we may be fine with using a generic name
assuming that Amlogic will not switch to a new IP block in the next
years
I'm not saying you have to change the name - I'm bringing this up so
you can decide for yourself based on examples from the past

here are a few examples:
- when Kevin upstreamed the MMC driver for GX he decided to use
MMC_MESON_GX for the Kconfig symbol name. it turns out that this is
smart because there are at least two other MMC controller IPs on the
32-bit SoCs. due to him including GX in the name the drivers are easy
to differentiate (MMC_MESON_MX_SDIO and MMC_MESON_MX_SDHC being the
other ones, while the latter is not upstream yet)
- when Carlo upstreamed the eFuse driver he decided to use MESON_EFUSE
for the Kconfig symbol name. I found out much later that the 32-bit
SoCs use a different IP (or at least direct register access instead of
going through Secure Monitor). the driver for the 32-bit SoCs now uses
MESON_MX_EFUSE. if you don't know which driver applies where then it's
easy to mix up MESON_EFUSE and MESON_MX_EFUSE
- when Jerome upstreamed the ALSA driver for AXG (which is also used
on G12A and G12B) he decided to use the SND_MESON_AXG_* prefix for the
Kconfig symbol names. in my opinion this was a good choice because GXM
and everything earlier (including the 32-bit SoCs) use a different
audio IP block. we won't have a Kconfig symbol name clash when a
driver for the "older" SoCs is upstreamed
- (there are more examples, Meson8b USB PHY driver, Meson8b DWMAC
glue, ... - just like there's many examples where the IP block is
mostly compatible with older generations: SAR ADC, RNG, SPI, ...)

I'm not sure what driver naming rules other mainline SoC teams use
to me it seems that the rule for Allwinner driver names is to use the
"code-name of the first SoC the IP block appeared in"

[...]
> >> +static int amlogic_thermal_get_temp(void *data, int *temp)
> >> +{
> >> +       unsigned int tvalue;
> >> +       struct amlogic_thermal *pdata = data;
> >> +
> >> +       if (!data)
> >> +               return -EINVAL;
> >> +
> >> +       regmap_read(pdata->regmap, TSENSOR_STAT0, &tvalue);
> >> +       *temp = code_to_temp(pdata,
> >> +                            tvalue & TSENSOR_READ_TEMP_MASK);
> > maybe simply move the implementation from code_to_temp here?
>
> for the optional function it could be a problem if i move all in code_to_temp.
>
> i prefer to have a function which are just do the conversion.
I didn't consider this before but you are right
if the other temperature registers (like IRQ thresholds) also use a
"temperature code" then it should be a dedicated function (so it'll be
easier to add more functionality to the driver)


Martin