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.
