Re: [PATCH v2 3/3] pinctrl: upboard: Add UP2 pinctrl and gpio driver

[Dan O'Donovan]

Thanks for your review feedback, Andy! I'll send a v3 shortly with those changes you suggested. I've added some comments inline below.

On 10/20/2018 12:40 PM, Andy Shevchenko wrote:
> On Fri, Oct 19, 2018 at 8:24 PM Dan O'Donovan <dan@xxxxxxxxxx> wrote:
>
>> +static int upboard_get_functions_count(struct pinctrl_dev *pctldev)
>> +{
>> +       return 0;
>> +}
>> +
>> +static int upboard_get_function_groups(struct pinctrl_dev *pctldev,
>> +                                      unsigned int selector,
>> +                                      const char * const **groups,
>> +                                      unsigned int *num_groups)
>> +{
>> +       *groups = NULL;
>> +       *num_groups = 0;
>> +       return 0;
>> +}
>> +
>> +static const char *upboard_get_function_name(struct pinctrl_dev *pctldev,
>> +                                            unsigned int selector)
>> +{
>> +       return NULL;
>> +}
>> +
>> +static int upboard_set_mux(struct pinctrl_dev *pctldev, unsigned int function,
>> +                          unsigned int group)
>> +{
>> +       return 0;
>> +}
> Hmm... Do you need those stubs? Same Q for other stubs in the file.
It looks like they're required by pinctrl core, which returns an error if they're not provided.

>> +static int upboard_gpio_request_enable(struct pinctrl_dev *pctldev,
>> +                                      struct pinctrl_gpio_range *range,
>> +                                      unsigned int pin)
>> +{
>> +       const struct pin_desc * const pd = pin_desc_get(pctldev, pin);
>> +       const struct upboard_pin *p;
>> +       int ret;
>> +
>> +       if (!pd)
>> +               return -EINVAL;
> When it possible to happen?
> Same Q for the rest same excerpts.
Agreed, it shouldn't be possible. I will remove these checks.

>> +
>> +       if (offset + 1 > pctrl->nsoc_gpios || !pctrl->soc_gpios[offset])
>> +               return ERR_PTR(-ENODEV);
> offset >= ?
> Is it even possible?
Agreed, it shouldn't be possible. I will remove these checks.

>> +static int upboard_pinctrl_probe(struct platform_device *pdev)
>> +{
>> +       struct device *dev = &pdev->dev;
>> +       struct pinctrl_desc *pctldesc;
>> +       struct upboard_pinctrl *pctrl;
>> +       struct upboard_pin *pins;
>> +       struct acpi_device *adev;
>> +       struct regmap *regmap;
>> +       unsigned int i;
>> +       int ret;
>> +       adev = ACPI_COMPANION(dev);
>> +       if (!adev || strcmp(acpi_device_hid(adev), "AANT0F01"))
>> +               return -ENODEV;
> Same Q as per LED driver.
I agree. I will remove this check, both here and in the LED driver.

>> +               if (pd->drv_data) {
>> +                       struct reg_field *field = pd->drv_data;
>> +
>> +                       pin->func_en = devm_regmap_field_alloc(dev, regmap,
>> +                                                              *field);
>> +                       if (IS_ERR(pin->func_en))
>> +                               return PTR_ERR(pin->func_en);
>> +               }
>> +
>> +               pin->gpio_en = upboard_field_alloc(dev, regmap,
>> +                                                  UPBOARD_REG_GPIO_EN0, i);
>> +               if (IS_ERR(pin->gpio_en))
>> +                       return PTR_ERR(pin->gpio_en);
>> +
>> +               pin->gpio_dir = upboard_field_alloc(dev, regmap,
>> +                                                   UPBOARD_REG_GPIO_DIR0, i);
>> +               if (IS_ERR(pin->gpio_dir))
>> +                       return PTR_ERR(pin->gpio_dir);
>> +
>> +               ((struct pinctrl_pin_desc *)pd)->drv_data = pin;
> I'm not sure I understand the purpose of this casting.
When the pd pointer is retrieved from struct pinctrl_desc, it has a const constraint. The purpose of the cast is to bypass that constraint for this use case, because this code is allocating and setting drv_data dynamically here at run-time rather than at compile-time.