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

From: Claudiu.Beznea
Date: Fri Feb 17 2023 - 06:24:55 EST


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))