lkml.org 
[lkml]   [2018]   [Apr]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC PATCH RESEND 3/3] pinctrl: upboard: Add UP2 pinctrl and gpio driver
On Wed, Apr 25, 2018 at 07:49:12PM +0300, Andy Shevchenko wrote:
> > For reference, here's the relevant ASL from the UP2 platform
> > controller.
>
> It should be in Documentation file or in commit message.

Perfect, I just wasn't sure whether dumping ASL this way would be
acceptable in commit history. In that case, I think I prefer the commit
body over keeping a Documentation/ file elsewhere that's easy to miss.

> > static const struct mfd_cell upboard_up2_mfd_cells[] = {
> > + { .name = "upboard-pinctrl" },
>
> I guess it should be 3 lines.

>
> > UPBOARD_LED_CELL(upboard_up2_led_data, 0),
> > UPBOARD_LED_CELL(upboard_up2_led_data, 1),
> > UPBOARD_LED_CELL(upboard_up2_led_data, 2),
>
> ...and honestly I would not use this macro and put 4 cells explicitly
> here.

The idea was to avoid repeating .platform_data + .pdata_size over and
over as there's a few LEDs per board, but explicit is good too.

May just go with your suggestion as that seems to be more popular than
macros in other mfd drivers.

> > +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's possible?

Just reread pin_request() from pinctrl core. You're right, it isn't.
I'll take out the check.

> > + if (!pd)
> > + return -EINVAL;
>
> Ditto.

As above.

>
> > + struct upboard_pinctrl *pctrl =
> > + container_of(gc, struct upboard_pinctrl, chip);
>
> Do define and use to_upboard_pinctrl().

Will do.

> > + if (offset + 1 > pctrl->nsoc_gpios || !pctrl-
> > >soc_gpios[offset])
> > + return ERR_PTR(-ENODEV);
>
> When this is a case?

Another unnecessary check if gpiolib already guarantees valid inputs.

> > +static int upboard_gpio_get_direction(struct gpio_chip *gc, unsigned
> > int offset)
> > +{
> > + struct gpio_desc *desc = upboard_offset_to_soc_gpio(gc,
> > offset);
> > +
>
> Split above to definition and assignment pieces. Put assignment
> immediately before condition.
>
> > + if (IS_ERR(desc))
> > + return PTR_ERR(desc);

I'll do that.

> > +static struct regmap_field * __init upboard_field_alloc(struct device
> > *dev,
> > + struct regmap
> > *regmap,
> > + unsigned int
> > base,
> > + unsigned int
> > number)
>
> You should really understand what __init means and when it's appropriate
> to use it.

As in the other email: the intention was to drop probe() and funcs only
used on module init, but I appear to have misunderstood something here.

> > +static int __init upboard_pinctrl_probe(struct platform_device *pdev)
> > +{
> > + struct acpi_device * const adev = ACPI_COMPANION(&pdev->dev);
>
> Huh, const in that place? Why?

You're right, it isn't a usual pattern. Dropped.

> > + if (!pdev->dev.parent)
> > + return -EINVAL;
> > +
> > + upboard = dev_get_drvdata(pdev->dev.parent);
> > + if (!upboard)
> > + return -EINVAL;
>
> Same comment as per LED driver.

I'll address that too.

> > + if (strcmp(acpi_device_hid(adev), "AANT0F01"))
> > + return -ENODEV;
>
> Huh?

Ugh, this is left over from a bit of code that selected the right
pinctrl_desc* for each UP HID. Of course, it doesn't make sense now.

I'll take that out.

> > + ((struct pinctrl_pin_desc *)pd)->drv_data = pin;
>
> What is that?! I mean ugly casting.

I agree, it's an eyesore. The intention was to drop const from
pinctrl_desc->pins as we're replacing the pins' drv_data on init.

It does look like I'm going at this the wrong way though. I'll take
another stab at it (pointers welcome of course).

> > +MODULE_LICENSE("GPL");
>
> License mismatch.

Will fix.

Thanks again for your time Andy! I really appreciate your help :)

\
 
 \ /
  Last update: 2018-04-26 04:39    [W:0.077 / U:0.432 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site