On Sat, Jul 13, 2024 at 10:13 AM Simon Glass <[email protected]> wrote: > > Hi Sam, >
Hi Simon, Thank you for the review! > On Sat, 13 Jul 2024 at 00:44, Sam Protsenko <[email protected]> > wrote: > > +config RNG_EXYNOS > > + bool "Samsung Exynos True Random Number Generator support" > > + depends on DM_RNG > > + help > > + Enable support for True Random Number Generator (TRNG) > > + available in Exynos SoCs. > > Can you mention the needed firmware here? > Will do in v2. > > + > > +struct exynos_trng_variant { > > please add comments > Will do in v2. > > + bool smc; > > + int (*init)(struct udevice *dev); > > + void (*exit)(struct udevice *dev); > > + int (*read)(struct udevice *dev, void *data, size_t len); > > +}; > > + > > +struct exynos_trng { > > The convention is to add a _priv suffix > Will add in v2. > > +static int exynos_trng_init_smc(struct udevice *dev) > > +{ > > + struct arm_smccc_res res; > > + int ret = 0; > > + > > + arm_smccc_smc(SMC_CMD_RANDOM, HWRNG_INIT, 0, 0, 0, 0, 0, 0, &res); > > + if (res.a0 != HWRNG_RET_OK) { > > + dev_err(dev, "SMC command for TRNG init failed (%d)\n", > > + (int)res.a0); > > + ret = -EIO; > > + } > > + if ((int)res.a0 == -1) > > + dev_info(dev, "Make sure LDFW is loaded\n"); > > return error here? > It's already done above: (res.a0 != HWRNG_RET_OK) condition includes -1 case. This line is basically only to help user figure out what might be wrong. > > +static int exynos_trng_of_to_plat(struct udevice *dev) > > +{ > > + struct exynos_trng *trng = dev_get_priv(dev); > > + > > + trng->data = (struct exynos_trng_variant *)dev_get_driver_data(dev); > > + if (!trng->data->smc) { > > + trng->base = dev_read_addr_ptr(dev); > > + if (!trng->base) > > + return -ENODEV; > > That has a special meaning in U-Boot (i.e. that there is no device). > Devicetree problems should return -EINVAL > Will fix in v2. > > + } > > + > > + trng->clk = devm_clk_get(dev, "secss"); > > + if (IS_ERR(trng->clk)) > > + return -ENODEV; > > Should return the actual error > Will fix in v2. > > + > > + trng->pclk = devm_clk_get_optional(dev, "pclk"); > > + if (IS_ERR(trng->pclk)) > > + return -ENODEV; > > same here > Will fix in v2. > > + > > + return 0; > > +} > > + > > +static int exynos_trng_probe(struct udevice *dev) > > +{ > > + struct exynos_trng *trng = dev_get_priv(dev); > > + int err; > > s/err/ret/ > Will fix in v2. [snip] > > Regards, > Simon
