diff mbox series

[v1] max77663-rtc: pass rtc address from device tree node if exists

Message ID 20230308083759.11692-1-clamor95@gmail.com
State New
Headers
Series [v1] max77663-rtc: pass rtc address from device tree node if exists |

Commit Message

Svyatoslav Ryhel March 8, 2023, 8:37 a.m. UTC 
  MAX77663 PMIC can have RTC on both 0x63 i2c address (like grouper)
which is main address but on some devices RTC is located on 0x48
i2c address (like p880 and p895 from LG). Lets add property to be
able to use alternative address if needed without breaking existing
bindings.

Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
 drivers/rtc/rtc-max77686.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Alexandre Belloni March 8, 2023, 8:44 a.m. UTC | #1 
Hello,

the subject is not correct, please use the subsystem style

On 08/03/2023 10:37:59+0200, Svyatoslav Ryhel wrote:
> MAX77663 PMIC can have RTC on both 0x63 i2c address (like grouper)
> which is main address but on some devices RTC is located on 0x48
> i2c address (like p880 and p895 from LG). Lets add property to be
> able to use alternative address if needed without breaking existing
> bindings.
> 
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> ---
>  drivers/rtc/rtc-max77686.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
> index b0250d91fb00..218177375531 100644
> --- a/drivers/rtc/rtc-max77686.c
> +++ b/drivers/rtc/rtc-max77686.c
> @@ -691,6 +691,7 @@ static int max77686_init_rtc_regmap(struct max77686_rtc_info *info)
>  {
>  	struct device *parent = info->dev->parent;
>  	struct i2c_client *parent_i2c = to_i2c_client(parent);
> +	int rtc_i2c_addr;
>  	int ret;
>  
>  	if (info->drv_data->rtc_irq_from_platform) {
> @@ -714,8 +715,13 @@ static int max77686_init_rtc_regmap(struct max77686_rtc_info *info)
>  		goto add_rtc_irq;
>  	}
>  
> +	ret = device_property_read_u32(parent, "maxim,rtc-i2c-address",
> +				       &rtc_i2c_addr);

This property needs to be documented

> +	if (ret)
> +		rtc_i2c_addr = info->drv_data->rtc_i2c_addr;
> +
>  	info->rtc = devm_i2c_new_dummy_device(info->dev, parent_i2c->adapter,
> -					      info->drv_data->rtc_i2c_addr);
> +					      rtc_i2c_addr);
>  	if (IS_ERR(info->rtc)) {
>  		dev_err(info->dev, "Failed to allocate I2C device for RTC\n");
>  		return PTR_ERR(info->rtc);
> -- 
> 2.37.2
>
Svyatoslav Ryhel March 8, 2023, 8:46 a.m. UTC | #2 
ср, 8 бер. 2023 р. о 10:44 Alexandre Belloni
<alexandre.belloni@bootlin.com> пише:
>
> Hello,
>
> the subject is not correct, please use the subsystem style
>
> On 08/03/2023 10:37:59+0200, Svyatoslav Ryhel wrote:
> > MAX77663 PMIC can have RTC on both 0x63 i2c address (like grouper)
> > which is main address but on some devices RTC is located on 0x48
> > i2c address (like p880 and p895 from LG). Lets add property to be
> > able to use alternative address if needed without breaking existing
> > bindings.
> >
> > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > ---
> >  drivers/rtc/rtc-max77686.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
> > index b0250d91fb00..218177375531 100644
> > --- a/drivers/rtc/rtc-max77686.c
> > +++ b/drivers/rtc/rtc-max77686.c
> > @@ -691,6 +691,7 @@ static int max77686_init_rtc_regmap(struct max77686_rtc_info *info)
> >  {
> >       struct device *parent = info->dev->parent;
> >       struct i2c_client *parent_i2c = to_i2c_client(parent);
> > +     int rtc_i2c_addr;
> >       int ret;
> >
> >       if (info->drv_data->rtc_irq_from_platform) {
> > @@ -714,8 +715,13 @@ static int max77686_init_rtc_regmap(struct max77686_rtc_info *info)
> >               goto add_rtc_irq;
> >       }
> >
> > +     ret = device_property_read_u32(parent, "maxim,rtc-i2c-address",
> > +                                    &rtc_i2c_addr);
>
> This property needs to be documented
>

max776xx pmic has no yaml to document

> > +     if (ret)
> > +             rtc_i2c_addr = info->drv_data->rtc_i2c_addr;
> > +
> >       info->rtc = devm_i2c_new_dummy_device(info->dev, parent_i2c->adapter,
> > -                                           info->drv_data->rtc_i2c_addr);
> > +                                           rtc_i2c_addr);
> >       if (IS_ERR(info->rtc)) {
> >               dev_err(info->dev, "Failed to allocate I2C device for RTC\n");
> >               return PTR_ERR(info->rtc);
> > --
> > 2.37.2
> >
>
> --
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
Krzysztof Kozlowski March 8, 2023, 8:49 a.m. UTC | #3 
On 08/03/2023 09:46, Svyatoslav Ryhel wrote:
> ср, 8 бер. 2023 р. о 10:44 Alexandre Belloni
> <alexandre.belloni@bootlin.com> пише:
>>
>> Hello,
>>
>> the subject is not correct, please use the subsystem style
>>
>> On 08/03/2023 10:37:59+0200, Svyatoslav Ryhel wrote:
>>> MAX77663 PMIC can have RTC on both 0x63 i2c address (like grouper)
>>> which is main address but on some devices RTC is located on 0x48
>>> i2c address (like p880 and p895 from LG). Lets add property to be
>>> able to use alternative address if needed without breaking existing
>>> bindings.
>>>
>>> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
>>> ---
>>>  drivers/rtc/rtc-max77686.c | 8 +++++++-
>>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
>>> index b0250d91fb00..218177375531 100644
>>> --- a/drivers/rtc/rtc-max77686.c
>>> +++ b/drivers/rtc/rtc-max77686.c
>>> @@ -691,6 +691,7 @@ static int max77686_init_rtc_regmap(struct max77686_rtc_info *info)
>>>  {
>>>       struct device *parent = info->dev->parent;
>>>       struct i2c_client *parent_i2c = to_i2c_client(parent);
>>> +     int rtc_i2c_addr;
>>>       int ret;
>>>
>>>       if (info->drv_data->rtc_irq_from_platform) {
>>> @@ -714,8 +715,13 @@ static int max77686_init_rtc_regmap(struct max77686_rtc_info *info)
>>>               goto add_rtc_irq;
>>>       }
>>>
>>> +     ret = device_property_read_u32(parent, "maxim,rtc-i2c-address",
>>> +                                    &rtc_i2c_addr);
>>
>> This property needs to be documented
>>
> 
> max776xx pmic has no yaml to document

Of course it has, otherwise on what did I work?

git grep max77686


Best regards,
Krzysztof
Svyatoslav Ryhel March 8, 2023, 8:51 a.m. UTC | #4 
Ooof, my bad. I will resend v2 with fix asap.

ср, 8 бер. 2023 р. о 10:49 Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> пише:
>
> On 08/03/2023 09:46, Svyatoslav Ryhel wrote:
> > ср, 8 бер. 2023 р. о 10:44 Alexandre Belloni
> > <alexandre.belloni@bootlin.com> пише:
> >>
> >> Hello,
> >>
> >> the subject is not correct, please use the subsystem style
> >>
> >> On 08/03/2023 10:37:59+0200, Svyatoslav Ryhel wrote:
> >>> MAX77663 PMIC can have RTC on both 0x63 i2c address (like grouper)
> >>> which is main address but on some devices RTC is located on 0x48
> >>> i2c address (like p880 and p895 from LG). Lets add property to be
> >>> able to use alternative address if needed without breaking existing
> >>> bindings.
> >>>
> >>> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> >>> ---
> >>>  drivers/rtc/rtc-max77686.c | 8 +++++++-
> >>>  1 file changed, 7 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
> >>> index b0250d91fb00..218177375531 100644
> >>> --- a/drivers/rtc/rtc-max77686.c
> >>> +++ b/drivers/rtc/rtc-max77686.c
> >>> @@ -691,6 +691,7 @@ static int max77686_init_rtc_regmap(struct max77686_rtc_info *info)
> >>>  {
> >>>       struct device *parent = info->dev->parent;
> >>>       struct i2c_client *parent_i2c = to_i2c_client(parent);
> >>> +     int rtc_i2c_addr;
> >>>       int ret;
> >>>
> >>>       if (info->drv_data->rtc_irq_from_platform) {
> >>> @@ -714,8 +715,13 @@ static int max77686_init_rtc_regmap(struct max77686_rtc_info *info)
> >>>               goto add_rtc_irq;
> >>>       }
> >>>
> >>> +     ret = device_property_read_u32(parent, "maxim,rtc-i2c-address",
> >>> +                                    &rtc_i2c_addr);
> >>
> >> This property needs to be documented
> >>
> >
> > max776xx pmic has no yaml to document
>
> Of course it has, otherwise on what did I work?
>
> git grep max77686
>
>
> Best regards,
> Krzysztof
>
Krzysztof Kozlowski March 8, 2023, 8:54 a.m. UTC | #5 
On 08/03/2023 09:37, Svyatoslav Ryhel wrote:
> MAX77663 PMIC can have RTC on both 0x63 i2c address (like grouper)
> which is main address but on some devices RTC is located on 0x48
> i2c address (like p880 and p895 from LG). Lets add property to be
> able to use alternative address if needed without breaking existing
> bindings.

Use subject prefixes matching the subsystem (which you can get for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching).

Anyway this is not max77663 driver.

The property does not make sense in the context of max77686. Maybe for
max77802/77620 and 77714 would have... but still max77663 is not there.
Don't add properties for unrelated and unsupported devices.

Best regards,
Krzysztof
Krzysztof Kozlowski March 8, 2023, 8:55 a.m. UTC | #6 
On 08/03/2023 09:51, Svyatoslav Ryhel wrote:
> Ooof, my bad. I will resend v2 with fix asap.

This was for max77686. Other PMICs have their own. None of them is
max77663, but that's another topic.

Best regards,
Krzysztof
Svyatoslav Ryhel March 8, 2023, 8:58 a.m. UTC | #7 
I would love to, but max77663 uses max77686 rtc
https://github.com/torvalds/linux/blob/master/drivers/mfd/max77620.c#L123
how to handle this?

ср, 8 бер. 2023 р. о 10:54 Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> пише:
>
> On 08/03/2023 09:37, Svyatoslav Ryhel wrote:
> > MAX77663 PMIC can have RTC on both 0x63 i2c address (like grouper)
> > which is main address but on some devices RTC is located on 0x48
> > i2c address (like p880 and p895 from LG). Lets add property to be
> > able to use alternative address if needed without breaking existing
> > bindings.
>
> Use subject prefixes matching the subsystem (which you can get for
> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> your patch is touching).
>
> Anyway this is not max77663 driver.
>
> The property does not make sense in the context of max77686. Maybe for
> max77802/77620 and 77714 would have... but still max77663 is not there.
> Don't add properties for unrelated and unsupported devices.
>
> Best regards,
> Krzysztof
>
Krzysztof Kozlowski March 8, 2023, 9:11 a.m. UTC | #8 
On 08/03/2023 09:58, Svyatoslav Ryhel wrote:
> I would love to, but max77663 uses max77686 rtc
> https://github.com/torvalds/linux/blob/master/drivers/mfd/max77620.c#L123
> how to handle this?

Don't top post.

Hm, so it seems max7763 is already documented via max77620. I missed
that. Add the new property to max77620, not to max77686 RTC. It does not
look like RTC's property, but the PMIC's.

Best regards,
Krzysztof
Krzysztof Kozlowski March 8, 2023, 9:14 a.m. UTC | #9 
On 08/03/2023 10:11, Krzysztof Kozlowski wrote:
> On 08/03/2023 09:58, Svyatoslav Ryhel wrote:
>> I would love to, but max77663 uses max77686 rtc
>> https://github.com/torvalds/linux/blob/master/drivers/mfd/max77620.c#L123
>> how to handle this?
> 
> Don't top post.
> 
> Hm, so it seems max7763 is already documented via max77620. I missed
> that. Add the new property to max77620, not to max77686 RTC. It does not
> look like RTC's property, but the PMIC's.

To clarify - the I2C address selection for regmap is in max77686 RTC,
but I meant DT property.

Different thing is that we do not pass addresses as property fields.
These should be devices on the I2C bus rather... unless you are aware of
existing property like this?

Best regards,
Krzysztof
Svyatoslav Ryhel March 8, 2023, 9:29 a.m. UTC | #10 
ср, 8 бер. 2023 р. о 11:11 Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> пише:
>
> On 08/03/2023 09:58, Svyatoslav Ryhel wrote:
> > I would love to, but max77663 uses max77686 rtc
> > https://github.com/torvalds/linux/blob/master/drivers/mfd/max77620.c#L123
> > how to handle this?
>
> Don't top post.
>
> Hm, so it seems max7763 is already documented via max77620. I missed
> that. Add the new property to max77620, not to max77686 RTC. It does not
> look like RTC's property, but the PMIC's.

There is no max77620 yaml, only txt. Add property to txt?

> Best regards,
> Krzysztof
>
Alexandre Belloni March 8, 2023, 9:42 a.m. UTC | #11 
On 08/03/2023 10:14:22+0100, Krzysztof Kozlowski wrote:
> On 08/03/2023 10:11, Krzysztof Kozlowski wrote:
> > On 08/03/2023 09:58, Svyatoslav Ryhel wrote:
> >> I would love to, but max77663 uses max77686 rtc
> >> https://github.com/torvalds/linux/blob/master/drivers/mfd/max77620.c#L123
> >> how to handle this?
> > 
> > Don't top post.
> > 
> > Hm, so it seems max7763 is already documented via max77620. I missed
> > that. Add the new property to max77620, not to max77686 RTC. It does not
> > look like RTC's property, but the PMIC's.
> 
> To clarify - the I2C address selection for regmap is in max77686 RTC,
> but I meant DT property.
> 
> Different thing is that we do not pass addresses as property fields.
> These should be devices on the I2C bus rather... unless you are aware of
> existing property like this?
>

I'd say that the RTC should have been modeled as a discrete component
from the beginning instead of using an i2c dummy device
 
> Best regards,
> Krzysztof
>
Krzysztof Kozlowski March 8, 2023, 10:46 a.m. UTC | #12 
On 08/03/2023 10:29, Svyatoslav Ryhel wrote:
> ср, 8 бер. 2023 р. о 11:11 Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> пише:
>>
>> On 08/03/2023 09:58, Svyatoslav Ryhel wrote:
>>> I would love to, but max77663 uses max77686 rtc
>>> https://github.com/torvalds/linux/blob/master/drivers/mfd/max77620.c#L123
>>> how to handle this?
>>
>> Don't top post.
>>
>> Hm, so it seems max7763 is already documented via max77620. I missed
>> that. Add the new property to max77620, not to max77686 RTC. It does not
>> look like RTC's property, but the PMIC's.
> 
> There is no max77620 yaml, only txt. Add property to txt?

If property was to accept, then unfortunately no, you need to convert first.

Best regards,
Krzysztof
Krzysztof Kozlowski March 8, 2023, 10:49 a.m. UTC | #13 
On 08/03/2023 10:42, Alexandre Belloni wrote:
> On 08/03/2023 10:14:22+0100, Krzysztof Kozlowski wrote:
>> On 08/03/2023 10:11, Krzysztof Kozlowski wrote:
>>> On 08/03/2023 09:58, Svyatoslav Ryhel wrote:
>>>> I would love to, but max77663 uses max77686 rtc
>>>> https://github.com/torvalds/linux/blob/master/drivers/mfd/max77620.c#L123
>>>> how to handle this?
>>>
>>> Don't top post.
>>>
>>> Hm, so it seems max7763 is already documented via max77620. I missed
>>> that. Add the new property to max77620, not to max77686 RTC. It does not
>>> look like RTC's property, but the PMIC's.
>>
>> To clarify - the I2C address selection for regmap is in max77686 RTC,
>> but I meant DT property.
>>
>> Different thing is that we do not pass addresses as property fields.
>> These should be devices on the I2C bus rather... unless you are aware of
>> existing property like this?
>>
> 
> I'd say that the RTC should have been modeled as a discrete component
> from the beginning instead of using an i2c dummy device

Yeah, exactly. Current design was working for existing use
cases/devices, but has limits, thus RTC should be reworked. Actually
even for oldest PMIC max77686, the RTC was a separate device on I2C bus.
We just made it for simplicity part of PMIC.

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
index b0250d91fb00..218177375531 100644
--- a/drivers/rtc/rtc-max77686.c
+++ b/drivers/rtc/rtc-max77686.c
@@ -691,6 +691,7 @@  static int max77686_init_rtc_regmap(struct max77686_rtc_info *info)
 {
 	struct device *parent = info->dev->parent;
 	struct i2c_client *parent_i2c = to_i2c_client(parent);
+	int rtc_i2c_addr;
 	int ret;
 
 	if (info->drv_data->rtc_irq_from_platform) {
@@ -714,8 +715,13 @@  static int max77686_init_rtc_regmap(struct max77686_rtc_info *info)
 		goto add_rtc_irq;
 	}
 
+	ret = device_property_read_u32(parent, "maxim,rtc-i2c-address",
+				       &rtc_i2c_addr);
+	if (ret)
+		rtc_i2c_addr = info->drv_data->rtc_i2c_addr;
+
 	info->rtc = devm_i2c_new_dummy_device(info->dev, parent_i2c->adapter,
-					      info->drv_data->rtc_i2c_addr);
+					      rtc_i2c_addr);
 	if (IS_ERR(info->rtc)) {
 		dev_err(info->dev, "Failed to allocate I2C device for RTC\n");
 		return PTR_ERR(info->rtc);