Re: [PATCH v2 2/3] pinctrl: bcm2835: Add support for generic pinctrl binding
Hi Eric,
thanks for reviewing it. I will send the v3 of the patch with your notes.
I will just wait for the Linus considerations.
Best Regards
Matheus Castello
On 03/05/2018 07:21 PM, Eric Anholt wrote:
> Matheus Castello writes:
>
>> To keep driver up to date we add generic pinctrl binding support, which
>> covers
>> the features used in this driver and has additional node properties that this
>> SoC has compatibility, so enabling future implementations of these properties
>> without the need to create new node properties in the device trees.
>>
>> The logic of this change maintain the old brcm legacy binding support in
>> order
>> to keep the ABI stable.
>>
>> Signed-off-by: Matheus Castello
>> ---
>>
>> A brief explanation of what I did:
>>
>> Add pinconf-generic header for use the defines and pinctrl-generic API.
>>
>> Add dt-bindings pinctrl bcm2835 header to use functions selections and
>> pulls definitions, which functions definitions where duplicated in the
>> enum bcm2835_fsel, I removed the duplicate defines from enum.
>>
>> In the bcm2835_pctl_dt_node_to_map_pull I used the generic macro for
>> pack the legacy param and arguments, since it will be unpacked along with
>> generic properties that is packed with this same macro.
>>
>> In bcm2835_pctl_dt_node_to_map I thougt it was better, and simpler, to use
>> pinctrl-generic parse code instead of parsing it inside the driver, so code
>> first check for generic binding parse, if something is parsed then it is
>> assumed that are using the new generic style, and when nothing is found then
>> parse continues to search for legacy properties.
>>
>> In the bcm2835_pinconf_set was changed the unpack legacy by the generic, and
>> was added a switch for the parameter tests, since pinctrl generic uses 3
>> properties to define the states of the pull instead of one with arguments,
>> that
>> was the reason too that bcm2835_pull_config_set function was added, for reuse
>> the code that set state of pull.
>>
>> drivers/pinctrl/bcm/pinctrl-bcm2835.c | 83
>> +++
>> 1 file changed, 54 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c
>> b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
>> index 785c366..755ea90 100644
>> --- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c
>> +++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
>> @@ -36,11 +36,13 @@
>> #include
>> #include
>> #include
>> +#include
>> #include
>> #include
>> #include
>> #include
>> #include
>> +#include
>>
>> #define MODULE_NAME "pinctrl-bcm2835"
>> #define BCM2835_NUM_GPIOS 54
>> @@ -213,14 +215,6 @@ static const char * const bcm2835_gpio_groups[] = {
>> };
>>
>> enum bcm2835_fsel {
>> -BCM2835_FSEL_GPIO_IN = 0,
>> -BCM2835_FSEL_GPIO_OUT = 1,
>> -BCM2835_FSEL_ALT0 = 4,
>> -BCM2835_FSEL_ALT1 = 5,
>> -BCM2835_FSEL_ALT2 = 6,
>> -BCM2835_FSEL_ALT3 = 7,
>> -BCM2835_FSEL_ALT4 = 3,
>> -BCM2835_FSEL_ALT5 = 2,
>> BCM2835_FSEL_COUNT = 8,
>> BCM2835_FSEL_MASK = 0x7,
>> };
>> @@ -714,7 +708,7 @@ static int bcm2835_pctl_dt_node_to_map_pull(struct
>> bcm2835_pinctrl *pc,
>> configs = kzalloc(sizeof(*configs), GFP_KERNEL);
>> if (!configs)
>> return -ENOMEM;
>> -configs[0] = BCM2835_PINCONF_PACK(BCM2835_PINCONF_PARAM_PULL, pull);
>> +configs[0] = PIN_CONF_PACKED(BCM2835_PINCONF_PARAM_PULL, pull);
>>
>> map->type = PIN_MAP_TYPE_CONFIGS_PIN;
>> map->data.configs.group_or_pin = bcm2835_gpio_pins[pin].name;
>> @@ -736,6 +730,12 @@ static int bcm2835_pctl_dt_node_to_map(struct
>> pinctrl_dev *pctldev,
>> int i, err;
>> u32 pin, func, pull;
>>
>> +/* Check for generic binding in this node */
>> +err = pinconf_generic_dt_node_to_map_all(pctldev, np, map, num_maps);
>> +if (err || *num_maps)
>> +return err;
>> +
>> +/* Generic binding did not find anything continue with legacy parse */
>> pins = of_find_property(np, "brcm,pins", NULL);
>> if (!pins) {
>> dev_err(pc->dev, "%pOF: missing brcm,pins property\n", np);
>> @@ -917,37 +917,62 @@ static int bcm2835_pinconf_get(struct pinctrl_dev
>> *pctldev,
>> return -ENOTSUPP;
>> }
>>
>> +static void bcm2835_pull_config_set(struct bcm2835_pinctrl *pc,
>> +unsigned pin, unsigned arg)
>> +{
>> +u32 off, bit;
>> +
>> +off = GPIO_REG_OFFSET(pin);
>> +bit = GPIO_REG_SHIFT(pin);
>> +
>> +bcm2835_gpio_wr(pc, GPPUD, arg & 3);
>> +/*
>> +* BCM2835 datasheet say to wait 150 cycles, but not of what.
>> +* But the VideoCore firmware delay for this operation
>> +* based nearly on the same amount of VPU cycles and this clock
>> +* runs at 250 MHz.
>> +*/
>> +udelay(1);
>> +bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), BIT(bit));
>> +udelay(1);
>> +bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), 0);
>> +}
>> +
>> static int bcm2835_pinconf_set(struct pinctrl_dev *pctldev,
>> unsign
Re: [PATCH v2 2/3] pinctrl: bcm2835: Add support for generic pinctrl binding
Matheus Castello writes:
> To keep driver up to date we add generic pinctrl binding support, which covers
> the features used in this driver and has additional node properties that this
> SoC has compatibility, so enabling future implementations of these properties
> without the need to create new node properties in the device trees.
>
> The logic of this change maintain the old brcm legacy binding support in order
> to keep the ABI stable.
>
> Signed-off-by: Matheus Castello
> ---
>
> A brief explanation of what I did:
>
> Add pinconf-generic header for use the defines and pinctrl-generic API.
>
> Add dt-bindings pinctrl bcm2835 header to use functions selections and
> pulls definitions, which functions definitions where duplicated in the
> enum bcm2835_fsel, I removed the duplicate defines from enum.
>
> In the bcm2835_pctl_dt_node_to_map_pull I used the generic macro for
> pack the legacy param and arguments, since it will be unpacked along with
> generic properties that is packed with this same macro.
>
> In bcm2835_pctl_dt_node_to_map I thougt it was better, and simpler, to use
> pinctrl-generic parse code instead of parsing it inside the driver, so code
> first check for generic binding parse, if something is parsed then it is
> assumed that are using the new generic style, and when nothing is found then
> parse continues to search for legacy properties.
>
> In the bcm2835_pinconf_set was changed the unpack legacy by the generic, and
> was added a switch for the parameter tests, since pinctrl generic uses 3
> properties to define the states of the pull instead of one with arguments,
> that
> was the reason too that bcm2835_pull_config_set function was added, for reuse
> the code that set state of pull.
>
> drivers/pinctrl/bcm/pinctrl-bcm2835.c | 83
> +++
> 1 file changed, 54 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c
> b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
> index 785c366..755ea90 100644
> --- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c
> +++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
> @@ -36,11 +36,13 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> #include
> #include
> +#include
>
> #define MODULE_NAME "pinctrl-bcm2835"
> #define BCM2835_NUM_GPIOS 54
> @@ -213,14 +215,6 @@ static const char * const bcm2835_gpio_groups[] = {
> };
>
> enum bcm2835_fsel {
> - BCM2835_FSEL_GPIO_IN = 0,
> - BCM2835_FSEL_GPIO_OUT = 1,
> - BCM2835_FSEL_ALT0 = 4,
> - BCM2835_FSEL_ALT1 = 5,
> - BCM2835_FSEL_ALT2 = 6,
> - BCM2835_FSEL_ALT3 = 7,
> - BCM2835_FSEL_ALT4 = 3,
> - BCM2835_FSEL_ALT5 = 2,
> BCM2835_FSEL_COUNT = 8,
> BCM2835_FSEL_MASK = 0x7,
> };
> @@ -714,7 +708,7 @@ static int bcm2835_pctl_dt_node_to_map_pull(struct
> bcm2835_pinctrl *pc,
> configs = kzalloc(sizeof(*configs), GFP_KERNEL);
> if (!configs)
> return -ENOMEM;
> - configs[0] = BCM2835_PINCONF_PACK(BCM2835_PINCONF_PARAM_PULL, pull);
> + configs[0] = PIN_CONF_PACKED(BCM2835_PINCONF_PARAM_PULL, pull);
>
> map->type = PIN_MAP_TYPE_CONFIGS_PIN;
> map->data.configs.group_or_pin = bcm2835_gpio_pins[pin].name;
> @@ -736,6 +730,12 @@ static int bcm2835_pctl_dt_node_to_map(struct
> pinctrl_dev *pctldev,
> int i, err;
> u32 pin, func, pull;
>
> + /* Check for generic binding in this node */
> + err = pinconf_generic_dt_node_to_map_all(pctldev, np, map, num_maps);
> + if (err || *num_maps)
> + return err;
> +
> + /* Generic binding did not find anything continue with legacy parse */
> pins = of_find_property(np, "brcm,pins", NULL);
> if (!pins) {
> dev_err(pc->dev, "%pOF: missing brcm,pins property\n", np);
> @@ -917,37 +917,62 @@ static int bcm2835_pinconf_get(struct pinctrl_dev
> *pctldev,
> return -ENOTSUPP;
> }
>
> +static void bcm2835_pull_config_set(struct bcm2835_pinctrl *pc,
> + unsigned pin, unsigned arg)
> +{
> + u32 off, bit;
> +
> + off = GPIO_REG_OFFSET(pin);
> + bit = GPIO_REG_SHIFT(pin);
> +
> + bcm2835_gpio_wr(pc, GPPUD, arg & 3);
> + /*
> + * BCM2835 datasheet say to wait 150 cycles, but not of what.
> + * But the VideoCore firmware delay for this operation
> + * based nearly on the same amount of VPU cycles and this clock
> + * runs at 250 MHz.
> + */
> + udelay(1);
> + bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), BIT(bit));
> + udelay(1);
> + bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), 0);
> +}
> +
> static int bcm2835_pinconf_set(struct pinctrl_dev *pctldev,
> unsigned pin, unsigned long *configs,
> unsigned num_configs)
> {
> struct bcm2835_pinctrl *pc = pinctrl_dev_get_drvdata(pctldev);
> - enum bcm2835_pinconf_param param;
> - u16 arg;
> - u32 off, bit;
> + u16 param, arg;
> int i;
>
> for (i = 0; i < n
