[PATCH 1/2] nvmem: NXP LPC18xx EEPROM memory NVMEM driver

[Joachim Eastwood]

Hi Ariel,

Thanks for working on this.

On 16 October 2015 at 16:07, Ariel D'Alessandro
<ariel at vanguardiasur.com.ar> wrote:
> This commit adds support for NXP LPC18xx EEPROM memory found in NXP LPC
> SoCs family, which includes LPC18xx/LPC43xx. Other SoCs in that family
> may share the same watchdog hardware.

Only LPC185x/3x and LPC435x/3x/2x/1x devices have the EEPROM.


> EEPROM size is 16384 bytes and it can be entirely read and
> written/erased with 1 word (4 bytes) granularity. The last page
> (128 bytes) contains the EEPROM initialization data and is not writable.
>
> Erase/program time is less than 3ms. The EEPROM device requires a
> ~1500 kHz clock (min 800 kHz, max 1600 kHz) that is generated dividing
> the system bus clock by the division factor, contained in the divider
> register (minus 1 encoded).

Nice description of the device.


> +static inline void lpc18xx_eeprom_writel(struct lpc18xx_eeprom_dev *eeprom,
> +                                        u32 reg, u32 val)
> +{
> +       writel(val, eeprom->reg_base + reg);
> +}
> +
> +static inline u32 lpc18xx_eeprom_readl(struct lpc18xx_eeprom_dev *eeprom,
> +                                      u32 reg)
> +{
> +       return readl(eeprom->reg_base + reg);
> +}

Read function seems to be unused.

Are these two wrappers really useful?


> +static int lpc18xx_eeprom_gather_write(void *context, const void *reg,
> +                                      size_t reg_size, const void *val,
> +                                      size_t val_size)
> +{
> +       struct lpc18xx_eeprom_dev *eeprom = context;
> +       unsigned int offset = *(u32 *)reg;
> +
> +       /* 3 ms of erase/program time between each writing */
> +       while (val_size) {
> +               writel(*(u32 *)val, eeprom->mem_base + offset);
> +               mdelay(3);

That's a long mdelay. Is it possible to sleep instead?


> +               val_size -= eeprom->val_bytes;
> +               val += eeprom->val_bytes;
> +               offset += eeprom->val_bytes;
> +       }
> +
> +       return 0;
> +}
> +
> +static int lpc18xx_eeprom_write(void *context, const void *data, size_t count)
> +{
> +       struct lpc18xx_eeprom_dev *eeprom = context;
> +       unsigned int offset = eeprom->reg_bytes;
> +
> +       BUG_ON(count <= offset);

BUG_ON seems a bit harsh. Could you return an error instead?


> +       return lpc18xx_eeprom_gather_write(context, data, eeprom->reg_bytes,
> +                                          data + offset, count - offset);
> +}
...
> +static bool lpc18xx_eeprom_writeable_reg(struct device *dev, unsigned int reg)
> +{
> +       /*
> +        * The last page contains the EEPROM initialization data and is not
> +        * writable.
> +        */
> +       return (reg <= lpc18xx_regmap_config.max_register -
> +                                               LPC18XX_EEPROM_PAGE_SIZE);

Unnecessary  parenthesis.

> +}
> +
> +static bool lpc18xx_eeprom_readable_reg(struct device *dev, unsigned int reg)
> +{
> +       return (reg <= lpc18xx_regmap_config.max_register);
> +}

Unnecessary  parenthesis.

> +
> +static struct nvmem_config lpc18xx_nvmem_config = {
> +       .name = "lpc18xx-eeprom",
> +       .owner = THIS_MODULE,
> +};
> +
> +static const struct of_device_id lpc18xx_eeprom_of_match[] = {
> +       { .compatible = "nxp,lpc1850-eeprom" },

Use lpc1857 as noted in the other thread.

> +       { },
> +};
> +MODULE_DEVICE_TABLE(of, lpc18xx_eeprom_of_match);


regards,
Joachim Eastwood