(adding Kyungmin Park and Samuel Ortiz) Hi, Yes, It happened unintentionally. I didn't know about your patch before submitting the initial version of my patches. I agree with you, I will review your patches and will try to incorporate extra features from your patches. On Wed, May 23, 2012 at 7:10 AM, <jonghwa3.lee@xxxxxxxxxxx> wrote: > Hi, Yadwinder, > As you know, both of us, recently, had competition for one driver > whether you intend or not. And now, i think it is time to stop this and > to find appropriate goal. From now on, i won't update this driver no > more. I recommend you to review my patch and apply feature that you can > apply. And also check comments that i wrote below. > > On 2012년 05월 22일 14:57, yadi.brar01@xxxxxxxxx wrote: > >> From: Yadwinder Singh Brar <yadi.brar@xxxxxxxxxxx> >> >> Add support for PMIC/regulator portion of MAX77686 multifunction device. >> MAX77686 provides LDOs[1-26] and BUCKs[1-9]. This is initial release of driv >> which supports setting and getting the voltage of a regulator with I2C >> interface. >> + return DIV_ROUND_UP(rdev->desc->uV_step * >> + abs(new_sel - old_sel), >> + 100); >> +} >> + > > > Does LDO also need waiting for voltage change? I afraid it's not. > Yes, according to technical reference manual which I have, ramp rate for LDOs is also 100mV/us. >> + >> + if (pdata->ramp_delay < MAX77686_RAMP_RATE_13MV || >> + pdata->ramp_delay > MAX77686_RAMP_RATE_100MV) >> + pdata->ramp_delay = MAX77686_RAMP_RATE_27MV; /* default */ If pdata doesn't have proper ramp_delay, it will get value MAX77686_RAMP_RATE_27MV. >> + >> + max77686->ramp_delay = pdata->ramp_delay - 1; > > > I think it is better to check pdata->ramp_delay is available. > If pdata doesn't have ramp_delay member it might be error. > Yes, we have taken care of this case above before setting value of max77686->ramp_delay. >> + max77686_update_reg(i2c, MAX77686_REG_BUCK2CTRL1, >> + max77686->ramp_delay << 6, RAMP_MASK); >> + max77686_update_reg(i2c, MAX77686_REG_BUCK3CTRL1, >> + max77686->ramp_delay << 6, RAMP_MASK); >> + max77686_update_reg(i2c, MAX77686_REG_BUCK4CTRL1, >> + max77686->ramp_delay << 6, RAMP_MASK); >> + > > > Why do you use i2c client still? If you registered regmap you can use > its API. I recommend you to use regmap_update_bits() directly. > > Yes, we are using regmap_update_bits(). max77686_update_reg() is just a wrapper over it. >> + platform_set_drvdata(pdev, max77686); >> + > > MAX77686 has crystal oscillator in it. And original version of this > driver which was written by Chiwoon Byun, registers it as a regulator. > As Mark says, we have to change it to use generic clock API. Where do > you think should we put them into? In my opinion, it is proper that just > leave them in regulator driver because this driver is almost core of > PMIC. I already applied generic API in my local repository but i > couldn't test yet. Because it crashed with SOC's private clock API. > Anyway if you register 32khz clock with generic API ,DEFINE_CLK_GATE() > will help you out which defined in linux/clk-private.h. > Yes, I was also thinking about where to put it. I am not sure whether this is a proper place to put them. Anyway I will again think about it. Thanks, Yadwinder. -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
 |