Re: [PATCH v2] mtd: hyperbus: add Renesas RPC-IF driver

[Vignesh Raghavendra <vigneshr@ti.com>]

On 9/15/20 3:07 PM, Behme Dirk (CM/ESO2) wrote:
> 
> 
> On 15.09.2020 07:27, Vignesh Raghavendra wrote:
>>
>>
>> On 9/15/20 9:37 AM, Behme Dirk (CM/ESO2) wrote:
>>>
>>>
>>> On 14.09.2020 15:09, Vignesh Raghavendra wrote:
>>>> Hi Sergei,
>>>>
>>>> On 5/14/20 2:13 AM, Sergei Shtylyov wrote:
>>>>> Add the HyperFLash driver for the Renesas RPC-IF.  It's the "front
>>>>> end"
>>>>> driver using the "back end" APIs in the main driver to talk to the
>>>>> real
>>>>> hardware.
>>>>>
>>>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>> [...]
>>>>>
>>>>>
>>>> [...]
>>>>> --- /dev/null
>>>>> +++ linux/drivers/mtd/hyperbus/rpc-if.c
>>>>> @@ -0,0 +1,165 @@
>>>>> +// SPDX-License-Identifier: GPL-2.0
>>>>> +/*
>>>>> + * Linux driver for RPC-IF HyperFlash
>>>>> + *
>>>>> + * Copyright (C) 2019-2020 Cogent Embedded, Inc.
>>>>> + */
>>>>> +
>>>>> +#include <linux/err.h>
>>>>> +#include <linux/kernel.h>
>>>>> +#include <linux/module.h>
>>>>> +#include <linux/mtd/hyperbus.h>
>>>>> +#include <linux/mtd/mtd.h>
>>>>> +#include <linux/mux/consumer.h>
>>>>> +#include <linux/of.h>
>>>>> +#include <linux/platform_device.h>
>>>>> +#include <linux/types.h>
>>>>> +
>>>>> +#include <memory/renesas-rpc-if.h>
>>>>> +
>>>>> +/* FIXME: How to drop this? */
>>>>> +#ifndef CONFIG_MTD_CFI_BE_BYTE_SWAP
>>>>> +#error Enable config "Flash cmd/query data swapping
>>>>> (BIG_ENDIAN_BYTE)"
>>>>> +#endif
>>>>
>>>> select MTD_CFI_BE_BYTE_SWAP in Kconfig does not help?
>>>
>>>
>>> If I remember correctly 'select MTD_CFI_BE_BYTE_SWAP' in Kconfig doesn't
>>> seem to work. Therefore the FIXME. I can't remember exactly the root
>>> cause for that any more, but maybe it was the 'bool' type of
>>> MTD_CFI_BE_BYTE_SWAP?
>>>
>>
>> Ah, sorry MTD_CFI_BE_BYTE_SWAP is a choice and select does not work on
>> choice menus. how about:
>>
>>> If anybody has a nice solution for that, it would be welcome :)
>>
>> Does this work:
>>
>>   config RPCIF_HYPERBUS
>>          tristate "Renesas RPC-IF HyperBus driver"
>>          depends on RENESAS_RPCIF
>> -       select MTD_CFI_ADV_OPTIONS
>> +       depends on MTD_CFI_BE_BYTE_SWAP || COMPILE_TEST
>>          help
>>            This option includes Renesas RPC-IF HyperFlash support.
> 
> 
> Well, it at least makes the whole RPCIF_HYPERBUS option to vanish in
> case the (quite uncommon?) MTD_CFI_BE_BYTE_SWAP is not set, no? Or
> rephrased: How to make the potential user aware that
> MTD_CFI_BE_BYTE_SWAP needs to be enabled to make this option at least
> visible? 

User can search for RPCIF_HYPERBUS symbol (say in menuconfig) and
dependencies will show up.


> From that point of view I feel the #error as given above more
> user friendly: RPCIF_HYPERBUS is there an can be enabled, but compiling
> is failing with a reasonable message if the dependencies are not met.
> 

No, this is uncommon.. If a driver needs a symbol to be enabled for it
to work, then Kconfig entry should indicate so with "depends on" vs
using "#error" clause.

Using "select" to force enable all dependencies is not recommended
either as it does not take care of dependencies of the symbol being
selected. "select" is generally is used for symbols that are not user
selectable (in menuconfig).

Drivers along with required dependencies for a platform can be added to
appropriate defconfig (See arch/*/configs/) to make end users job easy


Regards
Vignesh

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/