Re: [PATCH v5 09/10] regulator: add mt6357 regulator

[Alexandre Mergnat <amergnat@baylibre.com>]

Le mer. 16 nov. 2022 à 15:17, Matti Vaittinen
<mazziesaccount@gmail.com> a écrit :
>
> Hi Alexandre, All
>
> Please, treat my review more as initiation for discussion than 'hard
> requirements' for this driver. I am in no point or no "confidence level"
> to give you any requirements ;)

Hi Matti,
Understood, thanks for clarifying this.

>
> If I read this right, the device has separate register(s) for writing
> and reading the voltage? I wonder if this is a completely unique setup?
>
> If this is not unique, then it might be worth adding another field for
> 'vsel_get' register and a flag in regulator desc - and modify the
> generic regmap helpers to handle this in common code if the special
> register? Not sure if this HW design is common enough to warrant the
> added confusion though. You and Mark may have more insight.
>

I didn't write this driver and when I handled it, I found this weird.
In the datasheet, registers access are read and write.
After some read/write tests in the registers, I understood that read
vosel_reg always returns the wrong value.
That's why the debug register is used to get the value.
I'm not sure I understand your proposal, but it seems to add more
custom stuff and modify generic regmap instead of using the generic
regmap which already allows us to customize get and set functions
properly.
IMHO, modifying the generic regmap isn't a good solution because I
think this HW design isn't common.

> > +
> > +static const struct linear_range buck_volt_range1[] = {
> > +     REGULATOR_LINEAR_RANGE(518750, 0, 0x7f, 6250),
> > +};
> > +
> > +static const struct linear_range buck_volt_range2[] = {
> > +     REGULATOR_LINEAR_RANGE(500000, 0, 0x7f, 6250),
> > +};
> > +
> > +static const struct linear_range buck_volt_range3[] = {
> > +     REGULATOR_LINEAR_RANGE(500000, 0, 0x3f, 50000),
> > +};
> > +
> > +static const struct linear_range buck_volt_range4[] = {
> > +     REGULATOR_LINEAR_RANGE(1200000, 0, 0x7f, 12500),
> > +};
>
> I am unsure if we should aim for dropping the REGULATOR_LINEAR_RANGE()
> and using the LINEAR_RANGE(). If yes, then it might simplify things if
> new drivers used LINEAR_RANGE() from the day 1. If we don't, then it
> makes sense to keep consistently using REGULATOR_LINEAR_RANGE() for all
> of the drivers. I am not sure which way is the right way.

Good catch.
LINEAR_RANGE() is defined in "linear_range.h"
REGULATOR_LINEAR_RANGE() is defined in "regulator/driver.h"
"linear_range.h" is included in "regulator/driver.h"

Then, I would like to say that regulator drivers should use
REGULATOR_LINEAR_RANGE(). But duplicating the definition is weird,
this is probably something which needs to be fixed or clarified.
Also, that means mt6357-regulator.c no longer needs "#include
<linux/linear_range.h>". Then I will remove it.

>
> > +static int mt6357_regulator_probe(struct platform_device *pdev)
> > +{
> > +     struct mt6397_chip *mt6357 = dev_get_drvdata(pdev->dev.parent);
>
> I am unsure what data do you need from the parent. If it is just the
> regmap / device-tree node / device, then it does not (in my opinion)
> really warrant using parent's drvdata. One can often get away with the
> dev_get_regmap(pdev->dev.parent, NULL).

Ok thanks, I wasn't aware of that. I tried to apply this change but
I've got a kernel panic at boot because "mt6357_get_buck_voltage_sel"
needs to retrieve the regmap.