| Message ID | 1445863883-5187-6-git-send-email-alim.akhtar@samsung.com |
|---|---|
| State | Superseded |
| Headers | show |
On 26/10/2015 at 18:21:22 +0530, Alim Akhtar wrote : > RTC found in s2mps15 is almost same as one found in s2mps14. > This patch add required changes to enable s2mps15 rtc timer. > > Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com> One really small nitpick below else you can add my Acked-by: Alexandre Belloni <alexandre.belloni@free-electrons.com> > --- > drivers/rtc/rtc-s5m.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > @@ -317,7 +319,8 @@ static int s5m_rtc_read_time(struct device *dev, struct rtc_time *tm) > u8 data[info->regs->regs_count]; > int ret; > > - if (info->device_type == S2MPS14X || info->device_type == S2MPS13X) { > + if (info->device_type == S2MPS14X || info->device_type == S2MPS15X || > + info->device_type == S2MPS13X) { This line is not properly aligned. > ret = regmap_update_bits(info->regmap, > info->regs->rtc_udr_update, > S2MPS_RTC_RUDR_MASK, S2MPS_RTC_RUDR_MASK);
On 26.10.2015 21:51, Alim Akhtar wrote: > RTC found in s2mps15 is almost same as one found in s2mps14. > This patch add required changes to enable s2mps15 rtc timer. > > Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com> > --- > drivers/rtc/rtc-s5m.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) NACK. I don't have S2MPS15 datasheet but looking at the code right now this looks *exactly the same* device as S2MPS14. If that's true, then don't add new compatibles, new names etc. Re-use. No new code needed, no changes needed. Keep it simple. Best regards, Krzysztof > > diff --git a/drivers/rtc/rtc-s5m.c b/drivers/rtc/rtc-s5m.c > index f2504b4eef34..ac3286a267ba 100644 > --- a/drivers/rtc/rtc-s5m.c > +++ b/drivers/rtc/rtc-s5m.c > @@ -188,6 +188,7 @@ static inline int s5m_check_peding_alarm_interrupt(struct s5m_rtc_info *info, > ret = regmap_read(info->regmap, S5M_RTC_STATUS, &val); > val &= S5M_ALARM0_STATUS; > break; > + case S2MPS15X: > case S2MPS14X: > case S2MPS13X: > ret = regmap_read(info->s5m87xx->regmap_pmic, S2MPS14_REG_ST2, > @@ -253,6 +254,7 @@ static inline int s5m8767_rtc_set_alarm_reg(struct s5m_rtc_info *info) > data &= ~S5M_RTC_TIME_EN_MASK; > break; > case S2MPS14X: > + case S2MPS15X: > data |= S2MPS_RTC_RUDR_MASK; > break; > case S2MPS13X: > @@ -317,7 +319,8 @@ static int s5m_rtc_read_time(struct device *dev, struct rtc_time *tm) > u8 data[info->regs->regs_count]; > int ret; > > - if (info->device_type == S2MPS14X || info->device_type == S2MPS13X) { > + if (info->device_type == S2MPS14X || info->device_type == S2MPS15X || > + info->device_type == S2MPS13X) { > ret = regmap_update_bits(info->regmap, > info->regs->rtc_udr_update, > S2MPS_RTC_RUDR_MASK, S2MPS_RTC_RUDR_MASK); > @@ -339,6 +342,7 @@ static int s5m_rtc_read_time(struct device *dev, struct rtc_time *tm) > break; > > case S5M8767X: > + case S2MPS15X: > case S2MPS14X: > case S2MPS13X: > s5m8767_data_to_tm(data, tm, info->rtc_24hr_mode); > @@ -366,6 +370,7 @@ static int s5m_rtc_set_time(struct device *dev, struct rtc_time *tm) > s5m8763_tm_to_data(tm, data); > break; > case S5M8767X: > + case S2MPS15X: > case S2MPS14X: > case S2MPS13X: > ret = s5m8767_tm_to_data(tm, data); > @@ -414,6 +419,7 @@ static int s5m_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm) > break; > > case S5M8767X: > + case S2MPS15X: > case S2MPS14X: > case S2MPS13X: > s5m8767_data_to_tm(data, &alrm->time, info->rtc_24hr_mode); > @@ -463,6 +469,7 @@ static int s5m_rtc_stop_alarm(struct s5m_rtc_info *info) > break; > > case S5M8767X: > + case S2MPS15X: > case S2MPS14X: > case S2MPS13X: > for (i = 0; i < info->regs->regs_count; i++) > @@ -508,6 +515,7 @@ static int s5m_rtc_start_alarm(struct s5m_rtc_info *info) > break; > > case S5M8767X: > + case S2MPS15X: > case S2MPS14X: > case S2MPS13X: > data[RTC_SEC] |= ALARM_ENABLE_MASK; > @@ -548,6 +556,7 @@ static int s5m_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm) > break; > > case S5M8767X: > + case S2MPS15X: > case S2MPS14X: > case S2MPS13X: > s5m8767_tm_to_data(&alrm->time, data); > @@ -631,6 +640,7 @@ static int s5m8767_rtc_init_reg(struct s5m_rtc_info *info) > ret = regmap_raw_write(info->regmap, S5M_ALARM0_CONF, data, 2); > break; > > + case S2MPS15X: > case S2MPS14X: > case S2MPS13X: > data[0] = (0 << BCD_EN_SHIFT) | (1 << MODEL24_SHIFT); > @@ -679,6 +689,7 @@ static int s5m_rtc_probe(struct platform_device *pdev) > return -ENOMEM; > > switch (platform_get_device_id(pdev)->driver_data) { > + case S2MPS15X: > case S2MPS14X: > case S2MPS13X: > regmap_cfg = &s2mps14_rtc_regmap_config; > @@ -805,6 +816,7 @@ static const struct platform_device_id s5m_rtc_id[] = { > { "s5m-rtc", S5M8767X }, > { "s2mps13-rtc", S2MPS13X }, > { "s2mps14-rtc", S2MPS14X }, > + { "s2mps15-rtc", S2MPS15X }, > { }, > }; > MODULE_DEVICE_TABLE(platform, s5m_rtc_id); >
On Wed, Oct 28, 2015 at 10:29:56AM +0900, Krzysztof Kozlowski wrote: > If that's true, then don't add new compatibles, new names etc. Re-use. > No new code needed, no changes needed. Keep it simple. Well, it depends - it can be useful to get the information about it being a different part into DT so that if in future we realise that there is some difference (perhaps a bug workaround even if the IP is intended to be the same). Though in the case of a MFD that information can be obtained from the MFD for the device.
On 28.10.2015 10:53, Mark Brown wrote: > On Wed, Oct 28, 2015 at 10:29:56AM +0900, Krzysztof Kozlowski wrote: > >> If that's true, then don't add new compatibles, new names etc. Re-use. >> No new code needed, no changes needed. Keep it simple. > > Well, it depends - it can be useful to get the information about it > being a different part into DT so that if in future we realise that > there is some difference (perhaps a bug workaround even if the IP is > intended to be the same). Though in the case of a MFD that information > can be obtained from the MFD for the device. We can always differentiate later and introduce new compatible. Declaring a compatible right now would be useful only if we really cared about using the workaround on older DTBs. Since I cannot judge the difference (I don't have the datasheet of S2MPS15) then I don't see the need of adding new compatible/name for the "same device". Of course maybe there is such need? Alim? Best regards, Krzysztof
Hello, On 10/28/2015 07:47 AM, Krzysztof Kozlowski wrote: > On 28.10.2015 10:53, Mark Brown wrote: >> On Wed, Oct 28, 2015 at 10:29:56AM +0900, Krzysztof Kozlowski wrote: >> >>> If that's true, then don't add new compatibles, new names etc. Re-use. >>> No new code needed, no changes needed. Keep it simple. >> >> Well, it depends - it can be useful to get the information about it >> being a different part into DT so that if in future we realise that >> there is some difference (perhaps a bug workaround even if the IP is >> intended to be the same). Though in the case of a MFD that information >> can be obtained from the MFD for the device. > > We can always differentiate later and introduce new compatible. > Declaring a compatible right now would be useful only if we really cared > about using the workaround on older DTBs. > > Since I cannot judge the difference (I don't have the datasheet of > S2MPS15) then I don't see the need of adding new compatible/name for the > "same device". > > Of course maybe there is such need? Alim? > Well I did think of keeping the changes as minimal as possible, like just have "{ "s2mps15-rtc", S2MPS14X }", since I don't have access to s2mps14 UM, I could not confirm that s2mps14 and s2mps15 are exactly the same w.r.t rtc block. So I proposed the current changes. Well I do agree with Mark here, a name/compatible matching with the pmic is good to at least avoid confusion while looking at the sysfs. > Best regards, > Krzysztof >
On 28.10.2015 12:14, Alim Akhtar wrote: > Hello, > > On 10/28/2015 07:47 AM, Krzysztof Kozlowski wrote: >> On 28.10.2015 10:53, Mark Brown wrote: >>> On Wed, Oct 28, 2015 at 10:29:56AM +0900, Krzysztof Kozlowski wrote: >>> >>>> If that's true, then don't add new compatibles, new names etc. Re-use. >>>> No new code needed, no changes needed. Keep it simple. >>> >>> Well, it depends - it can be useful to get the information about it >>> being a different part into DT so that if in future we realise that >>> there is some difference (perhaps a bug workaround even if the IP is >>> intended to be the same). Though in the case of a MFD that information >>> can be obtained from the MFD for the device. >> >> We can always differentiate later and introduce new compatible. >> Declaring a compatible right now would be useful only if we really cared >> about using the workaround on older DTBs. >> >> Since I cannot judge the difference (I don't have the datasheet of >> S2MPS15) then I don't see the need of adding new compatible/name for the >> "same device". >> >> Of course maybe there is such need? Alim? >> > Well I did think of keeping the changes as minimal as possible, like > just have "{ "s2mps15-rtc", S2MPS14X }", since I don't have > access to s2mps14 UM, I could not confirm that s2mps14 and s2mps15 are > exactly the same w.r.t rtc block. So I proposed the current changes. > > Well I do agree with Mark here, a name/compatible matching with the pmic > is good to at least avoid confusion while looking at the sysfs. What kind of confusion in sysfs? I don't see any... and already the s2mps14-rtc name is used for S2MPS11 and S2MPS14. The s2mps13 clock driver added new name and compatible... which was probably totally unneeded (I missed that during review). We don't have to make this as a rule... Since we do not have any data about future workarounds and the differences then just follow Ockham's razor - use the same name and compatible. Best regards, Krzysztof
On 10/28/2015 09:01 AM, Krzysztof Kozlowski wrote: > On 28.10.2015 12:14, Alim Akhtar wrote: >> Hello, >> >> On 10/28/2015 07:47 AM, Krzysztof Kozlowski wrote: >>> On 28.10.2015 10:53, Mark Brown wrote: >>>> On Wed, Oct 28, 2015 at 10:29:56AM +0900, Krzysztof Kozlowski wrote: >>>> >>>>> If that's true, then don't add new compatibles, new names etc. Re-use. >>>>> No new code needed, no changes needed. Keep it simple. >>>> >>>> Well, it depends - it can be useful to get the information about it >>>> being a different part into DT so that if in future we realise that >>>> there is some difference (perhaps a bug workaround even if the IP is >>>> intended to be the same). Though in the case of a MFD that information >>>> can be obtained from the MFD for the device. >>> >>> We can always differentiate later and introduce new compatible. >>> Declaring a compatible right now would be useful only if we really cared >>> about using the workaround on older DTBs. >>> >>> Since I cannot judge the difference (I don't have the datasheet of >>> S2MPS15) then I don't see the need of adding new compatible/name for the >>> "same device". >>> >>> Of course maybe there is such need? Alim? >>> >> Well I did think of keeping the changes as minimal as possible, like >> just have "{ "s2mps15-rtc", S2MPS14X }", since I don't have >> access to s2mps14 UM, I could not confirm that s2mps14 and s2mps15 are >> exactly the same w.r.t rtc block. So I proposed the current changes. >> >> Well I do agree with Mark here, a name/compatible matching with the pmic >> is good to at least avoid confusion while looking at the sysfs. > > What kind of confusion in sysfs? I don't see any... and already the > s2mps14-rtc name is used for S2MPS11 and S2MPS14. > > The s2mps13 clock driver added new name and compatible... which was > probably totally unneeded (I missed that during review). We don't have > to make this as a rule... > > Since we do not have any data about future workarounds and the > differences then just follow Ockham's razor - use the same name and > compatible. > ok, have request s2smp14 UM, will cross check and update accordingly. Thanks. > Best regards, > Krzysztof >
On 28/10/2015 at 12:31:43 +0900, Krzysztof Kozlowski wrote : > The s2mps13 clock driver added new name and compatible... which was > probably totally unneeded (I missed that during review). We don't have > to make this as a rule... > > Since we do not have any data about future workarounds and the > differences then just follow Ockham's razor - use the same name and > compatible. > So you don't care about DT backward compatibility because when a workaround will be needed for one of the IPs, then you will have to update the old dtb to use it. Unless you are sure that the IP is the same, doing { "s2mps15-rtc", S2MPS14X }, is probably the best way to handle that. Note that I personally don't care about the DT ABI, I'm just pointing out what may happen ;)
Hi Alexandre, On 10/28/2015 03:18 PM, Alexandre Belloni wrote: > On 28/10/2015 at 12:31:43 +0900, Krzysztof Kozlowski wrote : >> The s2mps13 clock driver added new name and compatible... which was >> probably totally unneeded (I missed that during review). We don't have >> to make this as a rule... >> >> Since we do not have any data about future workarounds and the >> differences then just follow Ockham's razor - use the same name and >> compatible. >> > > So you don't care about DT backward compatibility because when a > workaround will be needed for one of the IPs, then you will have to > update the old dtb to use it. > > Unless you are sure that the IP is the same, doing > > { "s2mps15-rtc", S2MPS14X }, > > is probably the best way to handle that. Note that I personally don't > care about the DT ABI, I'm just pointing out what may happen ;) > Thats what my point is, anyway I am still looking into the fine prints of the s2mps15 and s2mps14 user manual, now I have found at least one difference in their one of the register bit, which might be a good reason to keep s2mps15-rtc device. Will update the same in v4. Thanks,
W dniu 28.10.2015 o 18:48, Alexandre Belloni pisze: > On 28/10/2015 at 12:31:43 +0900, Krzysztof Kozlowski wrote : >> The s2mps13 clock driver added new name and compatible... which was >> probably totally unneeded (I missed that during review). We don't have >> to make this as a rule... >> >> Since we do not have any data about future workarounds and the >> differences then just follow Ockham's razor - use the same name and >> compatible. >> > > So you don't care about DT backward compatibility because when a > workaround will be needed for one of the IPs, then you will have to > update the old dtb to use it. Nope, DTB does not contain the name for RTC driver (s2mps15-rtc). It will be exactly the same. > > Unless you are sure that the IP is the same, doing > > { "s2mps15-rtc", S2MPS14X }, > > is probably the best way to handle that. Note that I personally don't > care about the DT ABI, I'm just pointing out what may happen ;) In case of RTC driver there is no chance of DT ABI issues here. The clock driver is different because it contain a compatible. Best regards, Krzysztof
W dniu 28.10.2015 o 19:30, Alim Akhtar pisze: > Hi Alexandre, > > On 10/28/2015 03:18 PM, Alexandre Belloni wrote: >> On 28/10/2015 at 12:31:43 +0900, Krzysztof Kozlowski wrote : >>> The s2mps13 clock driver added new name and compatible... which was >>> probably totally unneeded (I missed that during review). We don't have >>> to make this as a rule... >>> >>> Since we do not have any data about future workarounds and the >>> differences then just follow Ockham's razor - use the same name and >>> compatible. >>> >> >> So you don't care about DT backward compatibility because when a >> workaround will be needed for one of the IPs, then you will have to >> update the old dtb to use it. >> >> Unless you are sure that the IP is the same, doing >> >> { "s2mps15-rtc", S2MPS14X }, >> >> is probably the best way to handle that. Note that I personally don't >> care about the DT ABI, I'm just pointing out what may happen ;) >> > Thats what my point is, anyway I am still looking into the fine prints > of the s2mps15 and s2mps14 user manual, now I have found at least one > difference in their one of the register bit, which might be a good > reason to keep s2mps15-rtc device. No, this is not a good reason. Devices may be different but from driver's perspective they behave the same. This means for example that they use the same compatible. They can use the same driver name. We have for example AHCI driver for entire range of different devices. Some of them are different enough that require special code. So they have different name and compatible. All of other use the same ahci driver. But devices are different... Best regards, Krzysztof
On 28/10/2015 at 22:31:48 +0900, Krzysztof Kozlowski wrote : > > So you don't care about DT backward compatibility because when a > > workaround will be needed for one of the IPs, then you will have to > > update the old dtb to use it. > > Nope, DTB does not contain the name for RTC driver (s2mps15-rtc). It > will be exactly the same. > Sure, I see it now. So no change is needed in the rtc driver until you need to use that particular bit that is different.
diff --git a/drivers/rtc/rtc-s5m.c b/drivers/rtc/rtc-s5m.c index f2504b4eef34..ac3286a267ba 100644 --- a/drivers/rtc/rtc-s5m.c +++ b/drivers/rtc/rtc-s5m.c @@ -188,6 +188,7 @@ static inline int s5m_check_peding_alarm_interrupt(struct s5m_rtc_info *info, ret = regmap_read(info->regmap, S5M_RTC_STATUS, &val); val &= S5M_ALARM0_STATUS; break; + case S2MPS15X: case S2MPS14X: case S2MPS13X: ret = regmap_read(info->s5m87xx->regmap_pmic, S2MPS14_REG_ST2, @@ -253,6 +254,7 @@ static inline int s5m8767_rtc_set_alarm_reg(struct s5m_rtc_info *info) data &= ~S5M_RTC_TIME_EN_MASK; break; case S2MPS14X: + case S2MPS15X: data |= S2MPS_RTC_RUDR_MASK; break; case S2MPS13X: @@ -317,7 +319,8 @@ static int s5m_rtc_read_time(struct device *dev, struct rtc_time *tm) u8 data[info->regs->regs_count]; int ret; - if (info->device_type == S2MPS14X || info->device_type == S2MPS13X) { + if (info->device_type == S2MPS14X || info->device_type == S2MPS15X || + info->device_type == S2MPS13X) { ret = regmap_update_bits(info->regmap, info->regs->rtc_udr_update, S2MPS_RTC_RUDR_MASK, S2MPS_RTC_RUDR_MASK); @@ -339,6 +342,7 @@ static int s5m_rtc_read_time(struct device *dev, struct rtc_time *tm) break; case S5M8767X: + case S2MPS15X: case S2MPS14X: case S2MPS13X: s5m8767_data_to_tm(data, tm, info->rtc_24hr_mode); @@ -366,6 +370,7 @@ static int s5m_rtc_set_time(struct device *dev, struct rtc_time *tm) s5m8763_tm_to_data(tm, data); break; case S5M8767X: + case S2MPS15X: case S2MPS14X: case S2MPS13X: ret = s5m8767_tm_to_data(tm, data); @@ -414,6 +419,7 @@ static int s5m_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm) break; case S5M8767X: + case S2MPS15X: case S2MPS14X: case S2MPS13X: s5m8767_data_to_tm(data, &alrm->time, info->rtc_24hr_mode); @@ -463,6 +469,7 @@ static int s5m_rtc_stop_alarm(struct s5m_rtc_info *info) break; case S5M8767X: + case S2MPS15X: case S2MPS14X: case S2MPS13X: for (i = 0; i < info->regs->regs_count; i++) @@ -508,6 +515,7 @@ static int s5m_rtc_start_alarm(struct s5m_rtc_info *info) break; case S5M8767X: + case S2MPS15X: case S2MPS14X: case S2MPS13X: data[RTC_SEC] |= ALARM_ENABLE_MASK; @@ -548,6 +556,7 @@ static int s5m_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm) break; case S5M8767X: + case S2MPS15X: case S2MPS14X: case S2MPS13X: s5m8767_tm_to_data(&alrm->time, data); @@ -631,6 +640,7 @@ static int s5m8767_rtc_init_reg(struct s5m_rtc_info *info) ret = regmap_raw_write(info->regmap, S5M_ALARM0_CONF, data, 2); break; + case S2MPS15X: case S2MPS14X: case S2MPS13X: data[0] = (0 << BCD_EN_SHIFT) | (1 << MODEL24_SHIFT); @@ -679,6 +689,7 @@ static int s5m_rtc_probe(struct platform_device *pdev) return -ENOMEM; switch (platform_get_device_id(pdev)->driver_data) { + case S2MPS15X: case S2MPS14X: case S2MPS13X: regmap_cfg = &s2mps14_rtc_regmap_config; @@ -805,6 +816,7 @@ static const struct platform_device_id s5m_rtc_id[] = { { "s5m-rtc", S5M8767X }, { "s2mps13-rtc", S2MPS13X }, { "s2mps14-rtc", S2MPS14X }, + { "s2mps15-rtc", S2MPS15X }, { }, }; MODULE_DEVICE_TABLE(platform, s5m_rtc_id);
RTC found in s2mps15 is almost same as one found in s2mps14. This patch add required changes to enable s2mps15 rtc timer. Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com> --- drivers/rtc/rtc-s5m.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)