On Sun, Nov 25, 2012 at 09:34:46AM +0530, Viresh Kumar wrote:
> Hi Wolfram,
>
> Thanks for sharing your opinion.
>
> On 25 November 2012 02:29, Wolfram Sang <w.sang@xxxxxxxxxxxxxx> wrote:
> > On Thu, Oct 04, 2012 at 04:34:53PM +0530, Viresh Kumar wrote:
>
> >> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
>
> >> +static int i2c_get_gpios_for_recovery(struct i2c_adapter *adap)
> >> +{
> >> + struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
> >> + struct device *dev = &adap->dev;
> >> + int ret = 0;
> >> +
> >> + if (bri->get_gpio) {
> >> + ret = bri->get_gpio(bri->scl_gpio);
> >> + if (ret) {
> >> + dev_warn(dev, "scl get_gpio: %d\n", bri->scl_gpio);
> >
> > This warning is probably not very helpful to a user.
>
> This is what i thought about it: This is a platform specific routine and
> most probably it will fail the first time this code is ever hit and so a
> warning would be very helpful for them.
>
> But, now i think platforms should have these prints in their get_gpio
> implementations. And we can make it have return type void.
The warning could be made helpful if properly rephrased. Actually, I
think returning an err is OK here, but if there is a warning it should
be a proper human readable sentence.
> >> + if (unlikely(ret ||
> >
> > Since the unlikely() are not in hot-paths, you probably better skip
> > them.
>
> It will be removed as get_gpio() would have return type void.
Keep in mind that it was not the only one. I think V7 had still one
left.
> >> + * @skip_sda_polling: if true, bus recovery will not poll sda line to check if
> >> + * it became high or not. Only required if recover_bus == NULL.
> >
> > Does a user really need to set this?
>
> This was done for platforms and i2c controllers which don't have configuration
> to control scl line. So i still feel we need it.
It might be needed internally, still not sure if a user needs to set it.
If so, there should be more documentation when she wants to use this.
> >> + * @clock_rate_khz: clock rate of dummy clock in khz. Required for both gpio and
> >> + * scl type recovery.
> >
> > Does a user really need this? We could probably use something close to
> > 100kHz always?
>
> Not sure. Doesn't it depend on the current clk rate of controller ?
> If not then yes it can be 100 KHz
It should be OK to drive the bus at x kHz and recover from it at y kHz
(as long as y < 100).
Regards,
Wolfram
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Attachment:
signature.asc
Description: Digital signature
