[PATCH v4 3/3] mtd: nand: jz4780: driver for NAND devices on JZ4780 SoCs

[Alex Smith]

Hi,

On 06/09/2015 22:21, Ezequiel Garcia wrote:
> On 27 Jul 03:21 PM, Alex Smith wrote:
>> Add a driver for NAND devices connected to the NEMC on JZ4780 SoCs, as
>> well as the hardware BCH controller. DMA is not currently implemented.
>>
>> While older 47xx SoCs also have a BCH controller, they are incompatible
>> with the one in the 4780 due to differing register/bit positions, which
>> would make implementing a common driver for them quite messy.
>>
> 
> If the difference is only in register/bit positions, a common driver
> might be fairly simple. See drivers/i2c/busses/i2c-mv64xxx.c,
> which supports two different register layouts.

I've just gone back and looked at the older SoCs and it doesn't seem as though this commit message really applies to the JZ4740, which is the only other Ingenic SoC currently supported upstream. The 4740 doesn't have a BCH controller at all and the NAND interface is fairly different. I think this driver could potentially be reused if support for the JZ4770 makes it upstream, for now though a separate driver is certainly needed for the 4780.


>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id jz4780_bch_dt_match[] = {
>> +	{ .compatible = "ingenic,jz4780-bch" },
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, jz4780_bch_dt_match);
>> +
>> +static struct platform_driver jz4780_bch_driver = {
>> +	.probe		= jz4780_bch_probe,
> 
> Why no remove?

Is it needed? Everything should be cleaned up due to the use of devm functions.


>> +static int jz4780_nand_init_chips(struct jz4780_nand *nand, struct device *dev)
>> +{
>> +	struct jz4780_nand_chip *chip;
>> +	const __be32 *prop;
>> +	u64 addr, size;
>> +	int i = 0;
>> +
>> +	/*
>> +	 * Iterate over each bank assigned to this device and request resources.
>> +	 * The bank numbers may not be consecutive, but nand_scan_ident()
>> +	 * expects chip numbers to be, so fill out a consecutive array of chips
>> +	 * which map chip number to actual bank number.
>> +	 */
>> +	while ((prop = of_get_address(dev->of_node, i, &size, NULL))) {
>> +		chip = &nand->chips[i];
>> +		chip->bank = of_read_number(prop, 1);
>> +
>> +		jz4780_nemc_set_type(nand->dev, chip->bank,
>> +				     JZ4780_NEMC_BANK_NAND);
>> +
>> +		addr = of_translate_address(dev->of_node, prop);
> 
> Are you sure you must translate the address yourself?
> Isn't this handled by the OF magic behing the ranges property
> in the NEMC DT node?

I think the reasoning behind doing this was because I already have to get the address property here in order to get the bank number out of it.

You're right though that I can just do "platform_get_resource(pdev, i)" and avoid doing the translation again, so I have changed it to do that.

I've fixed the rest of your comments as well.

Thanks,
Alex