Re: [PATCH v3 2/3] spi: of: allow instantiating slaves without a driver

[Michal Suchanek]

On 19 July 2016 at 12:52, Mark Brown  wrote:
> On Tue, Jul 19, 2016 at 10:31:54AM +0200, Michal Suchanek wrote:
>> On 19 July 2016 at 01:02, Mark Brown  wrote:
>> > On Tue, Jul 19, 2016 at 12:35:41AM +0200, Michal Suchanek wrote:
>
>> >> SPI slave devices are not created when looking up driver for the slave
>> >> fails. Create a device anyway so it can be used with spidev.
>
>> > Nothing has change since you last sent this patch which converts
>> > of_modailias_node() into something which looks up a driver so the
>> > patch description still fails to describe what the patch is doing.
>
>> I have split the other part of the patch. Regarding the commit message
>> if you have suggestion for better wording please do share it.
>
> As covered in SubmittingPatches your commit message should describe what
> the change does and what the intended effect is.  If we were looking for
> a device driver the code would be looking up a struct device_driver or
> some other struct that contains one.
>
>> From my point of view the conceptual change described in the commit message
>> is that whenever SPI slave node is encountered in devicetree you get either
>> a device with active driver or a device with no driver whereas
>> previously you either
>> got a device with active driver or no device. So yes, it's about
>
> This is not the case, it is perfectly possible to have a device with no
> driver bound to it otherwise it would not be possible to use loadable
> modules for drivers.

Ok, I missed this part. That makes the commit message indeed broken.

Thanks

Michal

Re: [PATCH v3 2/3] spi: of: allow instantiating slaves without a driver

[Michal Suchanek]

On 19 July 2016 at 12:52, Mark Brown  wrote:
> On Tue, Jul 19, 2016 at 10:31:54AM +0200, Michal Suchanek wrote:
>> On 19 July 2016 at 01:02, Mark Brown  wrote:
>> > On Tue, Jul 19, 2016 at 12:35:41AM +0200, Michal Suchanek wrote:
>
>> >> SPI slave devices are not created when looking up driver for the slave
>> >> fails. Create a device anyway so it can be used with spidev.
>
>> > Nothing has change since you last sent this patch which converts
>> > of_modailias_node() into something which looks up a driver so the
>> > patch description still fails to describe what the patch is doing.
>
>> I have split the other part of the patch. Regarding the commit message
>> if you have suggestion for better wording please do share it.
>
> As covered in SubmittingPatches your commit message should describe what
> the change does and what the intended effect is.  If we were looking for
> a device driver the code would be looking up a struct device_driver or
> some other struct that contains one.
>
>> From my point of view the conceptual change described in the commit message
>> is that whenever SPI slave node is encountered in devicetree you get either
>> a device with active driver or a device with no driver whereas
>> previously you either
>> got a device with active driver or no device. So yes, it's about
>
> This is not the case, it is perfectly possible to have a device with no
> driver bound to it otherwise it would not be possible to use loadable
> modules for drivers.

Ok, I missed this part. That makes the commit message indeed broken.

Thanks

Michal

Re: [PATCH v3 2/3] spi: of: allow instantiating slaves without a driver

[Mark Brown]

On Tue, Jul 19, 2016 at 10:31:54AM +0200, Michal Suchanek wrote:
> On 19 July 2016 at 01:02, Mark Brown  wrote:
> > On Tue, Jul 19, 2016 at 12:35:41AM +0200, Michal Suchanek wrote:

> >> SPI slave devices are not created when looking up driver for the slave
> >> fails. Create a device anyway so it can be used with spidev.

> > Nothing has change since you last sent this patch which converts
> > of_modailias_node() into something which looks up a driver so the
> > patch description still fails to describe what the patch is doing.

> I have split the other part of the patch. Regarding the commit message
> if you have suggestion for better wording please do share it.

As covered in SubmittingPatches your commit message should describe what
the change does and what the intended effect is.  If we were looking for
a device driver the code would be looking up a struct device_driver or
some other struct that contains one.  

> From my point of view the conceptual change described in the commit message
> is that whenever SPI slave node is encountered in devicetree you get either
> a device with active driver or a device with no driver whereas
> previously you either
> got a device with active driver or no device. So yes, it's about

This is not the case, it is perfectly possible to have a device with no
driver bound to it otherwise it would not be possible to use loadable
modules for drivers.


signature.asc
Description: PGP signature

Re: [PATCH v3 2/3] spi: of: allow instantiating slaves without a driver

[Mark Brown]

On Tue, Jul 19, 2016 at 10:31:54AM +0200, Michal Suchanek wrote:
> On 19 July 2016 at 01:02, Mark Brown  wrote:
> > On Tue, Jul 19, 2016 at 12:35:41AM +0200, Michal Suchanek wrote:

> >> SPI slave devices are not created when looking up driver for the slave
> >> fails. Create a device anyway so it can be used with spidev.

> > Nothing has change since you last sent this patch which converts
> > of_modailias_node() into something which looks up a driver so the
> > patch description still fails to describe what the patch is doing.

> I have split the other part of the patch. Regarding the commit message
> if you have suggestion for better wording please do share it.

As covered in SubmittingPatches your commit message should describe what
the change does and what the intended effect is.  If we were looking for
a device driver the code would be looking up a struct device_driver or
some other struct that contains one.  

> From my point of view the conceptual change described in the commit message
> is that whenever SPI slave node is encountered in devicetree you get either
> a device with active driver or a device with no driver whereas
> previously you either
> got a device with active driver or no device. So yes, it's about

