Hi Dmitry,
thanks for the immediate review!
> > This driver adds support for the keypad part of the LM8333 and provides
> > hooks for possible GPIO/PWM drivers. Note that this is not a MFD device
> > beause you cannot disable the keypad functionality which, thus, has to
> > be handled by the core anyhow.
>
> So it's like lm8323... In this case we should just add PWM handling
> directly to this driver.
Not sure. There is a PWM framework in the making.
> > +
> > +/* The accessors try twice because the first access may be needed for wakeup */
> > +
> > +int lm8333_read8(struct lm8333 *lm8333, u8 cmd)
> > +{
> > + int repeated = 0, ret;
> > +
> > + do {
> > + ret = i2c_smbus_read_byte_data(lm8333->client, cmd);
> > + } while (ret < 0 && !repeated++);
>
> So there is 1 retry... Why don't you have
>
> do {
> ...
> } while (ret < 0 && retries++ < LM8333_NUM_RETRIES);
>
> to make it completely obvious.
I thought '!repeated' was readable, but will change that.
> > + lm8333 = devm_kzalloc(&client->dev, sizeof(*lm8333), GFP_KERNEL);
> > + input = input_allocate_device();
> > + if (!lm8333 || !input)
>
> You leak memory here if input_allocate_device() succeeds but
> devm_kzalloc fails. Situations like this is why I do not like devm_*
> interface.
I see. Okay, the true flaw is that one should check for error cases
seperately, but with devm_* one is tempted to combine that.
> > +static int __devexit lm8333_remove(struct i2c_client *client)
> > +{
> > + struct lm8333 *lm8333 = i2c_get_clientdata(client);
> > +
> > + input_unregister_device(lm8333->input);
> > + input_free_device(lm8333->input);
>
> Also you need to free IRQ before you unregister (and free) input device,
> otherwise you risk dereferencing already freed memory. Another example
> why I hate devm_*.
Understood. The problem here is mixing devm_* setup with non_devm. So,
it is an either-or-thingie? Hmmm...
Regards,
Wolfram
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Attachment:
signature.asc
Description: Digital signature
