Re: [PATCH v2 1/2] pinctrl: at91-pio4: Implement the correct drive values for sama7g5

[<Claudiu.Beznea@xxxxxxxxxxxxx>]

On 17.02.2023 00:25, Ryan.Wanner@xxxxxxxxxxxxx wrote:
> From: Ryan Wanner <Ryan.Wanner@xxxxxxxxxxxxx>
> 
> Sama7g5 drive strength options have been updated from previous pio4
> products. Now values will correctly align with sama7g5 drive strength
> options highlighted in the sama7g5 data sheet.
> 
> Add xlate to separate the sama7g5 drive values and the sama5d27
> drive values.
> 
> Signed-off-by: Ryan Wanner <Ryan.Wanner@xxxxxxxxxxxxx>
> ---
>  drivers/pinctrl/pinctrl-at91-pio4.c | 27 ++++++++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-at91-pio4.c b/drivers/pinctrl/pinctrl-at91-pio4.c
> index 82b921fd630d..6b5a753ccd37 100644
> --- a/drivers/pinctrl/pinctrl-at91-pio4.c
> +++ b/drivers/pinctrl/pinctrl-at91-pio4.c
> @@ -72,17 +72,32 @@
>  /* Custom pinconf parameters */
>  #define ATMEL_PIN_CONFIG_DRIVE_STRENGTH	(PIN_CONFIG_END + 1)
>  
> +static const u32 drive_strength_xlate_sama7g5[4] = {

You can get rid of array length here.

> +	ATMEL_PIO_DRVSTR_LO_D,
> +	ATMEL_PIO_DRVSTR_HI_A,
> +	ATMEL_PIO_DRVSTR_LO_C,
> +	ATMEL_PIO_DRVSTR_LO_B
> +};
> +
> +static const u32 drive_strength_xlate_sama5d2[4] = {

ditto

> +	ATMEL_PIO_DRVSTR_LO,
> +	ATMEL_PIO_DRVSTR_LO_1,
> +	ATMEL_PIO_DRVSTR_ME,
> +	ATMEL_PIO_DRVSTR_HI
> +};

missing blank line.

>  /**
>   * struct atmel_pioctrl_data - Atmel PIO controller (pinmux + gpio) data struct
>   * @nbanks: number of PIO banks
>   * @last_bank_count: number of lines in the last bank (can be less than
>   *	the rest of the banks).
>   * @slew_rate_support: slew rate support
> + * @drv_stre_xlate: xlate for different drive values
>   */
>  struct atmel_pioctrl_data {
>  	unsigned int nbanks;
>  	unsigned int last_bank_count;
>  	unsigned int slew_rate_support;
> +	const unsigned int *drv_stre_xlate;

you're using unsigned int here and u32 in array definition. Can you fit
with a common type?

>  };
>  
>  struct atmel_group {
> @@ -121,6 +136,7 @@ struct atmel_pin {
>   * @dev: device entry for the Atmel PIO controller.
>   * @node: node of the Atmel PIO controller.
>   * @slew_rate_support: slew rate support
> + * @drv_stre_xlate: xlate for different drive values
>   */
>  struct atmel_pioctrl {
>  	void __iomem		*reg_base;
> @@ -143,6 +159,7 @@ struct atmel_pioctrl {
>  	struct device		*dev;
>  	struct device_node	*node;
>  	unsigned int		slew_rate_support;
> +	const unsigned int	*drv_stre_xlate;

same here.

>  };
>  
>  static const char * const atmel_functions[] = {
> @@ -872,12 +889,17 @@ static int atmel_conf_pin_config_group_set(struct pinctrl_dev *pctldev,
>  				conf &= ~ATMEL_PIO_SR_MASK;
>  			break;
>  		case ATMEL_PIN_CONFIG_DRIVE_STRENGTH:
> +			if (arg > ATMEL_PIO_DRVSTR_HI || arg < ATMEL_PIO_DRVSTR_LO) {
> +				dev_warn(pctldev->dev, "drive strength not updated (incorrect value)\n");
> +				break;
> +			}
>  			switch (arg) {
>  			case ATMEL_PIO_DRVSTR_LO:
> +			case ATMEL_PIO_DRVSTR_LO_1:

You may want to touch also atmel_conf_pin_config_dbg_show()?

>  			case ATMEL_PIO_DRVSTR_ME:
>  			case ATMEL_PIO_DRVSTR_HI:
>  				conf &= (~ATMEL_PIO_DRVSTR_MASK);
> -				conf |= arg << ATMEL_PIO_DRVSTR_OFFSET;
> +				conf |= atmel_pioctrl->drv_stre_xlate[arg] << ATMEL_PIO_DRVSTR_OFFSET;
>  				break;
>  			default:
>  				dev_warn(pctldev->dev, "drive strength not updated (incorrect value)\n");
> @@ -1013,12 +1035,14 @@ static const struct dev_pm_ops atmel_pctrl_pm_ops = {
>  static const struct atmel_pioctrl_data atmel_sama5d2_pioctrl_data = {
>  	.nbanks			= 4,
>  	.last_bank_count	= ATMEL_PIO_NPINS_PER_BANK,
> +	.drv_stre_xlate		= drive_strength_xlate_sama5d2,
>  };
>  
>  static const struct atmel_pioctrl_data microchip_sama7g5_pioctrl_data = {
>  	.nbanks			= 5,
>  	.last_bank_count	= 8, /* sama7g5 has only PE0 to PE7 */
>  	.slew_rate_support	= 1,
> +	.drv_stre_xlate		= drive_strength_xlate_sama7g5,
>  };
>  
>  static const struct of_device_id atmel_pctrl_of_match[] = {
> @@ -1064,6 +1088,7 @@ static int atmel_pinctrl_probe(struct platform_device *pdev)
>  		atmel_pioctrl->npins += atmel_pioctrl_data->last_bank_count;
>  	}
>  	atmel_pioctrl->slew_rate_support = atmel_pioctrl_data->slew_rate_support;
> +	atmel_pioctrl->drv_stre_xlate = atmel_pioctrl_data->drv_stre_xlate;
>  
>  	atmel_pioctrl->reg_base = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(atmel_pioctrl->reg_base))