Re: [PATCH v5 2/2] mtd: spi-nor: cadence-quadspi: Add support for Octal SPI controller

[Vignesh R <vigneshr@ti.com>]

On 10/02/19 6:49 PM, Boris Brezillon wrote:
> On Tue, 5 Feb 2019 11:43:46 +0530
> Vignesh R <vigneshr@ti.com> wrote:
> 
>>>>  static int cqspi_setup_flash(struct cqspi_st *cqspi, struct device_node *np)
>>>>  {
>>>> -	const struct spi_nor_hwcaps hwcaps = {
>>>> -		.mask = SNOR_HWCAPS_READ |
>>>> -			SNOR_HWCAPS_READ_FAST |
>>>> -			SNOR_HWCAPS_READ_1_1_2 |
>>>> -			SNOR_HWCAPS_READ_1_1_4 |
>>>> -			SNOR_HWCAPS_PP,
>>>> -	};
>>>>  	struct platform_device *pdev = cqspi->pdev;
>>>>  	struct device *dev = &pdev->dev;
>>>> +	const struct cqspi_driver_platdata *ddata;
>>>> +	struct spi_nor_hwcaps hwcaps;
>>>>  	struct cqspi_flash_pdata *f_pdata;
>>>>  	struct spi_nor *nor;
>>>>  	struct mtd_info *mtd;
>>>>  	unsigned int cs;
>>>>  	int i, ret;
>>>>  
>>>> +	ddata = of_device_get_match_data(dev);
>>>> +	if (!ddata)
>>>> +		hwcaps.mask = CQSPI_BASE_HWCAPS_MASK;  
>>>
>>> Now that .data is set in all cqspi_dt_ids[], maybe it's better to print a
>>> message and return an error here. But I guess it's a matter of taste, so not a
>>> show stopper.  
>>
>> Since, driver data is kernel internal field, I guess there is little
>> help in printing out the error to the user when its missing. I prefer to
>> keep this as is, as basic Quad mode is supported by all versions of the IP.
> 
> Well, if all compat entries have an associated platdata instance we
> don't need to support the !ddata case, right? I think enforcing the
> presence of such a platdata is actually is a sane thing to do (which
> implies returning an error when ddata is NULL).
> 

Ok, will respin with a dev_err() and return -EINVAL when ddata is NULL.

-- 
Regards
Vignesh