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