On Thu, 8 Jul 2010 20:37:08 +0100
Matthew Garrett <mjg59@xxxxxxxxxxxxx> wrote:
> On Thu, Jul 08, 2010 at 06:09:27PM +0100, Alan Cox wrote:
> > + else if (offset < 14)/* it is GPOSW */
> > + rc = intel_scu_ipc_update_register(GPOSWCTL0 +
> > offset - 8,
> > + GPOSW_DRV | GPOSW_DOU | GPOSW_RDRV,
> > + GPOSW_DRV | (value ? GPOSW_DOU :
> > 0));
> > + else if (offset > 15 && offset < 24)/* it is GPO */
>
> What happened to gpios 14 and 15?
Good question. I will double check this is correct but since everyone
is using the driver its probably right
> > +
> > +static int pmic_gpio_get(struct gpio_chip *chip, unsigned offset)
> > +{
> > + u8 r;
> > +
> > + /* we only have 8 GPIO pins we can use as input */
> > + if (offset > 8)
> > + return -1;
>
> Is there any chance of this getting passed up to userspace? Using
> something more descriptive would be nice (ditto for a couple of other
> places)
No but it can get back to the caller. Right now gpiolib isn't really
internally sure what to do about gpio reads failing. Given that I might
as well hand back -Efoo codes.
> > + if (intel_scu_ipc_ioread8(GPIO0 + offset, &r))
> > + return -1;
>
> ...especially since you get the same error for two different failure
> modes :)
>
> > +static int pmic_irq_type(unsigned irq, unsigned type)
> > +{
> > + struct pmic_gpio *pg = get_irq_chip_data(irq);
> > + u32 gpio = irq - pg->irq_base;
> > + unsigned long flags;
> > +
> > + if (gpio < 0 || gpio > pg->chip.ngpio)
>
> It's unsigned, don't need to check that it's < 0.
Fixed
> I don't think it's really necessary to indicate that the driver's
> loaded here.
Agreed, fixed
> > + if (!pdata || !pdata->gpio_base || !pdata->irq_base) {
> > + dev_dbg(dev, "incorrect or missing platform
> > data\n");
> > + return -EINVAL;
> > + }
>
> Shouldn't there be an is_mrst() check before trying to dereference
> this?
We check pdata == NULL so in the theoretical case someone loaded the
driver on the wrong platform and cooked up a device match you'd still
be ok
Alan
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
