| Message ID | 20210907113226.31876-9-rogerq@kernel.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | dt-bindings: memory-controllers: ti, gpmc: Convert to yaml | expand |
On 07/09/2021 13:32, Roger Quadros wrote: > Check for valid gpmc,device-width, nand-bus-width and bank-width > at one place. Default to 8-bit width if none present. I don't understand the message in the context of the patch. The title says one property is optional - that's it. The message says you consolidate checks. How is this related to the title? The patch itself moves around checking of properties and reads nand-bus-width *always*. It does not "check at one place" but rather "check always". In the same time, the patch does not remove gpmc,device-width check in other place. All three elements - the title, message and patch - do different things. What did you want to achieve here? Can you help in clarifying it? Best regards, Krzysztof > > Signed-off-by: Roger Quadros <rogerq@kernel.org> > --- > drivers/memory/omap-gpmc.c | 41 ++++++++++++++++++++++++-------------- > 1 file changed, 26 insertions(+), 15 deletions(-) > > diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c > index f80c2ea39ca4..32d7c665f33c 100644 > --- a/drivers/memory/omap-gpmc.c > +++ b/drivers/memory/omap-gpmc.c > @@ -2171,10 +2171,8 @@ static int gpmc_probe_generic_child(struct platform_device *pdev, > } > } > > - if (of_device_is_compatible(child, "ti,omap2-nand")) { > - /* NAND specific setup */ > - val = 8; > - of_property_read_u32(child, "nand-bus-width", &val); > + /* DT node can have "nand-bus-width" or "bank-width" or "gpmc,device-width" */ > + if (!of_property_read_u32(child, "nand-bus-width", &val)) { > switch (val) { > case 8: > gpmc_s.device_width = GPMC_DEVWIDTH_8BIT; > @@ -2183,24 +2181,37 @@ static int gpmc_probe_generic_child(struct platform_device *pdev, > gpmc_s.device_width = GPMC_DEVWIDTH_16BIT; > break; > default: > - dev_err(&pdev->dev, "%pOFn: invalid 'nand-bus-width'\n", > - child); > + dev_err(&pdev->dev, > + "%pOFn: invalid 'nand-bus-width':%d\n", child, val); > + ret = -EINVAL; > + goto err; > + } > + } else if (!of_property_read_u32(child, "bank-width", &val)) { > + if (val != 1 && val != 2) { > + dev_err(&pdev->dev, > + "%pOFn: invalid 'bank-width':%d\n", child, val); > ret = -EINVAL; > goto err; > } > + gpmc_s.device_width = val; > + } else if (!of_property_read_u32(child, "gpmc,device-width", &val)) { > + if (val != 1 && val != 2) { > + dev_err(&pdev->dev, > + "%pOFn: invalid 'gpmc,device-width':%d\n", child, val); > + ret = -EINVAL; > + goto err; > + } > + gpmc_s.device_width = val; > + } else { > + /* default to 8-bit */ > + gpmc_s.device_width = GPMC_DEVWIDTH_8BIT; > + } > > + if (of_device_is_compatible(child, "ti,omap2-nand")) { > + /* NAND specific setup */ > /* disable write protect */ > gpmc_configure(GPMC_CONFIG_WP, 0); > gpmc_s.device_nand = true; > - } else { > - ret = of_property_read_u32(child, "bank-width", > - &gpmc_s.device_width); > - if (ret < 0 && !gpmc_s.device_width) { > - dev_err(&pdev->dev, > - "%pOF has no 'gpmc,device-width' property\n", > - child); > - goto err; > - } > } > > /* Reserve wait pin if it is required and valid */ >
Hi Krzysztof, On 07/09/2021 15:36, Krzysztof Kozlowski wrote: > On 07/09/2021 13:32, Roger Quadros wrote: >> Check for valid gpmc,device-width, nand-bus-width and bank-width >> at one place. Default to 8-bit width if none present. > > I don't understand the message in the context of the patch. The title > says one property is optional - that's it. The message says you > consolidate checks. How is this related to the title? > > The patch itself moves around checking of properties and reads > nand-bus-width *always*. It does not "check at one place" but rather > "check always". In the same time, the patch does not remove > gpmc,device-width check in other place. > > All three elements - the title, message and patch - do different things. > What did you want to achieve here? Can you help in clarifying it? > OK I will explain it better in commit log in next revision. Let me explain here a bit. Prior to this patch it was working like this /* in gpmc_read_settings_dt() */ s->device_width = 0; /* invalid width, should be 1 for 8-bit, 2 for 16-bit */ of_property_read_u32(np, "gpmc,device-width", s->device_width); /* in gpmc_probe_generic_child () */ if (of_device_is_compatible(child, "ti,omap2-nand")) { /* check for nand-bus-width, if absent set s->device_width to 1 (i.e. 8-bit) */ } else { /* check for bank-width, if absent and s->device_width not set, error out */ } So that means if all three, "gpmc,device-width". "nand-bus-width" and "bank-width" are missing then it would create an error situation. The patch is doing 3 things. 1) Make sure all DT checks related to bus width are being done at one place for better readability. 2) even if all 3 width properties are absent, we will not treat it as error and default to 8-bit. 3) check for nand-bus-width regardless of whether compatible to "ti,omap2-nand" or not. Hope this explains well. cheers, -roger > > Best regards, > Krzysztof > > >> >> Signed-off-by: Roger Quadros <rogerq@kernel.org> >> --- >> drivers/memory/omap-gpmc.c | 41 ++++++++++++++++++++++++-------------- >> 1 file changed, 26 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c >> index f80c2ea39ca4..32d7c665f33c 100644 >> --- a/drivers/memory/omap-gpmc.c >> +++ b/drivers/memory/omap-gpmc.c >> @@ -2171,10 +2171,8 @@ static int gpmc_probe_generic_child(struct platform_device *pdev, >> } >> } >> >> - if (of_device_is_compatible(child, "ti,omap2-nand")) { >> - /* NAND specific setup */ >> - val = 8; >> - of_property_read_u32(child, "nand-bus-width", &val); >> + /* DT node can have "nand-bus-width" or "bank-width" or "gpmc,device-width" */ >> + if (!of_property_read_u32(child, "nand-bus-width", &val)) { >> switch (val) { >> case 8: >> gpmc_s.device_width = GPMC_DEVWIDTH_8BIT; >> @@ -2183,24 +2181,37 @@ static int gpmc_probe_generic_child(struct platform_device *pdev, >> gpmc_s.device_width = GPMC_DEVWIDTH_16BIT; >> break; >> default: >> - dev_err(&pdev->dev, "%pOFn: invalid 'nand-bus-width'\n", >> - child); >> + dev_err(&pdev->dev, >> + "%pOFn: invalid 'nand-bus-width':%d\n", child, val); >> + ret = -EINVAL; >> + goto err; >> + } >> + } else if (!of_property_read_u32(child, "bank-width", &val)) { >> + if (val != 1 && val != 2) { >> + dev_err(&pdev->dev, >> + "%pOFn: invalid 'bank-width':%d\n", child, val); >> ret = -EINVAL; >> goto err; >> } >> + gpmc_s.device_width = val; >> + } else if (!of_property_read_u32(child, "gpmc,device-width", &val)) { >> + if (val != 1 && val != 2) { >> + dev_err(&pdev->dev, >> + "%pOFn: invalid 'gpmc,device-width':%d\n", child, val); >> + ret = -EINVAL; >> + goto err; >> + } >> + gpmc_s.device_width = val; >> + } else { >> + /* default to 8-bit */ >> + gpmc_s.device_width = GPMC_DEVWIDTH_8BIT; >> + } >> >> + if (of_device_is_compatible(child, "ti,omap2-nand")) { >> + /* NAND specific setup */ >> /* disable write protect */ >> gpmc_configure(GPMC_CONFIG_WP, 0); >> gpmc_s.device_nand = true; >> - } else { >> - ret = of_property_read_u32(child, "bank-width", >> - &gpmc_s.device_width); >> - if (ret < 0 && !gpmc_s.device_width) { >> - dev_err(&pdev->dev, >> - "%pOF has no 'gpmc,device-width' property\n", >> - child); >> - goto err; >> - } >> } >> >> /* Reserve wait pin if it is required and valid */ >> > >
On 15/09/2021 11:11, Roger Quadros wrote: > Hi Krzysztof, > > On 07/09/2021 15:36, Krzysztof Kozlowski wrote: >> On 07/09/2021 13:32, Roger Quadros wrote: >>> Check for valid gpmc,device-width, nand-bus-width and bank-width >>> at one place. Default to 8-bit width if none present. >> >> I don't understand the message in the context of the patch. The title >> says one property is optional - that's it. The message says you >> consolidate checks. How is this related to the title? >> >> The patch itself moves around checking of properties and reads >> nand-bus-width *always*. It does not "check at one place" but rather >> "check always". In the same time, the patch does not remove >> gpmc,device-width check in other place. >> >> All three elements - the title, message and patch - do different things. >> What did you want to achieve here? Can you help in clarifying it? >> > > OK I will explain it better in commit log in next revision. Let me explain here a bit. > > Prior to this patch it was working like this > > /* in gpmc_read_settings_dt() */ > s->device_width = 0; /* invalid width, should be 1 for 8-bit, 2 for 16-bit */ > of_property_read_u32(np, "gpmc,device-width", s->device_width); > > /* in gpmc_probe_generic_child () */ > if (of_device_is_compatible(child, "ti,omap2-nand")) { > /* check for nand-bus-width, if absent set s->device_width to 1 (i.e. 8-bit) */ > } else { > /* check for bank-width, if absent and s->device_width not set, error out */ > } > > So that means if all three, "gpmc,device-width". "nand-bus-width" and "bank-width" are missing then > it would create an error situation. > > The patch is doing 3 things. > 1) Make sure all DT checks related to bus width are being done at one place for better readability. Not entirely. The gpmc,device-width is still done in the other place because you did not remove it from the code. Unless you meant parsing of gpmc,device-width not reading from DT? But then another round of checks is in gpmc_cs_program_settings() so not in one place. If you consolidate the checks to one place, I would expect the code to be removed from other places, so from gpmc_cs_program_settings() and gpmc_read_settings_dt(). Since this is not happening, the message confuses me. > 2) even if all 3 width properties are absent, we will not treat it as error and default to 8-bit. This is not mentioned in commit msg. > 3) check for nand-bus-width regardless of whether compatible to "ti,omap2-nand" or not. Also not mentioned in commit msg. Your commit reorganizes parsing and validating the child DT properties but it does not change from "multiple place" to "one place". At least I don't see it. Best regards, Krzysztof
On 16/09/2021 13:48, Krzysztof Kozlowski wrote: > On 15/09/2021 11:11, Roger Quadros wrote: >> Hi Krzysztof, >> >> On 07/09/2021 15:36, Krzysztof Kozlowski wrote: >>> On 07/09/2021 13:32, Roger Quadros wrote: >>>> Check for valid gpmc,device-width, nand-bus-width and bank-width >>>> at one place. Default to 8-bit width if none present. >>> >>> I don't understand the message in the context of the patch. The title >>> says one property is optional - that's it. The message says you >>> consolidate checks. How is this related to the title? >>> >>> The patch itself moves around checking of properties and reads >>> nand-bus-width *always*. It does not "check at one place" but rather >>> "check always". In the same time, the patch does not remove >>> gpmc,device-width check in other place. >>> >>> All three elements - the title, message and patch - do different things. >>> What did you want to achieve here? Can you help in clarifying it? >>> >> >> OK I will explain it better in commit log in next revision. Let me explain here a bit. >> >> Prior to this patch it was working like this >> >> /* in gpmc_read_settings_dt() */ >> s->device_width = 0; /* invalid width, should be 1 for 8-bit, 2 for 16-bit */ >> of_property_read_u32(np, "gpmc,device-width", s->device_width); >> >> /* in gpmc_probe_generic_child () */ >> if (of_device_is_compatible(child, "ti,omap2-nand")) { >> /* check for nand-bus-width, if absent set s->device_width to 1 (i.e. 8-bit) */ >> } else { >> /* check for bank-width, if absent and s->device_width not set, error out */ >> } >> >> So that means if all three, "gpmc,device-width". "nand-bus-width" and "bank-width" are missing then >> it would create an error situation. >> >> The patch is doing 3 things. >> 1) Make sure all DT checks related to bus width are being done at one place for better readability. > > Not entirely. The gpmc,device-width is still done in the other place > because you did not remove it from the code. Unless you meant parsing of > gpmc,device-width not reading from DT? But then another round of checks > is in gpmc_cs_program_settings() so not in one place. By checking I meant parsing. But you are right, I missed the part in gpmc_cs_program_settings(). > > If you consolidate the checks to one place, I would expect the code to > be removed from other places, so from gpmc_cs_program_settings() and > gpmc_read_settings_dt(). Since this is not happening, the message > confuses me. > >> 2) even if all 3 width properties are absent, we will not treat it as error and default to 8-bit. > > This is not mentioned in commit msg. > >> 3) check for nand-bus-width regardless of whether compatible to "ti,omap2-nand" or not. > > Also not mentioned in commit msg. > > Your commit reorganizes parsing and validating the child DT properties > but it does not change from "multiple place" to "one place". > > At least I don't see it. OK. I will write a better commit log next time. Thanks for the review :) cheers, -roger
diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c index f80c2ea39ca4..32d7c665f33c 100644 --- a/drivers/memory/omap-gpmc.c +++ b/drivers/memory/omap-gpmc.c @@ -2171,10 +2171,8 @@ static int gpmc_probe_generic_child(struct platform_device *pdev, } } - if (of_device_is_compatible(child, "ti,omap2-nand")) { - /* NAND specific setup */ - val = 8; - of_property_read_u32(child, "nand-bus-width", &val); + /* DT node can have "nand-bus-width" or "bank-width" or "gpmc,device-width" */ + if (!of_property_read_u32(child, "nand-bus-width", &val)) { switch (val) { case 8: gpmc_s.device_width = GPMC_DEVWIDTH_8BIT; @@ -2183,24 +2181,37 @@ static int gpmc_probe_generic_child(struct platform_device *pdev, gpmc_s.device_width = GPMC_DEVWIDTH_16BIT; break; default: - dev_err(&pdev->dev, "%pOFn: invalid 'nand-bus-width'\n", - child); + dev_err(&pdev->dev, + "%pOFn: invalid 'nand-bus-width':%d\n", child, val); + ret = -EINVAL; + goto err; + } + } else if (!of_property_read_u32(child, "bank-width", &val)) { + if (val != 1 && val != 2) { + dev_err(&pdev->dev, + "%pOFn: invalid 'bank-width':%d\n", child, val); ret = -EINVAL; goto err; } + gpmc_s.device_width = val; + } else if (!of_property_read_u32(child, "gpmc,device-width", &val)) { + if (val != 1 && val != 2) { + dev_err(&pdev->dev, + "%pOFn: invalid 'gpmc,device-width':%d\n", child, val); + ret = -EINVAL; + goto err; + } + gpmc_s.device_width = val; + } else { + /* default to 8-bit */ + gpmc_s.device_width = GPMC_DEVWIDTH_8BIT; + } + if (of_device_is_compatible(child, "ti,omap2-nand")) { + /* NAND specific setup */ /* disable write protect */ gpmc_configure(GPMC_CONFIG_WP, 0); gpmc_s.device_nand = true; - } else { - ret = of_property_read_u32(child, "bank-width", - &gpmc_s.device_width); - if (ret < 0 && !gpmc_s.device_width) { - dev_err(&pdev->dev, - "%pOF has no 'gpmc,device-width' property\n", - child); - goto err; - } } /* Reserve wait pin if it is required and valid */
Check for valid gpmc,device-width, nand-bus-width and bank-width at one place. Default to 8-bit width if none present. Signed-off-by: Roger Quadros <rogerq@kernel.org> --- drivers/memory/omap-gpmc.c | 41 ++++++++++++++++++++++++-------------- 1 file changed, 26 insertions(+), 15 deletions(-)