* [PATCH] i2c: mpc: Use of_property_read_reg() to parse "reg"
@ 2023-06-09 18:30 Rob Herring
2023-06-10 9:36 ` Andi Shyti
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Rob Herring @ 2023-06-09 18:30 UTC (permalink / raw)
To: Chris Packham; +Cc: linux-i2c, linux-kernel
Use the recently added of_property_read_reg() helper to get the
untranslated "reg" address value.
Signed-off-by: Rob Herring <robh@kernel.org>
---
drivers/i2c/busses/i2c-mpc.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index cfd074ee6d54..595dce9218ad 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -316,9 +316,10 @@ static void mpc_i2c_setup_512x(struct device_node *node,
if (node_ctrl) {
ctrl = of_iomap(node_ctrl, 0);
if (ctrl) {
+ u64 addr;
/* Interrupt enable bits for i2c-0/1/2: bit 24/26/28 */
- pval = of_get_property(node, "reg", NULL);
- idx = (*pval & 0xff) / 0x20;
+ of_property_read_reg(node, 0, &addr, NULL);
+ idx = (addr & 0xff) / 0x20;
setbits32(ctrl, 1 << (24 + idx * 2));
iounmap(ctrl);
}
--
2.39.2
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH] i2c: mpc: Use of_property_read_reg() to parse "reg" 2023-06-09 18:30 [PATCH] i2c: mpc: Use of_property_read_reg() to parse "reg" Rob Herring @ 2023-06-10 9:36 ` Andi Shyti 2023-06-12 19:27 ` Rob Herring 2023-06-14 8:36 ` Wolfram Sang 2023-06-21 16:44 ` Guenter Roeck 2 siblings, 1 reply; 7+ messages in thread From: Andi Shyti @ 2023-06-10 9:36 UTC (permalink / raw) To: Rob Herring; +Cc: Chris Packham, linux-i2c, linux-kernel Hi Rob, On Fri, Jun 09, 2023 at 12:30:44PM -0600, Rob Herring wrote: > Use the recently added of_property_read_reg() helper to get the > untranslated "reg" address value. > > Signed-off-by: Rob Herring <robh@kernel.org> > --- > drivers/i2c/busses/i2c-mpc.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c > index cfd074ee6d54..595dce9218ad 100644 > --- a/drivers/i2c/busses/i2c-mpc.c > +++ b/drivers/i2c/busses/i2c-mpc.c > @@ -316,9 +316,10 @@ static void mpc_i2c_setup_512x(struct device_node *node, > if (node_ctrl) { > ctrl = of_iomap(node_ctrl, 0); > if (ctrl) { > + u64 addr; > /* Interrupt enable bits for i2c-0/1/2: bit 24/26/28 */ > - pval = of_get_property(node, "reg", NULL); > - idx = (*pval & 0xff) / 0x20; > + of_property_read_reg(node, 0, &addr, NULL); because of_property_read_reg() can return error, can we check also the error value here? Thanks, Andi > + idx = (addr & 0xff) / 0x20; > setbits32(ctrl, 1 << (24 + idx * 2)); > iounmap(ctrl); > } > -- > 2.39.2 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] i2c: mpc: Use of_property_read_reg() to parse "reg" 2023-06-10 9:36 ` Andi Shyti @ 2023-06-12 19:27 ` Rob Herring 2023-06-12 21:08 ` Andi Shyti 0 siblings, 1 reply; 7+ messages in thread From: Rob Herring @ 2023-06-12 19:27 UTC (permalink / raw) To: Andi Shyti; +Cc: Chris Packham, linux-i2c, linux-kernel On Sat, Jun 10, 2023 at 3:36 AM Andi Shyti <andi.shyti@kernel.org> wrote: > > Hi Rob, > > On Fri, Jun 09, 2023 at 12:30:44PM -0600, Rob Herring wrote: > > Use the recently added of_property_read_reg() helper to get the > > untranslated "reg" address value. > > > > Signed-off-by: Rob Herring <robh@kernel.org> > > --- > > drivers/i2c/busses/i2c-mpc.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c > > index cfd074ee6d54..595dce9218ad 100644 > > --- a/drivers/i2c/busses/i2c-mpc.c > > +++ b/drivers/i2c/busses/i2c-mpc.c > > @@ -316,9 +316,10 @@ static void mpc_i2c_setup_512x(struct device_node *node, > > if (node_ctrl) { > > ctrl = of_iomap(node_ctrl, 0); > > if (ctrl) { > > + u64 addr; > > /* Interrupt enable bits for i2c-0/1/2: bit 24/26/28 */ > > - pval = of_get_property(node, "reg", NULL); > > - idx = (*pval & 0xff) / 0x20; > > + of_property_read_reg(node, 0, &addr, NULL); > > because of_property_read_reg() can return error, can we check > also the error value here? Why? The old code wasn't worried about of_get_property() returning NULL on the same possible errors. If anyone is still actually using mpc512x, I don't think their DTB will have an error at this point. IOW, is improving the error handling on this really worth it? Rob ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] i2c: mpc: Use of_property_read_reg() to parse "reg" 2023-06-12 19:27 ` Rob Herring @ 2023-06-12 21:08 ` Andi Shyti 2023-06-12 21:26 ` Rob Herring 0 siblings, 1 reply; 7+ messages in thread From: Andi Shyti @ 2023-06-12 21:08 UTC (permalink / raw) To: Rob Herring; +Cc: Chris Packham, linux-i2c, linux-kernel Hi Rob, On Mon, Jun 12, 2023 at 01:27:03PM -0600, Rob Herring wrote: > On Sat, Jun 10, 2023 at 3:36 AM Andi Shyti <andi.shyti@kernel.org> wrote: > > > > Hi Rob, > > > > On Fri, Jun 09, 2023 at 12:30:44PM -0600, Rob Herring wrote: > > > Use the recently added of_property_read_reg() helper to get the > > > untranslated "reg" address value. > > > > > > Signed-off-by: Rob Herring <robh@kernel.org> > > > --- > > > drivers/i2c/busses/i2c-mpc.c | 5 +++-- > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c > > > index cfd074ee6d54..595dce9218ad 100644 > > > --- a/drivers/i2c/busses/i2c-mpc.c > > > +++ b/drivers/i2c/busses/i2c-mpc.c > > > @@ -316,9 +316,10 @@ static void mpc_i2c_setup_512x(struct device_node *node, > > > if (node_ctrl) { > > > ctrl = of_iomap(node_ctrl, 0); > > > if (ctrl) { > > > + u64 addr; > > > /* Interrupt enable bits for i2c-0/1/2: bit 24/26/28 */ > > > - pval = of_get_property(node, "reg", NULL); > > > - idx = (*pval & 0xff) / 0x20; > > > + of_property_read_reg(node, 0, &addr, NULL); > > > > because of_property_read_reg() can return error, can we check > > also the error value here? > > Why? Because if a function can return an error, the error must be checked. Even if the property is "reg" and the binding says that it's required. Otherwise let's make those functions void. > The old code wasn't worried about of_get_property() returning > NULL on the same possible errors. Sure! Checking the error comes for free. The patch is fine as it is, mine was a little improvement I asked for. I can still ack it and add the error handling later myself :) > If anyone is still actually using > mpc512x, I don't think their DTB will have an error at this point. > IOW, is improving the error handling on this really worth it? In my view, every error needs to be checked as every error is unlikely to happen: it makes the code future proof and makes sure other components failure don't impact the normal functioning of this driver. Anyway, the patch is doing what you described and for the sole economy of this change: Acked-by: Andi Shyti <andi.shyti@kernel.org> Thank you, Andi ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] i2c: mpc: Use of_property_read_reg() to parse "reg" 2023-06-12 21:08 ` Andi Shyti @ 2023-06-12 21:26 ` Rob Herring 0 siblings, 0 replies; 7+ messages in thread From: Rob Herring @ 2023-06-12 21:26 UTC (permalink / raw) To: Andi Shyti; +Cc: Chris Packham, linux-i2c, linux-kernel On Mon, Jun 12, 2023 at 3:08 PM Andi Shyti <andi.shyti@kernel.org> wrote: > > Hi Rob, > > On Mon, Jun 12, 2023 at 01:27:03PM -0600, Rob Herring wrote: > > On Sat, Jun 10, 2023 at 3:36 AM Andi Shyti <andi.shyti@kernel.org> wrote: > > > > > > Hi Rob, > > > > > > On Fri, Jun 09, 2023 at 12:30:44PM -0600, Rob Herring wrote: > > > > Use the recently added of_property_read_reg() helper to get the > > > > untranslated "reg" address value. > > > > > > > > Signed-off-by: Rob Herring <robh@kernel.org> > > > > --- > > > > drivers/i2c/busses/i2c-mpc.c | 5 +++-- > > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c > > > > index cfd074ee6d54..595dce9218ad 100644 > > > > --- a/drivers/i2c/busses/i2c-mpc.c > > > > +++ b/drivers/i2c/busses/i2c-mpc.c > > > > @@ -316,9 +316,10 @@ static void mpc_i2c_setup_512x(struct device_node *node, > > > > if (node_ctrl) { > > > > ctrl = of_iomap(node_ctrl, 0); > > > > if (ctrl) { > > > > + u64 addr; > > > > /* Interrupt enable bits for i2c-0/1/2: bit 24/26/28 */ > > > > - pval = of_get_property(node, "reg", NULL); > > > > - idx = (*pval & 0xff) / 0x20; > > > > + of_property_read_reg(node, 0, &addr, NULL); > > > > > > because of_property_read_reg() can return error, can we check > > > also the error value here? > > > > Why? > > Because if a function can return an error, the error must be > checked. Even if the property is "reg" and the binding says that > it's required. Otherwise let's make those functions void. Then every function should have a must_check annotation, but they don't as the function is designed to work with optional properties where we want to ignore errors. > > The old code wasn't worried about of_get_property() returning > > NULL on the same possible errors. > > Sure! Checking the error comes for free. The patch is fine as it > is, mine was a little improvement I asked for. I can still ack > it and add the error handling later myself :) > > > If anyone is still actually using > > mpc512x, I don't think their DTB will have an error at this point. > > IOW, is improving the error handling on this really worth it? > > In my view, every error needs to be checked as every error is > unlikely to happen: it makes the code future proof and makes sure > other components failure don't impact the normal functioning of > this driver. An error in this case is a bad DT. It's not the kernel's job to ensure DT is correct. If it is, then it is doing a terrible job. The reason we have dtschema is to ensure correctness. Rob ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] i2c: mpc: Use of_property_read_reg() to parse "reg" 2023-06-09 18:30 [PATCH] i2c: mpc: Use of_property_read_reg() to parse "reg" Rob Herring 2023-06-10 9:36 ` Andi Shyti @ 2023-06-14 8:36 ` Wolfram Sang 2023-06-21 16:44 ` Guenter Roeck 2 siblings, 0 replies; 7+ messages in thread From: Wolfram Sang @ 2023-06-14 8:36 UTC (permalink / raw) To: Rob Herring; +Cc: Chris Packham, linux-i2c, linux-kernel [-- Attachment #1: Type: text/plain, Size: 252 bytes --] On Fri, Jun 09, 2023 at 12:30:44PM -0600, Rob Herring wrote: > Use the recently added of_property_read_reg() helper to get the > untranslated "reg" address value. > > Signed-off-by: Rob Herring <robh@kernel.org> Applied to for-next, thanks! [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] i2c: mpc: Use of_property_read_reg() to parse "reg" 2023-06-09 18:30 [PATCH] i2c: mpc: Use of_property_read_reg() to parse "reg" Rob Herring 2023-06-10 9:36 ` Andi Shyti 2023-06-14 8:36 ` Wolfram Sang @ 2023-06-21 16:44 ` Guenter Roeck 2 siblings, 0 replies; 7+ messages in thread From: Guenter Roeck @ 2023-06-21 16:44 UTC (permalink / raw) To: Rob Herring; +Cc: Chris Packham, linux-i2c, linux-kernel On Fri, Jun 09, 2023 at 12:30:44PM -0600, Rob Herring wrote: > Use the recently added of_property_read_reg() helper to get the > untranslated "reg" address value. > > Signed-off-by: Rob Herring <robh@kernel.org> This patch results in: Building powerpc:ppc32_allmodconfig ... failed -------------- Error log: drivers/i2c/busses/i2c-mpc.c: In function 'mpc_i2c_setup_512x': drivers/i2c/busses/i2c-mpc.c:310:20: error: unused variable 'pval' [-Werror=unused-variable] 310 | const u32 *pval; because pval is no longer used. Guenter > --- > drivers/i2c/busses/i2c-mpc.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c > index cfd074ee6d54..595dce9218ad 100644 > --- a/drivers/i2c/busses/i2c-mpc.c > +++ b/drivers/i2c/busses/i2c-mpc.c > @@ -316,9 +316,10 @@ static void mpc_i2c_setup_512x(struct device_node *node, > if (node_ctrl) { > ctrl = of_iomap(node_ctrl, 0); > if (ctrl) { > + u64 addr; > /* Interrupt enable bits for i2c-0/1/2: bit 24/26/28 */ > - pval = of_get_property(node, "reg", NULL); > - idx = (*pval & 0xff) / 0x20; > + of_property_read_reg(node, 0, &addr, NULL); > + idx = (addr & 0xff) / 0x20; > setbits32(ctrl, 1 << (24 + idx * 2)); > iounmap(ctrl); > } > -- > 2.39.2 > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-06-21 16:45 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-06-09 18:30 [PATCH] i2c: mpc: Use of_property_read_reg() to parse "reg" Rob Herring 2023-06-10 9:36 ` Andi Shyti 2023-06-12 19:27 ` Rob Herring 2023-06-12 21:08 ` Andi Shyti 2023-06-12 21:26 ` Rob Herring 2023-06-14 8:36 ` Wolfram Sang 2023-06-21 16:44 ` Guenter Roeck
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).