| Message ID | 20220630103606.83261-9-tmaimon77@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce Nuvoton Arbel NPCM8XX BMC SoC | expand |
Hi Philipp, Thanks for your comment. On Thu, 30 Jun 2022 at 13:59, Philipp Zabel <p.zabel@pengutronix.de> wrote: > > Hi Tomer, > > On Do, 2022-06-30 at 13:35 +0300, Tomer Maimon wrote: > Using syscon device tree property instead of device data to handle the > NPCM general control registers. > > In case the syscon not found the code still search for nuvoton,npcm750-gcr > to support DTS backward compatibility. > > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> > --- > drivers/reset/reset-npcm.c | 17 ++++++++--------- > 1 file changed, 8 insertions(+), 9 deletions(-) > > diff --git a/drivers/reset/reset-npcm.c b/drivers/reset/reset-npcm.c > index 2ea4d3136e15..431ff2b602c5 100644 > --- a/drivers/reset/reset-npcm.c > +++ b/drivers/reset/reset-npcm.c > @@ -138,8 +138,7 @@ static int npcm_reset_xlate(struct reset_controller_dev *rcdev, > } > > > static const struct of_device_id npcm_rc_match[] = { > - { .compatible = "nuvoton,npcm750-reset", > - .data = (void *)"nuvoton,npcm750-gcr" }, > + { .compatible = "nuvoton,npcm750-reset"}, > > Add a space. ^^ Will modify in V7 > > { } > }; > > > @@ -155,15 +154,15 @@ static int npcm_usb_reset(struct platform_device *pdev, struct npcm_rc_data *rc) > u32 ipsrst1_bits = 0; > u32 ipsrst2_bits = NPCM_IPSRST2_USB_HOST; > u32 ipsrst3_bits = 0; > - const char *gcr_dt; > > > - gcr_dt = (const char *) > - of_match_device(dev->driver->of_match_table, dev)->data; > - > - gcr_regmap = syscon_regmap_lookup_by_compatible(gcr_dt); > + gcr_regmap = syscon_regmap_lookup_by_phandle(dev->of_node, "nuvoton,sysgcr"); > if (IS_ERR(gcr_regmap)) { > - dev_err(&pdev->dev, "Failed to find %s\n", gcr_dt); > - return PTR_ERR(gcr_regmap); > + dev_warn(&pdev->dev, "Failed to find nuvoton,sysgcr search for nuvoton,npcm750-gcr for Poleg backward compatibility"); > > Is this warning useful to the user? Maybe add suggestion like "please > update the device tree". Also there is no further message if > nuvoton,npcm750-gcr is found and all is well. O.K. I think about two options: 1. Modify the message "Failed to find nuvoton,sysgcr property, please update the device tree\n Search for nuvoton,npcm750-gcr for Poleg backward compatibility" OR 2. if (IS_ERR(rc->gcr_regmap)) { dev_warn(&pdev->dev, "Failed to find nuvoton,sysgcr please update the device tree"); rc->gcr_regmap = syscon_regmap_lookup_by_compatible("nuvoton,npcm750-gcr"); if (IS_ERR(rc->gcr_regmap)) { dev_err(&pdev->dev, "Failed to find nuvoton,npcm750-gcr"); return PTR_ERR(rc->gcr_regmap); } dev_info(&pdev->dev, "found nuvoton,npcm750-gcr for Poleg backward compatibility"); } The only problem that I have with option 2 is if our customers will use the latest reset driver and they will not update their device tree they will see all the time the dev_info message. What do you think? > > regards > Philipp Best regards, Tomer
On Do, 2022-06-30 at 14:20 +0300, Tomer Maimon wrote: > Hi Philipp, > > Thanks for your comment. > > On Thu, 30 Jun 2022 at 13:59, Philipp Zabel <p.zabel@pengutronix.de> wrote: > > > > Hi Tomer, > > > > On Do, 2022-06-30 at 13:35 +0300, Tomer Maimon wrote: > > Using syscon device tree property instead of device data to handle the > > NPCM general control registers. > > > > In case the syscon not found the code still search for nuvoton,npcm750-gcr > > to support DTS backward compatibility. > > > > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> > > --- > > drivers/reset/reset-npcm.c | 17 ++++++++--------- > > 1 file changed, 8 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/reset/reset-npcm.c b/drivers/reset/reset-npcm.c > > index 2ea4d3136e15..431ff2b602c5 100644 > > --- a/drivers/reset/reset-npcm.c > > +++ b/drivers/reset/reset-npcm.c > > @@ -138,8 +138,7 @@ static int npcm_reset_xlate(struct reset_controller_dev *rcdev, > > } > > > > > > static const struct of_device_id npcm_rc_match[] = { > > - { .compatible = "nuvoton,npcm750-reset", > > - .data = (void *)"nuvoton,npcm750-gcr" }, > > + { .compatible = "nuvoton,npcm750-reset"}, > > > > Add a space. ^^ > Will modify in V7 > > > > { } > > }; > > > > > > @@ -155,15 +154,15 @@ static int npcm_usb_reset(struct platform_device *pdev, struct npcm_rc_data *rc) > > u32 ipsrst1_bits = 0; > > u32 ipsrst2_bits = NPCM_IPSRST2_USB_HOST; > > u32 ipsrst3_bits = 0; > > - const char *gcr_dt; > > > > > > - gcr_dt = (const char *) > > - of_match_device(dev->driver->of_match_table, dev)->data; > > - > > - gcr_regmap = syscon_regmap_lookup_by_compatible(gcr_dt); > > + gcr_regmap = syscon_regmap_lookup_by_phandle(dev->of_node, "nuvoton,sysgcr"); > > if (IS_ERR(gcr_regmap)) { > > - dev_err(&pdev->dev, "Failed to find %s\n", gcr_dt); > > - return PTR_ERR(gcr_regmap); > > + dev_warn(&pdev->dev, "Failed to find nuvoton,sysgcr search for nuvoton,npcm750-gcr for Poleg backward compatibility"); > > > > Is this warning useful to the user? Maybe add suggestion like "please > > update the device tree". Also there is no further message if > > nuvoton,npcm750-gcr is found and all is well. > > O.K. > I think about two options: > > 1. Modify the message "Failed to find nuvoton,sysgcr property, please > update the device tree\n Search for nuvoton,npcm750-gcr for Poleg > backward compatibility" I would replace "Search for" with "Using" The second line probably should be dev_info() level. > OR > > 2. > if (IS_ERR(rc->gcr_regmap)) { > dev_warn(&pdev->dev, "Failed to find nuvoton,sysgcr > please update the device tree"); > rc->gcr_regmap = > syscon_regmap_lookup_by_compatible("nuvoton,npcm750-gcr"); > if (IS_ERR(rc->gcr_regmap)) { > dev_err(&pdev->dev, "Failed to find > nuvoton,npcm750-gcr"); > return PTR_ERR(rc->gcr_regmap); > } > dev_info(&pdev->dev, "found nuvoton,npcm750-gcr for > Poleg backward compatibility"); > } > > The only problem that I have with option 2 is if our customers will > use the latest reset driver and they will not update their device tree > they will see all the time the dev_info message. > > What do you think? I'm fine with either. With the "please update DT" prompt it's clear how to get rid of the warning. regards Philipp
diff --git a/drivers/reset/reset-npcm.c b/drivers/reset/reset-npcm.c index 2ea4d3136e15..431ff2b602c5 100644 --- a/drivers/reset/reset-npcm.c +++ b/drivers/reset/reset-npcm.c @@ -138,8 +138,7 @@ static int npcm_reset_xlate(struct reset_controller_dev *rcdev, } static const struct of_device_id npcm_rc_match[] = { - { .compatible = "nuvoton,npcm750-reset", - .data = (void *)"nuvoton,npcm750-gcr" }, + { .compatible = "nuvoton,npcm750-reset"}, { } }; @@ -155,15 +154,15 @@ static int npcm_usb_reset(struct platform_device *pdev, struct npcm_rc_data *rc) u32 ipsrst1_bits = 0; u32 ipsrst2_bits = NPCM_IPSRST2_USB_HOST; u32 ipsrst3_bits = 0; - const char *gcr_dt; - gcr_dt = (const char *) - of_match_device(dev->driver->of_match_table, dev)->data; - - gcr_regmap = syscon_regmap_lookup_by_compatible(gcr_dt); + gcr_regmap = syscon_regmap_lookup_by_phandle(dev->of_node, "nuvoton,sysgcr"); if (IS_ERR(gcr_regmap)) { - dev_err(&pdev->dev, "Failed to find %s\n", gcr_dt); - return PTR_ERR(gcr_regmap); + dev_warn(&pdev->dev, "Failed to find nuvoton,sysgcr search for nuvoton,npcm750-gcr for Poleg backward compatibility"); + gcr_regmap = syscon_regmap_lookup_by_compatible("nuvoton,npcm750-gcr"); + if (IS_ERR(gcr_regmap)) { + dev_err(&pdev->dev, "Failed to find nuvoton,npcm750-gcr"); + return PTR_ERR(gcr_regmap); + } } /* checking which USB device is enabled */
Using syscon device tree property instead of device data to handle the NPCM general control registers. In case the syscon not found the code still search for nuvoton,npcm750-gcr to support DTS backward compatibility. Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> --- drivers/reset/reset-npcm.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)