This is not the case, it is perfectly possible to have a device with no
driver bound to it otherwise it would not be possible to use loadable
modules for drivers.


signature.asc
Description: PGP signature

Re: [PATCH v3 2/3] spi: of: allow instantiating slaves without a driver

[Michal Suchanek]

Hello,

On 19 July 2016 at 01:02, Mark Brown  wrote:
> On Tue, Jul 19, 2016 at 12:35:41AM +0200, Michal Suchanek wrote:
>> SPI slave devices are not created when looking up driver for the slave
>> fails. Create a device anyway so it can be used with spidev.
>
>>   rc = of_modalias_node(nc, spi->modalias,
>>   sizeof(spi->modalias));
>>   if (rc < 0) {
>> - dev_err(>dev, "cannot find modalias for %s\n",
>> + dev_warn(>dev, "cannot find modalias for %s\n",
>
> Nothing has change since you last sent this patch which converts
> of_modailias_node() into something which looks up a driver so the
> patch description still fails to describe what the patch is doing.
>
> Please don't ignore review comments, people are generally making them
> for a reason and are likely to have the same concerns if issues remain
> unaddressed.  Having to repeat the same comments can get repetitive and
> make people question the value of time spent reviewing.  If you disagree
> with the review comments that's fine but you need to reply and discuss
> your concerns so that the reviewer can understand your decisions.

I have split the other part of the patch. Regarding the commit message
if you have suggestion for better wording please do share it.

>From my point of view the conceptual change described in the commit message
is that whenever SPI slave node is encountered in devicetree you get either
a device with active driver or a device with no driver whereas
previously you either
got a device with active driver or no device. So yes, it's about
allowing SPI slave
devices without a driver bound to them.

The technical implementation detail that can be seen in the patch is
ignoring the
return value of of_modalias_node.

Thanks

Michal

Re: [PATCH v3 2/3] spi: of: allow instantiating slaves without a driver

[Michal Suchanek]

Hello,

On 19 July 2016 at 01:02, Mark Brown  wrote:
> On Tue, Jul 19, 2016 at 12:35:41AM +0200, Michal Suchanek wrote:
>> SPI slave devices are not created when looking up driver for the slave
>> fails. Create a device anyway so it can be used with spidev.
>
>>   rc = of_modalias_node(nc, spi->modalias,
>>   sizeof(spi->modalias));
>>   if (rc < 0) {
>> - dev_err(>dev, "cannot find modalias for %s\n",
>> + dev_warn(>dev, "cannot find modalias for %s\n",
>
> Nothing has change since you last sent this patch which converts
> of_modailias_node() into something which looks up a driver so the
> patch description still fails to describe what the patch is doing.
>
> Please don't ignore review comments, people are generally making them
> for a reason and are likely to have the same concerns if issues remain
> unaddressed.  Having to repeat the same comments can get repetitive and
> make people question the value of time spent reviewing.  If you disagree
> with the review comments that's fine but you need to reply and discuss
> your concerns so that the reviewer can understand your decisions.

I have split the other part of the patch. Regarding the commit message
if you have suggestion for better wording please do share it.

>From my point of view the conceptual change described in the commit message
is that whenever SPI slave node is encountered in devicetree you get either
a device with active driver or a device with no driver whereas
previously you either
got a device with active driver or no device. So yes, it's about
allowing SPI slave
devices without a driver bound to them.

The technical implementation detail that can be seen in the patch is
ignoring the
return value of of_modalias_node.

Thanks

Michal

Re: [PATCH v3 2/3] spi: of: allow instantiating slaves without a driver

[Mark Brown]

On Tue, Jul 19, 2016 at 12:35:41AM +0200, Michal Suchanek wrote:
> SPI slave devices are not created when looking up driver for the slave
> fails. Create a device anyway so it can be used with spidev.

>   rc = of_modalias_node(nc, spi->modalias,
>   sizeof(spi->modalias));
>   if (rc < 0) {
> - dev_err(>dev, "cannot find modalias for %s\n",
> + dev_warn(>dev, "cannot find modalias for %s\n",

Nothing has change since you last sent this patch which converts
of_modailias_node() into something which looks up a driver so the
patch description still fails to describe what the patch is doing.

Please don't ignore review comments, people are generally making them
for a reason and are likely to have the same concerns if issues remain
unaddressed.  Having to repeat the same comments can get repetitive and
make people question the value of time spent reviewing.  If you disagree
with the review comments that's fine but you need to reply and discuss
your concerns so that the reviewer can understand your decisions.


signature.asc
Description: PGP signature

Re: [PATCH v3 2/3] spi: of: allow instantiating slaves without a driver

[Mark Brown]

On Tue, Jul 19, 2016 at 12:35:41AM +0200, Michal Suchanek wrote:
> SPI slave devices are not created when looking up driver for the slave
> fails. Create a device anyway so it can be used with spidev.

>   rc = of_modalias_node(nc, spi->modalias,
>   sizeof(spi->modalias));
>   if (rc < 0) {
> - dev_err(>dev, "cannot find modalias for %s\n",
> + dev_warn(>dev, "cannot find modalias for %s\n",

Nothing has change since you last sent this patch which converts
of_modailias_node() into something which looks up a driver so the
patch description still fails to describe what the patch is doing.

Please don't ignore review comments, people are generally making them
for a reason and are likely to have the same concerns if issues remain
unaddressed.  Having to repeat the same comments can get repetitive and
make people question the value of time spent reviewing.  If you disagree
with the review comments that's fine but you need to reply and discuss
your concerns so that the reviewer can understand your decisions.


signature.asc
Description: PGP signature