RE: [PATCH v5 3/4] drivers: exynos-srom: Add support for bank configuration

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




 Hello!

> > +static int decode_sromc(struct exynos_srom *srom, struct device_node *np)
> 
> I missed that one previously: add prefix and more descriptive name, like:
> exynos_srom_parse_child()

 exynos_srom_configure_bank(), is this name OK?

> >  static int exynos_srom_probe(struct platform_device *pdev)
> >  {
> > -	struct device_node *np;
> > +	struct device_node *np, *child;
> >  	struct exynos_srom *srom;
> >  	struct device *dev = &pdev->dev;
> > +	bool error = false;
> 
> The 'error' name is misleading - like error for entire probe which is
> not true.
> 
> Instead split it to separate function like:
> 
> +static int exynos_srom_parse_children(....) {
> +       int ret = 0;
> +
> +	for_each_child_of_node(np, child) {
> +               ret = exynos_srom_parse_child(srom, child);
> +		if (ret) {
> +			dev_err(dev,
> +				"Could not decode bank configuration for %s: %d\n",
> +				child->name, ret);
> +			break;
> +		}
> +	}
> +
> +       return ret;
> +}

 Factoring out this loop is unnecessary, because i could just 'return 0' in the loop
instead of 'error = true'. Byt my idea is to go through all banks anyway, just in
case, to diagnose all of them. So that the user will be able to spot and fix all
broken banks at once, instead of doing one-by-one.
 I have renamed the variable to 'bool bad_bank_config', will this be OK?

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux