Hi Andi, Peter,
(resend as plain text, sorry to those that get duplicates)
On 12/10/23 23:49, Peter Rosin wrote:
> Hi!
>
> 2023-10-12 at 12:21, Andi Shyti wrote:
>> Hi Chris,
>>
>> ...
>>
>>> static struct mv64xxx_i2c_regs mv64xxx_i2c_regs_mv64xxx = {
>>> @@ -1083,6 +1084,10 @@ mv64xxx_i2c_probe(struct platform_device *pd)
>>> if (drv_data->irq < 0)
>>> return drv_data->irq;
>>>
>>> + drv_data->reset_gpio = devm_gpiod_get_optional(&pd->dev, "reset", GPIOD_OUT_HIGH);
>>> + if (IS_ERR(drv_data->reset_gpio))
>>> + return PTR_ERR(drv_data->reset_gpio);
>> if this optional why are we returning in case of error?
gpiod_get_optional() will return NULL if the property is not present.
The main error I care about here is -EPROBE_DEFER but I figure other
errors are also relevant. This same kind of pattern is used in other
drivers.
>>> +
>>> if (pdata) {
>>> drv_data->freq_m = pdata->freq_m;
>>> drv_data->freq_n = pdata->freq_n;
>>> @@ -1121,6 +1126,12 @@ mv64xxx_i2c_probe(struct platform_device *pd)
>>> goto exit_disable_pm;
>>> }
>>>
>>> + if (drv_data->reset_gpio) {
>>> + udelay(1);
>>> + gpiod_set_value_cansleep(drv_data->reset_gpio, 0);
>>> + udelay(1);
>> you like busy waiting :-)
sure do.
>> What is the reason behind these waits? Is there anything
>> specified by the datasheet?
Those particular times were lifted from the pca954x mux but they are
fairly arbitrary.
>> If not I would do a more relaxed sleeping like an usleep_range...
>> what do you think?
> Since this is apparently not intended to reset the bus driver itself,
> but instead various clients connected to the bus, there is not telling
> which datasheet to examine. It is simply impossible to hard-code a
> correct reset pulse here, when the targets of the pulse are unspecified
> and unknown.
I could probably follow what similar code does in the pci-mvebu.c driver
and make the delay a property as well. As you're highlighting I can't
possibly pick a value that's right for everyone. We really need to be
told that the hardware design requires X us of delay after reset.
> I find the reset-gpios naming extremely misleading.
I picked that mainly because that's the name of the property for
pci-mvebu.c and a few other end-point devices. The crux of the problem
I'm trying to solve is that I have multiple i2c muxes that share a
common reset GPIO in hardware. I can't associate the GPIO with multiple
devices as the ones that are probed after the first will get -EBUSY. I
can cheat and not have a reset-gpios property on the other muxes but
then if the GPIO is deferred (because the controller driver hasn't been
loaded) the muxes don't get reset at all.
> Cheers,
> Peter
