[v3,3/5] drivers: regulator: Add ADI MAX77541/MAX77540 Regulator Support
Commit Message
Regulator driver for both MAX77541 and MAX77540.
The MAX77541 is a high-efficiency step-down converter
with two 3A switching phases for single-cell Li+ battery
and 5VDC systems.
The MAX77540 is a high-efficiency step-down converter
with two 3A switching phases.
Signed-off-by: Okan Sahin <okan.sahin@analog.com>
---
MAINTAINERS | 1 +
drivers/regulator/Kconfig | 9 ++
drivers/regulator/Makefile | 1 +
drivers/regulator/max77541-regulator.c | 160 +++++++++++++++++++++++++
4 files changed, 171 insertions(+)
create mode 100644 drivers/regulator/max77541-regulator.c
Comments
On Wed, Jan 18, 2023 at 09:38:10AM +0300, Okan Sahin wrote:
> Regulator driver for both MAX77541 and MAX77540.
> The MAX77541 is a high-efficiency step-down converter
> with two 3A switching phases for single-cell Li+ battery
> and 5VDC systems.
>
> The MAX77540 is a high-efficiency step-down converter
> with two 3A switching phases.
...
> + * Copyright (c) 2022 Analog Devices, Inc.
Happy New Year!
...
> +static int max77541_regulator_probe(struct platform_device *pdev)
> +{
> + struct max77541 *max77541 = dev_get_drvdata(pdev->dev.parent);
> + struct regulator_config config = {};
> + const struct regulator_desc *desc;
> + struct device *dev = &pdev->dev;
You may rearrange this a bit
struct max77541 *max77541 = dev_get_drvdata(dev->parent);
> + struct regulator_dev *rdev;
> + int i;
> + config.dev = pdev->dev.parent;
dev->parent
> +
> + if (max77541->id == MAX77540)
> + desc = max77540_regulators_desc;
> + else if (max77541->id == MAX77541)
> + desc = max77541_regulators_desc;
> + else
> + return -EINVAL;
> +
> + for (i = 0; i < MAX77541_MAX_REGULATORS; i++) {
> + rdev = devm_regulator_register(dev,
> + &desc[i], &config);
This is perfectly one line.
> + if (IS_ERR(rdev))
> + return dev_err_probe(dev, PTR_ERR(rdev),
> + "Failed to register regulator\n");
> + }
> +
> + return 0;
> +}
...
> +static const struct of_device_id max77541_regulator_of_id[] = {
> + {
> + .compatible = "adi,max77540-regulator",
> + .data = (void *)MAX77540,
> + },
> + {
> + .compatible = "adi,max77541-regulator",
> + .data = (void *)MAX77541,
> + },
> + { /* sentinel */ }
As pointed out, better to use pointers directly.
> +};
Hi Andy,
Thank you for your feedback and efforts. I have a question below.
On Wed, 18 Jan 2022 11:20 AM
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>On Wed, Jan 18, 2023 at 09:38:10AM +0300, Okan Sahin wrote:
>> Regulator driver for both MAX77541 and MAX77540.
>> The MAX77541 is a high-efficiency step-down converter with two 3A
>> switching phases for single-cell Li+ battery and 5VDC systems.
>>
>> The MAX77540 is a high-efficiency step-down converter with two 3A
>> switching phases.
>
>...
>
>> + * Copyright (c) 2022 Analog Devices, Inc.
>
>Happy New Year!
>
>...
>
>> +static int max77541_regulator_probe(struct platform_device *pdev) {
>> + struct max77541 *max77541 = dev_get_drvdata(pdev->dev.parent);
>> + struct regulator_config config = {};
>> + const struct regulator_desc *desc;
>> + struct device *dev = &pdev->dev;
>
>You may rearrange this a bit
>
> struct max77541 *max77541 = dev_get_drvdata(dev->parent);
>
>> + struct regulator_dev *rdev;
>> + int i;
>
>> + config.dev = pdev->dev.parent;
>
>dev->parent
>
>> +
>> + if (max77541->id == MAX77540)
>> + desc = max77540_regulators_desc;
>> + else if (max77541->id == MAX77541)
>> + desc = max77541_regulators_desc;
>> + else
>> + return -EINVAL;
>> +
>> + for (i = 0; i < MAX77541_MAX_REGULATORS; i++) {
>
>> + rdev = devm_regulator_register(dev,
>> + &desc[i], &config);
>
>This is perfectly one line.
Thank you, I will arrange it.
>
>> + if (IS_ERR(rdev))
>> + return dev_err_probe(dev, PTR_ERR(rdev),
>> + "Failed to register regulator\n");
>> + }
>> +
>> + return 0;
>> +}
>
>...
However, this one is not fit when I set max-line-length argument as 80 in checkpatch script. What do you suggest? This line has 99 characters.
>
>> +static const struct of_device_id max77541_regulator_of_id[] = {
>> + {
>> + .compatible = "adi,max77540-regulator",
>> + .data = (void *)MAX77540,
>> + },
>> + {
>> + .compatible = "adi,max77541-regulator",
>> + .data = (void *)MAX77541,
>> + },
>> + { /* sentinel */ }
>
>As pointed out, better to use pointers directly.
>
>> +};
>
>--
>With Best Regards,
>Andy Shevchenko
>
Regards,
Okan Sahin
Hi Andy,
Sorry for second question. I do not want to bother you, but I realized that I need to be sure about driver_data before sending new patch. You said that you need to use pointers directly for driver_data then I fixed that part in mfd, but I do not need or use driver_data in regulator since chip_id comes from mfd device so I think using like below should be enough for my implementation.
static const struct platform_device_id max77541_regulator_platform_id[] = {
{ "max77540-regulator", },
{ "max77541-regulator", },
{ /* sentinel */ }
};
MODULE_DEVICE_TABLE(platform, max77541_regulator_platform_id);
static const struct of_device_id max77541_regulator_of_id[] = {
{ .compatible = "adi,max77540-regulator", },
{ .compatible = "adi,max77541-regulator", },
{ /* sentinel */ }
};
MODULE_DEVICE_TABLE(of, max77541_regulator_of_id);
What do you think?
On Tue, Jan 31, 2023 at 10:21 AM +0300, Okan Sahin wrote:
>Hi Andy,
>
>Thank you for your feedback and efforts. I have a question below.
>
>On Wed, 18 Jan 2022 11:20 AM
>Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>
>>On Wed, Jan 18, 2023 at 09:38:10AM +0300, Okan Sahin wrote:
>>> Regulator driver for both MAX77541 and MAX77540.
>>> The MAX77541 is a high-efficiency step-down converter with two 3A
>>> switching phases for single-cell Li+ battery and 5VDC systems.
>>>
>>> The MAX77540 is a high-efficiency step-down converter with two 3A
>>> switching phases.
>>
>>...
>>
>>> + * Copyright (c) 2022 Analog Devices, Inc.
>>
>>Happy New Year!
>>
>>...
>>
>>> +static int max77541_regulator_probe(struct platform_device *pdev) {
>>> + struct max77541 *max77541 = dev_get_drvdata(pdev->dev.parent);
>>> + struct regulator_config config = {};
>>> + const struct regulator_desc *desc;
>>> + struct device *dev = &pdev->dev;
>>
>>You may rearrange this a bit
>>
>> struct max77541 *max77541 = dev_get_drvdata(dev->parent);
>>
>>> + struct regulator_dev *rdev;
>>> + int i;
>>
>>> + config.dev = pdev->dev.parent;
>>
>>dev->parent
>>
>>> +
>>> + if (max77541->id == MAX77540)
>>> + desc = max77540_regulators_desc;
>>> + else if (max77541->id == MAX77541)
>>> + desc = max77541_regulators_desc;
>>> + else
>>> + return -EINVAL;
>>> +
>>> + for (i = 0; i < MAX77541_MAX_REGULATORS; i++) {
>>
>>> + rdev = devm_regulator_register(dev,
>>> + &desc[i], &config);
>>
>>This is perfectly one line.
>Thank you, I will arrange it.
>>
>>> + if (IS_ERR(rdev))
>>> + return dev_err_probe(dev, PTR_ERR(rdev),
>>> + "Failed to register regulator\n");
>>> + }
>>> +
>>> + return 0;
>>> +}
>>
>>...
>However, this one is not fit when I set max-line-length argument as 80 in
>checkpatch script. What do you suggest? This line has 99 characters.
>>
>>> +static const struct of_device_id max77541_regulator_of_id[] = {
>>> + {
>>> + .compatible = "adi,max77540-regulator",
>>> + .data = (void *)MAX77540,
>>> + },
>>> + {
>>> + .compatible = "adi,max77541-regulator",
>>> + .data = (void *)MAX77541,
>>> + },
>>> + { /* sentinel */ }
>>
>>As pointed out, better to use pointers directly.
>>
>>> +};
>>
>>--
>>With Best Regards,
>>Andy Shevchenko
>>
>
>Regards,
>Okan Sahin
Regards,
Okan
On Tue, Jan 31, 2023 at 07:20:52AM +0000, Sahin, Okan wrote:
> On Wed, 18 Jan 2022 11:20 AM
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> >On Wed, Jan 18, 2023 at 09:38:10AM +0300, Okan Sahin wrote:
...
> >> + rdev = devm_regulator_register(dev,
> >> + &desc[i], &config);
> >
> >This is perfectly one line.
> Thank you, I will arrange it.
> >
> >> + if (IS_ERR(rdev))
> >> + return dev_err_probe(dev, PTR_ERR(rdev),
> >> + "Failed to register regulator\n");
> >> + }
> >> +
> >> + return 0;
> >> +}
> However, this one is not fit when I set max-line-length argument as 80 in
> checkpatch script. What do you suggest? This line has 99 characters.
Which line do you refer to?
On Tue, Jan 31, 2023 at 09:27:48AM +0000, Sahin, Okan wrote:
First of all, please do avoid top-posting.
> Sorry for second question. I do not want to bother you, but I realized that I
> need to be sure about driver_data before sending new patch. You said that you
> need to use pointers directly for driver_data then I fixed that part in mfd,
> but I do not need or use driver_data in regulator since chip_id comes from
> mfd device so I think using like below should be enough for my
> implementation.
>
> static const struct platform_device_id max77541_regulator_platform_id[] = {
> { "max77540-regulator", },
> { "max77541-regulator", },
> { /* sentinel */ }
> };
> MODULE_DEVICE_TABLE(platform, max77541_regulator_platform_id);
>
> static const struct of_device_id max77541_regulator_of_id[] = {
> { .compatible = "adi,max77540-regulator", },
> { .compatible = "adi,max77541-regulator", },
> { /* sentinel */ }
> };
> MODULE_DEVICE_TABLE(of, max77541_regulator_of_id);
>
> What do you think?
If you have got all necessary data from the upper layer, why do you need to
have an ID table here? I'm not sure I understand how this OF ID table works
in this case.
On Tue, 31 Jan 2022 3:24 PM
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>On Tue, Jan 31, 2023 at 07:20:52AM +0000, Sahin, Okan wrote:
>> On Wed, 18 Jan 2022 11:20 AM
>> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>> >On Wed, Jan 18, 2023 at 09:38:10AM +0300, Okan Sahin wrote:
>
>...
>
>> >> + rdev = devm_regulator_register(dev,
>> >> + &desc[i], &config);
>> >
>> >This is perfectly one line.
>> Thank you, I will arrange it.
>> >
>> >> + if (IS_ERR(rdev))
>> >> + return dev_err_probe(dev, PTR_ERR(rdev),
>> >> + "Failed to register regulator\n");
>> >> + }
>> >> +
>> >> + return 0;
>> >> +}
>
>> However, this one is not fit when I set max-line-length argument as 80
>> in checkpatch script. What do you suggest? This line has 99 characters.
>
>Which line do you refer to?
I am referring "return dev_err_probe(dev, PTR_ERR(rdev), "Failed to register regulator\n");"
>
>--
>With Best Regards,
>Andy Shevchenko
>
Best Regards,
Okan Sahin
On Tue, 31 Jan 2022 3:27 PM
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>On Tue, Jan 31, 2023 at 09:27:48AM +0000, Sahin, Okan wrote:
>
>First of all, please do avoid top-posting.
>
>> Sorry for second question. I do not want to bother you, but I realized
>> that I need to be sure about driver_data before sending new patch. You
>> said that you need to use pointers directly for driver_data then I
>> fixed that part in mfd, but I do not need or use driver_data in
>> regulator since chip_id comes from mfd device so I think using like
>> below should be enough for my implementation.
>>
>> static const struct platform_device_id max77541_regulator_platform_id[] = {
>> { "max77540-regulator", },
>> { "max77541-regulator", },
>> { /* sentinel */ }
>> };
>> MODULE_DEVICE_TABLE(platform, max77541_regulator_platform_id);
>>
>> static const struct of_device_id max77541_regulator_of_id[] = {
>> { .compatible = "adi,max77540-regulator", },
>> { .compatible = "adi,max77541-regulator", },
>> { /* sentinel */ }
>> };
>> MODULE_DEVICE_TABLE(of, max77541_regulator_of_id);
>>
>> What do you think?
>
>If you have got all necessary data from the upper layer, why do you need to have
>an ID table here? I'm not sure I understand how this OF ID table works in this
>case.
I added it since there is regulator node in device tree. With the help of devm_regulator_register(..), driver takes parameters of regulator node. I also used id to select and to initialize regulator descriptors which are chip specific. So far there is no comment about OF ID table so I kept it. I thought I need to add both of id table and platform id table as name matching is required to initialize platform device from mfd.
>
>--
>With Best Regards,
>Andy Shevchenko
>
Best Regards,
Okan Sahin
On Tue, Jan 31, 2023 at 01:12:26PM +0000, Sahin, Okan wrote:
> On Tue, 31 Jan 2022 3:24 PM
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> >On Tue, Jan 31, 2023 at 07:20:52AM +0000, Sahin, Okan wrote:
> >> On Wed, 18 Jan 2022 11:20 AM
> >> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> >> >On Wed, Jan 18, 2023 at 09:38:10AM +0300, Okan Sahin wrote:
...
> >> >> + rdev = devm_regulator_register(dev,
> >> >> + &desc[i], &config);
> >> >
> >> >This is perfectly one line.
> >> Thank you, I will arrange it.
> >> >
> >> >> + if (IS_ERR(rdev))
> >> >> + return dev_err_probe(dev, PTR_ERR(rdev),
> >> >> + "Failed to register regulator\n");
> >> >> + }
> >> >> +
> >> >> + return 0;
> >> >> +}
> >
> >> However, this one is not fit when I set max-line-length argument as 80
> >> in checkpatch script. What do you suggest? This line has 99 characters.
> >
> >Which line do you refer to?
> I am referring "return dev_err_probe(dev, PTR_ERR(rdev), "Failed to register
> regulator\n");"
I have had no comments on that line.
On Tue, Jan 31, 2023 at 01:23:33PM +0000, Sahin, Okan wrote:
> On Tue, 31 Jan 2022 3:27 PM
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> >On Tue, Jan 31, 2023 at 09:27:48AM +0000, Sahin, Okan wrote:
...
> >> Sorry for second question. I do not want to bother you, but I realized
> >> that I need to be sure about driver_data before sending new patch. You
> >> said that you need to use pointers directly for driver_data then I
> >> fixed that part in mfd, but I do not need or use driver_data in
> >> regulator since chip_id comes from mfd device so I think using like
> >> below should be enough for my implementation.
> >>
> >> static const struct platform_device_id max77541_regulator_platform_id[] = {
> >> { "max77540-regulator", },
> >> { "max77541-regulator", },
> >> { /* sentinel */ }
> >> };
> >> MODULE_DEVICE_TABLE(platform, max77541_regulator_platform_id);
> >>
> >> static const struct of_device_id max77541_regulator_of_id[] = {
> >> { .compatible = "adi,max77540-regulator", },
> >> { .compatible = "adi,max77541-regulator", },
> >> { /* sentinel */ }
> >> };
> >> MODULE_DEVICE_TABLE(of, max77541_regulator_of_id);
> >>
> >> What do you think?
> >
> >If you have got all necessary data from the upper layer, why do you need to have
> >an ID table here? I'm not sure I understand how this OF ID table works in this
> >case.
> I added it since there is regulator node in device tree. With the help of
> devm_regulator_register(..), driver takes parameters of regulator node. I
> also used id to select and to initialize regulator descriptors which are chip
> specific. So far there is no comment about OF ID table so I kept it. I
> thought I need to add both of id table and platform id table as name matching
> is required to initialize platform device from mfd.
For platform device is one mechanism how to enumerate device, and bind it to
the driver. The OF ID table needs to be present in case you are using it for
direct DT enumeration (there is also something related to MFD child nodes, but
you need to check and explain how your device is enumerated by this driver).
I.o.w. please clarify how the OF ID table is being used.
On Tue, Jan 31, 2023 at 01:23:33PM +0000, Sahin, Okan wrote:
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> >> static const struct of_device_id max77541_regulator_of_id[] = {
> >> { .compatible = "adi,max77540-regulator", },
> >> { .compatible = "adi,max77541-regulator", },
> >> { /* sentinel */ }
> >> };
> >> MODULE_DEVICE_TABLE(of, max77541_regulator_of_id);
> >> What do you think?
> >If you have got all necessary data from the upper layer, why do you need to have
> >an ID table here? I'm not sure I understand how this OF ID table works in this
> >case.
> I added it since there is regulator node in device tree. With the help
> of devm_regulator_register(..), driver takes parameters of regulator
> node. I also used id to select and to initialize regulator descriptors
> which are chip specific. So far there is no comment about OF ID table
> so I kept it. I thought I need to add both of id table and platform id
> table as name matching is required to initialize platform device from
> mfd.
You probably shouldn't have compatibles for subdevices of a MFD unless
it's some reusable IP that'll appear elsewhere, especially for something
like the regulators where grouping them all into a single device is very
Linux specific.
On Tue, 31 Jan 2022 4:30 PM
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>On Tue, Jan 31, 2023 at 01:23:33PM +0000, Sahin, Okan wrote:
>> On Tue, 31 Jan 2022 3:27 PM
>> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>> >On Tue, Jan 31, 2023 at 09:27:48AM +0000, Sahin, Okan wrote:
>
>...
>
>> >> Sorry for second question. I do not want to bother you, but I
>> >> realized that I need to be sure about driver_data before sending
>> >> new patch. You said that you need to use pointers directly for
>> >> driver_data then I fixed that part in mfd, but I do not need or
>> >> use driver_data in regulator since chip_id comes from mfd device so
>> >> I think using like below should be enough for my implementation.
>> >>
>> >> static const struct platform_device_id max77541_regulator_platform_id[] =
>{
>> >> { "max77540-regulator", },
>> >> { "max77541-regulator", },
>> >> { /* sentinel */ }
>> >> };
>> >> MODULE_DEVICE_TABLE(platform, max77541_regulator_platform_id);
>> >>
>> >> static const struct of_device_id max77541_regulator_of_id[] = {
>> >> { .compatible = "adi,max77540-regulator", },
>> >> { .compatible = "adi,max77541-regulator", },
>> >> { /* sentinel */ }
>> >> };
>> >> MODULE_DEVICE_TABLE(of, max77541_regulator_of_id);
>> >>
>> >> What do you think?
>> >
>> >If you have got all necessary data from the upper layer, why do you
>> >need to have an ID table here? I'm not sure I understand how this OF
>> >ID table works in this case.
>
>> I added it since there is regulator node in device tree. With the help
>> of devm_regulator_register(..), driver takes parameters of regulator
>> node. I also used id to select and to initialize regulator descriptors
>> which are chip specific. So far there is no comment about OF ID table
>> so I kept it. I thought I need to add both of id table and platform id
>> table as name matching is required to initialize platform device from mfd.
>
>For platform device is one mechanism how to enumerate device, and bind it to
>the driver. The OF ID table needs to be present in case you are using it for direct
>DT enumeration (there is also something related to MFD child nodes, but you
>need to check and explain how your device is enumerated by this driver).
>
>I.o.w. please clarify how the OF ID table is being used.
>
>--
>With Best Regards,
>Andy Shevchenko
>
Hi Andy,
I do not use "of id table" directly in max77541-regulator.c so do I need to exclude it?
However, devm_regulator_register(..) method initialize each regulator with the nodes under "regulators node". If of_match in desc and name of node matches, then regulator will be initialized with parameters in the node under the regulators node in the device tree. Since I am using device tree to initialize regulators, I added of id table. I hope I explained the situation clearly.
Regards,
Okan Sahin
On Tue, Jan 31, 2023 at 01:59:45PM +0000, Sahin, Okan wrote:
> On Tue, 31 Jan 2022 4:30 PM
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> >On Tue, Jan 31, 2023 at 01:23:33PM +0000, Sahin, Okan wrote:
> >> On Tue, 31 Jan 2022 3:27 PM
> >> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> >> >On Tue, Jan 31, 2023 at 09:27:48AM +0000, Sahin, Okan wrote:
...
> >> >> Sorry for second question. I do not want to bother you, but I
> >> >> realized that I need to be sure about driver_data before sending
> >> >> new patch. You said that you need to use pointers directly for
> >> >> driver_data then I fixed that part in mfd, but I do not need or
> >> >> use driver_data in regulator since chip_id comes from mfd device so
> >> >> I think using like below should be enough for my implementation.
> >> >>
> >> >> static const struct platform_device_id max77541_regulator_platform_id[] =
> >{
> >> >> { "max77540-regulator", },
> >> >> { "max77541-regulator", },
> >> >> { /* sentinel */ }
> >> >> };
> >> >> MODULE_DEVICE_TABLE(platform, max77541_regulator_platform_id);
> >> >>
> >> >> static const struct of_device_id max77541_regulator_of_id[] = {
> >> >> { .compatible = "adi,max77540-regulator", },
> >> >> { .compatible = "adi,max77541-regulator", },
> >> >> { /* sentinel */ }
> >> >> };
> >> >> MODULE_DEVICE_TABLE(of, max77541_regulator_of_id);
> >> >>
> >> >> What do you think?
> >> >
> >> >If you have got all necessary data from the upper layer, why do you
> >> >need to have an ID table here? I'm not sure I understand how this OF
> >> >ID table works in this case.
> >
> >> I added it since there is regulator node in device tree. With the help
> >> of devm_regulator_register(..), driver takes parameters of regulator
> >> node. I also used id to select and to initialize regulator descriptors
> >> which are chip specific. So far there is no comment about OF ID table
> >> so I kept it. I thought I need to add both of id table and platform id
> >> table as name matching is required to initialize platform device from mfd.
> >
> >For platform device is one mechanism how to enumerate device, and bind it to
> >the driver. The OF ID table needs to be present in case you are using it for direct
> >DT enumeration (there is also something related to MFD child nodes, but you
> >need to check and explain how your device is enumerated by this driver).
> >
> >I.o.w. please clarify how the OF ID table is being used.
>
> I do not use "of id table" directly in max77541-regulator.c so do I need to exclude it?
Exactly my point. How does this OF ID table affect the device enumeration?
> However, devm_regulator_register(..) method initialize each regulator with
> the nodes under "regulators node". If of_match in desc and name of node
> matches, then regulator will be initialized with parameters in the node under
> the regulators node in the device tree. Since I am using device tree to
> initialize regulators, I added of id table. I hope I explained the situation
> clearly.
This is confusing. If your regulator is enumerated via DT, why do you need MFD?
On Tue, 31 Jan 2022 4:30 PM
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>On Tue, Jan 31, 2023 at 01:59:45PM +0000, Sahin, Okan wrote:
>> On Tue, 31 Jan 2022 4:30 PM
>> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>> >On Tue, Jan 31, 2023 at 01:23:33PM +0000, Sahin, Okan wrote:
>> >> On Tue, 31 Jan 2022 3:27 PM
>> >> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>> >> >On Tue, Jan 31, 2023 at 09:27:48AM +0000, Sahin, Okan wrote:
>
>...
>
>> >> >> Sorry for second question. I do not want to bother you, but I
>> >> >> realized that I need to be sure about driver_data before sending
>> >> >> new patch. You said that you need to use pointers directly for
>> >> >> driver_data then I fixed that part in mfd, but I do not need or
>> >> >> use driver_data in regulator since chip_id comes from mfd device
>> >> >> so I think using like below should be enough for my implementation.
>> >> >>
>> >> >> static const struct platform_device_id
>> >> >> max77541_regulator_platform_id[] =
>> >{
>> >> >> { "max77540-regulator", },
>> >> >> { "max77541-regulator", },
>> >> >> { /* sentinel */ }
>> >> >> };
>> >> >> MODULE_DEVICE_TABLE(platform, max77541_regulator_platform_id);
>> >> >>
>> >> >> static const struct of_device_id max77541_regulator_of_id[] = {
>> >> >> { .compatible = "adi,max77540-regulator", },
>> >> >> { .compatible = "adi,max77541-regulator", },
>> >> >> { /* sentinel */ }
>> >> >> };
>> >> >> MODULE_DEVICE_TABLE(of, max77541_regulator_of_id);
>> >> >>
>> >> >> What do you think?
>> >> >
>> >> >If you have got all necessary data from the upper layer, why do
>> >> >you need to have an ID table here? I'm not sure I understand how
>> >> >this OF ID table works in this case.
>> >
>> >> I added it since there is regulator node in device tree. With the
>> >> help of devm_regulator_register(..), driver takes parameters of
>> >> regulator node. I also used id to select and to initialize
>> >> regulator descriptors which are chip specific. So far there is no
>> >> comment about OF ID table so I kept it. I thought I need to add
>> >> both of id table and platform id table as name matching is required to
>initialize platform device from mfd.
>> >
>> >For platform device is one mechanism how to enumerate device, and
>> >bind it to the driver. The OF ID table needs to be present in case
>> >you are using it for direct DT enumeration (there is also something
>> >related to MFD child nodes, but you need to check and explain how your
>device is enumerated by this driver).
>> >
>> >I.o.w. please clarify how the OF ID table is being used.
>>
>> I do not use "of id table" directly in max77541-regulator.c so do I need to
>exclude it?
>
>Exactly my point. How does this OF ID table affect the device enumeration?
>
Since this is sub-device, it seems OF ID table does not affect the device enumarion, but as I stated before, I thought I need to add OF ID table because regulator's parameters are initialized via DT with the help of devm_regulator_register(..). It scans all the nodes under regulators node.
>> However, devm_regulator_register(..) method initialize each regulator
>> with the nodes under "regulators node". If of_match in desc and name
>> of node matches, then regulator will be initialized with parameters in
>> the node under the regulators node in the device tree. Since I am
>> using device tree to initialize regulators, I added of id table. I
>> hope I explained the situation clearly.
>
>This is confusing. If your regulator is enumerated via DT, why do you need MFD?
MAX77541 has also adc that is why I added MAX77541 as mfd device
>
>--
>With Best Regards,
>Andy Shevchenko
>
Hi Andy,
Thank you for your support and your time.
Regards,
Okan Sahin
@@ -12503,6 +12503,7 @@ L: linux-kernel@vger.kernel.org
S: Maintained
F: Documentation/devicetree/bindings/mfd/adi,max77541.yaml
F: drivers/mfd/max77541.c
+F: drivers/regulator/max77541-regulator.c
F: include/linux/mfd/max77541.h
MAXIM MAX77650 PMIC MFD DRIVER
@@ -556,6 +556,15 @@ config REGULATOR_MAX597X
The MAX5970/5978 is a smart switch with no output regulation, but
fault protection and voltage and current monitoring capabilities.
+config REGULATOR_MAX77541
+ tristate "Analog Devices MAX77541/77540 Regulator"
+ depends on MFD_MAX77541
+ help
+ This driver controls a Analog Devices MAX77541/77540 regulators
+ via I2C bus. Both MAX77540 and MAX77541 are dual-phase
+ high-efficiency buck converter. Say Y here to
+ enable the regulator driver.
+
config REGULATOR_MAX77620
tristate "Maxim 77620/MAX20024 voltage regulator"
depends on MFD_MAX77620 || COMPILE_TEST
@@ -68,6 +68,7 @@ obj-$(CONFIG_REGULATOR_LTC3676) += ltc3676.o
obj-$(CONFIG_REGULATOR_MAX14577) += max14577-regulator.o
obj-$(CONFIG_REGULATOR_MAX1586) += max1586.o
obj-$(CONFIG_REGULATOR_MAX597X) += max597x-regulator.o
+obj-$(CONFIG_REGULATOR_MAX77541) += max77541-regulator.o
obj-$(CONFIG_REGULATOR_MAX77620) += max77620-regulator.o
obj-$(CONFIG_REGULATOR_MAX77650) += max77650-regulator.o
obj-$(CONFIG_REGULATOR_MAX8649) += max8649.o
new file mode 100644
@@ -0,0 +1,160 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2022 Analog Devices, Inc.
+ * ADI Regulator driver for the MAX77540 and MAX77541
+ */
+
+#include <linux/i2c.h>
+#include <linux/mfd/max77541.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/of_regulator.h>
+
+static const struct regulator_ops max77541_buck_ops = {
+ .enable = regulator_enable_regmap,
+ .disable = regulator_disable_regmap,
+ .is_enabled = regulator_is_enabled_regmap,
+ .list_voltage = regulator_list_voltage_pickable_linear_range,
+ .get_voltage_sel = regulator_get_voltage_sel_pickable_regmap,
+ .set_voltage_sel = regulator_set_voltage_sel_pickable_regmap,
+};
+
+static const struct linear_range max77540_buck_ranges[] = {
+ /* Ranges when VOLT_SEL bits are 0x00 */
+ REGULATOR_LINEAR_RANGE(500000, 0x00, 0x8B, 5000),
+ REGULATOR_LINEAR_RANGE(1200000, 0x8C, 0xFF, 0),
+ /* Ranges when VOLT_SEL bits are 0x40 */
+ REGULATOR_LINEAR_RANGE(1200000, 0x00, 0x8B, 10000),
+ REGULATOR_LINEAR_RANGE(2400000, 0x8C, 0xFF, 0),
+ /* Ranges when VOLT_SEL bits are 0x80 */
+ REGULATOR_LINEAR_RANGE(2000000, 0x00, 0x9F, 20000),
+ REGULATOR_LINEAR_RANGE(5200000, 0xA0, 0xFF, 0),
+};
+
+static const struct linear_range max77541_buck_ranges[] = {
+ /* Ranges when VOLT_SEL bits are 0x00 */
+ REGULATOR_LINEAR_RANGE(300000, 0x00, 0xB3, 5000),
+ REGULATOR_LINEAR_RANGE(1200000, 0xB4, 0xFF, 0),
+ /* Ranges when VOLT_SEL bits are 0x40 */
+ REGULATOR_LINEAR_RANGE(1200000, 0x00, 0x8B, 10000),
+ REGULATOR_LINEAR_RANGE(2400000, 0x8C, 0xFF, 0),
+ /* Ranges when VOLT_SEL bits are 0x80 */
+ REGULATOR_LINEAR_RANGE(2000000, 0x00, 0x9F, 20000),
+ REGULATOR_LINEAR_RANGE(5200000, 0xA0, 0xFF, 0),
+};
+
+static const unsigned int max77541_buck_volt_range_sel[] = {
+ 0x00, 0x00, 0x40, 0x40, 0x80, 0x80,
+};
+
+#define MAX77540_BUCK(_id, _ops) \
+ { .id = MAX77541_BUCK ## _id, \
+ .name = "buck"#_id, \
+ .of_match = "buck"#_id, \
+ .regulators_node = "regulators", \
+ .enable_reg = MAX77541_REG_EN_CTRL, \
+ .enable_mask = MAX77541_BIT_M ## _id ## _EN, \
+ .ops = &(_ops), \
+ .type = REGULATOR_VOLTAGE, \
+ .linear_ranges = max77540_buck_ranges, \
+ .n_linear_ranges = ARRAY_SIZE(max77540_buck_ranges), \
+ .vsel_reg = MAX77541_REG_M ## _id ## _VOUT, \
+ .vsel_mask = MAX77541_BITS_MX_VOUT, \
+ .vsel_range_reg = MAX77541_REG_M ## _id ## _CFG1, \
+ .vsel_range_mask = MAX77541_BITS_MX_CFG1_RNG, \
+ .linear_range_selectors = max77541_buck_volt_range_sel, \
+ .owner = THIS_MODULE, \
+ }
+
+#define MAX77541_BUCK(_id, _ops) \
+ { .id = MAX77541_BUCK ## _id, \
+ .name = "buck"#_id, \
+ .of_match = "buck"#_id, \
+ .regulators_node = "regulators", \
+ .enable_reg = MAX77541_REG_EN_CTRL, \
+ .enable_mask = MAX77541_BIT_M ## _id ## _EN, \
+ .ops = &(_ops), \
+ .type = REGULATOR_VOLTAGE, \
+ .linear_ranges = max77541_buck_ranges, \
+ .n_linear_ranges = ARRAY_SIZE(max77541_buck_ranges), \
+ .vsel_reg = MAX77541_REG_M ## _id ## _VOUT, \
+ .vsel_mask = MAX77541_BITS_MX_VOUT, \
+ .vsel_range_reg = MAX77541_REG_M ## _id ## _CFG1, \
+ .vsel_range_mask = MAX77541_BITS_MX_CFG1_RNG, \
+ .linear_range_selectors = max77541_buck_volt_range_sel, \
+ .owner = THIS_MODULE, \
+ }
+
+static const struct regulator_desc max77540_regulators_desc[] = {
+ MAX77540_BUCK(1, max77541_buck_ops),
+ MAX77540_BUCK(2, max77541_buck_ops),
+};
+
+static const struct regulator_desc max77541_regulators_desc[] = {
+ MAX77541_BUCK(1, max77541_buck_ops),
+ MAX77541_BUCK(2, max77541_buck_ops),
+};
+
+static int max77541_regulator_probe(struct platform_device *pdev)
+{
+ struct max77541 *max77541 = dev_get_drvdata(pdev->dev.parent);
+ struct regulator_config config = {};
+ const struct regulator_desc *desc;
+ struct device *dev = &pdev->dev;
+ struct regulator_dev *rdev;
+ int i;
+
+ config.dev = pdev->dev.parent;
+
+ if (max77541->id == MAX77540)
+ desc = max77540_regulators_desc;
+ else if (max77541->id == MAX77541)
+ desc = max77541_regulators_desc;
+ else
+ return -EINVAL;
+
+ for (i = 0; i < MAX77541_MAX_REGULATORS; i++) {
+ rdev = devm_regulator_register(dev,
+ &desc[i], &config);
+ if (IS_ERR(rdev))
+ return dev_err_probe(dev, PTR_ERR(rdev),
+ "Failed to register regulator\n");
+ }
+
+ return 0;
+}
+
+static const struct platform_device_id max77541_regulator_platform_id[] = {
+ { "max77540-regulator", MAX77540 },
+ { "max77541-regulator", MAX77541 },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(platform, max77541_regulator_platform_id);
+
+static const struct of_device_id max77541_regulator_of_id[] = {
+ {
+ .compatible = "adi,max77540-regulator",
+ .data = (void *)MAX77540,
+ },
+ {
+ .compatible = "adi,max77541-regulator",
+ .data = (void *)MAX77541,
+ },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, max77541_regulator_of_id);
+
+static struct platform_driver max77541_regulator_driver = {
+ .driver = {
+ .name = "max77541-regulator",
+ .of_match_table = max77541_regulator_of_id,
+ },
+ .probe = max77541_regulator_probe,
+ .id_table = max77541_regulator_platform_id,
+};
+module_platform_driver(max77541_regulator_driver);
+
+MODULE_AUTHOR("Okan Sahin <Okan.Sahin@analog.com>");
+MODULE_DESCRIPTION("MAX77540/MAX77541 regulator driver");
+MODULE_LICENSE("GPL");