Re: [PATCH 3/5] gpio: mvebu: add pwm support for Armada 8K/7K

[Baruch Siach <baruch@tkos.co.il>]

Hi Andrew,

On Thu, Nov 19 2020, Andrew Lunn wrote:
> On Wed, Nov 18, 2020 at 12:30:44PM +0200, Baruch Siach wrote:
>> Use the pwm-offset DT property to store the location of PWM signal
>> duration registers.
>> 
>> Since we have more than two GPIO chips per system, we can't use the
>> alias id to differentiate between them. Use the offset value for that.
>> 
>> Move mvebu_pwm_probe() call before irq support code. The AP80x does not
>> provide irq support, but does provide PWM. Don't skip PWM probe because
>> of that.
>> 
>> Signed-off-by: Baruch Siach <baruch@tkos.co.il>

[snip]

>> @@ -781,51 +787,80 @@ static int mvebu_pwm_probe(struct platform_device *pdev,
>>  	struct device *dev = &pdev->dev;
>>  	struct mvebu_pwm *mvpwm;
>>  	void __iomem *base;
>> +	u32 offset;
>>  	u32 set;
>>  
>> -	if (!of_device_is_compatible(mvchip->chip.of_node,
>> -				     "marvell,armada-370-gpio"))
>> -		return 0;
>> -
>> -	/*
>> -	 * There are only two sets of PWM configuration registers for
>> -	 * all the GPIO lines on those SoCs which this driver reserves
>> -	 * for the first two GPIO chips. So if the resource is missing
>> -	 * we can't treat it as an error.
>> -	 */
>> -	if (!platform_get_resource_byname(pdev, IORESOURCE_MEM, "pwm"))
>> +	if (of_device_is_compatible(mvchip->chip.of_node,
>> +				    "marvell,armada-370-gpio")) {
>> +		/*
>> +		 * There are only two sets of PWM configuration registers for
>> +		 * all the GPIO lines on those SoCs which this driver reserves
>> +		 * for the first two GPIO chips. So if the resource is missing
>> +		 * we can't treat it as an error.
>> +		 */
>> +		if (!platform_get_resource_byname(pdev, IORESOURCE_MEM, "pwm"))
>> +			return 0;
>> +		offset = 0;
>> +	} else if (mvchip->soc_variant == MVEBU_GPIO_SOC_VARIANT_A8K) {
>> +		int ret = of_property_read_u32(dev->of_node, "pwm-offset",
>> +					       &offset);
>> +		if (ret < 0)
>> +			return 0;
>
> It would look more uniform if this was
>
> 	if (of_device_is_compatible(mvchip->chip.of_node,
> 				    "marvell,armada-8k-gpio")) {

Right. However I use soc_variant again below. I think that
of_device_is_compatible is too verbose for that.

In fact, I'd rather use soc_variant for marvell,armada-370-gpio as
well. The trouble is that marvell,armada-370-gpio is not equivalent to
MVEBU_GPIO_SOC_VARIANT_ORION. Changing that is more intrusive.

>> +	} else {
>>  		return 0;
>> +	}

[snip]

>> @@ -1200,6 +1235,13 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
>>  
>>  	devm_gpiochip_add_data(&pdev->dev, &mvchip->chip, mvchip);
>>  
>> +	/* Some MVEBU SoCs have simple PWM support for GPIO lines */
>> +	if (IS_ENABLED(CONFIG_PWM)) {
>> +		err = mvebu_pwm_probe(pdev, mvchip, id);
>> +		if (err)
>> +			return err;
>> +	}
>> +
>
> The existing error handling looks odd here. Why is there no goto
> err_domain when probing the PWMs fails? I wonder if this a bug from me
> from a long time again?

What would you release under the err_domain label? As far as I can see
all resources are allocated using devres, and released automatically on
failure exit.

baruch

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -