Re: [PATCH v1 17/30] mmc: sdhci-tegra: Support OPP and core voltage scaling

[Dmitry Osipenko]

05.11.2020 12:58, Viresh Kumar пишет:
>> +static void sdhci_tegra_deinit_opp_table(void *data)
>> +{
>> +       struct device *dev = data;
>> +       struct opp_table *opp_table;
>> +
>> +       opp_table = dev_pm_opp_get_opp_table(dev);
> So you need to get an OPP table to put one :)
> You need to save the pointer returned by dev_pm_opp_set_regulators() instead.

This is intentional because why do we need to save the pointer if we're
not using it and we know that we could get this pointer using OPP API?
This is exactly the same what I did for the CPUFreq driver [1] :)

[1]
https://elixir.bootlin.com/linux/v5.10-rc2/source/drivers/cpufreq/tegra20-cpufreq.c#L97

>> +       dev_pm_opp_of_remove_table(dev);
>> +       dev_pm_opp_put_regulators(opp_table);
>> +       dev_pm_opp_put_opp_table(opp_table);
>> +}
>> +
>> +static int devm_sdhci_tegra_init_opp_table(struct device *dev)
>> +{
>> +       struct opp_table *opp_table;
>> +       const char *rname = "core";
>> +       int err;
>> +
>> +       /* voltage scaling is optional */
>> +       if (device_property_present(dev, "core-supply"))
>> +               opp_table = dev_pm_opp_set_regulators(dev, &rname, 1);
>> +       else
> 
>> +               opp_table = dev_pm_opp_get_opp_table(dev);
> Nice. I didn't think that someone will end up abusing this API and so made it
> available for all, but someone just did that. I will fix that in the OPP core.

The dev_pm_opp_put_regulators() handles the case where regulator is
missing by acting as dev_pm_opp_get_opp_table(), but the
dev_pm_opp_set_regulators() doesn't do it. Hence I don't think this is
an abuse, but the OPP API drawback.

> Any idea why you are doing what you are doing here ?

Two reasons:

1. Voltage regulator is optional, but dev_pm_opp_set_regulators()
doesn't support optional regulators.

2. We need to balance the opp_table refcount in order to use OPP API
without polluting code with if(have_regulator), hence the
dev_pm_opp_get_opp_table() is needed for taking the opp_table reference
to have the same refcount as in the case of the dev_pm_opp_set_regulators().

I guess we could make dev_pm_opp_set_regulators(dev, count) to accept
regulators count=0 and then act as dev_pm_opp_get_opp_table(dev), will
it be acceptable?