On Mon, Jan 17, 2022 at 4:06 AM Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx> wrote:
> +struct rpi_priv;
> +struct rpi_machine_data {
> + unsigned int hw_id;
> +#define RPI_OLD_SCHEMA BIT(0)
> + unsigned int flags;
hw_id is only a byte. Both of these fields could be u8, which would
make the struct smaller.
> +struct rpi_priv {
> + struct device_d *dev;
> + const struct rpi_machine_data *dcfg;
Doesn't seem like there is any need to have dcfg saved in this struct.
It looks like it's used just once, in the same function that finds the
value. rpi_get_dcfg() could return the value directly, rather than
indirectly by writing it into a field of a struct passed as an
argument.
>
> +static int rpi_get_dcfg(struct rpi_priv *priv)
> +{
> + const struct rpi_machine_data *dcfg;
> + int ret;
> +
> + dcfg = of_device_get_match_data(priv->dev);
> + if (!dcfg) {
> + ret = -EINVAL;
> + goto exit_get_dcfg;
> + }
Then later:
> + ret = rpi_get_dcfg(priv);
> + if (ret)
> + goto free_priv;
It looks like any board that doesn't have match data will be rejected
and fail to init. But then what about these boards:
> static const struct of_device_id rpi_of_match[] = {
> /* BCM2711 based Boards */
> { .compatible = "raspberrypi,400" },
> @@ -465,24 +599,24 @@ static const struct of_device_id rpi_of_match[] = {
> { .compatible = "raspberrypi,4-model-b" },
Looks like they'll get rejected since they have no match data.
> +
> + for (; dcfg->hw_id != UINT_MAX; dcfg++) {
> + if (priv->hw_id & 0x800000) {
> + if (dcfg->hw_id != ((priv->hw_id >> 4) & 0xff))
> + continue;
> + } else {
> + if (!(dcfg->flags & RPI_OLD_SCHEMA))
> + continue;
> + if (dcfg->hw_id != (priv->hw_id & 0xff))
> + continue;
> + }
> +
> + priv->dcfg = dcfg;
> + break;
Could just return 0 here instead of break. Or better, "return dcfg",
as there's no reason not to just return the value like a normal
function.
> + }
> +
> + if (!priv->dcfg) {
> + ret = -ENODEV;
> + goto exit_get_dcfg;
> + }
Then this can become ret = -ENODEV and the if and goto go away.
> +
> + priv = xzalloc(sizeof(*priv));
> + if (!priv)
> + return -ENOMEM;
No need to check the return value of "x" alloc functions. That's the
whole point of the x version.
_______________________________________________
barebox mailing list
barebox@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/barebox
