Re: [PATCH v3 1/2] mfd: arizona: Update reset pin to use GPIOD

[Charles Keepax]

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