On Wed, Mar 07, 2018 at 01:28:12PM +0000, Lee Jones wrote:
> On Tue, 20 Feb 2018, Charles Keepax wrote:
> > + if (IS_ERR(pdata->reset)) {
> > + ret = PTR_ERR(pdata->reset);
> > + switch (ret) {
> > + case -ENOENT:
> > + case -ENOSYS:
> > + break;
> > + case -EPROBE_DEFER:
> > + return ret;
> > + default:
> > + dev_err(arizona->dev, "Reset GPIO malformed: %d\n",
> > + ret);
> > + break;
> > + }
>
> I haven't seen a switch statement used in the error handling path
> before. There is probably good reason for that.
>
There are a fair few of them "grep -rI "switch (ret)" | wc -l" ==
205. Although to be fair that has rarely been an argument in
somethings favour.
> What is the 'default' case? -EINVAL?
>
I would guess most of the time yes, but I would rather not assume
that. I can redo this as if's if you prefer it that way? The if is
slightly less lines although I do think the switch is much
clearer as to intent.
if (ret == -EPROBE_DEFER) {
return ret;
} else if (ret != -ENOENT && ret != -ENOSYS {
dev_err(arizona->dev, ....);
}
> > - if (arizona->pdata.reset) {
> > + if (!arizona->pdata.reset) {
> > /* Start out with /RESET low to put the chip into reset */
> > - ret = devm_gpio_request_one(arizona->dev, arizona->pdata.reset,
> > - GPIOF_DIR_OUT | GPIOF_INIT_LOW,
> > - "arizona /RESET");
> > - if (ret != 0) {
> > - dev_err(dev, "Failed to request /RESET: %d\n", ret);
> > - goto err_dcvdd;
> > + arizona->pdata.reset = devm_gpiod_get(arizona->dev, "reset",
> > + GPIOD_OUT_LOW);
> > + if (IS_ERR(arizona->pdata.reset)) {
> > + ret = PTR_ERR(arizona->pdata.reset);
> > + if (ret == -EPROBE_DEFER)
> > + goto err_dcvdd;
> > + else
> > + dev_err(arizona->dev,
> > + "Reset GPIO missing/malformed: %d\n",
> > + ret);
>
> You don't need the else.
Happy to drop.
> > --- a/include/linux/mfd/arizona/pdata.h
> > +++ b/include/linux/mfd/arizona/pdata.h
> > @@ -56,6 +56,7 @@
> > #define ARIZONA_MAX_PDM_SPK 2
> >
> > struct regulator_init_data;
> > +struct gpio_desc;
> >
> > struct arizona_micbias {
> > int mV; /** Regulated voltage */
> > @@ -77,7 +78,7 @@ struct arizona_micd_range {
> > };
> >
> > struct arizona_pdata {
> > - int reset; /** GPIO controlling /RESET, if any */
> > + struct gpio_desc *reset; /** GPIO controlling /RESET, if any */
> >
> > /** Regulator configuration for MICVDD */
> > struct arizona_micsupp_pdata micvdd;
>
> I know it doesn't have much to do with this patch, but it's probably
> better to use a Kerneldoc header for this struct.
>
Indeed I would agree that would be better, not sure what the
commenting style there is about. I should be able to find time to
make a patch for this soon, but seems unrelated to this series.
Thanks,
Charles
--
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
