[v6,5/5] mfd: max77541: Add ADI MAX77541/MAX77540 PMIC Support
Commit Message
MFD driver for MAX77541/MAX77540 to enable its sub
devices.
The MAX77541 is a multi-function devices. It includes
buck converter and ADC.
The MAX77540 is a high-efficiency buck converter
with two 3A switching phases.
They have same regmap except for ADC part of MAX77541.
Signed-off-by: Okan Sahin <okan.sahin@analog.com>
---
drivers/mfd/Kconfig | 13 ++
drivers/mfd/Makefile | 1 +
drivers/mfd/max77541.c | 224 +++++++++++++++++++++++++++++++++++
include/linux/mfd/max77541.h | 97 +++++++++++++++
4 files changed, 335 insertions(+)
create mode 100644 drivers/mfd/max77541.c
create mode 100644 include/linux/mfd/max77541.h
Comments
On Tue, 07 Mar 2023, Okan Sahin wrote:
> MFD driver for MAX77541/MAX77540 to enable its sub
> devices.
>
> The MAX77541 is a multi-function devices. It includes
> buck converter and ADC.
>
> The MAX77540 is a high-efficiency buck converter
> with two 3A switching phases.
>
> They have same regmap except for ADC part of MAX77541.
>
> Signed-off-by: Okan Sahin <okan.sahin@analog.com>
> ---
> drivers/mfd/Kconfig | 13 ++
> drivers/mfd/Makefile | 1 +
> drivers/mfd/max77541.c | 224 +++++++++++++++++++++++++++++++++++
> include/linux/mfd/max77541.h | 97 +++++++++++++++
> 4 files changed, 335 insertions(+)
> create mode 100644 drivers/mfd/max77541.c
> create mode 100644 include/linux/mfd/max77541.h
FYI: I'm not re-reviewing this since you've chosen to ignore some of my
previous review comments. Issues highlighted by review comments don't
just go away on resubmission.
--
Lee Jones [李琼斯]
On Wed, 15 Mar 2023, Lee Jones wrote:
> On Tue, 07 Mar 2023, Okan Sahin wrote:
>
> > MFD driver for MAX77541/MAX77540 to enable its sub
> > devices.
> >
> > The MAX77541 is a multi-function devices. It includes
> > buck converter and ADC.
> >
> > The MAX77540 is a high-efficiency buck converter
> > with two 3A switching phases.
> >
> > They have same regmap except for ADC part of MAX77541.
> >
> > Signed-off-by: Okan Sahin <okan.sahin@analog.com>
> > ---
> > drivers/mfd/Kconfig | 13 ++
> > drivers/mfd/Makefile | 1 +
> > drivers/mfd/max77541.c | 224 +++++++++++++++++++++++++++++++++++
> > include/linux/mfd/max77541.h | 97 +++++++++++++++
> > 4 files changed, 335 insertions(+)
> > create mode 100644 drivers/mfd/max77541.c
> > create mode 100644 include/linux/mfd/max77541.h
>
> FYI: I'm not re-reviewing this since you've chosen to ignore some of my
> previous review comments. Issues highlighted by review comments don't
> just go away on resubmission.
... and the subject is malformed.
--
Lee Jones [李琼斯]
>On Wed, 15 Mar 2023, Lee Jones wrote:
>
>> On Tue, 07 Mar 2023, Okan Sahin wrote:
>>
>> > MFD driver for MAX77541/MAX77540 to enable its sub devices.
>> >
>> > The MAX77541 is a multi-function devices. It includes buck converter
>> > and ADC.
>> >
>> > The MAX77540 is a high-efficiency buck converter with two 3A
>> > switching phases.
>> >
>> > They have same regmap except for ADC part of MAX77541.
>> >
>> > Signed-off-by: Okan Sahin <okan.sahin@analog.com>
>> > ---
>> > drivers/mfd/Kconfig | 13 ++
>> > drivers/mfd/Makefile | 1 +
>> > drivers/mfd/max77541.c | 224
>+++++++++++++++++++++++++++++++++++
>> > include/linux/mfd/max77541.h | 97 +++++++++++++++
>> > 4 files changed, 335 insertions(+)
>> > create mode 100644 drivers/mfd/max77541.c create mode 100644
>> > include/linux/mfd/max77541.h
>>
>> FYI: I'm not re-reviewing this since you've chosen to ignore some of
>> my previous review comments. Issues highlighted by review comments
>> don't just go away on resubmission.
>
>... and the subject is malformed.
>
>--
>Lee Jones [李琼斯]
Hi Lee,
I am sorry if I missed your review comments, this was not my intention. I want to thank you for your contribution. Your feedbacks are very valuable, and I am trying to understand and fix each one before sending the patch. Indeed, I sorted your feedback on previous patches. As far as I know, I have fixed all of them, is there a problem with any of them that I fixed, or is there any missing review? From you, there were some comments like "why did you use this?", I suppose I need to respond them before sending following patches. I thought I should not bother the maintainers unnecessarily. I am sorry for them.
For previous patch(v5), There was feedback from Andy. I did not fix them.
1)
> They have same regmap except for ADC part of MAX77541.
Extra space in the Subject.
...
> +#include <linux/of_device.h
This is my fault, I missed this comment. I will fix in following patch(v7).
2)
...
> +static const struct regmap_config max77541_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
Do you need lock of regmap?
> +};
...
Since I do not need lock of regmap, I did not change anything in regmap_config (v6). Do I need to answer this question even if I don't need lock of regmap?
For the other reviews, I fixed them as you said. Thank you for your time, and effort. Sorry for the misunderstanding and confusion.
Regards,
Okan
On Tue, Mar 28, 2023 at 08:26:41AM +0000, Sahin, Okan wrote:
> >On Wed, 15 Mar 2023, Lee Jones wrote:
> >> On Tue, 07 Mar 2023, Okan Sahin wrote:
...
> For previous patch(v5), There was feedback from Andy. I did not fix them.
Why not? :-)
> 1)
> > They have same regmap except for ADC part of MAX77541.
>
> Extra space in the Subject.
>
> ...
>
> > +#include <linux/of_device.h
>
> This is my fault, I missed this comment. I will fix in following patch(v7).
>
> 2)
> ...
>
> > +static const struct regmap_config max77541_regmap_config = {
> > + .reg_bits = 8,
> > + .val_bits = 8,
>
> Do you need lock of regmap?
>
> > +};
>
> ...
>
> Since I do not need lock of regmap, I did not change anything in
> regmap_config (v6). Do I need to answer this question even if I don't need
> lock of regmap?
IIRC the lock is opt-out. You need to explicitly disable it if not needed.
On Tue, 2023-03-28 at 15:51 +0300, Andy Shevchenko wrote:
> On Tue, Mar 28, 2023 at 08:26:41AM +0000, Sahin, Okan wrote:
> > > On Wed, 15 Mar 2023, Lee Jones wrote:
> > > > On Tue, 07 Mar 2023, Okan Sahin wrote:
>
> ...
>
> > For previous patch(v5), There was feedback from Andy. I did not fix
> > them.
>
> Why not? :-)
>
> > 1)
> > > They have same regmap except for ADC part of MAX77541.
> >
> > Extra space in the Subject.
> >
> > ...
> >
> > > +#include <linux/of_device.h
> >
> > This is my fault, I missed this comment. I will fix in following
> > patch(v7).
> >
> > 2)
> > ...
> >
> > > +static const struct regmap_config max77541_regmap_config = {
> > > + .reg_bits = 8,
> > > + .val_bits = 8,
> >
> > Do you need lock of regmap?
> >
> > > +};
> >
> > ...
> >
> > Since I do not need lock of regmap, I did not change anything in
> > regmap_config (v6). Do I need to answer this question even if I
> > don't need
> > lock of regmap?
>
> IIRC the lock is opt-out. You need to explicitly disable it if not
> needed.
>
IIRC, regmap_read() is not really reentrant and it is used in the IIO
driver on the sysfs interface. So, yeah, I think you need the regmap
lock and better just leave the config as is. Yes, the lock is opt-out
so let's not disable it :)
- Nuno Sá
On Tue, Mar 28, 2023 at 03:26:44PM +0200, Nuno Sá wrote:
> IIRC, regmap_read() is not really reentrant and it is used in the IIO
> driver on the sysfs interface. So, yeah, I think you need the regmap
> lock and better just leave the config as is. Yes, the lock is opt-out
> so let's not disable it :)
All the regmap operations are fully thread safe.
On Tue, 2023-03-28 at 14:46 +0100, Mark Brown wrote:
> On Tue, Mar 28, 2023 at 03:26:44PM +0200, Nuno Sá wrote:
>
> > IIRC, regmap_read() is not really reentrant and it is used in the
> > IIO
> > driver on the sysfs interface. So, yeah, I think you need the
> > regmap
> > lock and better just leave the config as is. Yes, the lock is opt-
> > out
> > so let's not disable it :)
>
> All the regmap operations are fully thread safe.
Even if 'config->disable_locking' is set? I think that is what's being
discussed in here...
- Nuno Sá
On Tue, Mar 28, 2023 at 04:18:30PM +0200, Nuno Sá wrote:
> On Tue, 2023-03-28 at 14:46 +0100, Mark Brown wrote:
> > On Tue, Mar 28, 2023 at 03:26:44PM +0200, Nuno Sá wrote:
> >
> > > IIRC, regmap_read() is not really reentrant and it is used in the
> > > IIO
> > > driver on the sysfs interface. So, yeah, I think you need the
> > > regmap
> > > lock and better just leave the config as is. Yes, the lock is opt-
> > > out
> > > so let's not disable it :)
> >
> > All the regmap operations are fully thread safe.
>
> Even if 'config->disable_locking' is set? I think that is what's being
> discussed in here...
In case the driver has its own lock to serialize IO how on earth the regmap
lock is needed. That's what I asked the author of the driver. He told the code
doesn't require the regmap lock, and I tend to believe the author. So, why to
keep it?
On 3/28/23 07:35, Andy Shevchenko wrote:
> On Tue, Mar 28, 2023 at 04:18:30PM +0200, Nuno Sá wrote:
>> On Tue, 2023-03-28 at 14:46 +0100, Mark Brown wrote:
>>> On Tue, Mar 28, 2023 at 03:26:44PM +0200, Nuno Sá wrote:
>>>
>>>> IIRC, regmap_read() is not really reentrant and it is used in the
>>>> IIO
>>>> driver on the sysfs interface. So, yeah, I think you need the
>>>> regmap
>>>> lock and better just leave the config as is. Yes, the lock is opt-
>>>> out
>>>> so let's not disable it :)
>>> All the regmap operations are fully thread safe.
>> Even if 'config->disable_locking' is set? I think that is what's being
>> discussed in here...
> In case the driver has its own lock to serialize IO how on earth the regmap
> lock is needed.
But the driver does not have its own lock.
On Tue, 2023-03-28 at 17:35 +0300, Andy Shevchenko wrote:
> On Tue, Mar 28, 2023 at 04:18:30PM +0200, Nuno Sá wrote:
> > On Tue, 2023-03-28 at 14:46 +0100, Mark Brown wrote:
> > > On Tue, Mar 28, 2023 at 03:26:44PM +0200, Nuno Sá wrote:
> > >
> > > > IIRC, regmap_read() is not really reentrant and it is used in
> > > > the
> > > > IIO
> > > > driver on the sysfs interface. So, yeah, I think you need the
> > > > regmap
> > > > lock and better just leave the config as is. Yes, the lock is
> > > > opt-
> > > > out
> > > > so let's not disable it :)
> > >
> > > All the regmap operations are fully thread safe.
> >
> > Even if 'config->disable_locking' is set? I think that is what's
> > being
> > discussed in here...
>
> In case the driver has its own lock to serialize IO how on earth the
> regmap
> lock is needed. That's what I asked the author of the driver. He told
> the code
Well, if the driver has it's own locking, then sure we do not need
regmap's lock...
> doesn't require the regmap lock, and I tend to believe the author.
> So, why to
> keep it?
>
However, if you look at the adc driver, I can see plain regmap_read()
calls without any "outside" locking.
- Nuno Sá
On Tue, Mar 28, 2023 at 04:18:30PM +0200, Nuno Sá wrote:
> On Tue, 2023-03-28 at 14:46 +0100, Mark Brown wrote:
> > On Tue, Mar 28, 2023 at 03:26:44PM +0200, Nuno Sá wrote:
> > > IIRC, regmap_read() is not really reentrant and it is used in the
> > > IIO
> > > driver on the sysfs interface. So, yeah, I think you need the
> > > regmap
> > > lock and better just leave the config as is. Yes, the lock is opt-
> > > out
> > > so let's not disable it :)
> > All the regmap operations are fully thread safe.
> Even if 'config->disable_locking' is set? I think that is what's being
> discussed in here...
In that case the caller has to ensure that the regmap is only used in a
thread safe fashion.
On Tue, Mar 28, 2023 at 04:51:18PM +0200, Nuno Sá wrote:
> On Tue, 2023-03-28 at 17:35 +0300, Andy Shevchenko wrote:
> > On Tue, Mar 28, 2023 at 04:18:30PM +0200, Nuno Sá wrote:
> > > On Tue, 2023-03-28 at 14:46 +0100, Mark Brown wrote:
> > > > On Tue, Mar 28, 2023 at 03:26:44PM +0200, Nuno Sá wrote:
> > > >
> > > > > IIRC, regmap_read() is not really reentrant and it is used in
> > > > > the
> > > > > IIO
> > > > > driver on the sysfs interface. So, yeah, I think you need the
> > > > > regmap
> > > > > lock and better just leave the config as is. Yes, the lock is
> > > > > opt-
> > > > > out
> > > > > so let's not disable it :)
> > > >
> > > > All the regmap operations are fully thread safe.
> > >
> > > Even if 'config->disable_locking' is set? I think that is what's
> > > being
> > > discussed in here...
> >
> > In case the driver has its own lock to serialize IO how on earth the
> > regmap
> > lock is needed. That's what I asked the author of the driver. He told
> > the code
>
> Well, if the driver has it's own locking, then sure we do not need
> regmap's lock...
>
> > doesn't require the regmap lock, and I tend to believe the author.
> > So, why to
> > keep it?
>
> However, if you look at the adc driver, I can see plain regmap_read()
> calls without any "outside" locking.
Then author of the code should know what they are doing. Right?
>On Tue, Mar 28, 2023 at 04:51:18PM +0200, Nuno Sá wrote:
>> On Tue, 2023-03-28 at 17:35 +0300, Andy Shevchenko wrote:
>> > On Tue, Mar 28, 2023 at 04:18:30PM +0200, Nuno Sá wrote:
>> > > On Tue, 2023-03-28 at 14:46 +0100, Mark Brown wrote:
>> > > > On Tue, Mar 28, 2023 at 03:26:44PM +0200, Nuno Sá wrote:
>> > > >
>> > > > > IIRC, regmap_read() is not really reentrant and it is used in
>> > > > > the IIO driver on the sysfs interface. So, yeah, I think you
>> > > > > need the regmap lock and better just leave the config as is.
>> > > > > Yes, the lock is
>> > > > > opt-
>> > > > > out
>> > > > > so let's not disable it :)
>> > > >
>> > > > All the regmap operations are fully thread safe.
>> > >
>> > > Even if 'config->disable_locking' is set? I think that is what's
>> > > being discussed in here...
>> >
>> > In case the driver has its own lock to serialize IO how on earth the
>> > regmap lock is needed. That's what I asked the author of the driver.
>> > He told the code
>>
>> Well, if the driver has it's own locking, then sure we do not need
>> regmap's lock...
>>
>> > doesn't require the regmap lock, and I tend to believe the author.
>> > So, why to
>> > keep it?
>>
>> However, if you look at the adc driver, I can see plain regmap_read()
>> calls without any "outside" locking.
>
>Then author of the code should know what they are doing. Right?
>
>--
>With Best Regards,
>Andy Shevchenko
>
Hi Andy,
Actually, I do not want to disable regmap lock that's why I did not update it.
Regards,
Okan
On Tue, 2023-03-28 at 18:47 +0300, Andy Shevchenko wrote:
> On Tue, Mar 28, 2023 at 04:51:18PM +0200, Nuno Sá wrote:
> > On Tue, 2023-03-28 at 17:35 +0300, Andy Shevchenko wrote:
> > > On Tue, Mar 28, 2023 at 04:18:30PM +0200, Nuno Sá wrote:
> > > > On Tue, 2023-03-28 at 14:46 +0100, Mark Brown wrote:
> > > > > On Tue, Mar 28, 2023 at 03:26:44PM +0200, Nuno Sá wrote:
> > > > >
> > > > > > IIRC, regmap_read() is not really reentrant and it is used
> > > > > > in
> > > > > > the
> > > > > > IIO
> > > > > > driver on the sysfs interface. So, yeah, I think you need
> > > > > > the
> > > > > > regmap
> > > > > > lock and better just leave the config as is. Yes, the lock
> > > > > > is
> > > > > > opt-
> > > > > > out
> > > > > > so let's not disable it :)
> > > > >
> > > > > All the regmap operations are fully thread safe.
> > > >
> > > > Even if 'config->disable_locking' is set? I think that is
> > > > what's
> > > > being
> > > > discussed in here...
> > >
> > > In case the driver has its own lock to serialize IO how on earth
> > > the
> > > regmap
> > > lock is needed. That's what I asked the author of the driver. He
> > > told
> > > the code
> >
> > Well, if the driver has it's own locking, then sure we do not need
> > regmap's lock...
> >
> > > doesn't require the regmap lock, and I tend to believe the
> > > author.
> > > So, why to
> > > keep it?
> >
> > However, if you look at the adc driver, I can see plain
> > regmap_read()
> > calls without any "outside" locking.
>
> Then author of the code should know what they are doing. Right?
>
In theory yes, but you know that's not always the case :)
- Nuno Sá
On Wed, Mar 29, 2023 at 09:01:30AM +0200, Nuno Sá wrote:
> On Tue, 2023-03-28 at 18:47 +0300, Andy Shevchenko wrote:
> > On Tue, Mar 28, 2023 at 04:51:18PM +0200, Nuno Sá wrote:
> > > On Tue, 2023-03-28 at 17:35 +0300, Andy Shevchenko wrote:
> > > > On Tue, Mar 28, 2023 at 04:18:30PM +0200, Nuno Sá wrote:
> > > > > On Tue, 2023-03-28 at 14:46 +0100, Mark Brown wrote:
> > > > > > On Tue, Mar 28, 2023 at 03:26:44PM +0200, Nuno Sá wrote:
> > > > > >
> > > > > > > IIRC, regmap_read() is not really reentrant and it is used
> > > > > > > in
> > > > > > > the
> > > > > > > IIO
> > > > > > > driver on the sysfs interface. So, yeah, I think you need
> > > > > > > the
> > > > > > > regmap
> > > > > > > lock and better just leave the config as is. Yes, the lock
> > > > > > > is
> > > > > > > opt-
> > > > > > > out
> > > > > > > so let's not disable it :)
> > > > > >
> > > > > > All the regmap operations are fully thread safe.
> > > > >
> > > > > Even if 'config->disable_locking' is set? I think that is
> > > > > what's
> > > > > being
> > > > > discussed in here...
> > > >
> > > > In case the driver has its own lock to serialize IO how on earth
> > > > the
> > > > regmap
> > > > lock is needed. That's what I asked the author of the driver. He
> > > > told
> > > > the code
> > >
> > > Well, if the driver has it's own locking, then sure we do not need
> > > regmap's lock...
> > >
> > > > doesn't require the regmap lock, and I tend to believe the
> > > > author.
> > > > So, why to
> > > > keep it?
> > >
> > > However, if you look at the adc driver, I can see plain
> > > regmap_read()
> > > calls without any "outside" locking.
> >
> > Then author of the code should know what they are doing. Right?
>
> In theory yes, but you know that's not always the case :)
Exactly. That's why I want to hear from the author of the code to make sure
they know _their_ code.
On Tue, Mar 28, 2023 at 04:01:21PM +0000, Sahin, Okan wrote:
> >On Tue, Mar 28, 2023 at 04:51:18PM +0200, Nuno Sá wrote:
> >> On Tue, 2023-03-28 at 17:35 +0300, Andy Shevchenko wrote:
> >> > On Tue, Mar 28, 2023 at 04:18:30PM +0200, Nuno Sá wrote:
> >> > > On Tue, 2023-03-28 at 14:46 +0100, Mark Brown wrote:
> >> > > > On Tue, Mar 28, 2023 at 03:26:44PM +0200, Nuno Sá wrote:
> >> > > >
> >> > > > > IIRC, regmap_read() is not really reentrant and it is used in
> >> > > > > the IIO driver on the sysfs interface. So, yeah, I think you
> >> > > > > need the regmap lock and better just leave the config as is.
> >> > > > > Yes, the lock is
> >> > > > > opt-
> >> > > > > out
> >> > > > > so let's not disable it :)
> >> > > >
> >> > > > All the regmap operations are fully thread safe.
> >> > >
> >> > > Even if 'config->disable_locking' is set? I think that is what's
> >> > > being discussed in here...
> >> >
> >> > In case the driver has its own lock to serialize IO how on earth the
> >> > regmap lock is needed. That's what I asked the author of the driver.
> >> > He told the code
> >>
> >> Well, if the driver has it's own locking, then sure we do not need
> >> regmap's lock...
> >>
> >> > doesn't require the regmap lock, and I tend to believe the author.
> >> > So, why to
> >> > keep it?
> >>
> >> However, if you look at the adc driver, I can see plain regmap_read()
> >> calls without any "outside" locking.
> >
> >Then author of the code should know what they are doing. Right?
> Actually, I do not want to disable regmap lock that's why I did not update it.
If you have something like
func1()
regmap_read(reg1)
regmap_read/write(reg2)
func2()
regmap_read/write(regX) // X may or may not be 1 or 2
and func1() and func2() can be run in parallel then the code is racy.
Do you have such in your code?
On Wed, Mar 29, 2023 at 05:08:44PM +0300, Andy Shevchenko wrote:
> On Tue, Mar 28, 2023 at 04:01:21PM +0000, Sahin, Okan wrote:
> > >On Tue, Mar 28, 2023 at 04:51:18PM +0200, Nuno Sá wrote:
> > >> On Tue, 2023-03-28 at 17:35 +0300, Andy Shevchenko wrote:
> > >> > On Tue, Mar 28, 2023 at 04:18:30PM +0200, Nuno Sá wrote:
> > >> > > On Tue, 2023-03-28 at 14:46 +0100, Mark Brown wrote:
> > >> > > > On Tue, Mar 28, 2023 at 03:26:44PM +0200, Nuno Sá wrote:
> > >> > > >
> > >> > > > > IIRC, regmap_read() is not really reentrant and it is used in
> > >> > > > > the IIO driver on the sysfs interface. So, yeah, I think you
> > >> > > > > need the regmap lock and better just leave the config as is.
> > >> > > > > Yes, the lock is
> > >> > > > > opt-
> > >> > > > > out
> > >> > > > > so let's not disable it :)
> > >> > > >
> > >> > > > All the regmap operations are fully thread safe.
> > >> > >
> > >> > > Even if 'config->disable_locking' is set? I think that is what's
> > >> > > being discussed in here...
> > >> >
> > >> > In case the driver has its own lock to serialize IO how on earth the
> > >> > regmap lock is needed. That's what I asked the author of the driver.
> > >> > He told the code
> > >>
> > >> Well, if the driver has it's own locking, then sure we do not need
> > >> regmap's lock...
> > >>
> > >> > doesn't require the regmap lock, and I tend to believe the author.
> > >> > So, why to
> > >> > keep it?
> > >>
> > >> However, if you look at the adc driver, I can see plain regmap_read()
> > >> calls without any "outside" locking.
> > >
> > >Then author of the code should know what they are doing. Right?
>
> > Actually, I do not want to disable regmap lock that's why I did not update it.
>
> If you have something like
>
> func1()
> regmap_read(reg1)
> regmap_read/write(reg2)
>
> func2()
> regmap_read/write(regX) // X may or may not be 1 or 2
>
> and func1() and func2() can be run in parallel then the code is racy.
I have to add that it's racy depending on the hardware of course.
In some cases it may be not a problem, in some it can. _Strictly_
speaking it's racy.
> Do you have such in your code?
Please, double check that. It's recommended to explain your locking schema
somewhere in the code top comment so anybody who reads it later and tries
to modify will know what to expect.
On Tue, 28 Mar 2023, Sahin, Okan wrote:
> >On Wed, 15 Mar 2023, Lee Jones wrote:
> >
> >> On Tue, 07 Mar 2023, Okan Sahin wrote:
> >>
> >> > MFD driver for MAX77541/MAX77540 to enable its sub devices.
> >> >
> >> > The MAX77541 is a multi-function devices. It includes buck converter
> >> > and ADC.
> >> >
> >> > The MAX77540 is a high-efficiency buck converter with two 3A
> >> > switching phases.
> >> >
> >> > They have same regmap except for ADC part of MAX77541.
> >> >
> >> > Signed-off-by: Okan Sahin <okan.sahin@analog.com>
> >> > ---
> >> > drivers/mfd/Kconfig | 13 ++
> >> > drivers/mfd/Makefile | 1 +
> >> > drivers/mfd/max77541.c | 224
> >+++++++++++++++++++++++++++++++++++
> >> > include/linux/mfd/max77541.h | 97 +++++++++++++++
> >> > 4 files changed, 335 insertions(+)
> >> > create mode 100644 drivers/mfd/max77541.c create mode 100644
> >> > include/linux/mfd/max77541.h
> >>
> >> FYI: I'm not re-reviewing this since you've chosen to ignore some of
> >> my previous review comments. Issues highlighted by review comments
> >> don't just go away on resubmission.
> >
> >... and the subject is malformed.
> >
> >--
> >Lee Jones [李琼斯]
>
> Hi Lee,
>
> I am sorry if I missed your review comments, this was not my intention. I want to thank you for your contribution. Your feedbacks are very valuable, and I am trying to understand and fix each one before sending the patch. Indeed, I sorted your feedback on previous patches. As far as I know, I have fixed all of them, is there a problem with any of them that I fixed, or is there any missing review? From you, there were some comments like "why did you use this?", I suppose I need to respond them before sending following patches. I thought I should not bother the maintainers unnecessarily. I am sorry for them.
Please ask your email client to line-wrap.
Here is the part of the review you ignored:
[...]
> +static const struct chip_info chip[] = {
Why do you need this require sub-structure?
> + [MAX77540] = {
> + .id = MAX77540,
> + .n_devs = ARRAY_SIZE(max77540_devs),
> + .devs = max77540_devs,
> + },
> + [MAX77541] = {
> + .id = MAX77541,
> + .n_devs = ARRAY_SIZE(max77541_devs),
> + .devs = max77541_devs,
> + },
> +};
[...]
> +static const struct of_device_id max77541_of_id[] = {
> + {
> + .compatible = "adi,max77540",
> + .data = &chip[MAX77540],
> + },
> + {
> + .compatible = "adi,max77541",
> + .data = &chip[MAX77541],
> + },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, max77541_of_id);
> +
> +static const struct i2c_device_id max77541_i2c_id[] = {
> + { "max77540", (kernel_ulong_t)&chip[MAX77540] },
> + { "max77541", (kernel_ulong_t)&chip[MAX77541] },
Just 'MAX77540' is fine.
> + { /* sentinel */ }
Remove the comment, we know how terminators work.
Same comments for max77541_of_id.
--
Lee Jones [李琼斯]
On Wed, Mar 29, 2023 at 03:36:15PM +0100, Lee Jones wrote:
> On Tue, 28 Mar 2023, Sahin, Okan wrote:
> > >On Wed, 15 Mar 2023, Lee Jones wrote:
> > >> On Tue, 07 Mar 2023, Okan Sahin wrote:
...
> > +static const struct i2c_device_id max77541_i2c_id[] = {
> > + { "max77540", (kernel_ulong_t)&chip[MAX77540] },
> > + { "max77541", (kernel_ulong_t)&chip[MAX77541] },
>
> Just 'MAX77540' is fine.
I tend to disagree.
There is an error prone approach esp. when we talk with some functions
that unifies OF/ACPI driver data retrieval with legacy ID tables.
In such a case the 0 from enum is hard to distinguish from NULL when
the driver data is not set or not found. On top of that the simple integer
in the legacy driver data will require additional code to be added in
the ->probe().
On Wed, 29 Mar 2023, Andy Shevchenko wrote:
> On Wed, Mar 29, 2023 at 03:36:15PM +0100, Lee Jones wrote:
> > On Tue, 28 Mar 2023, Sahin, Okan wrote:
> > > >On Wed, 15 Mar 2023, Lee Jones wrote:
> > > >> On Tue, 07 Mar 2023, Okan Sahin wrote:
>
> ...
>
> > > +static const struct i2c_device_id max77541_i2c_id[] = {
> > > + { "max77540", (kernel_ulong_t)&chip[MAX77540] },
> > > + { "max77541", (kernel_ulong_t)&chip[MAX77541] },
> >
> > Just 'MAX77540' is fine.
>
> I tend to disagree.
>
> There is an error prone approach esp. when we talk with some functions
> that unifies OF/ACPI driver data retrieval with legacy ID tables.
> In such a case the 0 from enum is hard to distinguish from NULL when
> the driver data is not set or not found. On top of that the simple integer
> in the legacy driver data will require additional code to be added in
> the ->probe().
Use a !0 enum?
The extra handling is expected and normal.
--
Lee Jones [李琼斯]
On Wed, 29 Mar 2023, Lee Jones wrote:
> On Wed, 29 Mar 2023, Andy Shevchenko wrote:
>
> > On Wed, Mar 29, 2023 at 03:36:15PM +0100, Lee Jones wrote:
> > > On Tue, 28 Mar 2023, Sahin, Okan wrote:
> > > > >On Wed, 15 Mar 2023, Lee Jones wrote:
> > > > >> On Tue, 07 Mar 2023, Okan Sahin wrote:
> >
> > ...
> >
> > > > +static const struct i2c_device_id max77541_i2c_id[] = {
> > > > + { "max77540", (kernel_ulong_t)&chip[MAX77540] },
> > > > + { "max77541", (kernel_ulong_t)&chip[MAX77541] },
> > >
> > > Just 'MAX77540' is fine.
> >
> > I tend to disagree.
> >
> > There is an error prone approach esp. when we talk with some functions
> > that unifies OF/ACPI driver data retrieval with legacy ID tables.
> > In such a case the 0 from enum is hard to distinguish from NULL when
> > the driver data is not set or not found. On top of that the simple integer
> > in the legacy driver data will require additional code to be added in
> > the ->probe().
>
> Use a !0 enum?
>
> The extra handling is expected and normal.
I've always disliked mixing platform initialisation strategies. Passing
pointers to MFD structs through I2C/Device Tree registration opens the
doors to all sorts of funky interlaced nonsense.
Pass the device ID and then match in C-code please.
--
Lee Jones [李琼斯]
On Wed, 2023-03-29 at 17:11 +0300, Andy Shevchenko wrote:
> On Wed, Mar 29, 2023 at 05:08:44PM +0300, Andy Shevchenko wrote:
> > On Tue, Mar 28, 2023 at 04:01:21PM +0000, Sahin, Okan wrote:
> > > > On Tue, Mar 28, 2023 at 04:51:18PM +0200, Nuno Sá wrote:
> > > > > On Tue, 2023-03-28 at 17:35 +0300, Andy Shevchenko wrote:
> > > > > > On Tue, Mar 28, 2023 at 04:18:30PM +0200, Nuno Sá wrote:
> > > > > > > On Tue, 2023-03-28 at 14:46 +0100, Mark Brown wrote:
> > > > > > > > On Tue, Mar 28, 2023 at 03:26:44PM +0200, Nuno Sá
> > > > > > > > wrote:
> > > > > > > >
> > > > > > > > > IIRC, regmap_read() is not really reentrant and it is
> > > > > > > > > used in
> > > > > > > > > the IIO driver on the sysfs interface. So, yeah, I
> > > > > > > > > think you
> > > > > > > > > need the regmap lock and better just leave the config
> > > > > > > > > as is.
> > > > > > > > > Yes, the lock is
> > > > > > > > > opt-
> > > > > > > > > out
> > > > > > > > > so let's not disable it :)
> > > > > > > >
> > > > > > > > All the regmap operations are fully thread safe.
> > > > > > >
> > > > > > > Even if 'config->disable_locking' is set? I think that is
> > > > > > > what's
> > > > > > > being discussed in here...
> > > > > >
> > > > > > In case the driver has its own lock to serialize IO how on
> > > > > > earth the
> > > > > > regmap lock is needed. That's what I asked the author of
> > > > > > the driver.
> > > > > > He told the code
> > > > >
> > > > > Well, if the driver has it's own locking, then sure we do not
> > > > > need
> > > > > regmap's lock...
> > > > >
> > > > > > doesn't require the regmap lock, and I tend to believe the
> > > > > > author.
> > > > > > So, why to
> > > > > > keep it?
> > > > >
> > > > > However, if you look at the adc driver, I can see plain
> > > > > regmap_read()
> > > > > calls without any "outside" locking.
> > > >
> > > > Then author of the code should know what they are doing. Right?
> >
> > > Actually, I do not want to disable regmap lock that's why I did
> > > not update it.
> >
> > If you have something like
> >
> > func1()
> > regmap_read(reg1)
> > regmap_read/write(reg2)
> >
I would also add that as soon as you have more than one access to
regmap (or spi, or i2c...) in one func and that function is accessible
through sysfs (and likely any other userspace interface), then you
should already take care. So, it might be even the case that func1()
and func2() don't have to run in parallel.
- Nuno Sá
>
On 29/03/2023 17:06, Lee Jones wrote:
> On Wed, 29 Mar 2023, Lee Jones wrote:
>
>> On Wed, 29 Mar 2023, Andy Shevchenko wrote:
>>
>>> On Wed, Mar 29, 2023 at 03:36:15PM +0100, Lee Jones wrote:
>>>> On Tue, 28 Mar 2023, Sahin, Okan wrote:
>>>>>> On Wed, 15 Mar 2023, Lee Jones wrote:
>>>>>>> On Tue, 07 Mar 2023, Okan Sahin wrote:
>>>
>>> ...
>>>
>>>>> +static const struct i2c_device_id max77541_i2c_id[] = {
>>>>> + { "max77540", (kernel_ulong_t)&chip[MAX77540] },
>>>>> + { "max77541", (kernel_ulong_t)&chip[MAX77541] },
>>>>
>>>> Just 'MAX77540' is fine.
>>>
>>> I tend to disagree.
>>>
>>> There is an error prone approach esp. when we talk with some functions
>>> that unifies OF/ACPI driver data retrieval with legacy ID tables.
>>> In such a case the 0 from enum is hard to distinguish from NULL when
>>> the driver data is not set or not found. On top of that the simple integer
>>> in the legacy driver data will require additional code to be added in
>>> the ->probe().
>>
>> Use a !0 enum?
>>
>> The extra handling is expected and normal.
>
> I've always disliked mixing platform initialisation strategies. Passing
> pointers to MFD structs through I2C/Device Tree registration opens the
> doors to all sorts of funky interlaced nonsense.
>
> Pass the device ID and then match in C-code please.
I agree. Especially that casting through ulong_t drops the const, so the
cast back needs const which can be forgotten. The patch already makes
here mistake!
Best regards,
Krzysztof
On 07/03/2023 12:28, Okan Sahin wrote:
> MFD driver for MAX77541/MAX77540 to enable its sub
> devices.
Please wrap commit message according to Linux coding style / submission
process (neither too early nor over the limit):
https://elixir.bootlin.com/linux/v5.18-rc4/source/Documentation/process/submitting-patches.rst#L586
>
> The MAX77541 is a multi-function devices. It includes
> buck converter and ADC.
>
> The MAX77540 is a high-efficiency buck converter
> with two 3A switching phases.
>
> They have same regmap except for ADC part of MAX77541.
>
> +static int max77541_probe(struct i2c_client *client)
> +{
> + const struct i2c_device_id *id = i2c_client_get_device_id(client);
> + struct device *dev = &client->dev;
> + struct max77541 *max77541;
> +
> + max77541 = devm_kzalloc(dev, sizeof(*max77541), GFP_KERNEL);
> + if (!max77541)
> + return -ENOMEM;
> +
> + i2c_set_clientdata(client, max77541);
> + max77541->i2c = client;
> +
> + max77541->chip = device_get_match_data(dev);
> + if (!max77541->chip)
> + max77541->chip = (struct chip_info *)id->driver_data;
You have odd indentation/coding style before '='. Use Linux coding style.
Best regards,
Krzysztof
On 30/03/2023 10:04, Krzysztof Kozlowski wrote:
> On 29/03/2023 17:06, Lee Jones wrote:
>> On Wed, 29 Mar 2023, Lee Jones wrote:
>>
>>> On Wed, 29 Mar 2023, Andy Shevchenko wrote:
>>>
>>>> On Wed, Mar 29, 2023 at 03:36:15PM +0100, Lee Jones wrote:
>>>>> On Tue, 28 Mar 2023, Sahin, Okan wrote:
>>>>>>> On Wed, 15 Mar 2023, Lee Jones wrote:
>>>>>>>> On Tue, 07 Mar 2023, Okan Sahin wrote:
>>>>
>>>> ...
>>>>
>>>>>> +static const struct i2c_device_id max77541_i2c_id[] = {
>>>>>> + { "max77540", (kernel_ulong_t)&chip[MAX77540] },
>>>>>> + { "max77541", (kernel_ulong_t)&chip[MAX77541] },
>>>>>
>>>>> Just 'MAX77540' is fine.
>>>>
>>>> I tend to disagree.
>>>>
>>>> There is an error prone approach esp. when we talk with some functions
>>>> that unifies OF/ACPI driver data retrieval with legacy ID tables.
>>>> In such a case the 0 from enum is hard to distinguish from NULL when
>>>> the driver data is not set or not found. On top of that the simple integer
>>>> in the legacy driver data will require additional code to be added in
>>>> the ->probe().
>>>
>>> Use a !0 enum?
>>>
>>> The extra handling is expected and normal.
>>
>> I've always disliked mixing platform initialisation strategies. Passing
>> pointers to MFD structs through I2C/Device Tree registration opens the
>> doors to all sorts of funky interlaced nonsense.
>>
>> Pass the device ID and then match in C-code please.
>
> I agree. Especially that casting through ulong_t drops the const, so the
> cast back needs const which can be forgotten. The patch already makes
> here mistake!
Uh, no, the code is correct - chip_info member is const. Yet it is a
mistake easy to make for the device ID tables using void * or ulong.
Best regards,
Krzysztof
>On Tue, 28 Mar 2023, Sahin, Okan wrote:
>
>> >On Wed, 15 Mar 2023, Lee Jones wrote:
>> >
>> >> On Tue, 07 Mar 2023, Okan Sahin wrote:
>> >>
>> >> > MFD driver for MAX77541/MAX77540 to enable its sub devices.
>> >> >
>> >> > The MAX77541 is a multi-function devices. It includes buck
>> >> > converter and ADC.
>> >> >
>> >> > The MAX77540 is a high-efficiency buck converter with two 3A
>> >> > switching phases.
>> >> >
>> >> > They have same regmap except for ADC part of MAX77541.
>> >> >
>> >> > Signed-off-by: Okan Sahin <okan.sahin@analog.com>
>> >> > ---
>> >> > drivers/mfd/Kconfig | 13 ++
>> >> > drivers/mfd/Makefile | 1 +
>> >> > drivers/mfd/max77541.c | 224
>> >+++++++++++++++++++++++++++++++++++
>> >> > include/linux/mfd/max77541.h | 97 +++++++++++++++
>> >> > 4 files changed, 335 insertions(+) create mode 100644
>> >> > drivers/mfd/max77541.c create mode 100644
>> >> > include/linux/mfd/max77541.h
>> >>
>> >> FYI: I'm not re-reviewing this since you've chosen to ignore some
>> >> of my previous review comments. Issues highlighted by review
>> >> comments don't just go away on resubmission.
>> >
>> >... and the subject is malformed.
>> >
>> >--
>> >Lee Jones [李琼斯]
>>
>> Hi Lee,
>>
>> I am sorry if I missed your review comments, this was not my intention. I want
>to thank you for your contribution. Your feedbacks are very valuable, and I am
>trying to understand and fix each one before sending the patch. Indeed, I sorted
>your feedback on previous patches. As far as I know, I have fixed all of them, is
>there a problem with any of them that I fixed, or is there any missing review?
>From you, there were some comments like "why did you use this?", I suppose I
>need to respond them before sending following patches. I thought I should not
>bother the maintainers unnecessarily. I am sorry for them.
>
>Please ask your email client to line-wrap.
>
>Here is the part of the review you ignored:
>
>[...]
>
>> +static const struct chip_info chip[] = {
>
>Why do you need this require sub-structure?
>
>> + [MAX77540] = {
>> + .id = MAX77540,
>> + .n_devs = ARRAY_SIZE(max77540_devs),
>> + .devs = max77540_devs,
>> + },
>> + [MAX77541] = {
>> + .id = MAX77541,
>> + .n_devs = ARRAY_SIZE(max77541_devs),
>> + .devs = max77541_devs,
>> + },
>> +};
>
>[...]
>
>> +static const struct of_device_id max77541_of_id[] = {
>> + {
>> + .compatible = "adi,max77540",
>> + .data = &chip[MAX77540],
>> + },
>> + {
>> + .compatible = "adi,max77541",
>> + .data = &chip[MAX77541],
>> + },
>> + { /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, max77541_of_id);
>> +
>> +static const struct i2c_device_id max77541_i2c_id[] = {
>> + { "max77540", (kernel_ulong_t)&chip[MAX77540] },
>> + { "max77541", (kernel_ulong_t)&chip[MAX77541] },
>
>Just 'MAX77540' is fine.
>
>> + { /* sentinel */ }
>
>Remove the comment, we know how terminators work.
>
>Same comments for max77541_of_id.
>
>--
>Lee Jones [李琼斯]
Hi Lee,
In fact, one of the maintainers suggested assigning chip_info to data instead of enumeration. Then I added chip_info and put devices into sub-structure above. I will replace chip_info with id structure in max77541 device structure, right? I will use enumeration for data as I will assign it to id, and distinguish different devices.
Regards
Okan Sahin
On Mon, 03 Apr 2023, Sahin, Okan wrote:
> >On Tue, 28 Mar 2023, Sahin, Okan wrote:
> >
> >> >On Wed, 15 Mar 2023, Lee Jones wrote:
> >> >
> >> >> On Tue, 07 Mar 2023, Okan Sahin wrote:
> >> >>
> >> >> > MFD driver for MAX77541/MAX77540 to enable its sub devices.
> >> >> >
> >> >> > The MAX77541 is a multi-function devices. It includes buck
> >> >> > converter and ADC.
> >> >> >
> >> >> > The MAX77540 is a high-efficiency buck converter with two 3A
> >> >> > switching phases.
> >> >> >
> >> >> > They have same regmap except for ADC part of MAX77541.
> >> >> >
> >> >> > Signed-off-by: Okan Sahin <okan.sahin@analog.com>
> >> >> > ---
> >> >> > drivers/mfd/Kconfig | 13 ++
> >> >> > drivers/mfd/Makefile | 1 +
> >> >> > drivers/mfd/max77541.c | 224
> >> >+++++++++++++++++++++++++++++++++++
> >> >> > include/linux/mfd/max77541.h | 97 +++++++++++++++
> >> >> > 4 files changed, 335 insertions(+) create mode 100644
> >> >> > drivers/mfd/max77541.c create mode 100644
> >> >> > include/linux/mfd/max77541.h
> >> >>
> >> >> FYI: I'm not re-reviewing this since you've chosen to ignore some
> >> >> of my previous review comments. Issues highlighted by review
> >> >> comments don't just go away on resubmission.
> >> >
> >> >... and the subject is malformed.
> >> >
> >> >--
> >> >Lee Jones [李琼斯]
> >>
> >> Hi Lee,
> >>
> >> I am sorry if I missed your review comments, this was not my intention. I want
> >to thank you for your contribution. Your feedbacks are very valuable, and I am
> >trying to understand and fix each one before sending the patch. Indeed, I sorted
> >your feedback on previous patches. As far as I know, I have fixed all of them, is
> >there a problem with any of them that I fixed, or is there any missing review?
> >From you, there were some comments like "why did you use this?", I suppose I
> >need to respond them before sending following patches. I thought I should not
> >bother the maintainers unnecessarily. I am sorry for them.
> >
> >Please ask your email client to line-wrap.
> >
> >Here is the part of the review you ignored:
> >
> >[...]
> >
> >> +static const struct chip_info chip[] = {
> >
> >Why do you need this require sub-structure?
> >
> >> + [MAX77540] = {
> >> + .id = MAX77540,
> >> + .n_devs = ARRAY_SIZE(max77540_devs),
> >> + .devs = max77540_devs,
> >> + },
> >> + [MAX77541] = {
> >> + .id = MAX77541,
> >> + .n_devs = ARRAY_SIZE(max77541_devs),
> >> + .devs = max77541_devs,
> >> + },
> >> +};
> >
> >[...]
> >
> >> +static const struct of_device_id max77541_of_id[] = {
> >> + {
> >> + .compatible = "adi,max77540",
> >> + .data = &chip[MAX77540],
> >> + },
> >> + {
> >> + .compatible = "adi,max77541",
> >> + .data = &chip[MAX77541],
> >> + },
> >> + { /* sentinel */ }
> >> +};
> >> +MODULE_DEVICE_TABLE(of, max77541_of_id);
> >> +
> >> +static const struct i2c_device_id max77541_i2c_id[] = {
> >> + { "max77540", (kernel_ulong_t)&chip[MAX77540] },
> >> + { "max77541", (kernel_ulong_t)&chip[MAX77541] },
> >
> >Just 'MAX77540' is fine.
> >
> >> + { /* sentinel */ }
> >
> >Remove the comment, we know how terminators work.
> >
> >Same comments for max77541_of_id.
Your mailer is still broken. Please line wrap.
> In fact, one of the maintainers suggested assigning chip_info to data instead of enumeration. Then I added chip_info and put devices into sub-structure above. I will replace chip_info with id structure in max77541 device structure, right? I will use enumeration for data as I will assign it to id, and distinguish different devices.
Yes, that's correct. Please remove chip_info altogether.
--
Lee Jones [李琼斯]
On Mon, Apr 03, 2023 at 03:09:50PM +0100, Lee Jones wrote:
> On Mon, 03 Apr 2023, Sahin, Okan wrote:
...
> > In fact, one of the maintainers suggested assigning chip_info to data
> > instead of enumeration. Then I added chip_info and put devices into
> > sub-structure above. I will replace chip_info with id structure in max77541
> > device structure, right? I will use enumeration for data as I will assign
> > it to id, and distinguish different devices.
>
> Yes, that's correct. Please remove chip_info altogether.
Then it will provoke casting in the OF ID table which I believe is not what
we want. I would agree on your first suggestion to have a plain number in I²C
ID table, but I'm against it in OF and/or ACPI ID table.
On Wed, 05 Apr 2023, Andy Shevchenko wrote:
> On Mon, Apr 03, 2023 at 03:09:50PM +0100, Lee Jones wrote:
> > On Mon, 03 Apr 2023, Sahin, Okan wrote:
>
> ...
>
>
> > > In fact, one of the maintainers suggested assigning chip_info to data
> > > instead of enumeration. Then I added chip_info and put devices into
> > > sub-structure above. I will replace chip_info with id structure in max77541
> > > device structure, right? I will use enumeration for data as I will assign
> > > it to id, and distinguish different devices.
> >
> > Yes, that's correct. Please remove chip_info altogether.
>
> Then it will provoke casting in the OF ID table which I believe is not what
> we want. I would agree on your first suggestion to have a plain number in I²C
> ID table, but I'm against it in OF and/or ACPI ID table.
And I'm against passing MFD information through the OF/ACPI APIs.
You can put through raw platform data or a device descriptor.
Ref: git grep -A5 "struct of_device_id.*{" -- drivers/mfd
--
Lee Jones [李琼斯]
>On Wed, 05 Apr 2023, Andy Shevchenko wrote:
>
>> On Mon, Apr 03, 2023 at 03:09:50PM +0100, Lee Jones wrote:
>> > On Mon, 03 Apr 2023, Sahin, Okan wrote:
>>
>> ...
>>
>>
>> > > In fact, one of the maintainers suggested assigning chip_info to data
>> > > instead of enumeration. Then I added chip_info and put devices into
>> > > sub-structure above. I will replace chip_info with id structure in max77541
>> > > device structure, right? I will use enumeration for data as I will assign
>> > > it to id, and distinguish different devices.
>> >
>> > Yes, that's correct. Please remove chip_info altogether.
>>
>> Then it will provoke casting in the OF ID table which I believe is not what
>> we want. I would agree on your first suggestion to have a plain number in I²C
>> ID table, but I'm against it in OF and/or ACPI ID table.
>
>And I'm against passing MFD information through the OF/ACPI APIs.
>
>You can put through raw platform data or a device descriptor.
>
>Ref: git grep -A5 "struct of_device_id.*{" -- drivers/mfd
>
>--
>Lee Jones [李琼斯]
Hi Lee,
Right now, as you suggested I rewrote code like below
For of_device_id,
. data = (void *)MAX77540,
.data = (void *)MAX77541,
For i2c_device_id,
.data = MAX77540,
.data = MAX77541
I also rewrote other part as chip_info is excluded. I want to be sure before
sending new patch.
Does it seem correct?
Regards,
Okan Sahin
On Sun, 09 Apr 2023, Sahin, Okan wrote:
> >On Wed, 05 Apr 2023, Andy Shevchenko wrote:
> >
> >> On Mon, Apr 03, 2023 at 03:09:50PM +0100, Lee Jones wrote:
> >> > On Mon, 03 Apr 2023, Sahin, Okan wrote:
> >>
> >> ...
> >>
> >>
> >> > > In fact, one of the maintainers suggested assigning chip_info to data
> >> > > instead of enumeration. Then I added chip_info and put devices into
> >> > > sub-structure above. I will replace chip_info with id structure in max77541
> >> > > device structure, right? I will use enumeration for data as I will assign
> >> > > it to id, and distinguish different devices.
> >> >
> >> > Yes, that's correct. Please remove chip_info altogether.
> >>
> >> Then it will provoke casting in the OF ID table which I believe is not what
> >> we want. I would agree on your first suggestion to have a plain number in I²C
> >> ID table, but I'm against it in OF and/or ACPI ID table.
> >
> >And I'm against passing MFD information through the OF/ACPI APIs.
> >
> >You can put through raw platform data or a device descriptor.
> >
> >Ref: git grep -A5 "struct of_device_id.*{" -- drivers/mfd
> >
> >--
> >Lee Jones [李琼斯]
>
> Hi Lee,
>
> Right now, as you suggested I rewrote code like below
> For of_device_id,
> . data = (void *)MAX77540,
> .data = (void *)MAX77541,
> For i2c_device_id,
> .data = MAX77540,
> .data = MAX77541
> I also rewrote other part as chip_info is excluded. I want to be sure before
> sending new patch.
>
> Does it seem correct?
This is one suitable method, yes.
--
Lee Jones [李琼斯]
>On Sun, 09 Apr 2023, Sahin, Okan wrote:
>
>> >On Wed, 05 Apr 2023, Andy Shevchenko wrote:
>> >
>> >> On Mon, Apr 03, 2023 at 03:09:50PM +0100, Lee Jones wrote:
>> >> > On Mon, 03 Apr 2023, Sahin, Okan wrote:
>> >>
>> >> ...
>> >>
>> >>
>> >> > > In fact, one of the maintainers suggested assigning chip_info to data
>> >> > > instead of enumeration. Then I added chip_info and put devices into
>> >> > > sub-structure above. I will replace chip_info with id structure in max77541
>> >> > > device structure, right? I will use enumeration for data as I will assign
>> >> > > it to id, and distinguish different devices.
>> >> >
>> >> > Yes, that's correct. Please remove chip_info altogether.
>> >>
>> >> Then it will provoke casting in the OF ID table which I believe is not what
>> >> we want. I would agree on your first suggestion to have a plain number in I²C
>> >> ID table, but I'm against it in OF and/or ACPI ID table.
>> >
>> >And I'm against passing MFD information through the OF/ACPI APIs.
>> >
>> >You can put through raw platform data or a device descriptor.
>> >
>> >Ref: git grep -A5 "struct of_device_id.*{" -- drivers/mfd
>> >
>> >--
>> >Lee Jones [李琼斯]
>>
>> Hi Lee,
>>
>> Right now, as you suggested I rewrote code like below
>> For of_device_id,
>> . data = (void *)MAX77540,
>> .data = (void *)MAX77541,
>> For i2c_device_id,
>> .data = MAX77540,
>> .data = MAX77541
>> I also rewrote other part as chip_info is excluded. I want to be sure before
>> sending new patch.
>>
>> Does it seem correct?
>
>This is one suitable method, yes.
>
>--
>Lee Jones [李琼斯]
Hi Lee,
Thank you for your support.
Regards,
Okan Sahin
@@ -777,6 +777,19 @@ config MFD_MAX14577
additional drivers must be enabled in order to use the functionality
of the device.
+config MFD_MAX77541
+ tristate "Analog Devices MAX77541/77540 PMIC Support"
+ depends on I2C=y
+ select MFD_CORE
+ select REGMAP_I2C
+ select REGMAP_IRQ
+ help
+ Say yes here to add support for Analog Devices MAX77541 and
+ MAX77540 Power Management ICs. This driver provides
+ common support for accessing the device; additional drivers
+ must be enabled in order to use the functionality of the device.
+ There are regulators and adc.
+
config MFD_MAX77620
bool "Maxim Semiconductor MAX77620 and MAX20024 PMIC Support"
depends on I2C=y
@@ -151,6 +151,7 @@ obj-$(CONFIG_MFD_DA9063) += da9063.o
obj-$(CONFIG_MFD_DA9150) += da9150-core.o
obj-$(CONFIG_MFD_MAX14577) += max14577.o
+obj-$(CONFIG_MFD_MAX77541) += max77541.o
obj-$(CONFIG_MFD_MAX77620) += max77620.o
obj-$(CONFIG_MFD_MAX77650) += max77650.o
obj-$(CONFIG_MFD_MAX77686) += max77686.o
new file mode 100644
@@ -0,0 +1,224 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2022 Analog Devices, Inc.
+ * Driver for the MAX77540 and MAX77541
+ */
+
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/max77541.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+
+static const struct regmap_config max77541_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+};
+
+static const struct regmap_irq max77541_src_irqs[] = {
+ { .mask = MAX77541_BIT_INT_SRC_TOPSYS },
+ { .mask = MAX77541_BIT_INT_SRC_BUCK },
+};
+
+static const struct regmap_irq_chip max77541_src_irq_chip = {
+ .name = "max77541-src",
+ .status_base = MAX77541_REG_INT_SRC,
+ .mask_base = MAX77541_REG_INT_SRC_M,
+ .num_regs = 1,
+ .irqs = max77541_src_irqs,
+ .num_irqs = ARRAY_SIZE(max77541_src_irqs),
+};
+
+static const struct regmap_irq max77541_topsys_irqs[] = {
+ { .mask = MAX77541_BIT_TOPSYS_INT_TJ_120C },
+ { .mask = MAX77541_BIT_TOPSYS_INT_TJ_140C },
+ { .mask = MAX77541_BIT_TOPSYS_INT_TSHDN },
+ { .mask = MAX77541_BIT_TOPSYS_INT_UVLO },
+ { .mask = MAX77541_BIT_TOPSYS_INT_ALT_SWO },
+ { .mask = MAX77541_BIT_TOPSYS_INT_EXT_FREQ_DET },
+};
+
+static const struct regmap_irq_chip max77541_topsys_irq_chip = {
+ .name = "max77541-topsys",
+ .status_base = MAX77541_REG_TOPSYS_INT,
+ .mask_base = MAX77541_REG_TOPSYS_INT_M,
+ .num_regs = 1,
+ .irqs = max77541_topsys_irqs,
+ .num_irqs = ARRAY_SIZE(max77541_topsys_irqs),
+};
+
+static const struct regmap_irq max77541_buck_irqs[] = {
+ { .mask = MAX77541_BIT_BUCK_INT_M1_POK_FLT },
+ { .mask = MAX77541_BIT_BUCK_INT_M2_POK_FLT },
+ { .mask = MAX77541_BIT_BUCK_INT_M1_SCFLT },
+ { .mask = MAX77541_BIT_BUCK_INT_M2_SCFLT },
+};
+
+static const struct regmap_irq_chip max77541_buck_irq_chip = {
+ .name = "max77541-buck",
+ .status_base = MAX77541_REG_BUCK_INT,
+ .mask_base = MAX77541_REG_BUCK_INT_M,
+ .num_regs = 1,
+ .irqs = max77541_buck_irqs,
+ .num_irqs = ARRAY_SIZE(max77541_buck_irqs),
+};
+
+static const struct regmap_irq max77541_adc_irqs[] = {
+ { .mask = MAX77541_BIT_ADC_INT_CH1_I },
+ { .mask = MAX77541_BIT_ADC_INT_CH2_I },
+ { .mask = MAX77541_BIT_ADC_INT_CH3_I },
+ { .mask = MAX77541_BIT_ADC_INT_CH6_I },
+};
+
+static const struct regmap_irq_chip max77541_adc_irq_chip = {
+ .name = "max77541-adc",
+ .status_base = MAX77541_REG_ADC_INT,
+ .mask_base = MAX77541_REG_ADC_INT_M,
+ .num_regs = 1,
+ .irqs = max77541_adc_irqs,
+ .num_irqs = ARRAY_SIZE(max77541_adc_irqs),
+};
+
+static const struct mfd_cell max77540_devs[] = {
+ MFD_CELL_OF("max77540-regulator", NULL, NULL, 0, 0, NULL),
+};
+
+static const struct mfd_cell max77541_devs[] = {
+ MFD_CELL_OF("max77541-regulator", NULL, NULL, 0, 0, NULL),
+ MFD_CELL_OF("max77541-adc", NULL, NULL, 0, 0, NULL),
+};
+
+static const struct chip_info chip[] = {
+ [MAX77540] = {
+ .id = MAX77540,
+ .n_devs = ARRAY_SIZE(max77540_devs),
+ .devs = max77540_devs,
+ },
+ [MAX77541] = {
+ .id = MAX77541,
+ .n_devs = ARRAY_SIZE(max77541_devs),
+ .devs = max77541_devs,
+ },
+};
+
+static int max77541_pmic_irq_init(struct device *dev)
+{
+ struct max77541 *max77541 = dev_get_drvdata(dev);
+ int irq = max77541->i2c->irq;
+ int ret;
+
+ ret = devm_regmap_add_irq_chip(dev, max77541->regmap, irq,
+ IRQF_ONESHOT | IRQF_SHARED, 0,
+ &max77541_src_irq_chip,
+ &max77541->irq_data);
+ if (ret)
+ return ret;
+
+ ret = devm_regmap_add_irq_chip(dev, max77541->regmap, irq,
+ IRQF_ONESHOT | IRQF_SHARED, 0,
+ &max77541_topsys_irq_chip,
+ &max77541->irq_topsys);
+ if (ret)
+ return ret;
+
+ ret = devm_regmap_add_irq_chip(dev, max77541->regmap, irq,
+ IRQF_ONESHOT | IRQF_SHARED, 0,
+ &max77541_buck_irq_chip,
+ &max77541->irq_buck);
+ if (ret)
+ return ret;
+
+ if (max77541->chip->id == MAX77541) {
+ ret = devm_regmap_add_irq_chip(dev, max77541->regmap, irq,
+ IRQF_ONESHOT | IRQF_SHARED, 0,
+ &max77541_adc_irq_chip,
+ &max77541->irq_adc);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
+static int max77541_pmic_setup(struct device *dev)
+{
+ struct max77541 *max77541 = dev_get_drvdata(dev);
+ int ret;
+
+ ret = max77541_pmic_irq_init(dev);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to initialize IRQ\n");
+
+ ret = device_init_wakeup(dev, true);
+ if (ret)
+ return dev_err_probe(dev, ret, "Unable to init wakeup\n");
+
+ return devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE,
+ max77541->chip->devs,
+ max77541->chip->n_devs,
+ NULL, 0, NULL);
+}
+
+static int max77541_probe(struct i2c_client *client)
+{
+ const struct i2c_device_id *id = i2c_client_get_device_id(client);
+ struct device *dev = &client->dev;
+ struct max77541 *max77541;
+
+ max77541 = devm_kzalloc(dev, sizeof(*max77541), GFP_KERNEL);
+ if (!max77541)
+ return -ENOMEM;
+
+ i2c_set_clientdata(client, max77541);
+ max77541->i2c = client;
+
+ max77541->chip = device_get_match_data(dev);
+ if (!max77541->chip)
+ max77541->chip = (struct chip_info *)id->driver_data;
+
+ if (!max77541->chip)
+ return -EINVAL;
+
+ max77541->regmap = devm_regmap_init_i2c(client,
+ &max77541_regmap_config);
+ if (IS_ERR(max77541->regmap))
+ return dev_err_probe(dev, PTR_ERR(max77541->regmap),
+ "Failed to allocate register map\n");
+
+ return max77541_pmic_setup(dev);
+}
+
+static const struct of_device_id max77541_of_id[] = {
+ {
+ .compatible = "adi,max77540",
+ .data = &chip[MAX77540],
+ },
+ {
+ .compatible = "adi,max77541",
+ .data = &chip[MAX77541],
+ },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, max77541_of_id);
+
+static const struct i2c_device_id max77541_id[] = {
+ { "max77540", (kernel_ulong_t)&chip[MAX77540] },
+ { "max77541", (kernel_ulong_t)&chip[MAX77541] },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, max77541_id);
+
+static struct i2c_driver max77541_driver = {
+ .driver = {
+ .name = "max77541",
+ .of_match_table = max77541_of_id,
+ },
+ .probe_new = max77541_probe,
+ .id_table = max77541_id,
+};
+module_i2c_driver(max77541_driver);
+
+MODULE_DESCRIPTION("MAX7740/MAX7741 Driver");
+MODULE_AUTHOR("Okan Sahin <okan.sahin@analog.com>");
+MODULE_LICENSE("GPL");
new file mode 100644
@@ -0,0 +1,97 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#ifndef __MFD_MAX77541_H
+#define __MFD_MAX77541_H
+
+#include <linux/bits.h>
+#include <linux/types.h>
+
+/* REGISTERS */
+#define MAX77541_REG_INT_SRC 0x00
+#define MAX77541_REG_INT_SRC_M 0x01
+
+#define MAX77541_BIT_INT_SRC_TOPSYS BIT(0)
+#define MAX77541_BIT_INT_SRC_BUCK BIT(1)
+
+#define MAX77541_REG_TOPSYS_INT 0x02
+#define MAX77541_REG_TOPSYS_INT_M 0x03
+
+#define MAX77541_BIT_TOPSYS_INT_TJ_120C BIT(0)
+#define MAX77541_BIT_TOPSYS_INT_TJ_140C BIT(1)
+#define MAX77541_BIT_TOPSYS_INT_TSHDN BIT(2)
+#define MAX77541_BIT_TOPSYS_INT_UVLO BIT(3)
+#define MAX77541_BIT_TOPSYS_INT_ALT_SWO BIT(4)
+#define MAX77541_BIT_TOPSYS_INT_EXT_FREQ_DET BIT(5)
+
+/* REGULATORS */
+#define MAX77541_REG_BUCK_INT 0x20
+#define MAX77541_REG_BUCK_INT_M 0x21
+
+#define MAX77541_BIT_BUCK_INT_M1_POK_FLT BIT(0)
+#define MAX77541_BIT_BUCK_INT_M2_POK_FLT BIT(1)
+#define MAX77541_BIT_BUCK_INT_M1_SCFLT BIT(4)
+#define MAX77541_BIT_BUCK_INT_M2_SCFLT BIT(5)
+
+#define MAX77541_REG_EN_CTRL 0x0B
+
+#define MAX77541_BIT_M1_EN BIT(0)
+#define MAX77541_BIT_M2_EN BIT(1)
+
+#define MAX77541_REG_M1_VOUT 0x23
+#define MAX77541_REG_M2_VOUT 0x33
+
+#define MAX77541_BITS_MX_VOUT GENMASK(7, 0)
+
+#define MAX77541_REG_M1_CFG1 0x25
+#define MAX77541_REG_M2_CFG1 0x35
+
+#define MAX77541_BITS_MX_CFG1_RNG GENMASK(7, 6)
+
+/* ADC */
+#define MAX77541_REG_ADC_INT 0x70
+#define MAX77541_REG_ADC_INT_M 0x71
+
+#define MAX77541_BIT_ADC_INT_CH1_I BIT(0)
+#define MAX77541_BIT_ADC_INT_CH2_I BIT(1)
+#define MAX77541_BIT_ADC_INT_CH3_I BIT(2)
+#define MAX77541_BIT_ADC_INT_CH6_I BIT(5)
+
+#define MAX77541_REG_ADC_DATA_CH1 0x72
+#define MAX77541_REG_ADC_DATA_CH2 0x73
+#define MAX77541_REG_ADC_DATA_CH3 0x74
+#define MAX77541_REG_ADC_DATA_CH6 0x77
+
+/* INTERRUPT MASKS*/
+#define MAX77541_REG_INT_SRC_MASK 0x00
+#define MAX77541_REG_TOPSYS_INT_MASK 0x00
+#define MAX77541_REG_BUCK_INT_MASK 0x00
+
+#define MAX77541_MAX_REGULATORS 2
+
+enum max7754x_ids {
+ MAX77540,
+ MAX77541,
+};
+
+struct chip_info {
+ enum max7754x_ids id;
+ int n_devs;
+ const struct mfd_cell *devs;
+};
+
+struct regmap;
+struct regmap_irq_chip_data;
+struct i2c_client;
+
+struct max77541 {
+ struct i2c_client *i2c;
+ struct regmap *regmap;
+ const struct chip_info *chip;
+
+ struct regmap_irq_chip_data *irq_data;
+ struct regmap_irq_chip_data *irq_buck;
+ struct regmap_irq_chip_data *irq_topsys;
+ struct regmap_irq_chip_data *irq_adc;
+};
+
+#endif /* __MFD_MAX77541_H */