diff mbox series

[v3,8/8] memory: gpmc-omap: "gpmc,device-width" DT property is optional

Message ID 20210907113226.31876-9-rogerq@kernel.org (mailing list archive)
State New, archived
Headers show
Series dt-bindings: memory-controllers: ti,gpmc: Convert to yaml | expand

Commit Message

Roger Quadros Sept. 7, 2021, 11:32 a.m. UTC
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(-)

Comments

Krzysztof Kozlowski Sept. 7, 2021, 12:36 p.m. UTC | #1
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 */
>
Roger Quadros Sept. 15, 2021, 9:11 a.m. UTC | #2
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 */
>>
> 
>
Krzysztof Kozlowski Sept. 16, 2021, 10:48 a.m. UTC | #3
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
Roger Quadros Sept. 17, 2021, 7:17 a.m. UTC | #4
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 mbox series

Patch

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 */